Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Commit0e933f0

Browse files
authored
chore: refactor user -> rbac.subject into a function (#13624)
* chore: refactor user subject logic to be in 1 place* test: implement test to assert deleted custom roles are omitted* add unit test for deleted role
1 parent3ef12ac commit0e933f0

File tree

7 files changed

+320
-86
lines changed

7 files changed

+320
-86
lines changed

‎coderd/httpmw/apikey.go

Lines changed: 27 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -406,16 +406,15 @@ func ExtractAPIKey(rw http.ResponseWriter, r *http.Request, cfg ExtractAPIKeyCon
406406
// If the key is valid, we also fetch the user roles and status.
407407
// The roles are used for RBAC authorize checks, and the status
408408
// is to block 'suspended' users from accessing the platform.
409-
//nolint:gocritic // system needs to update user roles
410-
roles,err:=cfg.DB.GetAuthorizationUserRoles(dbauthz.AsSystemRestricted(ctx),key.UserID)
409+
actor,userStatus,err:=UserRBACSubject(ctx,cfg.DB,key.UserID,rbac.ScopeName(key.Scope))
411410
iferr!=nil {
412411
returnwrite(http.StatusUnauthorized, codersdk.Response{
413412
Message:internalErrorMessage,
414413
Detail:fmt.Sprintf("Internal error fetching user's roles. %s",err.Error()),
415414
})
416415
}
417416

418-
ifroles.Status==database.UserStatusDormant {
417+
ifuserStatus==database.UserStatusDormant {
419418
// If coder confirms that the dormant user is valid, it can switch their account to active.
420419
// nolint:gocritic
421420
u,err:=cfg.DB.UpdateUserStatus(dbauthz.AsSystemRestricted(ctx), database.UpdateUserStatusParams{
@@ -429,47 +428,50 @@ func ExtractAPIKey(rw http.ResponseWriter, r *http.Request, cfg ExtractAPIKeyCon
429428
Detail:fmt.Sprintf("can't activate a dormant user: %s",err.Error()),
430429
})
431430
}
432-
roles.Status=u.Status
431+
userStatus=u.Status
433432
}
434433

435-
ifroles.Status!=database.UserStatusActive {
434+
ifuserStatus!=database.UserStatusActive {
436435
returnwrite(http.StatusUnauthorized, codersdk.Response{
437-
Message:fmt.Sprintf("User is not active (status = %q). Contact an admin to reactivate your account.",roles.Status),
436+
Message:fmt.Sprintf("User is not active (status = %q). Contact an admin to reactivate your account.",userStatus),
438437
})
439438
}
440439

440+
ifcfg.PostAuthAdditionalHeadersFunc!=nil {
441+
cfg.PostAuthAdditionalHeadersFunc(actor,rw.Header())
442+
}
443+
444+
returnkey,&actor,true
445+
}
446+
447+
// UserRBACSubject fetches a user's rbac.Subject from the database. It pulls all roles from both
448+
// site and organization scopes. It also pulls the groups, and the user's status.
449+
funcUserRBACSubject(ctx context.Context,db database.Store,userID uuid.UUID,scope rbac.ExpandableScope) (rbac.Subject, database.UserStatus,error) {
450+
//nolint:gocritic // system needs to update user roles
451+
roles,err:=db.GetAuthorizationUserRoles(dbauthz.AsSystemRestricted(ctx),userID)
452+
iferr!=nil {
453+
return rbac.Subject{},"",xerrors.Errorf("get authorization user roles: %w",err)
454+
}
455+
441456
roleNames,err:=roles.RoleNames()
442457
iferr!=nil {
443-
returnwrite(http.StatusInternalServerError, codersdk.Response{
444-
Message:"Internal Server Error",
445-
Detail:err.Error(),
446-
})
458+
return rbac.Subject{},"",xerrors.Errorf("expand role names: %w",err)
447459
}
448460

449461
//nolint:gocritic // Permission to lookup custom roles the user has assigned.
450-
rbacRoles,err:=rolestore.Expand(dbauthz.AsSystemRestricted(ctx),cfg.DB,roleNames)
462+
rbacRoles,err:=rolestore.Expand(dbauthz.AsSystemRestricted(ctx),db,roleNames)
451463
iferr!=nil {
452-
returnwrite(http.StatusInternalServerError, codersdk.Response{
453-
Message:"Failed to expand authenticated user roles",
454-
Detail:err.Error(),
455-
Validations:nil,
456-
})
464+
return rbac.Subject{},"",xerrors.Errorf("expand role names: %w",err)
457465
}
458466

459-
// Actor is the user's authorization context.
460467
actor:= rbac.Subject{
461468
FriendlyName:roles.Username,
462-
ID:key.UserID.String(),
469+
ID:userID.String(),
463470
Roles:rbacRoles,
464471
Groups:roles.Groups,
465-
Scope:rbac.ScopeName(key.Scope),
472+
Scope:scope,
466473
}.WithCachedASTValue()
467-
468-
ifcfg.PostAuthAdditionalHeadersFunc!=nil {
469-
cfg.PostAuthAdditionalHeadersFunc(actor,rw.Header())
470-
}
471-
472-
returnkey,&actor,true
474+
returnactor,roles.Status,nil
473475
}
474476

475477
// APITokenFromRequest returns the api token from the request.

‎coderd/httpmw/apikey_test.go

Lines changed: 169 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,16 +14,20 @@ import (
1414
"testing"
1515
"time"
1616

17+
"github.com/google/uuid"
1718
"github.com/stretchr/testify/assert"
1819
"github.com/stretchr/testify/require"
20+
"golang.org/x/exp/slices"
1921
"golang.org/x/oauth2"
2022

2123
"github.com/coder/coder/v2/coderd/database"
24+
"github.com/coder/coder/v2/coderd/database/dbauthz"
2225
"github.com/coder/coder/v2/coderd/database/dbgen"
2326
"github.com/coder/coder/v2/coderd/database/dbmem"
2427
"github.com/coder/coder/v2/coderd/database/dbtime"
2528
"github.com/coder/coder/v2/coderd/httpapi"
2629
"github.com/coder/coder/v2/coderd/httpmw"
30+
"github.com/coder/coder/v2/coderd/rbac"
2731
"github.com/coder/coder/v2/codersdk"
2832
"github.com/coder/coder/v2/cryptorand"
2933
"github.com/coder/coder/v2/testutil"
@@ -38,6 +42,37 @@ func randomAPIKeyParts() (id string, secret string) {
3842
funcTestAPIKey(t*testing.T) {
3943
t.Parallel()
4044

45+
// assertActorOk asserts all the properties of the user auth are ok.
46+
assertActorOk:=func(t*testing.T,r*http.Request) {
47+
t.Helper()
48+
49+
actor,ok:=dbauthz.ActorFromContext(r.Context())
50+
assert.True(t,ok,"dbauthz actor ok")
51+
ifok {
52+
_,err:=actor.Roles.Expand()
53+
assert.NoError(t,err,"actor roles ok")
54+
55+
_,err=actor.Scope.Expand()
56+
assert.NoError(t,err,"actor scope ok")
57+
58+
err=actor.RegoValueOk()
59+
assert.NoError(t,err,"actor rego ok")
60+
}
61+
62+
auth,ok:=httpmw.UserAuthorizationOptional(r)
63+
assert.True(t,ok,"httpmw auth ok")
64+
ifok {
65+
_,err:=auth.Roles.Expand()
66+
assert.NoError(t,err,"auth roles ok")
67+
68+
_,err=auth.Scope.Expand()
69+
assert.NoError(t,err,"auth scope ok")
70+
71+
err=auth.RegoValueOk()
72+
assert.NoError(t,err,"auth rego ok")
73+
}
74+
}
75+
4176
successHandler:=http.HandlerFunc(func(rw http.ResponseWriter,r*http.Request) {
4277
// Only called if the API key passes through the handler.
4378
httpapi.Write(context.Background(),rw,http.StatusOK, codersdk.Response{
@@ -256,6 +291,7 @@ func TestAPIKey(t *testing.T) {
256291
})(http.HandlerFunc(func(rw http.ResponseWriter,r*http.Request) {
257292
// Checks that it exists on the context!
258293
_=httpmw.APIKey(r)
294+
assertActorOk(t,r)
259295
httpapi.Write(r.Context(),rw,http.StatusOK, codersdk.Response{
260296
Message:"It worked!",
261297
})
@@ -296,6 +332,7 @@ func TestAPIKey(t *testing.T) {
296332
// Checks that it exists on the context!
297333
apiKey:=httpmw.APIKey(r)
298334
assert.Equal(t,database.APIKeyScopeApplicationConnect,apiKey.Scope)
335+
assertActorOk(t,r)
299336

300337
httpapi.Write(r.Context(),rw,http.StatusOK, codersdk.Response{
301338
Message:"it worked!",
@@ -330,6 +367,8 @@ func TestAPIKey(t *testing.T) {
330367
})(http.HandlerFunc(func(rw http.ResponseWriter,r*http.Request) {
331368
// Checks that it exists on the context!
332369
_=httpmw.APIKey(r)
370+
assertActorOk(t,r)
371+
333372
httpapi.Write(r.Context(),rw,http.StatusOK, codersdk.Response{
334373
Message:"It worked!",
335374
})
@@ -633,7 +672,7 @@ func TestAPIKey(t *testing.T) {
633672
require.Equal(t,sentAPIKey.LoginType,gotAPIKey.LoginType)
634673
})
635674

636-
t.Run("MissongConfig",func(t*testing.T) {
675+
t.Run("MissingConfig",func(t*testing.T) {
637676
t.Parallel()
638677
var (
639678
db=dbmem.New()
@@ -667,4 +706,133 @@ func TestAPIKey(t *testing.T) {
667706
out,_:=io.ReadAll(res.Body)
668707
require.Contains(t,string(out),"Unable to refresh")
669708
})
709+
710+
t.Run("CustomRoles",func(t*testing.T) {
711+
t.Parallel()
712+
var (
713+
db=dbmem.New()
714+
org=dbgen.Organization(t,db, database.Organization{})
715+
customRole=dbgen.CustomRole(t,db, database.CustomRole{
716+
Name:"custom-role",
717+
OrgPermissions: []database.CustomRolePermission{},
718+
OrganizationID: uuid.NullUUID{
719+
UUID:org.ID,
720+
Valid:true,
721+
},
722+
})
723+
user=dbgen.User(t,db, database.User{
724+
RBACRoles: []string{},
725+
})
726+
_=dbgen.OrganizationMember(t,db, database.OrganizationMember{
727+
UserID:user.ID,
728+
OrganizationID:org.ID,
729+
CreatedAt: time.Time{},
730+
UpdatedAt: time.Time{},
731+
Roles: []string{
732+
rbac.RoleOrgAdmin(),
733+
customRole.Name,
734+
},
735+
})
736+
_,token=dbgen.APIKey(t,db, database.APIKey{
737+
UserID:user.ID,
738+
ExpiresAt:dbtime.Now().AddDate(0,0,1),
739+
})
740+
741+
r=httptest.NewRequest("GET","/",nil)
742+
rw=httptest.NewRecorder()
743+
)
744+
r.Header.Set(codersdk.SessionTokenHeader,token)
745+
746+
httpmw.ExtractAPIKeyMW(httpmw.ExtractAPIKeyConfig{
747+
DB:db,
748+
RedirectToLogin:false,
749+
})(http.HandlerFunc(func(rw http.ResponseWriter,r*http.Request) {
750+
assertActorOk(t,r)
751+
752+
auth:=httpmw.UserAuthorization(r)
753+
754+
roles,err:=auth.Roles.Expand()
755+
assert.NoError(t,err,"expand user roles")
756+
// Assert built in org role
757+
assert.True(t,slices.ContainsFunc(roles,func(role rbac.Role)bool {
758+
returnrole.Identifier.Name==rbac.RoleOrgAdmin()&&role.Identifier.OrganizationID==org.ID
759+
}),"org admin role")
760+
// Assert custom role
761+
assert.True(t,slices.ContainsFunc(roles,func(role rbac.Role)bool {
762+
returnrole.Identifier.Name==customRole.Name&&role.Identifier.OrganizationID==org.ID
763+
}),"custom org role")
764+
765+
httpapi.Write(r.Context(),rw,http.StatusOK, codersdk.Response{
766+
Message:"It worked!",
767+
})
768+
})).ServeHTTP(rw,r)
769+
res:=rw.Result()
770+
deferres.Body.Close()
771+
require.Equal(t,http.StatusOK,res.StatusCode)
772+
})
773+
774+
// There is no sql foreign key constraint to require all assigned roles
775+
// still exist in the database. We need to handle deleted roles.
776+
t.Run("RoleNotExists",func(t*testing.T) {
777+
t.Parallel()
778+
var (
779+
roleNotExistsName="role-not-exists"
780+
db=dbmem.New()
781+
org=dbgen.Organization(t,db, database.Organization{})
782+
user=dbgen.User(t,db, database.User{
783+
RBACRoles: []string{
784+
// Also provide an org not exists. In practice this makes no sense
785+
// to store org roles in the user table, but there is no org to
786+
// store it in. So just throw this here for even more unexpected
787+
// behavior handling!
788+
rbac.RoleIdentifier{Name:roleNotExistsName,OrganizationID:uuid.New()}.String(),
789+
},
790+
})
791+
_=dbgen.OrganizationMember(t,db, database.OrganizationMember{
792+
UserID:user.ID,
793+
OrganizationID:org.ID,
794+
CreatedAt: time.Time{},
795+
UpdatedAt: time.Time{},
796+
Roles: []string{
797+
rbac.RoleOrgAdmin(),
798+
roleNotExistsName,
799+
},
800+
})
801+
_,token=dbgen.APIKey(t,db, database.APIKey{
802+
UserID:user.ID,
803+
ExpiresAt:dbtime.Now().AddDate(0,0,1),
804+
})
805+
806+
r=httptest.NewRequest("GET","/",nil)
807+
rw=httptest.NewRecorder()
808+
)
809+
r.Header.Set(codersdk.SessionTokenHeader,token)
810+
811+
httpmw.ExtractAPIKeyMW(httpmw.ExtractAPIKeyConfig{
812+
DB:db,
813+
RedirectToLogin:false,
814+
})(http.HandlerFunc(func(rw http.ResponseWriter,r*http.Request) {
815+
assertActorOk(t,r)
816+
auth:=httpmw.UserAuthorization(r)
817+
818+
roles,err:=auth.Roles.Expand()
819+
assert.NoError(t,err,"expand user roles")
820+
// Assert built in org role
821+
assert.True(t,slices.ContainsFunc(roles,func(role rbac.Role)bool {
822+
returnrole.Identifier.Name==rbac.RoleOrgAdmin()&&role.Identifier.OrganizationID==org.ID
823+
}),"org admin role")
824+
825+
// Assert the role-not-exists is not returned
826+
assert.False(t,slices.ContainsFunc(roles,func(role rbac.Role)bool {
827+
returnrole.Identifier.Name==roleNotExistsName
828+
}),"role should not exist")
829+
830+
httpapi.Write(r.Context(),rw,http.StatusOK, codersdk.Response{
831+
Message:"It worked!",
832+
})
833+
})).ServeHTTP(rw,r)
834+
res:=rw.Result()
835+
deferres.Body.Close()
836+
require.Equal(t,http.StatusOK,res.StatusCode)
837+
})
670838
}

‎coderd/identityprovider/tokens.go

Lines changed: 6 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -209,27 +209,14 @@ func authorizationCodeGrant(ctx context.Context, db database.Store, app database
209209
}
210210

211211
// Grab the user roles so we can perform the exchange as the user.
212-
//nolint:gocritic // In the token exchange, there is no user actor.
213-
roles,err:=db.GetAuthorizationUserRoles(dbauthz.AsSystemRestricted(ctx),dbCode.UserID)
212+
actor,_,err:=httpmw.UserRBACSubject(ctx,db,dbCode.UserID,rbac.ScopeAll)
214213
iferr!=nil {
215-
return oauth2.Token{},err
216-
}
217-
218-
roleNames,err:=roles.RoleNames()
219-
iferr!=nil {
220-
return oauth2.Token{},xerrors.Errorf("role names: %w",err)
221-
}
222-
223-
userSubj:= rbac.Subject{
224-
ID:dbCode.UserID.String(),
225-
Roles:rbac.RoleIdentifiers(roleNames),
226-
Groups:roles.Groups,
227-
Scope:rbac.ScopeAll,
214+
return oauth2.Token{},xerrors.Errorf("fetch user actor: %w",err)
228215
}
229216

230217
// Do the actual token exchange in the database.
231218
err=db.InTx(func(tx database.Store)error {
232-
ctx:=dbauthz.As(ctx,userSubj)
219+
ctx:=dbauthz.As(ctx,actor)
233220
err=tx.DeleteOAuth2ProviderAppCodeByID(ctx,dbCode.ID)
234221
iferr!=nil {
235222
returnxerrors.Errorf("delete oauth2 app code: %w",err)
@@ -311,22 +298,10 @@ func refreshTokenGrant(ctx context.Context, db database.Store, app database.OAut
311298
iferr!=nil {
312299
return oauth2.Token{},err
313300
}
314-
//nolint:gocritic // There is no user yet so we must use the system.
315-
roles,err:=db.GetAuthorizationUserRoles(dbauthz.AsSystemRestricted(ctx),prevKey.UserID)
316-
iferr!=nil {
317-
return oauth2.Token{},err
318-
}
319301

320-
roleNames,err:=roles.RoleNames()
302+
actor,_,err:=httpmw.UserRBACSubject(ctx,db,prevKey.UserID,rbac.ScopeAll)
321303
iferr!=nil {
322-
return oauth2.Token{},xerrors.Errorf("role names: %w",err)
323-
}
324-
325-
userSubj:= rbac.Subject{
326-
ID:prevKey.UserID.String(),
327-
Roles:rbac.RoleIdentifiers(roleNames),
328-
Groups:roles.Groups,
329-
Scope:rbac.ScopeAll,
304+
return oauth2.Token{},xerrors.Errorf("fetch user actor: %w",err)
330305
}
331306

332307
// Generate a new refresh token.
@@ -351,7 +326,7 @@ func refreshTokenGrant(ctx context.Context, db database.Store, app database.OAut
351326

352327
// Replace the token.
353328
err=db.InTx(func(tx database.Store)error {
354-
ctx:=dbauthz.As(ctx,userSubj)
329+
ctx:=dbauthz.As(ctx,actor)
355330
err=tx.DeleteAPIKeyByID(ctx,prevKey.ID)// This cascades to the token.
356331
iferr!=nil {
357332
returnxerrors.Errorf("delete oauth2 app token: %w",err)

0 commit comments

Comments
 (0)
[8]ページ先頭
©2009-2025 Movatter.jp