-
Notifications
You must be signed in to change notification settings - Fork 2.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix Ajax Session loss Feature for 401 Standard Conformity #209
Conversation
format.any(:xml, :js, :json) { head :unauthorized, "Reason" => "login needed", 'WWW-Authenticate' => 'Session realm="OpenProject API"' } | ||
else | ||
format.any(:xml, :js, :json) { head :unauthorized, 'WWW-Authenticate' => 'Basic realm="OpenProject API"' } | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Assuming you didn't remove the "Reason" => "login needed" on purpose, I'd suggest this code
authentication_scheme = if request.headers["X-ACCEPT-AUTH"] == "Session"
'Session' else 'Basic'
end
format.any(:xml, :js, :json) { head :unauthorized,
"Reason" => "login needed",
'WWW-Authenticate' => authentication_scheme + ' realm="OpenProject API"' }
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right. I changed the code.
I'd like to have a test for the new header. Any reason you chose the all-caps |
|
||
authentication_scheme = if request.headers["X-Authentication-Scheme"] == "Session" | ||
'Session' else 'Basic' | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should correct the intendation:
authentication_scheme = if request.headers["X-Authentication-Scheme"] == "Session"
'Session' else 'Basic'
end
Please merge after indentation is fixed and updating the changelog. |
This reverts commit 9ede533.
This PR should be subject to discussion!
@meeee @Cnrk @tessi