Oauth Account System #182

stadust · 2024-10-05T09:30:33Z

This is a continuation of #135, but I messed up pushing to @peonii's branch, and now cannot update that PR anymore 💀

License Acceptance

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

stadust · 2024-10-05T09:31:30Z

Only difference to #135 so far is that I squashed everything, and resolved some left over merge markers

stadust · 2024-10-05T10:26:03Z

pointercrate-user-api/src/endpoints/auth.rs

 pub async fn invalidate(mut auth: BasicAuth) -> Result<Status> {
    match auth.user {
        AuthenticatedUser::Legacy(legacy) => legacy.invalidate_all_tokens(auth.secret, &mut auth.connection).await?,
+        AuthenticatedUser::OAuth2(oauth) => oauth.invalidate_all_tokens(&mut auth.connection).await? // I have no clue what we'll do here


mh, this still doesn't work though, because the parameter here is BasicAuth, meaning for oauth2 accounts we'll hit UNAUTHORIZED way before this point.

stadust · 2024-10-05T10:27:09Z

pointercrate-user-api/src/endpoints/auth.rs

+        + "&response_type=code"
+        + "&prompt=consent"
+        + "&scope=email%20profile"
+        + "&redirect_uri=http%3A%2F%2Flocalhost%3A1971%2Fapi%2Fv1%2Fauth%2Fcallback";


this needs to be updated to not point to localhost anymore

stadust · 2024-10-05T12:12:08Z

pointercrate-user-api/src/endpoints/auth.rs

+    if legacy.is_some() {
+        let legacy_cookie = Cookie::build(("legacy", "true"))
+            .http_only(true)
+            .same_site(SameSite::Strict)
+            .path("/");
+
+        cookies.add(legacy_cookie);
+    }


This stuff also needs to be done a bit differently. We need to setup a state parameter that contains the user id of the account that was logged in when the request start, a random number, as well as a cryptographic hash of these two fields (signed with the account's secret). Then the random number gets stored as a cookie with short expiration (5 minutes or so). In the callback, we validate the hash, we validate the account id against the account id logged in when the callback URI is requested, and we validate the random number against the cookie.

That way, an attacker can only influence our flow by

intercepting the uri to which google redirects us after successful authentication

replacing the code with the code for their account

getting the user to click this modified link within 5 minutes of initiating the flow

There's no replay attacks possible beyond that because the random number part of the state changes every 5 minutes. There's also no tricking a user into clicking a callback link generated using a different pointercrate account, because the user id in the state parameter would mismatch.

It needs to be set for both the API and the pages crates. Otherwise, the frontend will just receive a 404 when registering. Signed-off-by: stadust <[email protected]>

stadust · 2024-10-05T15:59:33Z

pointercrate-user/src/auth/oauth2/get.rs

+            ("code", code),
+            ("client_id", &config::google_client_id()),
+            ("client_secret", &config::google_client_secret()),
+            ("redirect_uri", &config::google_redirect_uri()),


I think this needs to be set to the same value as the redirect_uri field in the original request that returned the code in the first place.

codecov · 2024-10-05T16:14:03Z

Codecov Report

Attention: Patch coverage is 21.18644% with 279 lines in your changes missing coverage. Please review.

Project coverage is 27.21%. Comparing base (53ce7e8) to head (e3a689e).
Report is 52 commits behind head on master.

Files with missing lines	Patch %	Lines
pointercrate-user-api/src/pages.rs	2.32%	84 Missing ⚠️
pointercrate-user/src/auth/oauth2/get.rs	0.00%	51 Missing ⚠️
pointercrate-user/src/auth/oauth2/post.rs	0.00%	41 Missing ⚠️
pointercrate-user/src/auth/oauth2/mod.rs	0.00%	23 Missing ⚠️
pointercrate-user-pages/src/login.rs	0.00%	17 Missing ⚠️
pointercrate-user/src/auth/oauth2/patch.rs	0.00%	15 Missing ⚠️
pointercrate-user/src/auth/mod.rs	78.46%	14 Missing ⚠️
pointercrate-user/src/config.rs	45.00%	11 Missing ⚠️
pointercrate-user-pages/src/account/profile.rs	0.00%	10 Missing ⚠️
pointercrate-user/src/auth/get.rs	52.38%	10 Missing ⚠️
... and 2 more

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #182      +/-   ##
==========================================
- Coverage   28.00%   27.21%   -0.80%     
==========================================
  Files         116      121       +5     
  Lines        8231     8507     +276     
==========================================
+ Hits         2305     2315      +10     
- Misses       5926     6192     +266

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

`generate_jwt` and `validate_jwt` create and validate JWTs signed with the given `AuthenticatedUsers` signing key. They are generic over the claims, giing a generic API for managing JWTs without needing to make `jwt_secret` a public function. While we're at it, use the JWT standard `sub` field to store the user ID we are expecting, and use the jsonwebtoken library's validation to ensure we always get the expected `sub` value. Also switch CSRF token's to this generalized procedure. This has the consequence that now they are signed using the user's secret key, instead of the global one. No functional change should follow from this, however to ensure that CSRF tokens do not work as access tokens, annotate all claims structs with `serde(deny_unknown_fields)`. Also move the "read ID from jwt by unsafely decoding it" stuff into `auth/mod.rs`, for better encapsulation of the claims structs. Signed-off-by: stadust <[email protected]>

We were not doing anything with it anyway, and the check of the `exp` field is completely independent of it. Signed-off-by: stadust <[email protected]>

Move the `secret()` function from pointercrate-core to pointercrate-user, since the secret key is only used in pointercrate-user. Also opens us up for having more pointercrate-user specific configuration options (such as oauth client id). Signed-off-by: stadust <[email protected]>

+ Moved google account id migration after all migrations added to master in the meantime. + Dropped change of password_hash to being optional + Break AuthenticatedUser::oauth2_callback into smaller functions + Drop `email_address` column from database. Was originally added when I only wanted to add email-based password reset, but with oauth there's no need. + Repurpose the `password_hash` field into a generic `salt` field for oauth accounts. This allows implementation the token reset. + Remove UserError::NotApplicable. It was the same as UserError::NonLegacy + Move the oauth endpoints to `pages.rs`, because they're not really part of the API (they cannot be used programmatically) commit 5bd3860920aad144dc90665569c9dd06e33ae07e Author: Natalia <[email protected]> Date: Sat Sep 14 21:24:35 2024 +0200 fix merge conflicts, (mostly) move over to feature flags commit 8730c89 Merge: 2734b86 5b6b7b3 Author: Natalia <[email protected]> Date: Sat Sep 14 21:24:31 2024 +0200 fix merge conflicts, (mostly) move over to feature flags commit 2734b86 Author: peony <[email protected]> Date: Tue Jul 9 11:27:55 2024 +0200 make lax cookies strict again commit e585a31 Author: peony <[email protected]> Date: Tue Jul 9 11:25:07 2024 +0200 minor fixes commit 0abfc86 Author: peony <[email protected]> Date: Sun Jul 7 23:01:06 2024 +0200 remove unnecessary endpoints commit e334894 Merge: 8d44dd9 00a6d38 Author: peony <[email protected]> Date: Sun Jul 7 22:55:28 2024 +0200 Merge branch 'master' into master commit 8d44dd9 Author: peony <[email protected]> Date: Sun Jul 7 22:53:39 2024 +0200 account linking commit d91dfe1 Author: peony <[email protected]> Date: Mon May 20 23:35:00 2024 +0200 code cleanup, add explanation for possibly questionable code commit da39d62 Author: peony <[email protected]> Date: Mon May 20 23:20:23 2024 +0200 google oauth2 stub, nullable password migration Signed-off-by: stadust <[email protected]>

stadust · 2025-03-09T19:49:33Z

Superceded by #229

stadust force-pushed the oauth branch 3 times, most recently from 9eac557 to bfd0e97 Compare October 5, 2024 10:24

stadust commented Oct 5, 2024

View reviewed changes

stadust force-pushed the oauth branch 2 times, most recently from c8d65f1 to b8fadcb Compare October 5, 2024 11:47

stadust commented Oct 5, 2024

View reviewed changes

fix: enable legacy_accounts feature right in pointercrate-example

e790392

It needs to be set for both the API and the pages crates. Otherwise, the frontend will just receive a 404 when registering. Signed-off-by: stadust <[email protected]>

stadust force-pushed the oauth branch 3 times, most recently from a412629 to 65b9b93 Compare October 5, 2024 15:58

stadust commented Oct 5, 2024

View reviewed changes

stadust force-pushed the oauth branch from 65b9b93 to e4a1696 Compare October 5, 2024 16:07

stadust force-pushed the oauth branch 3 times, most recently from c9b50ac to 3e1b0d2 Compare October 5, 2024 20:21

stadust added 3 commits October 5, 2024 21:23

Drop iat field from CSRF tokens

f23ddee

We were not doing anything with it anyway, and the check of the `exp` field is completely independent of it. Signed-off-by: stadust <[email protected]>

stadust force-pushed the oauth branch 3 times, most recently from e1968ce to 932e139 Compare October 6, 2024 10:34

stadust force-pushed the oauth branch from 932e139 to e3a689e Compare October 6, 2024 11:38

stadust force-pushed the master branch 3 times, most recently from 62df64c to be0d5b7 Compare March 8, 2025 14:45

stadust closed this Mar 9, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Oauth Account System #182

Oauth Account System #182

Uh oh!

stadust commented Oct 5, 2024

Uh oh!

stadust commented Oct 5, 2024

Uh oh!

stadust Oct 5, 2024

Uh oh!

stadust Oct 5, 2024

Uh oh!

stadust Oct 5, 2024

Uh oh!

stadust Oct 5, 2024

Uh oh!

codecov bot commented Oct 5, 2024 •

edited

Loading

Uh oh!

stadust commented Mar 9, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Oauth Account System #182

Oauth Account System #182

Uh oh!

Conversation

stadust commented Oct 5, 2024

License Acceptance

Uh oh!

stadust commented Oct 5, 2024

Uh oh!

stadust Oct 5, 2024

Choose a reason for hiding this comment

Uh oh!

stadust Oct 5, 2024

Choose a reason for hiding this comment

Uh oh!

stadust Oct 5, 2024

Choose a reason for hiding this comment

Uh oh!

stadust Oct 5, 2024

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Oct 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

stadust commented Mar 9, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

codecov bot commented Oct 5, 2024 •

edited

Loading