diff --git a/coderd/apidoc/docs.go b/coderd/apidoc/docs.go index 9676e20e4fa3f..a0c5ef30649a3 100644 --- a/coderd/apidoc/docs.go +++ b/coderd/apidoc/docs.go @@ -12224,8 +12224,8 @@ const docTemplate = `{ "tags": [ "Authorization" ], - "summary": "Convert user from password to oauth authentication", - "operationId": "convert-user-from-password-to-oauth-authentication", + "summary": "Convert user to oauth authentication", + "operationId": "convert-user-to-oauth-authentication", "parameters": [ { "description": "Convert request", @@ -21432,11 +21432,11 @@ const docTemplate = `{ "codersdk.ConvertLoginRequest": { "type": "object", "required": [ - "password", "to_type" ], "properties": { "password": { + "description": "Password is required for password-authenticated accounts.", "type": "string" }, "to_type": { diff --git a/coderd/apidoc/swagger.json b/coderd/apidoc/swagger.json index 5bd07bf91c4c5..7661218955ed3 100644 --- a/coderd/apidoc/swagger.json +++ b/coderd/apidoc/swagger.json @@ -10853,8 +10853,8 @@ "consumes": ["application/json"], "produces": ["application/json"], "tags": ["Authorization"], - "summary": "Convert user from password to oauth authentication", - "operationId": "convert-user-from-password-to-oauth-authentication", + "summary": "Convert user to oauth authentication", + "operationId": "convert-user-to-oauth-authentication", "parameters": [ { "description": "Convert request", @@ -19414,9 +19414,10 @@ }, "codersdk.ConvertLoginRequest": { "type": "object", - "required": ["password", "to_type"], + "required": ["to_type"], "properties": { "password": { + "description": "Password is required for password-authenticated accounts.", "type": "string" }, "to_type": { diff --git a/coderd/coderdtest/oidctest/idp.go b/coderd/coderdtest/oidctest/idp.go index 5a1fb7a5055f2..66605b187b6ec 100644 --- a/coderd/coderdtest/oidctest/idp.go +++ b/coderd/coderdtest/oidctest/idp.go @@ -1309,7 +1309,7 @@ func (f *FakeIDP) httpHandler(t testing.TB) http.Handler { { Key: f.locked.PrivateKey().Public(), KeyID: "test-key", - Algorithm: "RSA", + Algorithm: "RS256", }, }, } diff --git a/coderd/coderdtest/oidctest/idp_test.go b/coderd/coderdtest/oidctest/idp_test.go index 622dd7b013747..825ab6ba815ec 100644 --- a/coderd/coderdtest/oidctest/idp_test.go +++ b/coderd/coderdtest/oidctest/idp_test.go @@ -68,6 +68,26 @@ func TestFakeIDPBasicFlow(t *testing.T) { require.NotEmpty(t, refreshed.AccessToken, "access token is empty on refresh") } +func TestFakeIDPRemoteKeySet(t *testing.T) { + t.Parallel() + + fake := oidctest.NewFakeIDP(t, oidctest.WithServing()) + oauthConfig := fake.OauthConfig(t, nil) + provider, err := oidc.NewProvider(context.Background(), fake.IssuerURL().String()) + require.NoError(t, err) + + token, err := fake.GenerateAuthenticatedToken(jwt.MapClaims{"sub": "test-subject"}) + require.NoError(t, err) + idToken, ok := token.Extra("id_token").(string) + require.True(t, ok) + + _, err = provider.Verifier(&oidc.Config{ + ClientID: oauthConfig.ClientID, + SupportedSigningAlgs: []string{"RS256"}, + }).Verify(context.Background(), idToken) + require.NoError(t, err) +} + // TestIDPIssuerMismatch emulates a situation where the IDP issuer url does // not match the one in the well-known config and claims. // This can happen in some edge cases and in some azure configurations. diff --git a/coderd/database/dbauthz/dbauthz.go b/coderd/database/dbauthz/dbauthz.go index 3bb40f84678c9..6fee6a9a31035 100644 --- a/coderd/database/dbauthz/dbauthz.go +++ b/coderd/database/dbauthz/dbauthz.go @@ -518,6 +518,26 @@ var ( }.WithCachedASTValue() } + subjectLoginTypeConverter = func(userID uuid.UUID) rbac.Subject { + return rbac.Subject{ + Type: rbac.SubjectTypeLoginTypeConverter, + FriendlyName: "Login Type Converter", + ID: userID.String(), + Roles: rbac.Roles([]rbac.Role{ + { + Identifier: rbac.RoleIdentifier{Name: "logintypeconverter"}, + DisplayName: "Login Type Converter", + Site: []rbac.Permission{}, + User: rbac.Permissions(map[string][]policy.Action{ + rbac.ResourceUser.Type: {policy.ActionRead, policy.ActionUpdatePersonal}, + }), + ByOrgID: map[string]rbac.OrgPermissions{}, + }, + }), + Scope: rbac.ScopeAll, + }.WithCachedASTValue() + } + subjectSystemRestricted = rbac.Subject{ Type: rbac.SubjectTypeSystemRestricted, FriendlyName: "System", @@ -968,6 +988,12 @@ func AsChatdTokenOwner(ctx context.Context, userID uuid.UUID) context.Context { return As(ctx, subjectChatdTokenOwner(userID)) } +// AsLoginTypeConverter returns a context scoped to the specified user's login +// type conversion. +func AsLoginTypeConverter(ctx context.Context, userID uuid.UUID) context.Context { + return As(ctx, subjectLoginTypeConverter(userID)) +} + // AsSystemRestricted returns a context with an actor that has permissions // required for various system operations (login, logout, metrics cache). // DO NOT USE THIS UNLESS YOU HAVE ABSOLUTELY NO OTHER CHOICE. Prefer using a @@ -8474,7 +8500,7 @@ func (q *querier) UpdateUserLinkedID(ctx context.Context, arg database.UpdateUse } func (q *querier) UpdateUserLoginType(ctx context.Context, arg database.UpdateUserLoginTypeParams) (database.User, error) { - if err := q.authorizeContext(ctx, policy.ActionUpdate, rbac.ResourceSystem); err != nil { + if err := q.authorizeContext(ctx, policy.ActionUpdatePersonal, rbac.ResourceUserObject(arg.UserID)); err != nil { return database.User{}, err } return q.db.UpdateUserLoginType(ctx, arg) diff --git a/coderd/database/dbauthz/dbauthz_test.go b/coderd/database/dbauthz/dbauthz_test.go index 0da0753cdb95a..c42114c935e76 100644 --- a/coderd/database/dbauthz/dbauthz_test.go +++ b/coderd/database/dbauthz/dbauthz_test.go @@ -5539,7 +5539,7 @@ func (s *MethodTestSuite) TestSystemFunctions() { u := testutil.Fake(s.T(), faker, database.User{}) arg := database.UpdateUserLoginTypeParams{NewLoginType: database.LoginTypePassword, UserID: u.ID} dbm.EXPECT().UpdateUserLoginType(gomock.Any(), arg).Return(testutil.Fake(s.T(), faker, database.User{}), nil).AnyTimes() - check.Args(arg).Asserts(rbac.ResourceSystem, policy.ActionUpdate) + check.Args(arg).Asserts(rbac.ResourceUserObject(arg.UserID), policy.ActionUpdatePersonal) })) s.Run("GetWorkspaceAgentStatsAndLabels", s.Mocked(func(dbm *dbmock.MockStore, _ *gofakeit.Faker, check *expects) { t := time.Time{} @@ -7762,6 +7762,33 @@ func TestAsAPIKeyRevoker(t *testing.T) { }) } +func TestAsLoginTypeConverter(t *testing.T) { + t.Parallel() + + userID := uuid.New() + otherUserID := uuid.New() + ctx := dbauthz.AsLoginTypeConverter(context.Background(), userID) + actor, ok := dbauthz.ActorFromContext(ctx) + require.True(t, ok, "actor must be present") + require.Equal(t, rbac.SubjectTypeLoginTypeConverter, actor.Type) + require.Equal(t, userID.String(), actor.ID) + + auth := rbac.NewStrictCachingAuthorizer(prometheus.NewRegistry()) + for _, action := range rbac.ResourceUser.AvailableActions() { + err := auth.Authorize(ctx, actor, action, rbac.ResourceUserObject(userID)) + if action == policy.ActionRead || action == policy.ActionUpdatePersonal { + require.NoError(t, err, "own user should allow %s", action) + continue + } + require.Error(t, err, "own user should deny %s", action) + } + + for _, action := range []policy.Action{policy.ActionRead, policy.ActionUpdatePersonal} { + err := auth.Authorize(ctx, actor, action, rbac.ResourceUserObject(otherUserID)) + require.Error(t, err, "other user should deny %s", action) + } +} + func TestAsChatdKeyMinter(t *testing.T) { t.Parallel() diff --git a/coderd/rbac/authz.go b/coderd/rbac/authz.go index 4ea4eadd55da3..558b2f1b48f6c 100644 --- a/coderd/rbac/authz.go +++ b/coderd/rbac/authz.go @@ -78,6 +78,7 @@ const ( SubjectTypeAPIKeyRevoker SubjectType = "api_key_revoker" // #nosec G101, not a credential. SubjectTypeChatdKeyMinter SubjectType = "chatd_key_minter" // #nosec G101, not a credential. SubjectTypeChatdTokenOwner SubjectType = "chatd_token_owner" // #nosec G101, not a credential. + SubjectTypeLoginTypeConverter SubjectType = "login_type_converter" SubjectTypeNotifier SubjectType = "notifier" SubjectTypeSubAgentAPI SubjectType = "sub_agent_api" SubjectTypeFileReader SubjectType = "file_reader" diff --git a/coderd/userauth.go b/coderd/userauth.go index fb80c2230b5a4..5ba5bd457f31a 100644 --- a/coderd/userauth.go +++ b/coderd/userauth.go @@ -77,8 +77,8 @@ func (o *OAuthConvertStateClaims) Validate(e jwt.Expected) error { // postConvertLoginType replies with an oauth state token capable of converting // the user to an oauth user. // -// @Summary Convert user from password to oauth authentication -// @ID convert-user-from-password-to-oauth-authentication +// @Summary Convert user to oauth authentication +// @ID convert-user-to-oauth-authentication // @Security CoderSessionToken // @Accept json // @Produce json @@ -123,23 +123,34 @@ func (api *API) postConvertLoginType(rw http.ResponseWriter, r *http.Request) { return } - // This handles the email/pass checking. - user, _, ok := api.loginRequest(ctx, rw, codersdk.LoginWithPasswordRequest{ - Email: user.Email, - Password: req.Password, - }) - if !ok { - return - } + switch user.LoginType { + case database.LoginTypePassword: + authenticatedUser, _, ok := api.loginRequest(ctx, rw, codersdk.LoginWithPasswordRequest{ + Email: user.Email, + Password: req.Password, + }) + if !ok { + return + } + user = authenticatedUser + case database.LoginTypeGithub: + if req.ToType != codersdk.LoginTypeOIDC { + httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ + Message: "GitHub-authenticated accounts can only convert to OpenID Connect.", + }) + return + } - // Only support converting from password auth. - if user.LoginType != database.LoginTypePassword { - // This is checked in loginRequest, but checked again here in case that shared - // function changes its checks. Just some defensive programming. - // This login type is **required** to be password based to prevent - // users from converting other login types to OIDC. + apiKey := httpmw.APIKey(r) + if apiKey.UserID != user.ID || apiKey.LoginType != database.LoginTypeGithub { + httpapi.Write(ctx, rw, http.StatusForbidden, codersdk.Response{ + Message: "GitHub account conversion must be initiated by the user's GitHub session.", + }) + return + } + default: httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ - Message: "User account must have password based authentication.", + Message: "User account must have password or GitHub authentication.", }) return } @@ -1520,9 +1531,8 @@ func (api *API) userOIDC(rw http.ResponseWriter, r *http.Request) { return } - // If a new user is authenticating for the first time - // the audit action is 'register', not 'login' - if user.ID == uuid.Nil { + // An unmatched OIDC conversion is not a registration. + if user.ID == uuid.Nil && !isMergeStateString(state.StateString) { aReq.Action = database.AuditActionRegister } @@ -1759,8 +1769,6 @@ func (api *API) oauthLogin(r *http.Request, params *oauthLoginParams) ([]*http.C user = params.User link = params.Link - // If you do a convert to OIDC and your email does not match, we need to - // catch this and not make a new account. if isMergeStateString(params.State.StateString) { // Always clear this cookie. If it succeeds, we no longer need it. // If it fails, we no longer care about it. @@ -2053,7 +2061,10 @@ func (api *API) oauthLogin(r *http.Request, params *oauthLoginParams) ([]*http.C if ok && oldKey != nil && isConvertLoginType { // If this is a convert login type, and it succeeds, then delete the old // session. Force the user to log back in. - err := api.Database.DeleteAPIKeyByID(r.Context(), oldKey.ID) + err := api.Database.DeleteAPIKeyByID( + dbauthz.AsAPIKeyRevoker(r.Context(), user.ID), + oldKey.ID, + ) if err != nil { // Do not block this login if we fail to delete the old API key. // Just delete the cookie and continue. @@ -2090,20 +2101,9 @@ func (api *API) oauthLogin(r *http.Request, params *oauthLoginParams) ([]*http.C return cookies, user, key, nil } -// convertUserToOauth will convert a user from password base loginType to -// an oauth login type. If it fails, it will return a httpError func (api *API) convertUserToOauth(ctx context.Context, r *http.Request, db database.Store, params *oauthLoginParams) (database.User, error) { user := params.User - // Trying to convert to OIDC, but the email does not match. - // So do not make a new user, just block the request. - if user.ID == uuid.Nil { - return database.User{}, idpsync.HTTPError{ - Code: http.StatusBadRequest, - Msg: fmt.Sprintf("The oidc account with the email %q does not match the email of the account you are trying to convert. Contact your administrator to resolve this issue.", params.Email), - } - } - jwtCookie, err := r.Cookie(OAuthConvertCookieValue) if err != nil { return database.User{}, idpsync.HTTPError{ @@ -2129,8 +2129,6 @@ func (api *API) convertUserToOauth(ctx context.Context, r *http.Request, db data } } - // At this point, this request could be an attempt to convert from - // password auth to oauth auth. Always log these attempts. var ( auditor = *api.Auditor.Load() oauthConvertAudit = params.initAuditRequest(&audit.RequestParams{ @@ -2158,6 +2156,43 @@ func (api *API) convertUserToOauth(ctx context.Context, r *http.Request, db data } } + conversionCtx := dbauthz.AsLoginTypeConverter(ctx, claims.UserID) + + if claims.FromLoginType == codersdk.LoginTypeGithub && claims.ToLoginType == codersdk.LoginTypeOIDC { + // Only a GitHub-authenticated conversion with signed state can bypass email matching. + sourceUser, err := db.GetUserByID(conversionCtx, claims.UserID) + if err != nil { + if !errors.Is(err, sql.ErrNoRows) { + return database.User{}, idpsync.HTTPError{ + Code: http.StatusInternalServerError, + Msg: "Failed to load source account.", + } + } + + return database.User{}, idpsync.HTTPError{ + Code: http.StatusForbidden, + Msg: "Request to convert login type failed. The source account no longer exists.", + } + } + + if user.ID != uuid.Nil && user.ID != sourceUser.ID { + return database.User{}, idpsync.HTTPError{ + Code: http.StatusForbidden, + Msg: "The OpenID Connect identity is already linked to another Coder user.", + } + } + user = sourceUser + oauthConvertAudit.Old = user + } + + // Other conversion paths require a matched user. + if user.ID == uuid.Nil { + return database.User{}, idpsync.HTTPError{ + Code: http.StatusBadRequest, + Msg: fmt.Sprintf("The oidc account with the email %q does not match the email of the account you are trying to convert. Contact your administrator to resolve this issue.", params.Email), + } + } + // Make sure the merge state generated matches this OIDC login request. // It needs to have the correct login type information for this // user. @@ -2173,9 +2208,7 @@ func (api *API) convertUserToOauth(ctx context.Context, r *http.Request, db data // Convert the user and default to the normal login flow. // If the login succeeds, this transaction will commit and the user // will be converted. - // nolint:gocritic // system query to update user login type. The user already - // provided their password to authenticate this request. - user, err = db.UpdateUserLoginType(dbauthz.AsSystemRestricted(ctx), database.UpdateUserLoginTypeParams{ + user, err = db.UpdateUserLoginType(conversionCtx, database.UpdateUserLoginTypeParams{ NewLoginType: params.LoginType, UserID: user.ID, }) diff --git a/coderd/userauth_test.go b/coderd/userauth_test.go index 45ed29274c33d..0e33c307f582b 100644 --- a/coderd/userauth_test.go +++ b/coderd/userauth_test.go @@ -31,6 +31,7 @@ import ( "cdr.dev/slog/v3" "cdr.dev/slog/v3/sloggers/slogtest" "github.com/coder/coder/v2/coderd" + "github.com/coder/coder/v2/coderd/apikey" "github.com/coder/coder/v2/coderd/audit" "github.com/coder/coder/v2/coderd/coderdtest" "github.com/coder/coder/v2/coderd/coderdtest/oidctest" @@ -50,6 +51,34 @@ import ( "github.com/coder/coder/v2/testutil" ) +func setGitHubSession(ctx context.Context, t testing.TB, api *coderd.API, client *codersdk.Client, userID uuid.UUID) { + t.Helper() + + _, err := api.Database.UpdateUserLoginType(dbauthz.AsSystemRestricted(ctx), database.UpdateUserLoginTypeParams{ + NewLoginType: database.LoginTypeGithub, + UserID: userID, + }) + require.NoError(t, err) + + _, err = api.Database.InsertUserLink(dbauthz.AsSystemRestricted(ctx), database.InsertUserLinkParams{ + UserID: userID, + LoginType: database.LoginTypeGithub, + LinkedID: uuid.NewString(), + Claims: database.UserLinkClaims{}, + }) + require.NoError(t, err) + + key, token, err := apikey.Generate(apikey.CreateParams{ + UserID: userID, + LoginType: database.LoginTypeGithub, + ExpiresAt: time.Now().Add(time.Hour), + }) + require.NoError(t, err) + _, err = api.Database.InsertAPIKey(dbauthz.AsSystemRestricted(ctx), key) + require.NoError(t, err) + client.SetSessionToken(token) +} + // This test specifically tests logging in with OIDC when an expired // OIDC session token exists. // The token refreshing should not happen since we are reauthenticating. @@ -2313,6 +2342,224 @@ func TestUserOIDC(t *testing.T) { require.Equal(t, codersdk.LoginTypeOIDC, info.LoginType) }) + t.Run("GitHubToOIDCConvert", func(t *testing.T) { + t.Parallel() + + auditor := audit.NewMock() + fake := oidctest.NewFakeIDP(t, + oidctest.WithRefresh(func(_ string) error { + return xerrors.New("refreshing token should never occur") + }), + oidctest.WithServing(), + ) + cfg := fake.OIDCConfig(t, nil, func(cfg *coderd.OIDCConfig) { + cfg.AllowSignups = true + }) + + client, _, api := coderdtest.NewWithAPI(t, &coderdtest.Options{ + Auditor: auditor, + OIDCConfig: cfg, + }) + + owner := coderdtest.CreateFirstUser(t, client) + user, userData := coderdtest.CreateAnotherUser(t, client, owner.OrganizationID) + ctx := testutil.Context(t, testutil.WaitShort) + setGitHubSession(ctx, t, api, user, userData.ID) + + const oidcEmail = "oidc-user@example.com" + claims := jwt.MapClaims{ + "email": oidcEmail, + "email_verified": true, + "sub": uuid.NewString(), + } + var err error + user.HTTPClient.Jar, err = cookiejar.New(nil) + require.NoError(t, err) + user.HTTPClient.Transport = http.DefaultTransport.(*http.Transport).Clone() + + convertResponse, err := user.ConvertLoginType(ctx, codersdk.ConvertLoginRequest{ + ToType: codersdk.LoginTypeOIDC, + }) + require.NoError(t, err) + + _, _ = fake.LoginWithClient(t, user, claims, func(r *http.Request) { + r.URL.RawQuery = url.Values{ + "oidc_merge_state": {convertResponse.StateString}, + }.Encode() + r.Header.Set(codersdk.SessionTokenHeader, user.SessionToken()) + for _, cookie := range user.HTTPClient.Jar.Cookies(r.URL) { + r.AddCookie(cookie) + } + }) + + info, err := client.User(ctx, userData.ID.String()) + require.NoError(t, err) + require.Equal(t, userData.ID, info.ID) + require.ElementsMatch(t, userData.OrganizationIDs, info.OrganizationIDs) + require.Equal(t, codersdk.LoginTypeOIDC, info.LoginType) + require.Equal(t, oidcEmail, info.Email) + + _, err = user.User(ctx, codersdk.Me) + require.Error(t, err) + }) + + t.Run("GitHubToOIDCConvertRejectsIdentityLinkedToAnotherUser", func(t *testing.T) { + t.Parallel() + + fake := oidctest.NewFakeIDP(t, + oidctest.WithServing(), + ) + cfg := fake.OIDCConfig(t, nil, func(cfg *coderd.OIDCConfig) { + cfg.AllowSignups = true + }) + + client, _, api := coderdtest.NewWithAPI(t, &coderdtest.Options{ + OIDCConfig: cfg, + }) + owner := coderdtest.CreateFirstUser(t, client) + source, sourceData := coderdtest.CreateAnotherUser(t, client, owner.OrganizationID) + _, targetData := coderdtest.CreateAnotherUser(t, client, owner.OrganizationID) + ctx := testutil.Context(t, testutil.WaitShort) + setGitHubSession(ctx, t, api, source, sourceData.ID) + + oidcSubject := uuid.NewString() + _, err := api.Database.UpdateUserLoginType(dbauthz.AsSystemRestricted(ctx), database.UpdateUserLoginTypeParams{ + NewLoginType: database.LoginTypeOIDC, + UserID: targetData.ID, + }) + require.NoError(t, err) + _, err = api.Database.InsertUserLink(dbauthz.AsSystemRestricted(ctx), database.InsertUserLinkParams{ + UserID: targetData.ID, + LoginType: database.LoginTypeOIDC, + LinkedID: fake.IssuerURL().String() + "||" + oidcSubject, + Claims: database.UserLinkClaims{}, + }) + require.NoError(t, err) + + source.HTTPClient.Jar, err = cookiejar.New(nil) + require.NoError(t, err) + source.HTTPClient.Transport = http.DefaultTransport.(*http.Transport).Clone() + + convertResponse, err := source.ConvertLoginType(ctx, codersdk.ConvertLoginRequest{ + ToType: codersdk.LoginTypeOIDC, + }) + require.NoError(t, err) + + _, response := fake.LoginWithClient(t, source, jwt.MapClaims{ + "email": "oidc-user@example.com", + "email_verified": true, + "sub": oidcSubject, + }, func(r *http.Request) { + r.URL.RawQuery = url.Values{ + "oidc_merge_state": {convertResponse.StateString}, + }.Encode() + r.Header.Set(codersdk.SessionTokenHeader, source.SessionToken()) + for _, cookie := range source.HTTPClient.Jar.Cookies(r.URL) { + r.AddCookie(cookie) + } + }) + require.Equal(t, http.StatusForbidden, response.StatusCode) + + sourceInfo, err := client.User(ctx, sourceData.ID.String()) + require.NoError(t, err) + require.Equal(t, codersdk.LoginTypeGithub, sourceInfo.LoginType) + require.Equal(t, sourceData.Email, sourceInfo.Email) + + targetInfo, err := client.User(ctx, targetData.ID.String()) + require.NoError(t, err) + require.Equal(t, codersdk.LoginTypeOIDC, targetInfo.LoginType) + }) + + t.Run("PasswordToOIDCConvertRejectsMismatchedEmail", func(t *testing.T) { + t.Parallel() + + fake := oidctest.NewFakeIDP(t, oidctest.WithServing()) + cfg := fake.OIDCConfig(t, nil, func(cfg *coderd.OIDCConfig) { + cfg.AllowSignups = true + }) + + client := coderdtest.New(t, &coderdtest.Options{ + OIDCConfig: cfg, + }) + owner := coderdtest.CreateFirstUser(t, client) + user, userData := coderdtest.CreateAnotherUser(t, client, owner.OrganizationID) + ctx := testutil.Context(t, testutil.WaitShort) + + var err error + user.HTTPClient.Jar, err = cookiejar.New(nil) + require.NoError(t, err) + user.HTTPClient.Transport = http.DefaultTransport.(*http.Transport).Clone() + + convertResponse, err := user.ConvertLoginType(ctx, codersdk.ConvertLoginRequest{ + ToType: codersdk.LoginTypeOIDC, + Password: "SomeSecurePassword!", + }) + require.NoError(t, err) + + _, response := fake.LoginWithClient(t, user, jwt.MapClaims{ + "email": "oidc-user@example.com", + "email_verified": true, + "sub": uuid.NewString(), + }, func(r *http.Request) { + r.URL.RawQuery = url.Values{ + "oidc_merge_state": {convertResponse.StateString}, + }.Encode() + r.Header.Set(codersdk.SessionTokenHeader, user.SessionToken()) + for _, cookie := range user.HTTPClient.Jar.Cookies(r.URL) { + r.AddCookie(cookie) + } + }) + require.Equal(t, http.StatusBadRequest, response.StatusCode) + + info, err := client.User(ctx, userData.ID.String()) + require.NoError(t, err) + require.Equal(t, codersdk.LoginTypePassword, info.LoginType) + require.Equal(t, userData.Email, info.Email) + }) + + t.Run("GitHubToOIDCConvertRequiresGitHubSession", func(t *testing.T) { + t.Parallel() + + client, _, api := coderdtest.NewWithAPI(t, nil) + owner := coderdtest.CreateFirstUser(t, client) + ctx := testutil.Context(t, testutil.WaitShort) + passwordSessionToken := client.SessionToken() + setGitHubSession(ctx, t, api, client, owner.UserID) + + client.SetSessionToken(passwordSessionToken) + _, err := client.ConvertLoginType(ctx, codersdk.ConvertLoginRequest{ + ToType: codersdk.LoginTypeOIDC, + }) + require.Error(t, err) + var apiErr *codersdk.Error + require.ErrorAs(t, err, &apiErr) + require.Equal(t, http.StatusForbidden, apiErr.StatusCode()) + }) + + t.Run("GitHubToOIDCConvertRequiresOwnAccount", func(t *testing.T) { + t.Parallel() + + client, _, api := coderdtest.NewWithAPI(t, nil) + owner := coderdtest.CreateFirstUser(t, client) + _, victim := coderdtest.CreateAnotherUser(t, client, owner.OrganizationID) + ctx := testutil.Context(t, testutil.WaitShort) + setGitHubSession(ctx, t, api, client, owner.UserID) + victimClient := codersdk.New(client.URL) + setGitHubSession(ctx, t, api, victimClient, victim.ID) + + _, err := client.ConvertUserLoginType(ctx, victim.ID.String(), codersdk.ConvertLoginRequest{ + ToType: codersdk.LoginTypeOIDC, + }) + require.Error(t, err) + var apiErr *codersdk.Error + require.ErrorAs(t, err, &apiErr) + require.Equal(t, http.StatusForbidden, apiErr.StatusCode()) + + victim, err = client.User(ctx, victim.ID.String()) + require.NoError(t, err) + require.Equal(t, codersdk.LoginTypeGithub, victim.LoginType) + }) + t.Run("BadJWT", func(t *testing.T) { t.Parallel() diff --git a/codersdk/users.go b/codersdk/users.go index 3ac64a73632a4..d8c16968ae3bc 100644 --- a/codersdk/users.go +++ b/codersdk/users.go @@ -412,8 +412,9 @@ type UserRoles struct { type ConvertLoginRequest struct { // ToType is the login type to convert to. - ToType LoginType `json:"to_type" validate:"required"` - Password string `json:"password" validate:"required"` + ToType LoginType `json:"to_type" validate:"required"` + // Password is required for password-authenticated accounts. + Password string `json:"password,omitempty" validate:""` } // LoginWithPasswordRequest enables callers to authenticate with email and password. @@ -945,16 +946,14 @@ func (c *Client) ChangePasswordWithOneTimePasscode(ctx context.Context, req Chan return nil } -// ConvertLoginType will send a request to convert the user from password -// based authentication to oauth based. The response has the oauth state code -// to use in the oauth flow. +// ConvertLoginType starts converting the current user from password or GitHub +// to OAuth authentication. func (c *Client) ConvertLoginType(ctx context.Context, req ConvertLoginRequest) (OAuthConversionResponse, error) { return c.ConvertUserLoginType(ctx, Me, req) } -// ConvertUserLoginType will send a request to convert the user from password -// based authentication to oauth based. The response has the oauth state code -// to use in the oauth flow. +// ConvertUserLoginType starts converting a user from password or GitHub to +// OAuth authentication. func (c *Client) ConvertUserLoginType(ctx context.Context, user string, req ConvertLoginRequest) (OAuthConversionResponse, error) { res, err := c.Request(ctx, http.MethodPost, fmt.Sprintf("/api/v2/users/%s/convert-login", user), req) if err != nil { diff --git a/docs/reference/api/authorization.md b/docs/reference/api/authorization.md index 52baefb7da21b..f4ab2d09f0e8e 100644 --- a/docs/reference/api/authorization.md +++ b/docs/reference/api/authorization.md @@ -260,7 +260,7 @@ curl -X POST http://coder-server:8080/api/v2/users/validate-password \ To perform this operation, you must be authenticated. [Learn more](authentication.md). -## Convert user from password to oauth authentication +## Convert user to oauth authentication ### Code samples diff --git a/docs/reference/api/schemas.md b/docs/reference/api/schemas.md index 0182d4f6535bb..06bffed4dc158 100644 --- a/docs/reference/api/schemas.md +++ b/docs/reference/api/schemas.md @@ -5705,10 +5705,10 @@ AuthorizationObject can represent a "set" of objects, such as: all workspaces in ### Properties -| Name | Type | Required | Restrictions | Description | -|------------|------------------------------------------|----------|--------------|------------------------------------------| -| `password` | string | true | | | -| `to_type` | [codersdk.LoginType](#codersdklogintype) | true | | To type is the login type to convert to. | +| Name | Type | Required | Restrictions | Description | +|------------|------------------------------------------|----------|--------------|-----------------------------------------------------------| +| `password` | string | false | | Password is required for password-authenticated accounts. | +| `to_type` | [codersdk.LoginType](#codersdklogintype) | true | | To type is the login type to convert to. | ## codersdk.CreateAIGatewayKeyRequest diff --git a/site/src/api/typesGenerated.ts b/site/src/api/typesGenerated.ts index 46532f0689b3e..a6e0f13e0250b 100644 --- a/site/src/api/typesGenerated.ts +++ b/site/src/api/typesGenerated.ts @@ -3816,7 +3816,10 @@ export interface ConvertLoginRequest { * ToType is the login type to convert to. */ readonly to_type: LoginType; - readonly password: string; + /** + * Password is required for password-authenticated accounts. + */ + readonly password?: string; } // From codersdk/aigatewaykeys.go diff --git a/site/src/pages/UserSettingsPage/SecurityPage/SecurityPageView.stories.tsx b/site/src/pages/UserSettingsPage/SecurityPage/SecurityPageView.stories.tsx index 437feb9ae4e38..53237f9fc7713 100644 --- a/site/src/pages/UserSettingsPage/SecurityPage/SecurityPageView.stories.tsx +++ b/site/src/pages/UserSettingsPage/SecurityPage/SecurityPageView.stories.tsx @@ -1,13 +1,31 @@ import type { Meta, StoryObj } from "@storybook/react-vite"; import set from "lodash/fp/set"; -import type { ComponentProps } from "react"; +import { type ComponentProps, type FC, useState } from "react"; import { action } from "storybook/actions"; +import { expect, fn, userEvent, within } from "storybook/test"; import { MockAuthMethodsAll, MockAuthMethodsPasswordOnly, } from "#/testHelpers/entities"; import { SecurityPageView } from "./SecurityPage"; +type SingleSignOnSectionProps = NonNullable< + ComponentProps["oidc"] +>["section"]; + +const defaultSingleSignOnSection: SingleSignOnSectionProps = { + userLoginType: { + login_type: "password", + }, + authMethods: MockAuthMethodsPasswordOnly, + closeConfirmation: action("closeConfirmation"), + confirm: action("confirm"), + error: null, + isConfirming: false, + isUpdating: false, + openConfirmation: action("openConfirmation"), +}; + const defaultArgs: ComponentProps = { security: { form: { @@ -18,21 +36,36 @@ const defaultArgs: ComponentProps = { }, }, oidc: { - section: { - userLoginType: { - login_type: "password", - }, - authMethods: MockAuthMethodsPasswordOnly, - closeConfirmation: action("closeConfirmation"), - confirm: action("confirm"), - error: null, - isConfirming: false, - isUpdating: false, - openConfirmation: action("openConfirmation"), - }, + section: defaultSingleSignOnSection, }, }; +const GitHubToOIDCConversionHarness: FC< + ComponentProps +> = ({ security, oidc }) => { + const [isConfirming, setIsConfirming] = useState(false); + + if (!oidc) { + return ; + } + + return ( + setIsConfirming(false), + isConfirming, + openConfirmation: () => setIsConfirming(true), + }, + }} + /> + ); +}; + +const confirmGitHubToOIDCConversion = fn(); + const meta: Meta = { title: "pages/UserSettingsPage/SecurityPageView", component: SecurityPageView, @@ -59,7 +92,7 @@ export const ConfirmingOIDCConversion: Story = { args: set( "oidc.section", { - ...defaultArgs.oidc?.section, + ...defaultSingleSignOnSection, authMethods: MockAuthMethodsAll, isConfirming: true, }, @@ -72,7 +105,7 @@ export const AuthenticatedWithGithub: Story = { ...defaultArgs, oidc: { section: { - ...defaultArgs.oidc!.section, + ...defaultSingleSignOnSection, userLoginType: { login_type: "github", }, @@ -87,7 +120,7 @@ export const AuthenticatedWithOIDC: Story = { ...defaultArgs, oidc: { section: { - ...defaultArgs.oidc!.section, + ...defaultSingleSignOnSection, userLoginType: { login_type: "oidc", }, @@ -96,3 +129,36 @@ export const AuthenticatedWithOIDC: Story = { }, }, }; + +export const ConvertGithubToOIDC: Story = { + args: { + ...defaultArgs, + oidc: { + section: { + ...defaultSingleSignOnSection, + authMethods: MockAuthMethodsAll, + confirm: confirmGitHubToOIDCConversion, + userLoginType: { + login_type: "github", + }, + }, + }, + }, + render: (args) => , + play: async ({ canvasElement }) => { + const canvas = within(canvasElement); + const user = userEvent.setup(); + await user.click( + await canvas.findByRole("button", { name: /Switch to Google/ }), + ); + + const dialog = await within(document.body).findByRole("dialog", { + name: "Change login type", + }); + expect(within(dialog).queryByLabelText("Confirm your password")).toBeNull(); + await user.click( + await within(dialog).findByRole("button", { name: "Update" }), + ); + expect(confirmGitHubToOIDCConversion).toHaveBeenCalledWith(undefined); + }, +}; diff --git a/site/src/pages/UserSettingsPage/SecurityPage/SingleSignOnSection.tsx b/site/src/pages/UserSettingsPage/SecurityPage/SingleSignOnSection.tsx index 093eb8e7baae9..b168d4afb609f 100644 --- a/site/src/pages/UserSettingsPage/SecurityPage/SingleSignOnSection.tsx +++ b/site/src/pages/UserSettingsPage/SecurityPage/SingleSignOnSection.tsx @@ -81,7 +81,7 @@ export const useSingleSignOnSection = () => { mutation.reset(); }; - const confirm = (password: string) => { + const confirm = (password?: string) => { if (!loginTypeConfirmation.selectedType) { throw new Error("No login type selected"); } @@ -182,24 +182,40 @@ export const SingleSignOnSection: FC = ({ {noSsoEnabled && } ) : ( -
- - - Authenticated with{" "} - - {userLoginType.login_type === "github" - ? "GitHub" - : getOIDCLabel(authMethods.oidc)} - - -
- {userLoginType.login_type === "github" ? ( - - ) : ( - - )} + <> +
+ + + Authenticated with{" "} + + {userLoginType.login_type === "github" + ? "GitHub" + : getOIDCLabel(authMethods.oidc)} + + +
+ {userLoginType.login_type === "github" ? ( + + ) : ( + + )} +
-
+ + {userLoginType.login_type === "github" && + authMethods.oidc.enabled && ( + + )} + )}
@@ -209,6 +225,7 @@ export const SingleSignOnSection: FC = ({ loading={isUpdating} onClose={closeConfirmation} onConfirm={confirm} + requiresPassword={userLoginType.login_type === "password"} /> ); @@ -241,7 +258,8 @@ interface ConfirmLoginTypeChangeModalProps { loading: boolean; error: unknown; onClose: () => void; - onConfirm: (password: string) => void; + onConfirm: (password?: string) => void; + requiresPassword: boolean; } const ConfirmLoginTypeChangeModal: FC = ({ @@ -250,17 +268,23 @@ const ConfirmLoginTypeChangeModal: FC = ({ error, onClose, onConfirm, + requiresPassword, }) => { const [password, setPassword] = useState(""); const passwordId = useId(); const errorId = useId(); const hasError = Boolean(error); const errorMessage = error - ? getErrorMessage(error, "Your password is incorrect") + ? getErrorMessage( + error, + requiresPassword + ? "Your password is incorrect" + : "Unable to change your login type", + ) : undefined; const handleConfirm = () => { - onConfirm(password); + onConfirm(requiresPassword ? password : undefined); }; return ( @@ -281,30 +305,32 @@ const ConfirmLoginTypeChangeModal: FC = ({ After changing your login type, you will not be able to change it again. Are you sure you want to proceed and change your login type?

-
- - { - if (event.key === "Enter") { - handleConfirm(); - } - }} - name="confirm-password" - id={passwordId} - value={password} - onChange={(e) => setPassword(e.currentTarget.value)} - type="password" - aria-invalid={hasError} - aria-describedby={hasError ? errorId : undefined} - className={cn(hasError && "border-border-destructive")} - /> - {hasError && ( - - {errorMessage} - - )} -
+ {requiresPassword && ( +
+ + { + if (event.key === "Enter") { + handleConfirm(); + } + }} + name="confirm-password" + id={passwordId} + value={password} + onChange={(e) => setPassword(e.currentTarget.value)} + type="password" + aria-invalid={hasError} + aria-describedby={hasError ? errorId : undefined} + className={cn(hasError && "border-border-destructive")} + /> +
+ )} + {hasError && ( + + {errorMessage} + + )} } />