Skip to content

Conversation

Martimiz
Copy link

This works, but it does probably bypass CORS allow-headers settings for Authorization. So probably they should be validated in some way as well, do you think?

@Martimiz
Copy link
Author

can't fix codeclimate complaining about static access :(

@Firesphere
Copy link
Owner

Don't worry too much about the static access errors, I happily ignore those ;)

$authHeader = $request->getHeader('Authorization');
if (!$authHeader) {
$envVars = Environment::getVariables();
if (isset($envVars['_SERVER']['REDIRECT_HTTP_AUTHORIZATION'])) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this should be an .htaccess setting, which should forward the HTTP authorisation. Not something inside the scope of this module?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A bit late, but on second thought, I do see value in this, but I'd rather not reed directly from a header, could you update this to use filter_input?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose I could, shouldn’t be hard. But it’s been a while and I don’t have that setup anymore where this was an issue, so can’t actually test it now. Try to find a moment to set something up...

@Martimiz
Copy link
Author

Martimiz commented Jul 23, 2018

Took me quite a while to figure out what was going on the last time, because it just doesn’t work, without any errors... and it’s a nice and easy fix 🙂

Also SilverStripe core BasicAuth has taken this same approach, so I think it would be consistent...

And also, but that could be a personal issue, I couldn’t get the htaccess solution working, nor did porting it to lighttpd work...

@Firesphere
Copy link
Owner

Any status update on this?

I still think an .htaccess change is the way to go....

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants