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

Remove no-longer-needed transitional cookie code #8865

Closed
wants to merge 2 commits into from

Conversation

seanh
Copy link
Contributor

@seanh seanh commented Aug 8, 2024

Remove some code to do with transitioning already-logged-in users to our new separate API auth cookie. This code is no longer needed once all production users have been transitioned.

This PR shouldn't be merged until at least 30 days after #8861 is merged (by then all HTML auth cookies that might have been issued without an API auth cookie being issued by the same login response will have expired, so there won't be any more users logged-in with just an HTML auth cookie and no API one).

@seanh seanh requested a review from robertknight August 8, 2024 14:32
@seanh seanh force-pushed the remove-transitional-cookie-code branch 2 times, most recently from 42d0c09 to 82acc01 Compare August 8, 2024 14:44
@robertknight
Copy link
Member

With the changes in this PR, users will get a confusing error if the API cookie expires or gets removed but the main site cookie still exists. I did this by manually removing the cookie in browser dev tools, but I think we should assume that there are other ways it could happen.

api-cookie-missing

The workaround for the user here is to log out and back in again, but this is not clear from the error message.

Use two separate auth cookies to authenticate requests for HTML pages
(these use the pre-existing auth cookie) and requests from h's own
frontend code to h's API endpoints (these use a new, separate API auth
cookie that's introduced by this commit).

This enables the API auth cookie to be more secure. In particular, the
API auth cookie can use SameSite=Strict rather than Lax, which gives
better protection against CSRF attacks but would not be desirable for
the HTML auth cookie (because whenever a user followed a link from
another site to Hypothesis they would find themselves not authenticated).

How this works: `CookiePolicy` (which is used for HTML page requests)
sets and deletes both the HTML auth cookie and the API auth cookie on
log in and log out, but reads only the HTML auth cookie to authenticate
HTML page requests. `APICookiePolicy` reads only the API auth cookie to
authenticate API requests, and never sets or deletes either cookie.

In detail:

1. Recall that the top-level security policy (`TopLevelPolicy`)
   delegates all method calls to either `CookiePolicy` (for HTML
   endpoints) or `APIPolicy` (for API endpoints).

2. When a user logs in the view calls `pyramid.security.remember()`
   which calls `TopLevelPolicy.remember()`. The login form submission
   is an HTML endpoint so `TopLevelPolicy.remember()` delegates to
   `CookiePolicy.remember()` which sets both the HTML and API auth
   cookies.

3. Similarly when the user logs out `CookiePolicy.forget()` deletes both
   the HTML and API auth cookies.

4. To authenticate HTML page requests `CookiePolicy.identity()` reads
   only the HTML auth cookie.

5. To authenticate API requests `TopLevelPolicy` delegates to
   `APIPolicy` which delegates to a list of API subpolicies
   (`BearerTokenPolicy`, `AuthClientPolicy` and `APICookiePolicy`).
   If there's no bearer token or auth client authentication in the
   request then `APICookiePolicy` reads only the API auth cookie
   to authenticate API requests.
@robertknight
Copy link
Member

@seanh - Do you still plan to go ahead and land this as-is following our discussion at the end of last week or do you plan to make any changes?

Base automatically changed from separate-api-cookie to main August 20, 2024 10:13
@seanh
Copy link
Contributor Author

seanh commented Aug 21, 2024

We'll need to rebase this if we decide to merge it.

I've added a log message (#8889) so we can see whether this transitional cookie code is needed or not. Note that for the next 30 days we'd expect to see this message getting logged, after that maybe not.

@robertknight robertknight removed their request for review August 27, 2024 08:34
Copy link

This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days.

@github-actions github-actions bot added the stale Used by https://github.com/probot/stale to label stale issues and pull requests before closing them label Sep 26, 2024
@github-actions github-actions bot closed this Oct 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Do not merge stale Used by https://github.com/probot/stale to label stale issues and pull requests before closing them
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants