From a7b49788f591d0f3c1dd2859ee2107a889317426 Mon Sep 17 00:00:00 2001 From: Jon Ayers Date: Mon, 22 Aug 2022 18:13:46 -0500 Subject: [PATCH] chore: deduplicate OAuth login code (#3575) --- coderd/userauth.go | 480 ++++++++++++++++++---------------------- coderd/userauth_test.go | 9 +- coderd/users.go | 61 ++--- 3 files changed, 252 insertions(+), 298 deletions(-) diff --git a/coderd/userauth.go b/coderd/userauth.go index 58d101671c..ff45846de3 100644 --- a/coderd/userauth.go +++ b/coderd/userauth.go @@ -122,149 +122,46 @@ func (api *API) userOAuth2Github(rw http.ResponseWriter, r *http.Request) { return } - verifiedEmails := make([]string, 0, len(emails)) + var verifiedEmail *github.UserEmail for _, email := range emails { - if !email.GetVerified() { - continue + if email.GetVerified() && email.GetPrimary() { + verifiedEmail = email + break } - verifiedEmails = append(verifiedEmails, email.GetEmail()) } - if len(verifiedEmails) == 0 { - httpapi.Write(rw, http.StatusForbidden, codersdk.Response{ - Message: "Verify an email address on Github to authenticate!", + if verifiedEmail == nil { + httpapi.Write(rw, http.StatusPreconditionRequired, codersdk.Response{ + Message: "Your primary email must be verified on GitHub!", }) return } - user, link, err := findLinkedUser(ctx, api.Database, githubLinkedID(ghUser), verifiedEmails...) + cookie, err := api.oauthLogin(r, oauthLoginParams{ + State: state, + LinkedID: githubLinkedID(ghUser), + LoginType: database.LoginTypeGithub, + AllowSignups: api.GithubOAuth2Config.AllowSignups, + Email: verifiedEmail.GetEmail(), + Username: ghUser.GetLogin(), + }) + var httpErr httpError + if xerrors.As(err, &httpErr) { + httpapi.Write(rw, httpErr.code, codersdk.Response{ + Message: httpErr.msg, + Detail: httpErr.detail, + }) + return + } if err != nil { httpapi.Write(rw, http.StatusInternalServerError, codersdk.Response{ - Message: "An internal error occurred.", + Message: "Failed to process OAuth login.", Detail: err.Error(), }) return } - if user.ID != uuid.Nil && user.LoginType != database.LoginTypeGithub { - httpapi.Write(rw, http.StatusForbidden, codersdk.Response{ - Message: fmt.Sprintf("Incorrect login type, attempting to use %q but user is of login type %q", database.LoginTypeGithub, user.LoginType), - }) - return - } - - // If the user doesn't exist, create a new one! - if user.ID == uuid.Nil { - if !api.GithubOAuth2Config.AllowSignups { - httpapi.Write(rw, http.StatusForbidden, codersdk.Response{ - Message: "Signups are disabled for Github authentication!", - }) - return - } - - var organizationID uuid.UUID - organizations, _ := api.Database.GetOrganizations(r.Context()) - if len(organizations) > 0 { - // Add the user to the first organization. Once multi-organization - // support is added, we should enable a configuration map of user - // email to organization. - organizationID = organizations[0].ID - } - var verifiedEmail *github.UserEmail - for _, email := range emails { - if !email.GetPrimary() || !email.GetVerified() { - continue - } - verifiedEmail = email - break - } - if verifiedEmail == nil { - httpapi.Write(rw, http.StatusPreconditionRequired, codersdk.Response{ - Message: "Your primary email must be verified on GitHub!", - }) - return - } - user, _, err = api.createUser(ctx, createUserRequest{ - CreateUserRequest: codersdk.CreateUserRequest{ - Email: *verifiedEmail.Email, - Username: *ghUser.Login, - OrganizationID: organizationID, - }, - LoginType: database.LoginTypeGithub, - }) - if err != nil { - httpapi.Write(rw, http.StatusInternalServerError, codersdk.Response{ - Message: "Internal error creating user.", - Detail: err.Error(), - }) - return - } - } - - // This can happen if a user is a built-in user but is signing in - // with Github for the first time. - if link.UserID == uuid.Nil { - link, err = api.Database.InsertUserLink(ctx, database.InsertUserLinkParams{ - UserID: user.ID, - LoginType: database.LoginTypeGithub, - LinkedID: githubLinkedID(ghUser), - OAuthAccessToken: state.Token.AccessToken, - OAuthRefreshToken: state.Token.RefreshToken, - OAuthExpiry: state.Token.Expiry, - }) - if err != nil { - httpapi.Write(rw, http.StatusInternalServerError, codersdk.Response{ - Message: "A database error occurred.", - Detail: fmt.Sprintf("insert user link: %s", err.Error()), - }) - return - } - } - - // LEGACY: Remove 10/2022. - // We started tracking linked IDs later so it's possible for a user to be a - // pre-existing Github user and not have a linked ID. The migration - // to user_links did not populate this field as it requires calling out - // to Github to query the user's ID. - if link.LinkedID == "" { - link, err = api.Database.UpdateUserLinkedID(ctx, database.UpdateUserLinkedIDParams{ - UserID: user.ID, - LoginType: database.LoginTypeGithub, - LinkedID: githubLinkedID(ghUser), - }) - if err != nil { - httpapi.Write(rw, http.StatusInternalServerError, codersdk.Response{ - Message: "A database error occurred.", - Detail: fmt.Sprintf("update user link: %s", err.Error()), - }) - return - } - } - - if link.UserID != uuid.Nil { - link, err = api.Database.UpdateUserLink(ctx, database.UpdateUserLinkParams{ - UserID: user.ID, - LoginType: database.LoginTypeGithub, - OAuthAccessToken: state.Token.AccessToken, - OAuthRefreshToken: state.Token.RefreshToken, - OAuthExpiry: state.Token.Expiry, - }) - if err != nil { - httpapi.Write(rw, http.StatusInternalServerError, codersdk.Response{ - Message: "A database error occurred.", - Detail: fmt.Sprintf("update user link: %s", err.Error()), - }) - return - } - } - - _, created := api.createAPIKey(rw, r, createAPIKeyParams{ - UserID: user.ID, - LoginType: database.LoginTypeGithub, - }) - if !created { - return - } + http.SetCookie(rw, cookie) redirect := state.Redirect if redirect == "" { @@ -352,153 +249,31 @@ func (api *API) userOIDC(rw http.ResponseWriter, r *http.Request) { } } - user, link, err := findLinkedUser(ctx, api.Database, oidcLinkedID(idToken), claims.Email) + cookie, err := api.oauthLogin(r, oauthLoginParams{ + State: state, + LinkedID: oidcLinkedID(idToken), + LoginType: database.LoginTypeOIDC, + AllowSignups: api.OIDCConfig.AllowSignups, + Email: claims.Email, + Username: claims.Username, + }) + var httpErr httpError + if xerrors.As(err, &httpErr) { + httpapi.Write(rw, httpErr.code, codersdk.Response{ + Message: httpErr.msg, + Detail: httpErr.detail, + }) + return + } if err != nil { httpapi.Write(rw, http.StatusInternalServerError, codersdk.Response{ - Message: "Failed to find user.", + Message: "Failed to process OAuth login.", Detail: err.Error(), }) return } - if user.ID == uuid.Nil && !api.OIDCConfig.AllowSignups { - httpapi.Write(rw, http.StatusForbidden, codersdk.Response{ - Message: "Signups are disabled for OIDC authentication!", - }) - return - } - - if user.ID != uuid.Nil && user.LoginType != database.LoginTypeOIDC { - httpapi.Write(rw, http.StatusForbidden, codersdk.Response{ - Message: fmt.Sprintf("Incorrect login type, attempting to use %q but user is of login type %q", database.LoginTypeOIDC, user.LoginType), - }) - return - } - - // This can happen if a user is a built-in user but is signing in - // with OIDC for the first time. - if user.ID == uuid.Nil { - var organizationID uuid.UUID - organizations, _ := api.Database.GetOrganizations(ctx) - if len(organizations) > 0 { - // Add the user to the first organization. Once multi-organization - // support is added, we should enable a configuration map of user - // email to organization. - organizationID = organizations[0].ID - } - - user, _, err = api.createUser(ctx, createUserRequest{ - CreateUserRequest: codersdk.CreateUserRequest{ - Email: claims.Email, - Username: claims.Username, - OrganizationID: organizationID, - }, - LoginType: database.LoginTypeOIDC, - }) - if err != nil { - httpapi.Write(rw, http.StatusInternalServerError, codersdk.Response{ - Message: "Internal error creating user.", - Detail: err.Error(), - }) - return - } - if err != nil { - httpapi.Write(rw, http.StatusInternalServerError, codersdk.Response{ - Message: "Failed to insert user auth metadata.", - Detail: err.Error(), - }) - return - } - } - - if link.UserID == uuid.Nil { - link, err = api.Database.InsertUserLink(ctx, database.InsertUserLinkParams{ - UserID: user.ID, - LoginType: database.LoginTypeOIDC, - LinkedID: oidcLinkedID(idToken), - OAuthAccessToken: state.Token.AccessToken, - OAuthRefreshToken: state.Token.RefreshToken, - OAuthExpiry: state.Token.Expiry, - }) - if err != nil { - httpapi.Write(rw, http.StatusInternalServerError, codersdk.Response{ - Message: "A database error occurred.", - Detail: fmt.Sprintf("insert user link: %s", err.Error()), - }) - return - } - } - - // LEGACY: Remove 10/2022. - // We started tracking linked IDs later so it's possible for a user to be a - // pre-existing OIDC user and not have a linked ID. - // The migration that added the user_links table could not populate - // the 'linked_id' field since it requires fields off the access token. - if link.LinkedID == "" { - link, err = api.Database.UpdateUserLinkedID(ctx, database.UpdateUserLinkedIDParams{ - UserID: user.ID, - LoginType: database.LoginTypeOIDC, - LinkedID: oidcLinkedID(idToken), - }) - if err != nil { - httpapi.Write(rw, http.StatusInternalServerError, codersdk.Response{ - Message: "A database error occurred.", - Detail: fmt.Sprintf("update user link: %s", err.Error()), - }) - return - } - } - - if link.UserID != uuid.Nil { - link, err = api.Database.UpdateUserLink(ctx, database.UpdateUserLinkParams{ - UserID: user.ID, - LoginType: database.LoginTypeOIDC, - OAuthAccessToken: state.Token.AccessToken, - OAuthRefreshToken: state.Token.RefreshToken, - OAuthExpiry: state.Token.Expiry, - }) - if err != nil { - httpapi.Write(rw, http.StatusInternalServerError, codersdk.Response{ - Message: "A database error occurred.", - Detail: fmt.Sprintf("update user link: %s", err.Error()), - }) - return - } - } - - // If the upstream email or username has changed we should mirror - // that in Coder. Many enterprises use a user's email/username as - // security auditing fields so they need to stay synced. - if user.Email != claims.Email || user.Username != claims.Username { - // TODO(JonA): Since we're processing updates to a user's upstream - // email/username, it's possible for a different built-in user to - // have already claimed the username. - // In such cases in the current implementation this user can now no - // longer sign in until an administrator finds the offending built-in - // user and changes their username. - user, err = api.Database.UpdateUserProfile(ctx, database.UpdateUserProfileParams{ - ID: user.ID, - Email: claims.Email, - // TODO: This should run in a transaction. - Username: user.Username, - UpdatedAt: database.Now(), - }) - if err != nil { - httpapi.Write(rw, http.StatusInternalServerError, codersdk.Response{ - Message: "Failed to update user profile.", - Detail: fmt.Sprintf("update user profile: %s", err.Error()), - }) - return - } - } - - _, created := api.createAPIKey(rw, r, createAPIKeyParams{ - UserID: user.ID, - LoginType: database.LoginTypeOIDC, - }) - if !created { - return - } + http.SetCookie(rw, cookie) redirect := state.Redirect if redirect == "" { @@ -507,6 +282,175 @@ func (api *API) userOIDC(rw http.ResponseWriter, r *http.Request) { http.Redirect(rw, r, redirect, http.StatusTemporaryRedirect) } +type oauthLoginParams struct { + State httpmw.OAuth2State + LinkedID string + LoginType database.LoginType + + // The following are necessary in order to + // create new users. + AllowSignups bool + Email string + Username string +} + +type httpError struct { + code int + msg string + detail string +} + +func (e httpError) Error() string { + if e.detail != "" { + return e.detail + } + + return e.msg +} + +func (api *API) oauthLogin(r *http.Request, params oauthLoginParams) (*http.Cookie, error) { + var ( + ctx = r.Context() + user database.User + ) + + err := api.Database.InTx(func(tx database.Store) error { + var ( + link database.UserLink + err error + ) + + user, link, err = findLinkedUser(ctx, tx, params.LinkedID, params.Email) + if err != nil { + return xerrors.Errorf("find linked user: %w", err) + } + + if user.ID == uuid.Nil && !params.AllowSignups { + return httpError{ + code: http.StatusForbidden, + msg: fmt.Sprintf("Signups are not allowed for login type %q", params.LoginType), + } + } + + if user.ID != uuid.Nil && user.LoginType != params.LoginType { + return httpError{ + code: http.StatusForbidden, + msg: fmt.Sprintf("Incorrect login type, attempting to use %q but user is of login type %q", + params.LoginType, + user.LoginType, + ), + } + } + + // This can happen if a user is a built-in user but is signing in + // with OIDC for the first time. + if user.ID == uuid.Nil { + var organizationID uuid.UUID + organizations, _ := tx.GetOrganizations(ctx) + if len(organizations) > 0 { + // Add the user to the first organization. Once multi-organization + // support is added, we should enable a configuration map of user + // email to organization. + organizationID = organizations[0].ID + } + + user, _, err = api.createUser(ctx, tx, createUserRequest{ + CreateUserRequest: codersdk.CreateUserRequest{ + Email: params.Email, + Username: params.Username, + OrganizationID: organizationID, + }, + LoginType: database.LoginTypeOIDC, + }) + if err != nil { + return xerrors.Errorf("create user: %w", err) + } + } + + if link.UserID == uuid.Nil { + link, err = tx.InsertUserLink(ctx, database.InsertUserLinkParams{ + UserID: user.ID, + LoginType: params.LoginType, + LinkedID: params.LinkedID, + OAuthAccessToken: params.State.Token.AccessToken, + OAuthRefreshToken: params.State.Token.RefreshToken, + OAuthExpiry: params.State.Token.Expiry, + }) + if err != nil { + return xerrors.Errorf("insert user link: %w", err) + } + } + + // LEGACY: Remove 10/2022. + // We started tracking linked IDs later so it's possible for a user to be a + // pre-existing OAuth user and not have a linked ID. + // The migration that added the user_links table could not populate + // the 'linked_id' field since it requires fields off the access token. + if link.LinkedID == "" { + link, err = tx.UpdateUserLinkedID(ctx, database.UpdateUserLinkedIDParams{ + UserID: user.ID, + LoginType: params.LoginType, + LinkedID: params.LinkedID, + }) + if err != nil { + return xerrors.Errorf("update user linked ID: %w", err) + } + } + + if link.UserID != uuid.Nil { + link, err = tx.UpdateUserLink(ctx, database.UpdateUserLinkParams{ + UserID: user.ID, + LoginType: params.LoginType, + OAuthAccessToken: params.State.Token.AccessToken, + OAuthRefreshToken: params.State.Token.RefreshToken, + OAuthExpiry: params.State.Token.Expiry, + }) + if err != nil { + return xerrors.Errorf("update user link: %w", err) + } + } + + // If the upstream email or username has changed we should mirror + // that in Coder. Many enterprises use a user's email/username as + // security auditing fields so they need to stay synced. + // NOTE: username updating has been halted since it can have infrastructure + // provisioning consequences (updates to usernames may delete persistent + // resources such as user home volumes). + if user.Email != params.Email { + // TODO(JonA): Since we're processing updates to a user's upstream + // email/username, it's possible for a different built-in user to + // have already claimed the username. + // In such cases in the current implementation this user can now no + // longer sign in until an administrator finds the offending built-in + // user and changes their username. + user, err = tx.UpdateUserProfile(ctx, database.UpdateUserProfileParams{ + ID: user.ID, + Email: params.Email, + Username: user.Username, + UpdatedAt: database.Now(), + }) + if err != nil { + return xerrors.Errorf("update user profile: %w", err) + } + } + + return nil + }) + if err != nil { + return nil, xerrors.Errorf("in tx: %w", err) + } + + cookie, err := api.createAPIKey(r, createAPIKeyParams{ + UserID: user.ID, + LoginType: params.LoginType, + }) + if err != nil { + return nil, xerrors.Errorf("create API key: %w", err) + } + + return cookie, nil +} + // githubLinkedID returns the unique ID for a GitHub user. func githubLinkedID(u *github.User) string { return strconv.FormatInt(u.GetID(), 10) diff --git a/coderd/userauth_test.go b/coderd/userauth_test.go index a1e850a090..59f8c1d5e0 100644 --- a/coderd/userauth_test.go +++ b/coderd/userauth_test.go @@ -150,7 +150,7 @@ func TestUserOAuth2Github(t *testing.T) { }) _ = coderdtest.CreateFirstUser(t, client) resp := oauth2Callback(t, client) - require.Equal(t, http.StatusForbidden, resp.StatusCode) + require.Equal(t, http.StatusPreconditionRequired, resp.StatusCode) }) t.Run("BlockSignups", func(t *testing.T) { t.Parallel() @@ -169,7 +169,11 @@ func TestUserOAuth2Github(t *testing.T) { return &github.User{}, nil }, ListEmails: func(ctx context.Context, client *http.Client) ([]*github.UserEmail, error) { - return []*github.UserEmail{}, nil + return []*github.UserEmail{{ + Email: github.String("testuser@coder.com"), + Verified: github.Bool(true), + Primary: github.Bool(true), + }}, nil }, }, }) @@ -196,6 +200,7 @@ func TestUserOAuth2Github(t *testing.T) { return []*github.UserEmail{{ Email: github.String("testuser@coder.com"), Verified: github.Bool(true), + Primary: github.Bool(true), }}, nil }, }, diff --git a/coderd/users.go b/coderd/users.go index e77364eeba..d3460631a9 100644 --- a/coderd/users.go +++ b/coderd/users.go @@ -77,7 +77,7 @@ func (api *API) postFirstUser(rw http.ResponseWriter, r *http.Request) { return } - user, organizationID, err := api.createUser(r.Context(), createUserRequest{ + user, organizationID, err := api.createUser(r.Context(), api.Database, createUserRequest{ CreateUserRequest: codersdk.CreateUserRequest{ Email: createUser.Email, Username: createUser.Username, @@ -246,7 +246,7 @@ func (api *API) postUser(rw http.ResponseWriter, r *http.Request) { return } - user, _, err := api.createUser(r.Context(), createUserRequest{ + user, _, err := api.createUser(r.Context(), api.Database, createUserRequest{ CreateUserRequest: req, LoginType: database.LoginTypePassword, }) @@ -722,16 +722,22 @@ func (api *API) postLogin(rw http.ResponseWriter, r *http.Request) { return } - sessionToken, created := api.createAPIKey(rw, r, createAPIKeyParams{ + cookie, err := api.createAPIKey(r, createAPIKeyParams{ UserID: user.ID, LoginType: database.LoginTypePassword, }) - if !created { + if err != nil { + httpapi.Write(rw, http.StatusInternalServerError, codersdk.Response{ + Message: "Failed to create API key.", + Detail: err.Error(), + }) return } + http.SetCookie(rw, cookie) + httpapi.Write(rw, http.StatusCreated, codersdk.LoginWithPasswordResponse{ - SessionToken: sessionToken, + SessionToken: cookie.Value, }) } @@ -745,7 +751,7 @@ func (api *API) postAPIKey(rw http.ResponseWriter, r *http.Request) { } lifeTime := time.Hour * 24 * 7 - sessionToken, created := api.createAPIKey(rw, r, createAPIKeyParams{ + cookie, err := api.createAPIKey(r, createAPIKeyParams{ UserID: user.ID, LoginType: database.LoginTypePassword, // All api generated keys will last 1 week. Browser login tokens have @@ -753,11 +759,19 @@ func (api *API) postAPIKey(rw http.ResponseWriter, r *http.Request) { ExpiresAt: database.Now().Add(lifeTime), LifetimeSeconds: int64(lifeTime.Seconds()), }) - if !created { + if err != nil { + httpapi.Write(rw, http.StatusInternalServerError, codersdk.Response{ + Message: "Failed to create API key.", + Detail: err.Error(), + }) return } - httpapi.Write(rw, http.StatusCreated, codersdk.GenerateAPIKeyResponse{Key: sessionToken}) + // We intentionally do not set the cookie on the response here. + // Setting the cookie will couple the browser sesion to the API + // key we return here, meaning logging out of the website would + // invalid your CLI key. + httpapi.Write(rw, http.StatusCreated, codersdk.GenerateAPIKeyResponse{Key: cookie.Value}) } func (api *API) apiKey(rw http.ResponseWriter, r *http.Request) { @@ -840,14 +854,10 @@ type createAPIKeyParams struct { LifetimeSeconds int64 } -func (api *API) createAPIKey(rw http.ResponseWriter, r *http.Request, params createAPIKeyParams) (string, bool) { +func (api *API) createAPIKey(r *http.Request, params createAPIKeyParams) (*http.Cookie, error) { keyID, keySecret, err := generateAPIKeyIDSecret() if err != nil { - httpapi.Write(rw, http.StatusInternalServerError, codersdk.Response{ - Message: "Internal error generating API key.", - Detail: err.Error(), - }) - return "", false + return nil, xerrors.Errorf("generate API key: %w", err) } hashed := sha256.Sum256([]byte(keySecret)) @@ -885,11 +895,7 @@ func (api *API) createAPIKey(rw http.ResponseWriter, r *http.Request, params cre LoginType: params.LoginType, }) if err != nil { - httpapi.Write(rw, http.StatusInternalServerError, codersdk.Response{ - Message: "Internal error inserting API key.", - Detail: err.Error(), - }) - return "", false + return nil, xerrors.Errorf("insert API key: %w", err) } api.Telemetry.Report(&telemetry.Snapshot{ @@ -898,15 +904,14 @@ func (api *API) createAPIKey(rw http.ResponseWriter, r *http.Request, params cre // This format is consumed by the APIKey middleware. sessionToken := fmt.Sprintf("%s-%s", keyID, keySecret) - http.SetCookie(rw, &http.Cookie{ + return &http.Cookie{ Name: codersdk.SessionTokenKey, Value: sessionToken, Path: "/", HttpOnly: true, SameSite: http.SameSiteLaxMode, Secure: api.SecureAuthCookie, - }) - return sessionToken, true + }, nil } type createUserRequest struct { @@ -914,13 +919,13 @@ type createUserRequest struct { LoginType database.LoginType } -func (api *API) createUser(ctx context.Context, req createUserRequest) (database.User, uuid.UUID, error) { +func (api *API) createUser(ctx context.Context, store database.Store, req createUserRequest) (database.User, uuid.UUID, error) { var user database.User - return user, req.OrganizationID, api.Database.InTx(func(db database.Store) error { + return user, req.OrganizationID, store.InTx(func(tx database.Store) error { orgRoles := make([]string, 0) // If no organization is provided, create a new one for the user. if req.OrganizationID == uuid.Nil { - organization, err := db.InsertOrganization(ctx, database.InsertOrganizationParams{ + organization, err := tx.InsertOrganization(ctx, database.InsertOrganizationParams{ ID: uuid.New(), Name: req.Username, CreatedAt: database.Now(), @@ -953,7 +958,7 @@ func (api *API) createUser(ctx context.Context, req createUserRequest) (database } var err error - user, err = db.InsertUser(ctx, params) + user, err = tx.InsertUser(ctx, params) if err != nil { return xerrors.Errorf("create user: %w", err) } @@ -962,7 +967,7 @@ func (api *API) createUser(ctx context.Context, req createUserRequest) (database if err != nil { return xerrors.Errorf("generate user gitsshkey: %w", err) } - _, err = db.InsertGitSSHKey(ctx, database.InsertGitSSHKeyParams{ + _, err = tx.InsertGitSSHKey(ctx, database.InsertGitSSHKeyParams{ UserID: user.ID, CreatedAt: database.Now(), UpdatedAt: database.Now(), @@ -972,7 +977,7 @@ func (api *API) createUser(ctx context.Context, req createUserRequest) (database if err != nil { return xerrors.Errorf("insert user gitsshkey: %w", err) } - _, err = db.InsertOrganizationMember(ctx, database.InsertOrganizationMemberParams{ + _, err = tx.InsertOrganizationMember(ctx, database.InsertOrganizationMemberParams{ OrganizationID: req.OrganizationID, UserID: user.ID, CreatedAt: database.Now(),