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

Lock down cookie based authentication on API endpoints #7028

Open
amercader opened this issue Aug 23, 2022 · 1 comment · Fixed by #7058
Open

Lock down cookie based authentication on API endpoints #7028

amercader opened this issue Aug 23, 2022 · 1 comment · Fixed by #7058
Assignees

Comments

@amercader
Copy link
Member

Discussed on 2022-08-23 dev meeting:

  1. Default behaviour: discard cookie based authentication in the API if it doesn't include the CSRF token
    • Is this equivalent to remove the @csrf.exempt decorator here?
  2. Add new configuration option: turn off entirely cookie based authentication on the API endpoint
    • Not sure how to implement this, but probably we need to fiddle at the Flask-login level. Maybe this is helpful?
  3. (Optional): Allow cookie based authentication without valid CSRF tokens on development mode

@wardi @EricSoroos did I capture that correctly?

@wardi wardi changed the title Look down cookie based authentication on API endpoints Lock down cookie based authentication on API endpoints Aug 23, 2022
@amercader
Copy link
Member Author

@TomeCirun you were kindly volunteered to work on this by @tino097 :)

Let's start with the first point. I think what we need to do is:

  1. Remove the @csrf.exempt check from the API action endpoint. This will cause all client side JS API calls to fail, so
  2. We need to update the JS calls to include the CSRF header, like you did for the follow module on [#6158] Add CSRF Protection #6863 . Remember to move the addCsrfTokenHeader() function to the base module class. Or maybe we even don't have to change any modules and just modify the client class they use to add the header
  3. Document the behaviour change (No cookie based authentication in the API without CSRF token) and the changes needed in JS if they are not using the ckan client

Does this make sense?

amercader added a commit that referenced this issue Sep 15, 2022
Refs #7028

Site maintainers can choose to completely ignore cookie based
authentication for API calls. When
`ckan.auth.disable_cookie_auth_in_api` is set to False, if there is a
user identified via the cookie and it is an API request,
it will be ignored entirely and CKAN will fall back to token based authentication.

This will probably break some JS widgets and view plugins from the
frontend so it should be used with caution.
amercader added a commit that referenced this issue Sep 15, 2022
amercader added a commit that referenced this issue Sep 16, 2022
@amercader amercader reopened this Sep 19, 2022
smotornyuk added a commit that referenced this issue Sep 19, 2022
…h-in-api

[#7028] Allow to shut down cookie based auth for API entirely
amercader added a commit that referenced this issue Sep 29, 2022
amercader added a commit that referenced this issue Sep 29, 2022
The request.endpoint does not always match the actual view function, eg
organization.edit or custom_group.edit both point to group.edit, which
is what the csrf library expects in the exempt list
amercader added a commit that referenced this issue Sep 29, 2022
If we mock the logged in user directly, the CSRF validation fails.
Using environ_overrides with {"REMOTE_USER": name} or {"Authorization":
token} bypasses the CSRF protection.

In these cases we need to use the REMOTE_USER variant because the
Authorization header is not passed to redirects, so we can't get the
final request content to test it
amercader added a commit that referenced this issue Sep 29, 2022
amercader added a commit that referenced this issue Oct 5, 2022
wardi added a commit that referenced this issue Oct 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants