Add Google OAuth2 Integration #135

peonii · 2024-05-20T21:39:36Z

Google OAuth2 Integration

Implements integration from issue #127.

To-do

Sign in flow
Registration flow
Support for null password fields
Integration with existing auth system (token creation)
Account linking
[X] YouTube channel linking - This will be implemented in a future PR, right now it's just the barebones to get the system going

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.

peonii · 2024-07-07T20:57:10Z

@stadust there's one issue here, maybe you'll have some idea on how to resolve this;
when logging via google to an account that's been linked after previously being created with the register endpoint, the signature of the JWT is invalid

stadust

Thank you so much for working on this!!

I've left a bunch of random comments (didn't really go through everything in depth because I have to go to work now 😭). I don't really have an immediate idea why the access tokens have invalid signatures, I'll need to actually play around with it myself a bit I'm afraid :(

Just to clarify though, you don't mean that access tokens from before linking become invalid after linking an account, right (e.g. logging in again fixes it)?

pointercrate-user/src/auth/get.rs

migrations/20240520201641_remove_password_not_null_constraint.down.sql

pointercrate-user/src/auth/patch.rs

pointercrate-user/src/auth/get.rs

pointercrate-user-api/src/ratelimits.rs

pointercrate-user-api/src/pages.rs

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

peonii · 2024-07-09T09:34:31Z

okay Another Problem has arisen
do we force the user to go through the oauth2 flow again each time they'd like to make any change on their profile or do we trust the access token!

for applications that have an oauth2 login flow (im using Notion as a reference) they usually just trust the token and let it do destructive changes but im asking before we implement anything

stadust · 2024-07-09T17:06:08Z

okay Another Problem has arisen do we force the user to go through the oauth2 flow again each time they'd like to make any change on their profile or do we trust the access token!

for applications that have an oauth2 login flow (im using Notion as a reference) they usually just trust the token and let it do destructive changes but im asking before we implement anything

The current system trusts the access token instead of requiring the user to re-enter their password for everything (with few exceptions, such as changing the password), so I think keeping it like that is fine

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

stadust · 2024-07-09T17:30:54Z

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

+    }
+
+    let redirect_uri = "https://accounts.google.com/o/oauth2/v2/auth".to_string()
+        + format!("?client_id={}", std::env::var("GOOGLE_CLIENT_ID").unwrap()).as_str()


config.rs thingy here as well :)

stadust · 2024-07-09T17:31:39Z

pointercrate-user-pages/src/account/profile.rs

                        input.button.red.hover #delete-account type = "button" style = "margin: 15px auto 0px;" value="Delete My Account";
-                        input.button.blue.hover #change-password type = "button" style = "margin: 15px auto 0px;" value="Change Password";
+                        @if !authenticated_user.is_google_linked() {
+                            input.button.blue.hover #change-password type = "button" style = "margin: 15px auto 0px;" value="Change Password";


"Change Password" seems to be the wrong text here. Maybe "Link with google"?

stadust · 2024-07-09T17:32:38Z

pointercrate-user-pages/src/login.rs

-                        div.grow {}
-                        input.button.blue.hover type = "submit" style = "margin: 15px auto 0px;" value = "Register";
+                    form.flex.col.grow #google-form novalidate = "" {
+                        input.button.blue.hover type = "submit" style = "margin: 15px auto 0px;" value = "Log in with Google";


Mhh, I was hoping we could use the buttons from the google SDK here instead of our own

stadust · 2024-07-09T17:35:59Z

pointercrate-user/src/auth/get.rs

+                    // This will never conflict with an existing user
+                    // According to Google, the account ID is always unique
+                    // https://developers.google.com/identity/openid-connect/openid-connect#an-id-tokens-payload
+                    let name = format!("{}#{}", user_info.name, user_info.id);
+
+                    let id = sqlx::query!(
+                        "INSERT INTO
+                            members (email_address, name, display_name, google_account_id)
+                        VALUES
+                            (($1::text)::email, $2, $3, $4) RETURNING member_id
+                        ",
+                        user_info.email,
+                        name,


nit: maybe do the concatenation in the query?

Suggested change

// This will never conflict with an existing user

// According to Google, the account ID is always unique

// https://developers.google.com/identity/openid-connect/openid-connect#an-id-tokens-payload

let name = format!("{}#{}", user_info.name, user_info.id);

let id = sqlx::query!(

"INSERT INTO

members (email_address, name, display_name, google_account_id)

VALUES

(($1::text)::email, $2, $3, $4) RETURNING member_id

",

user_info.email,

name,

// This will never conflict with an existing user

// According to Google, the account ID is always unique

// https://developers.google.com/identity/openid-connect/openid-connect#an-id-tokens-payload

let id = sqlx::query!(

"INSERT INTO

members (email_address, name, display_name, google_account_id)

VALUES

(($1::text)::email, $2 || $3, $4, $5) RETURNING member_id

",

user_info.email,

user_info.name,

user_info.id,

stadust · 2024-07-09T17:37:23Z

pointercrate-user/src/auth/mod.rs

+        if self.password_hash.is_none() {
+            return Vec::new();
+        }
+
+        let raw_parts: Vec<_> = self.password_hash.as_ref().unwrap().split('$').filter(|s| !s.is_empty()).collect();


let's use match instead of unwrap here:

Suggested change

if self.password_hash.is_none() {

return Vec::new();

}

let raw_parts: Vec<_> = self.password_hash.as_ref().unwrap().split('$').filter(|s| !s.is_empty()).collect();

let raw_parts = match self.password_hash {

None => return Vec::new(),

Some(ref hash) => hash.split('$').filter(|s| !s.is_empty()).collect();

};

stadust · 2024-07-09T17:39:41Z

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);
+    }


Instead of using a cookie, can we somehow give this information to google, and have google return it to us when it calls our callback? Maybe via query parameter?

stadust · 2024-07-09T17:46:21Z

pointercrate-user/src/auth/get.rs

+                google_account_id: Some(user_info.id),
+                email_address: Some(user_info.email),
+            }),
+            Err(UserError::UserNotFoundGoogleAccount { .. }) => {


Let's factor this entire match arm into some more functions:

one for updating the google account id of an existing user (can probably go into auth/patch.rs)

one for creating a new user account from a google account (maybe into auth/post.rs?)

stadust · 2024-07-09T17:47:30Z

pointercrate-user/src/error.rs

+    #[display(fmt = "The user is already linked to a Google account")]
+    AlreadyLinked,
+
+    #[display(fmt = "This operation is not applicable to this user")]


Let's be a bit more descriptive and have this variant be explicitly about changing passwords on oauth accounts :)

stadust · 2024-07-09T17:54:23Z

pointercrate-user/src/auth/get.rs

+                    }
+
+                    let updated_user = sqlx::query!(
+                        "UPDATE members SET google_account_id = $1, email_address = ($2::text)::email WHERE member_id = $3 RETURNING member_id",


I think here is the problem why this flow gives you an invalid access token: this query is missing a SET password_hash = NULL. Right now in line 126 onward, you're constructing an AuthenticatedUser with password_hash: None. This means that if you call generate_access_token() on the user returned here, it will only sign the token with the server secret. However, the database will still contain the user's old password hash. This means that when it comes to validating the token, it will retrieve an AuthenticatedUser with password_hash = hash of password from before account was linked, and thus it tries to validate the token using a secret that's a combination of server secret and password hash. But that's the wrong secret, because we signed the token with only the server secret!

stadust · 2024-07-09T18:06:40Z

migrations/20240520201323_add_google_account_id.up.sql

+-- Add up migration script here
+
+ALTER TABLE members
+    ADD COLUMN google_account_id VARCHAR(255) NULL;


As per discussion on discord: Let's put a UNIQUE constraint here as a failsafe for raceconditions that allow adding the same google account to multiple pointercrate accounts

stadust · 2024-07-16T17:34:58Z

pointercrate-user/src/error.rs

    UserNotFoundName { user_name: String },

+    #[display(fmt = "No user with google account {} found", google_account_id)]
+    UserNotFoundGoogleAccount { google_account_id: String },


Suggested change

UserNotFoundGoogleAccount { google_account_id: String },

UserNotFoundGoogleAccount { google_account_id: u64 },

Originally, this was intended as a way to reset passwords. However, I could never be bothered to actually set up a mail server. Now, we will instead offer the option to link pointercrate accounts to google accounts in #135, so this half-implemented feature is truly dead. Signed-off-by: stadust <43299462+stadust@users.noreply.github.com>

stadust reviewed Jul 9, 2024

View reviewed changes

stadust mentioned this pull request Jul 12, 2024

Consider adding email verification for password resets #65

Closed

stadust reviewed Jul 16, 2024

View reviewed changes

stadust force-pushed the master branch 2 times, most recently from 1edd5f6 to 22dd991 Compare September 8, 2024 10:34

stadust closed this Oct 5, 2024

stadust force-pushed the master branch from a2861be to 53ce7e8 Compare October 5, 2024 09:26

stadust mentioned this pull request Oct 5, 2024

Oauth Account System #182

Closed

	UserNotFoundGoogleAccount { google_account_id: String },
	UserNotFoundGoogleAccount { google_account_id: u64 },

Add Google OAuth2 Integration #135

Add Google OAuth2 Integration #135

Uh oh!

Conversation

peonii commented May 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Google OAuth2 Integration

To-do

License Acceptance

Uh oh!

peonii commented Jul 7, 2024

Uh oh!

stadust left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

peonii commented Jul 9, 2024

Uh oh!

stadust commented Jul 9, 2024

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

peonii commented May 20, 2024 •

edited

Loading