CSRF exempt for WebhookView when authenticate via user/pass

[humitos]

I think this bug was introduced at f95cae3

Closes #4986

[agjohnson]

I'm waffling on this PR.

Removing this would likely(?) be opening ourselves up to CSRF attacks that use a logged in user's session to perform actions on our API. Without a CSRF token from RTD, a malicious site could use JS to interact with our API on behalf of the logged in user. We can mitigate this by requiring a CSRF token from RTD, in much the same way that we protect our dashboard forms against malicious JS. But i agree that this is extra burden on the user that makes for a worse UX.

This is one of the reasons why I lean towards divorcing user sessions/auth and API auth, though I suppose that offering token auth for a case like this removes the burden of CSRF tokens and session auth on API requests.

[humitos]

I agree with all you said here.

Removing this would likely(?) be opening ourselves up to CSRF attacks that use a logged in user's session to perform actions on our API

I'm exempting only the API view for the Webhooks not all the API endpoints. It's still "dangerous" because a malicious JS could trigger a crazy amount of builds, but it won't be able to change anything in the project.

On the other hand, if we are going to not allow this, we need to update our docs at

https://docs.readthedocs.io/en/latest/webhooks.html#authentication

and reply to @ianw at #4986.

This is one of the reasons why I lean towards divorcing user sessions/auth and API auth, though I suppose that offering token auth for a case like this removes the burden of CSRF tokens and session auth on API requests.

This is the solution to me as well, and APIv3 will behave in this way.

[ianw]

This is one of the reasons why I lean towards divorcing user sessions/auth and API auth, though I suppose that offering token auth for a case like this removes the burden of CSRF tokens and session auth on API requests.

By this you mean JWT type tokens for the user? Rather than project-specific tokens?

As an infrastructure project hosting many projects that want to publish to RTD via us, it is much easier for us to run things via a single authenticated user and to publish. It means for our users they simply add the CI user to their project (so it can publish) and add the publishing job with their non-secret project id to their CI release pipeline. We only need to manage the one one secret that is the CI user password. We could do a non-expiring token just the same.

We can take the alternative approach of each project using their per-project token to trigger builds, but it means managing many more secrets. Each client project needs to then get their token, encrypt it and write a job to use it. And because we don't want to leak the tokens we don't publish the logs, so then it becomes another step if debugging where projects probably need to get infrastructure admins involved.

[gorshunovr]

Team, please, review this patch. Thank you very much.

[ericholscher]

I think this makes sense for the webhook view. I agree on not exposing it for all API views, but this is breaking a documented use case that users depend on, so I'm 👍

[humitos]

Upgrade the PR to solve conflicts. Ready to merge once tests pass.

[ericholscher]

I thought we were doing basic auth here, not session auth?

Seems like it should be using https://www.django-rest-framework.org/api-guide/authentication/#basicauthentication

[ericholscher]

Holding off on approval until I understand the session auth changes.

[stsewd]

I guess we can close this one. Problem fixed in #5311