Skip to content
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

Use 403 when actions are forbidden, not 401 #2846

Merged
merged 4 commits into from Mar 28, 2016
Merged

Conversation

@wardi
Copy link
Contributor

@wardi wardi commented Jan 26, 2016

401 is for when a user can't authenticate. All our current 401 errors are causing users to be logged out by repoze (which is reasonable). Instead we want to say "yes, I know who you are, you just aren't allowed to access this page"

@wardi
Copy link
Contributor Author

@wardi wardi commented Jan 27, 2016

This change won't fix extensions that have copied ckan controller code and still use abort(401) Also, this might break extensions that rely on getting 401 errors sent to IAuthenticator plugin abort() methods.

It's not clear to me what the IAuthenticator abort() method it intended for. Does it allow a redirect or some other action? If so, because a number of _show calls for unauthorized users are now 404's instead of 401's that could cause more breakage

@bzar
Copy link
Contributor

@bzar bzar commented Jan 28, 2016

This PR seems to fix any issues I've had regarding spontaneous logouts.

@nigelbabu nigelbabu merged commit c041bbf into master Mar 28, 2016
3 checks passed
@nigelbabu nigelbabu deleted the 2846-403-forbidden branch Mar 28, 2016
@wardi
Copy link
Contributor Author

@wardi wardi commented Mar 28, 2016

@nigelbabu thanks! I'll look into the test failures, possibly some new tests added since I created this PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants