From 743c19ea3bd94bd83e10e688090d692814714e6a Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Fri, 21 Jun 2024 08:56:07 -0500 Subject: [PATCH 1/5] test: implement test to assert deleted custom roles are omitted --- coderd/httpmw/apikey_test.go | 127 ++++++++++++++++++++++++++++- coderd/rbac/rolestore/rolestore.go | 2 + 2 files changed, 128 insertions(+), 1 deletion(-) diff --git a/coderd/httpmw/apikey_test.go b/coderd/httpmw/apikey_test.go index 33ba90a4d728c..280d938d062fc 100644 --- a/coderd/httpmw/apikey_test.go +++ b/coderd/httpmw/apikey_test.go @@ -14,8 +14,10 @@ import ( "testing" "time" + "github.com/google/uuid" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "golang.org/x/exp/slices" "golang.org/x/oauth2" "github.com/coder/coder/v2/coderd/database" @@ -24,6 +26,7 @@ import ( "github.com/coder/coder/v2/coderd/database/dbtime" "github.com/coder/coder/v2/coderd/httpapi" "github.com/coder/coder/v2/coderd/httpmw" + "github.com/coder/coder/v2/coderd/rbac" "github.com/coder/coder/v2/codersdk" "github.com/coder/coder/v2/cryptorand" "github.com/coder/coder/v2/testutil" @@ -633,7 +636,7 @@ func TestAPIKey(t *testing.T) { require.Equal(t, sentAPIKey.LoginType, gotAPIKey.LoginType) }) - t.Run("MissongConfig", func(t *testing.T) { + t.Run("MissingConfig", func(t *testing.T) { t.Parallel() var ( db = dbmem.New() @@ -667,4 +670,126 @@ func TestAPIKey(t *testing.T) { out, _ := io.ReadAll(res.Body) require.Contains(t, string(out), "Unable to refresh") }) + + t.Run("CustomRoles", func(t *testing.T) { + t.Parallel() + var ( + db = dbmem.New() + org = dbgen.Organization(t, db, database.Organization{}) + customRole = dbgen.CustomRole(t, db, database.CustomRole{ + Name: "custom-role", + + OrgPermissions: []database.CustomRolePermission{}, + OrganizationID: uuid.NullUUID{ + UUID: org.ID, + Valid: true, + }, + }) + user = dbgen.User(t, db, database.User{ + RBACRoles: []string{ + rbac.ScopedRoleOrgAdmin(org.ID).String(), + customRole.RoleIdentifier().String(), + }, + }) + sentAPIKey, token = dbgen.APIKey(t, db, database.APIKey{ + UserID: user.ID, + ExpiresAt: dbtime.Now().AddDate(0, 0, 1), + }) + + r = httptest.NewRequest("GET", "/", nil) + rw = httptest.NewRecorder() + ) + r.Header.Set(codersdk.SessionTokenHeader, token) + + httpmw.ExtractAPIKeyMW(httpmw.ExtractAPIKeyConfig{ + DB: db, + RedirectToLogin: false, + })(http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { + // Checks that it exists on the context! + _ = httpmw.APIKey(r) + auth := httpmw.UserAuthorization(r) + roles, err := auth.Roles.Expand() + assert.NoError(t, err, "expand user roles") + // Assert built in org role + assert.True(t, slices.ContainsFunc(roles, func(role rbac.Role) bool { + return role.Identifier.Name == rbac.RoleOrgAdmin() && role.Identifier.OrganizationID == org.ID + }), "org admin role") + // Assert custom role + assert.True(t, slices.ContainsFunc(roles, func(role rbac.Role) bool { + return role.Identifier.Name == customRole.Name && role.Identifier.OrganizationID == org.ID + }), "custom org role") + + httpapi.Write(r.Context(), rw, http.StatusOK, codersdk.Response{ + Message: "It worked!", + }) + })).ServeHTTP(rw, r) + res := rw.Result() + defer res.Body.Close() + require.Equal(t, http.StatusOK, res.StatusCode) + + gotAPIKey, err := db.GetAPIKeyByID(r.Context(), sentAPIKey.ID) + require.NoError(t, err) + + require.Equal(t, sentAPIKey.ExpiresAt, gotAPIKey.ExpiresAt) + }) + + // There is no sql foreign key constraint to require all assigned roles + // still exist in the database. We need to handle deleted roles. + t.Run("RoleNotExists", func(t *testing.T) { + t.Parallel() + var ( + roleNotExistsName = "role-not-exists" + db = dbmem.New() + org = dbgen.Organization(t, db, database.Organization{}) + user = dbgen.User(t, db, database.User{ + RBACRoles: []string{ + rbac.ScopedRoleOrgAdmin(org.ID).String(), + rbac.RoleIdentifier{Name: roleNotExistsName, OrganizationID: org.ID}.String(), + // Also provide an org not exists + rbac.RoleIdentifier{Name: roleNotExistsName, OrganizationID: uuid.New()}.String(), + }, + }) + sentAPIKey, token = dbgen.APIKey(t, db, database.APIKey{ + UserID: user.ID, + ExpiresAt: dbtime.Now().AddDate(0, 0, 1), + }) + + r = httptest.NewRequest("GET", "/", nil) + rw = httptest.NewRecorder() + ) + r.Header.Set(codersdk.SessionTokenHeader, token) + + httpmw.ExtractAPIKeyMW(httpmw.ExtractAPIKeyConfig{ + DB: db, + RedirectToLogin: false, + })(http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { + // Checks that it exists on the context! + _ = httpmw.APIKey(r) + auth := httpmw.UserAuthorization(r) + roles, err := auth.Roles.Expand() + assert.NoError(t, err, "expand user roles") + // Assert built in org role + assert.True(t, slices.ContainsFunc(roles, func(role rbac.Role) bool { + return role.Identifier.Name == rbac.RoleOrgAdmin() && role.Identifier.OrganizationID == org.ID + }), "org admin role") + + // Assert the role-not-exists is not returned + assert.False(t, slices.ContainsFunc(roles, func(role rbac.Role) bool { + return role.Identifier.Name == roleNotExistsName + }), "role should not exist") + + httpapi.Write(r.Context(), rw, http.StatusOK, codersdk.Response{ + Message: "It worked!", + }) + })).ServeHTTP(rw, r) + res := rw.Result() + defer res.Body.Close() + require.Equal(t, http.StatusOK, res.StatusCode) + + gotAPIKey, err := db.GetAPIKeyByID(r.Context(), sentAPIKey.ID) + require.NoError(t, err) + + require.Equal(t, sentAPIKey.ExpiresAt, gotAPIKey.ExpiresAt) + }) + } diff --git a/coderd/rbac/rolestore/rolestore.go b/coderd/rbac/rolestore/rolestore.go index 26d354084c7b5..610b04c06aa19 100644 --- a/coderd/rbac/rolestore/rolestore.go +++ b/coderd/rbac/rolestore/rolestore.go @@ -39,6 +39,8 @@ func roleCache(ctx context.Context) *syncmap.Map[string, rbac.Role] { } // Expand will expand built in roles, and fetch custom roles from the database. +// If a custom role is defined, but does not exist, the role will be omitted on +// the response. This means deleted roles are silently dropped. func Expand(ctx context.Context, db database.Store, names []rbac.RoleIdentifier) (rbac.Roles, error) { if len(names) == 0 { // That was easy From b8fa2518e5abe54c39cd2b2370c73de82a7253e7 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Fri, 21 Jun 2024 09:12:27 -0500 Subject: [PATCH 2/5] Add comments and more assertions --- coderd/httpmw/apikey_test.go | 45 ++++++++++++++++++++++++++++++++---- coderd/rbac/authz.go | 11 +++++++++ 2 files changed, 52 insertions(+), 4 deletions(-) diff --git a/coderd/httpmw/apikey_test.go b/coderd/httpmw/apikey_test.go index 280d938d062fc..36cdb6225c76a 100644 --- a/coderd/httpmw/apikey_test.go +++ b/coderd/httpmw/apikey_test.go @@ -21,6 +21,7 @@ import ( "golang.org/x/oauth2" "github.com/coder/coder/v2/coderd/database" + "github.com/coder/coder/v2/coderd/database/dbauthz" "github.com/coder/coder/v2/coderd/database/dbgen" "github.com/coder/coder/v2/coderd/database/dbmem" "github.com/coder/coder/v2/coderd/database/dbtime" @@ -41,6 +42,37 @@ func randomAPIKeyParts() (id string, secret string) { func TestAPIKey(t *testing.T) { t.Parallel() + // assertActorOk asserts all the properties of the user auth are ok. + assertActorOk := func(t *testing.T, r *http.Request) { + t.Helper() + + actor, ok := dbauthz.ActorFromContext(r.Context()) + assert.True(t, ok, "dbauthz actor ok") + if ok { + _, err := actor.Roles.Expand() + assert.NoError(t, err, "actor roles ok") + + _, err = actor.Scope.Expand() + assert.NoError(t, err, "actor scope ok") + + err = actor.RegoValueOk() + assert.NoError(t, err, "actor rego ok") + } + + auth, ok := httpmw.UserAuthorizationOptional(r) + assert.True(t, ok, "httpmw auth ok") + if ok { + _, err := auth.Roles.Expand() + assert.NoError(t, err, "auth roles ok") + + _, err = auth.Scope.Expand() + assert.NoError(t, err, "auth scope ok") + + err = auth.RegoValueOk() + assert.NoError(t, err, "auth rego ok") + } + } + successHandler := http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { // Only called if the API key passes through the handler. httpapi.Write(context.Background(), rw, http.StatusOK, codersdk.Response{ @@ -259,6 +291,7 @@ func TestAPIKey(t *testing.T) { })(http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { // Checks that it exists on the context! _ = httpmw.APIKey(r) + assertActorOk(t, r) httpapi.Write(r.Context(), rw, http.StatusOK, codersdk.Response{ Message: "It worked!", }) @@ -299,6 +332,7 @@ func TestAPIKey(t *testing.T) { // Checks that it exists on the context! apiKey := httpmw.APIKey(r) assert.Equal(t, database.APIKeyScopeApplicationConnect, apiKey.Scope) + assertActorOk(t, r) httpapi.Write(r.Context(), rw, http.StatusOK, codersdk.Response{ Message: "it worked!", @@ -333,6 +367,8 @@ func TestAPIKey(t *testing.T) { })(http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { // Checks that it exists on the context! _ = httpmw.APIKey(r) + assertActorOk(t, r) + httpapi.Write(r.Context(), rw, http.StatusOK, codersdk.Response{ Message: "It worked!", }) @@ -705,9 +741,10 @@ func TestAPIKey(t *testing.T) { DB: db, RedirectToLogin: false, })(http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { - // Checks that it exists on the context! - _ = httpmw.APIKey(r) + assertActorOk(t, r) + auth := httpmw.UserAuthorization(r) + roles, err := auth.Roles.Expand() assert.NoError(t, err, "expand user roles") // Assert built in org role @@ -763,9 +800,9 @@ func TestAPIKey(t *testing.T) { DB: db, RedirectToLogin: false, })(http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { - // Checks that it exists on the context! - _ = httpmw.APIKey(r) + assertActorOk(t, r) auth := httpmw.UserAuthorization(r) + roles, err := auth.Roles.Expand() assert.NoError(t, err, "expand user roles") // Assert built in org role diff --git a/coderd/rbac/authz.go b/coderd/rbac/authz.go index 614150bb1522c..224e153a8b4b7 100644 --- a/coderd/rbac/authz.go +++ b/coderd/rbac/authz.go @@ -75,6 +75,17 @@ type Subject struct { cachedASTValue ast.Value } +// RegoValueOk is only used for unit testing. There is no easy way +// to get the error for the unexported method, and this is intentional. +// Failed rego values can default to the backup json marshal method, +// so errors are not fatal. Unit tests should be aware when the custom +// rego marshaller fails. +func (s Subject) RegoValueOk() error { + tmp := s + _, err := tmp.regoValue() + return err +} + // WithCachedASTValue can be called if the subject is static. This will compute // the ast value once and cache it for future calls. func (s Subject) WithCachedASTValue() Subject { From 0213df9460d8566e2e399cd89c521a737fe2e619 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Fri, 21 Jun 2024 09:17:38 -0500 Subject: [PATCH 3/5] fixup test --- coderd/httpmw/apikey_test.go | 48 ++++++++++++++++++++---------------- 1 file changed, 27 insertions(+), 21 deletions(-) diff --git a/coderd/httpmw/apikey_test.go b/coderd/httpmw/apikey_test.go index 36cdb6225c76a..c2e69eb7ae686 100644 --- a/coderd/httpmw/apikey_test.go +++ b/coderd/httpmw/apikey_test.go @@ -713,8 +713,7 @@ func TestAPIKey(t *testing.T) { db = dbmem.New() org = dbgen.Organization(t, db, database.Organization{}) customRole = dbgen.CustomRole(t, db, database.CustomRole{ - Name: "custom-role", - + Name: "custom-role", OrgPermissions: []database.CustomRolePermission{}, OrganizationID: uuid.NullUUID{ UUID: org.ID, @@ -722,12 +721,19 @@ func TestAPIKey(t *testing.T) { }, }) user = dbgen.User(t, db, database.User{ - RBACRoles: []string{ - rbac.ScopedRoleOrgAdmin(org.ID).String(), - customRole.RoleIdentifier().String(), + RBACRoles: []string{}, + }) + _ = dbgen.OrganizationMember(t, db, database.OrganizationMember{ + UserID: user.ID, + OrganizationID: org.ID, + CreatedAt: time.Time{}, + UpdatedAt: time.Time{}, + Roles: []string{ + rbac.RoleOrgAdmin(), + customRole.Name, }, }) - sentAPIKey, token = dbgen.APIKey(t, db, database.APIKey{ + _, token = dbgen.APIKey(t, db, database.APIKey{ UserID: user.ID, ExpiresAt: dbtime.Now().AddDate(0, 0, 1), }) @@ -763,11 +769,6 @@ func TestAPIKey(t *testing.T) { res := rw.Result() defer res.Body.Close() require.Equal(t, http.StatusOK, res.StatusCode) - - gotAPIKey, err := db.GetAPIKeyByID(r.Context(), sentAPIKey.ID) - require.NoError(t, err) - - require.Equal(t, sentAPIKey.ExpiresAt, gotAPIKey.ExpiresAt) }) // There is no sql foreign key constraint to require all assigned roles @@ -780,13 +781,24 @@ func TestAPIKey(t *testing.T) { org = dbgen.Organization(t, db, database.Organization{}) user = dbgen.User(t, db, database.User{ RBACRoles: []string{ - rbac.ScopedRoleOrgAdmin(org.ID).String(), - rbac.RoleIdentifier{Name: roleNotExistsName, OrganizationID: org.ID}.String(), - // Also provide an org not exists + // Also provide an org not exists. In practice this makes no sense + // to store org roles in the user table, but there is no org to + // store it in. So just throw this here for even more unexpected + // behavior handling! rbac.RoleIdentifier{Name: roleNotExistsName, OrganizationID: uuid.New()}.String(), }, }) - sentAPIKey, token = dbgen.APIKey(t, db, database.APIKey{ + _ = dbgen.OrganizationMember(t, db, database.OrganizationMember{ + UserID: user.ID, + OrganizationID: org.ID, + CreatedAt: time.Time{}, + UpdatedAt: time.Time{}, + Roles: []string{ + rbac.RoleOrgAdmin(), + roleNotExistsName, + }, + }) + _, token = dbgen.APIKey(t, db, database.APIKey{ UserID: user.ID, ExpiresAt: dbtime.Now().AddDate(0, 0, 1), }) @@ -822,11 +834,5 @@ func TestAPIKey(t *testing.T) { res := rw.Result() defer res.Body.Close() require.Equal(t, http.StatusOK, res.StatusCode) - - gotAPIKey, err := db.GetAPIKeyByID(r.Context(), sentAPIKey.ID) - require.NoError(t, err) - - require.Equal(t, sentAPIKey.ExpiresAt, gotAPIKey.ExpiresAt) }) - } From 9466f74865bfa8f3ae80b3937652aa2408b57f10 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Fri, 21 Jun 2024 09:47:29 -0500 Subject: [PATCH 4/5] chore: refactor user subject logic to be in 1 place --- coderd/httpmw/apikey.go | 52 ++++++++++++++++-------------- coderd/identityprovider/tokens.go | 37 ++++----------------- coderd/userauth.go | 44 +++++++++---------------- enterprise/coderd/userauth_test.go | 43 ++++++++++++++++++++++++ 4 files changed, 91 insertions(+), 85 deletions(-) diff --git a/coderd/httpmw/apikey.go b/coderd/httpmw/apikey.go index bce375337b5c9..c4d1c7f202533 100644 --- a/coderd/httpmw/apikey.go +++ b/coderd/httpmw/apikey.go @@ -406,8 +406,7 @@ func ExtractAPIKey(rw http.ResponseWriter, r *http.Request, cfg ExtractAPIKeyCon // If the key is valid, we also fetch the user roles and status. // The roles are used for RBAC authorize checks, and the status // is to block 'suspended' users from accessing the platform. - //nolint:gocritic // system needs to update user roles - roles, err := cfg.DB.GetAuthorizationUserRoles(dbauthz.AsSystemRestricted(ctx), key.UserID) + actor, userStatus, err := UserRBACSubject(ctx, cfg.DB, key.UserID, rbac.ScopeName(key.Scope)) if err != nil { return write(http.StatusUnauthorized, codersdk.Response{ Message: internalErrorMessage, @@ -415,7 +414,7 @@ func ExtractAPIKey(rw http.ResponseWriter, r *http.Request, cfg ExtractAPIKeyCon }) } - if roles.Status == database.UserStatusDormant { + if userStatus == database.UserStatusDormant { // If coder confirms that the dormant user is valid, it can switch their account to active. // nolint:gocritic u, err := cfg.DB.UpdateUserStatus(dbauthz.AsSystemRestricted(ctx), database.UpdateUserStatusParams{ @@ -429,47 +428,50 @@ func ExtractAPIKey(rw http.ResponseWriter, r *http.Request, cfg ExtractAPIKeyCon Detail: fmt.Sprintf("can't activate a dormant user: %s", err.Error()), }) } - roles.Status = u.Status + userStatus = u.Status } - if roles.Status != database.UserStatusActive { + if userStatus != database.UserStatusActive { return write(http.StatusUnauthorized, codersdk.Response{ - Message: fmt.Sprintf("User is not active (status = %q). Contact an admin to reactivate your account.", roles.Status), + Message: fmt.Sprintf("User is not active (status = %q). Contact an admin to reactivate your account.", userStatus), }) } + if cfg.PostAuthAdditionalHeadersFunc != nil { + cfg.PostAuthAdditionalHeadersFunc(actor, rw.Header()) + } + + return key, &actor, true +} + +// UserRBACSubject fetches a user's rbac.Subject from the database. It pulls all roles from both +// site and organization scopes. It also pulls the groups, and the user's status. +func UserRBACSubject(ctx context.Context, db database.Store, userID uuid.UUID, scope rbac.ExpandableScope) (rbac.Subject, database.UserStatus, error) { + //nolint:gocritic // system needs to update user roles + roles, err := db.GetAuthorizationUserRoles(dbauthz.AsSystemRestricted(ctx), userID) + if err != nil { + return rbac.Subject{}, "", xerrors.Errorf("get authorization user roles: %w", err) + } + roleNames, err := roles.RoleNames() if err != nil { - return write(http.StatusInternalServerError, codersdk.Response{ - Message: "Internal Server Error", - Detail: err.Error(), - }) + return rbac.Subject{}, "", xerrors.Errorf("expand role names: %w", err) } //nolint:gocritic // Permission to lookup custom roles the user has assigned. - rbacRoles, err := rolestore.Expand(dbauthz.AsSystemRestricted(ctx), cfg.DB, roleNames) + rbacRoles, err := rolestore.Expand(dbauthz.AsSystemRestricted(ctx), db, roleNames) if err != nil { - return write(http.StatusInternalServerError, codersdk.Response{ - Message: "Failed to expand authenticated user roles", - Detail: err.Error(), - Validations: nil, - }) + return rbac.Subject{}, "", xerrors.Errorf("expand role names: %w", err) } - // Actor is the user's authorization context. actor := rbac.Subject{ FriendlyName: roles.Username, - ID: key.UserID.String(), + ID: userID.String(), Roles: rbacRoles, Groups: roles.Groups, - Scope: rbac.ScopeName(key.Scope), + Scope: scope, }.WithCachedASTValue() - - if cfg.PostAuthAdditionalHeadersFunc != nil { - cfg.PostAuthAdditionalHeadersFunc(actor, rw.Header()) - } - - return key, &actor, true + return actor, roles.Status, nil } // APITokenFromRequest returns the api token from the request. diff --git a/coderd/identityprovider/tokens.go b/coderd/identityprovider/tokens.go index 324ff2819400d..0e41ba940298f 100644 --- a/coderd/identityprovider/tokens.go +++ b/coderd/identityprovider/tokens.go @@ -209,27 +209,14 @@ func authorizationCodeGrant(ctx context.Context, db database.Store, app database } // Grab the user roles so we can perform the exchange as the user. - //nolint:gocritic // In the token exchange, there is no user actor. - roles, err := db.GetAuthorizationUserRoles(dbauthz.AsSystemRestricted(ctx), dbCode.UserID) + actor, _, err := httpmw.UserRBACSubject(ctx, db, dbCode.UserID, rbac.ScopeAll) if err != nil { - return oauth2.Token{}, err - } - - roleNames, err := roles.RoleNames() - if err != nil { - return oauth2.Token{}, xerrors.Errorf("role names: %w", err) - } - - userSubj := rbac.Subject{ - ID: dbCode.UserID.String(), - Roles: rbac.RoleIdentifiers(roleNames), - Groups: roles.Groups, - Scope: rbac.ScopeAll, + return oauth2.Token{}, xerrors.Errorf("fetch user actor: %w", err) } // Do the actual token exchange in the database. err = db.InTx(func(tx database.Store) error { - ctx := dbauthz.As(ctx, userSubj) + ctx := dbauthz.As(ctx, actor) err = tx.DeleteOAuth2ProviderAppCodeByID(ctx, dbCode.ID) if err != nil { return xerrors.Errorf("delete oauth2 app code: %w", err) @@ -311,22 +298,10 @@ func refreshTokenGrant(ctx context.Context, db database.Store, app database.OAut if err != nil { return oauth2.Token{}, err } - //nolint:gocritic // There is no user yet so we must use the system. - roles, err := db.GetAuthorizationUserRoles(dbauthz.AsSystemRestricted(ctx), prevKey.UserID) - if err != nil { - return oauth2.Token{}, err - } - roleNames, err := roles.RoleNames() + actor, _, err := httpmw.UserRBACSubject(ctx, db, prevKey.UserID, rbac.ScopeAll) if err != nil { - return oauth2.Token{}, xerrors.Errorf("role names: %w", err) - } - - userSubj := rbac.Subject{ - ID: prevKey.UserID.String(), - Roles: rbac.RoleIdentifiers(roleNames), - Groups: roles.Groups, - Scope: rbac.ScopeAll, + return oauth2.Token{}, xerrors.Errorf("fetch user actor: %w", err) } // Generate a new refresh token. @@ -351,7 +326,7 @@ func refreshTokenGrant(ctx context.Context, db database.Store, app database.OAut // Replace the token. err = db.InTx(func(tx database.Store) error { - ctx := dbauthz.As(ctx, userSubj) + ctx := dbauthz.As(ctx, actor) err = tx.DeleteAPIKeyByID(ctx, prevKey.ID) // This cascades to the token. if err != nil { return xerrors.Errorf("delete oauth2 app token: %w", err) diff --git a/coderd/userauth.go b/coderd/userauth.go index bb7f0ee64c293..c7550b89d05f7 100644 --- a/coderd/userauth.go +++ b/coderd/userauth.go @@ -231,7 +231,7 @@ func (api *API) postLogin(rw http.ResponseWriter, r *http.Request) { return } - user, roles, ok := api.loginRequest(ctx, rw, loginWithPassword) + user, actor, ok := api.loginRequest(ctx, rw, loginWithPassword) // 'user.ID' will be empty, or will be an actual value. Either is correct // here. aReq.UserID = user.ID @@ -240,21 +240,8 @@ func (api *API) postLogin(rw http.ResponseWriter, r *http.Request) { return } - roleNames, err := roles.RoleNames() - if err != nil { - httpapi.InternalServerError(rw, err) - return - } - - userSubj := rbac.Subject{ - ID: user.ID.String(), - Roles: rbac.RoleIdentifiers(roleNames), - Groups: roles.Groups, - Scope: rbac.ScopeAll, - } - //nolint:gocritic // Creating the API key as the user instead of as system. - cookie, key, err := api.createAPIKey(dbauthz.As(ctx, userSubj), apikey.CreateParams{ + cookie, key, err := api.createAPIKey(dbauthz.As(ctx, actor), apikey.CreateParams{ UserID: user.ID, LoginType: database.LoginTypePassword, RemoteAddr: r.RemoteAddr, @@ -284,7 +271,7 @@ func (api *API) postLogin(rw http.ResponseWriter, r *http.Request) { // // The user struct is always returned, even if authentication failed. This is // to support knowing what user attempted to login. -func (api *API) loginRequest(ctx context.Context, rw http.ResponseWriter, req codersdk.LoginWithPasswordRequest) (database.User, database.GetAuthorizationUserRolesRow, bool) { +func (api *API) loginRequest(ctx context.Context, rw http.ResponseWriter, req codersdk.LoginWithPasswordRequest) (database.User, rbac.Subject, bool) { logger := api.Logger.Named(userAuthLoggerName) //nolint:gocritic // In order to login, we need to get the user first! @@ -296,7 +283,7 @@ func (api *API) loginRequest(ctx context.Context, rw http.ResponseWriter, req co httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ Message: "Internal error.", }) - return user, database.GetAuthorizationUserRolesRow{}, false + return user, rbac.Subject{}, false } // If the user doesn't exist, it will be a default struct. @@ -306,7 +293,7 @@ func (api *API) loginRequest(ctx context.Context, rw http.ResponseWriter, req co httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ Message: "Internal error.", }) - return user, database.GetAuthorizationUserRolesRow{}, false + return user, rbac.Subject{}, false } if !equal { @@ -315,7 +302,7 @@ func (api *API) loginRequest(ctx context.Context, rw http.ResponseWriter, req co httpapi.Write(ctx, rw, http.StatusUnauthorized, codersdk.Response{ Message: "Incorrect email or password.", }) - return user, database.GetAuthorizationUserRolesRow{}, false + return user, rbac.Subject{}, false } // If password authentication is disabled and the user does not have the @@ -324,14 +311,14 @@ func (api *API) loginRequest(ctx context.Context, rw http.ResponseWriter, req co httpapi.Write(ctx, rw, http.StatusForbidden, codersdk.Response{ Message: "Password authentication is disabled.", }) - return user, database.GetAuthorizationUserRolesRow{}, false + return user, rbac.Subject{}, false } if user.LoginType != database.LoginTypePassword { httpapi.Write(ctx, rw, http.StatusForbidden, codersdk.Response{ Message: fmt.Sprintf("Incorrect login type, attempting to use %q but user is of login type %q", database.LoginTypePassword, user.LoginType), }) - return user, database.GetAuthorizationUserRolesRow{}, false + return user, rbac.Subject{}, false } if user.Status == database.UserStatusDormant { @@ -346,29 +333,28 @@ func (api *API) loginRequest(ctx context.Context, rw http.ResponseWriter, req co httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ Message: "Internal error occurred. Try again later, or contact an admin for assistance.", }) - return user, database.GetAuthorizationUserRolesRow{}, false + return user, rbac.Subject{}, false } } - //nolint:gocritic // System needs to fetch user roles in order to login user. - roles, err := api.Database.GetAuthorizationUserRoles(dbauthz.AsSystemRestricted(ctx), user.ID) + subject, userStatus, err := httpmw.UserRBACSubject(ctx, api.Database, user.ID, rbac.ScopeAll) if err != nil { logger.Error(ctx, "unable to fetch authorization user roles", slog.Error(err)) httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ Message: "Internal error.", }) - return user, database.GetAuthorizationUserRolesRow{}, false + return user, rbac.Subject{}, false } // If the user logged into a suspended account, reject the login request. - if roles.Status != database.UserStatusActive { + if userStatus != database.UserStatusActive { httpapi.Write(ctx, rw, http.StatusUnauthorized, codersdk.Response{ - Message: fmt.Sprintf("Your account is %s. Contact an admin to reactivate your account.", roles.Status), + Message: fmt.Sprintf("Your account is %s. Contact an admin to reactivate your account.", userStatus), }) - return user, database.GetAuthorizationUserRolesRow{}, false + return user, rbac.Subject{}, false } - return user, roles, true + return user, subject, true } // Clear the user's session cookie. diff --git a/enterprise/coderd/userauth_test.go b/enterprise/coderd/userauth_test.go index f30474d607319..e4d397e35b66a 100644 --- a/enterprise/coderd/userauth_test.go +++ b/enterprise/coderd/userauth_test.go @@ -683,6 +683,49 @@ func TestGroupSync(t *testing.T) { } } +func TestUserLogin(t *testing.T) { + t.Run("CustomRole", func(t *testing.T) { + t.Parallel() + dv := coderdtest.DeploymentValues(t) + dv.Experiments = []string{string(codersdk.ExperimentCustomRoles)} + ownerClient, owner := coderdenttest.New(t, &coderdenttest.Options{ + Options: &coderdtest.Options{ + DeploymentValues: dv, + }, + LicenseOptions: &coderdenttest.LicenseOptions{ + Features: license.Features{ + codersdk.FeatureCustomRoles: 1, + }, + }, + }) + + ctx := testutil.Context(t, testutil.WaitShort) + //nolint:gocritic // owner required + customRole, err := ownerClient.PatchOrganizationRole(ctx, owner.OrganizationID, codersdk.Role{ + Name: "custom-role", + OrganizationID: owner.OrganizationID.String(), + OrganizationPermissions: []codersdk.Permission{}, + }) + require.NoError(t, err, "create custom role") + + anotherClient, anotherUser := coderdtest.CreateAnotherUserMutators(t, ownerClient, owner.OrganizationID, []rbac.RoleIdentifier{ + { + Name: customRole.Name, + OrganizationID: owner.OrganizationID, + }, + }, func(r *codersdk.CreateUserRequest) { + r.Password = "" + r.UserLoginType = codersdk.LoginTypeNone + }) + + _, err = anotherClient.LoginWithPassword(context.Background(), codersdk.LoginWithPasswordRequest{ + Email: anotherUser.Email, + Password: "SomeSecurePassword!", + }) + require.Error(t, err) + }) +} + // oidcTestRunner is just a helper to setup and run oidc tests. // An actual Coderd instance is used to run the tests. type oidcTestRunner struct { From cb7d004b08a7b1ad8db788bca319eac2e94307eb Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Fri, 21 Jun 2024 09:56:45 -0500 Subject: [PATCH 5/5] add unit test for deleted role --- enterprise/coderd/userauth_test.go | 57 +++++++++++++++++++++++++++--- 1 file changed, 52 insertions(+), 5 deletions(-) diff --git a/enterprise/coderd/userauth_test.go b/enterprise/coderd/userauth_test.go index e4d397e35b66a..a5f4214db48b5 100644 --- a/enterprise/coderd/userauth_test.go +++ b/enterprise/coderd/userauth_test.go @@ -15,6 +15,7 @@ import ( "github.com/coder/coder/v2/coderd/coderdtest/oidctest" "github.com/coder/coder/v2/coderd/database" "github.com/coder/coder/v2/coderd/database/dbauthz" + "github.com/coder/coder/v2/coderd/database/dbtestutil" "github.com/coder/coder/v2/coderd/rbac" "github.com/coder/coder/v2/coderd/util/slice" "github.com/coder/coder/v2/codersdk" @@ -683,7 +684,10 @@ func TestGroupSync(t *testing.T) { } } -func TestUserLogin(t *testing.T) { +func TestEnterpriseUserLogin(t *testing.T) { + t.Parallel() + + // Login to a user with a custom organization role set. t.Run("CustomRole", func(t *testing.T) { t.Parallel() dv := coderdtest.DeploymentValues(t) @@ -714,15 +718,58 @@ func TestUserLogin(t *testing.T) { OrganizationID: owner.OrganizationID, }, }, func(r *codersdk.CreateUserRequest) { - r.Password = "" - r.UserLoginType = codersdk.LoginTypeNone + r.Password = "SomeSecurePassword!" + r.UserLoginType = codersdk.LoginTypePassword + }) + + _, err = anotherClient.LoginWithPassword(ctx, codersdk.LoginWithPasswordRequest{ + Email: anotherUser.Email, + Password: "SomeSecurePassword!", + }) + require.NoError(t, err) + }) + + // Login to a user with a custom organization role that no longer exists + t.Run("DeletedRole", func(t *testing.T) { + t.Parallel() + + // The dbauthz layer protects against deleted roles. So use the underlying + // database directly to corrupt it. + rawDB, pubsub := dbtestutil.NewDB(t) + + dv := coderdtest.DeploymentValues(t) + dv.Experiments = []string{string(codersdk.ExperimentCustomRoles)} + ownerClient, owner := coderdenttest.New(t, &coderdenttest.Options{ + Options: &coderdtest.Options{ + DeploymentValues: dv, + Database: rawDB, + Pubsub: pubsub, + }, + LicenseOptions: &coderdenttest.LicenseOptions{ + Features: license.Features{ + codersdk.FeatureCustomRoles: 1, + }, + }, + }) + + anotherClient, anotherUser := coderdtest.CreateAnotherUserMutators(t, ownerClient, owner.OrganizationID, nil, func(r *codersdk.CreateUserRequest) { + r.Password = "SomeSecurePassword!" + r.UserLoginType = codersdk.LoginTypePassword + }) + + ctx := testutil.Context(t, testutil.WaitShort) + _, err := rawDB.UpdateMemberRoles(ctx, database.UpdateMemberRolesParams{ + GrantedRoles: []string{"not-exists"}, + UserID: anotherUser.ID, + OrgID: owner.OrganizationID, }) + require.NoError(t, err, "assign not-exists role") - _, err = anotherClient.LoginWithPassword(context.Background(), codersdk.LoginWithPasswordRequest{ + _, err = anotherClient.LoginWithPassword(ctx, codersdk.LoginWithPasswordRequest{ Email: anotherUser.Email, Password: "SomeSecurePassword!", }) - require.Error(t, err) + require.NoError(t, err) }) }