Skip to content

Commit 7544e37

Browse files
EmyrkEmyrk
committed
fix: Group ACL list fixed
1 parent ba1953a commit 7544e37

File tree

8 files changed

+37
-27
lines changed

8 files changed

+37
-27
lines changed

‎coderd/authorize.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@ import (
1212

1313
func AuthorizeFilter[O rbac.Objecter](h *HTTPAuthorizer, r *http.Request, action rbac.Action, objects []O) ([]O, error) {
1414
roles := httpmw.UserAuthorization(r)
15-
objects, err := rbac.Filter(r.Context(), h.Authorizer, roles.ID.String(), roles.Roles, roles.Scope.ToRBAC(), action, objects)
15+
objects, err := rbac.Filter(r.Context(), h.Authorizer, roles.ID.String(), roles.Roles, roles.Groups, roles.Scope.ToRBAC(), action, objects)
1616
if err != nil {
1717
// Log the error as Filter should not be erroring.
1818
h.Logger.Error(r.Context(), "filter failed",
@@ -57,7 +57,7 @@ func (api *API) Authorize(r *http.Request, action rbac.Action, object rbac.Objec
5757
// }
5858
func (h *HTTPAuthorizer) Authorize(r *http.Request, action rbac.Action, object rbac.Objecter) bool {
5959
roles := httpmw.UserAuthorization(r)
60-
err := h.Authorizer.ByRoleName(r.Context(), roles.ID.String(), roles.Roles, roles.Scope.ToRBAC(), action, object.RBACObject())
60+
err := h.Authorizer.ByRoleName(r.Context(), roles.ID.String(), roles.Roles, roles.Groups, roles.Scope.ToRBAC(), action, object.RBACObject())
6161
if err != nil {
6262
// Log the errors for debugging
6363
internalError := new(rbac.UnauthorizedError)

‎coderd/coderdtest/authorize.go

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -498,6 +498,7 @@ func (a *AuthTester) Test(ctx context.Context, assertRoute map[string]RouteCheck
498498
type authCall struct {
499499
SubjectID string
500500
Roles []string
501+
Groups []string
501502
Scope rbac.Scope
502503
Action rbac.Action
503504
Object rbac.Object
@@ -510,24 +511,26 @@ type RecordingAuthorizer struct {
510511

511512
var _ rbac.Authorizer = (*RecordingAuthorizer)(nil)
512513

513-
func (r *RecordingAuthorizer) ByRoleName(_ context.Context, subjectID string, roleNames []string, scope rbac.Scope, action rbac.Action, object rbac.Object) error {
514+
func (r *RecordingAuthorizer) ByRoleName(_ context.Context, subjectID string, roleNames []string, groups []string, scope rbac.Scope, action rbac.Action, object rbac.Object) error {
514515
r.Called = &authCall{
515516
SubjectID: subjectID,
516517
Roles: roleNames,
518+
Groups: groups,
517519
Scope: scope,
518520
Action: action,
519521
Object: object,
520522
}
521523
return r.AlwaysReturn
522524
}
523525

524-
func (r *RecordingAuthorizer) PrepareByRoleName(_ context.Context, subjectID string, roles []string, scope rbac.Scope, action rbac.Action, _ string) (rbac.PreparedAuthorized, error) {
526+
func (r *RecordingAuthorizer) PrepareByRoleName(_ context.Context, subjectID string, roles []string, groups []string, scope rbac.Scope, action rbac.Action, _ string) (rbac.PreparedAuthorized, error) {
525527
return &fakePreparedAuthorizer{
526528
Original: r,
527529
SubjectID: subjectID,
528530
Roles: roles,
529531
Scope: scope,
530532
Action: action,
533+
Groups: groups,
531534
}, nil
532535
}
533536

@@ -539,10 +542,11 @@ type fakePreparedAuthorizer struct {
539542
Original *RecordingAuthorizer
540543
SubjectID string
541544
Roles []string
545+
Groups []string
542546
Scope rbac.Scope
543547
Action rbac.Action
544548
}
545549

546550
func (f *fakePreparedAuthorizer) Authorize(ctx context.Context, object rbac.Object) error {
547-
return f.Original.ByRoleName(ctx, f.SubjectID, f.Roles, f.Scope, f.Action, object)
551+
return f.Original.ByRoleName(ctx, f.SubjectID, f.Roles, f.Groups, f.Scope, f.Action, object)
548552
}

‎coderd/httpmw/apikey.go

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,7 @@ type Authorization struct {
4141
ID uuid.UUID
4242
Username string
4343
Roles []string
44+
Groups []string
4445
Scope database.APIKeyScope
4546
}
4647

@@ -336,6 +337,7 @@ func ExtractAPIKey(db database.Store, oauth *OAuth2Configs, redirectToLogin bool
336337
Username: roles.Username,
337338
Roles: roles.Roles,
338339
Scope: key.Scope,
340+
Groups: roles.Groups,
339341
})
340342

341343
next.ServeHTTP(rw, r.WithContext(ctx))

‎coderd/rbac/authz.go

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -13,8 +13,8 @@ import (
1313
)
1414

1515
type Authorizer interface {
16-
ByRoleName(ctx context.Context, subjectID string, roleNames []string, scope Scope, action Action, object Object) error
17-
PrepareByRoleName(ctx context.Context, subjectID string, roleNames []string, scope Scope, action Action, objectType string) (PreparedAuthorized, error)
16+
ByRoleName(ctx context.Context, subjectID string, roleNames []string, groups []string, scope Scope, action Action, object Object) error
17+
PrepareByRoleName(ctx context.Context, subjectID string, roleNames []string, groups []string, scope Scope, action Action, objectType string) (PreparedAuthorized, error)
1818
}
1919

2020
type PreparedAuthorized interface {
@@ -25,7 +25,7 @@ type PreparedAuthorized interface {
2525
// the elements the subject does not have permission for. This function slows
2626
// down if the list contains objects of multiple types. Attempt to only
2727
// filter objects of the same type for faster performance.
28-
func Filter[O Objecter](ctx context.Context, auth Authorizer, subjID string, subjRoles []string, scope Scope, action Action, objects []O) ([]O, error) {
28+
func Filter[O Objecter](ctx context.Context, auth Authorizer, subjID string, subjRoles []string, groups []string, scope Scope, action Action, objects []O) ([]O, error) {
2929
ctx, span := tracing.StartSpan(ctx, trace.WithAttributes(
3030
attribute.String("subject_id", subjID),
3131
attribute.StringSlice("subject_roles", subjRoles),
@@ -48,7 +48,7 @@ func Filter[O Objecter](ctx context.Context, auth Authorizer, subjID string, sub
4848
objectAuth, ok := prepared[object.RBACObject().Type]
4949
if !ok {
5050
var err error
51-
objectAuth, err = auth.PrepareByRoleName(ctx, subjID, subjRoles, scope, action, objectType)
51+
objectAuth, err = auth.PrepareByRoleName(ctx, subjID, subjRoles, groups, scope, action, objectType)
5252
if err != nil {
5353
return nil, xerrors.Errorf("prepare: %w", err)
5454
}
@@ -158,19 +158,19 @@ func (a RegoAuthorizer) Authorize(ctx context.Context, subjectID string, roles [
158158

159159
// Prepare will partially execute the rego policy leaving the object fields unknown (except for the type).
160160
// This will vastly speed up performance if batch authorization on the same type of objects is needed.
161-
func (RegoAuthorizer) Prepare(ctx context.Context, subjectID string, roles []Role, scope Scope, action Action, objectType string) (*PartialAuthorizer, error) {
161+
func (RegoAuthorizer) Prepare(ctx context.Context, subjectID string, roles []Role, groups []string, scope Scope, action Action, objectType string) (*PartialAuthorizer, error) {
162162
ctx, span := tracing.StartSpan(ctx)
163163
defer span.End()
164164

165-
auth, err := newPartialAuthorizer(ctx, subjectID, roles, scope, action, objectType)
165+
auth, err := newPartialAuthorizer(ctx, subjectID, roles, groups, scope, action, objectType)
166166
if err != nil {
167167
return nil, xerrors.Errorf("new partial authorizer: %w", err)
168168
}
169169

170170
return auth, nil
171171
}
172172

173-
func (a RegoAuthorizer) PrepareByRoleName(ctx context.Context, subjectID string, roleNames []string, scope Scope, action Action, objectType string) (PreparedAuthorized, error) {
173+
func (a RegoAuthorizer) PrepareByRoleName(ctx context.Context, subjectID string, roleNames []string, groups []string, scope Scope, action Action, objectType string) (PreparedAuthorized, error) {
174174
ctx, span := tracing.StartSpan(ctx)
175175
defer span.End()
176176

@@ -179,5 +179,5 @@ func (a RegoAuthorizer) PrepareByRoleName(ctx context.Context, subjectID string,
179179
return nil, err
180180
}
181181

182-
return a.Prepare(ctx, subjectID, roles, scope, action, objectType)
182+
return a.Prepare(ctx, subjectID, roles, groups, scope, action, objectType)
183183
}

‎coderd/rbac/authz_internal_test.go

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,8 @@ type subject struct {
1919
// For the unit test we want to pass in the roles directly, instead of just
2020
// by name. This allows us to test custom roles that do not exist in the product,
2121
// but test edge cases of the implementation.
22-
Roles []Role `json:"roles"`
22+
Roles []Role `json:"roles"`
23+
Groups []string `json:"groups"`
2324
}
2425

2526
type fakeObject struct {
@@ -162,7 +163,7 @@ func TestFilter(t *testing.T) {
162163
var allowedCount int
163164
for i, obj := range localObjects {
164165
obj.Type = tc.ObjectType
165-
err := auth.ByRoleName(ctx, tc.SubjectID, tc.Roles, scope, ActionRead, obj.RBACObject())
166+
err := auth.ByRoleName(ctx, tc.SubjectID, tc.Roles, []string{}, scope, ActionRead, obj.RBACObject())
166167
obj.Allowed = err == nil
167168
if err == nil {
168169
allowedCount++
@@ -171,7 +172,7 @@ func TestFilter(t *testing.T) {
171172
}
172173

173174
// Run by filter
174-
list, err := Filter(ctx, auth, tc.SubjectID, tc.Roles, scope, tc.Action, localObjects)
175+
list, err := Filter(ctx, auth, tc.SubjectID, tc.Roles, []string{}, scope, tc.Action, localObjects)
175176
require.NoError(t, err)
176177
require.Equal(t, allowedCount, len(list), "expected number of allowed")
177178
for _, obj := range list {
@@ -714,7 +715,7 @@ func testAuthorize(t *testing.T, name string, subject subject, sets ...[]authTes
714715
ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitShort)
715716
t.Cleanup(cancel)
716717

717-
authError := authorizer.Authorize(ctx, subject.UserID, subject.Roles, a, c.resource)
718+
authError := authorizer.Authorize(ctx, subject.UserID, subject.Roles, subject.Groups, a, c.resource)
718719

719720
// Logging only
720721
if authError != nil {
@@ -739,7 +740,7 @@ func testAuthorize(t *testing.T, name string, subject subject, sets ...[]authTes
739740
assert.Error(t, authError, "expected unauthorized")
740741
}
741742

742-
partialAuthz, err := authorizer.Prepare(ctx, subject.UserID, subject.Roles, ScopeAll, a, c.resource.Type)
743+
partialAuthz, err := authorizer.Prepare(ctx, subject.UserID, subject.Roles, subject.Groups, ScopeAll, a, c.resource.Type)
743744
require.NoError(t, err, "make prepared authorizer")
744745

745746
// Also check the rego policy can form a valid partial query result.

‎coderd/rbac/builtin_test.go

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,7 @@ func BenchmarkRBACFilter(b *testing.B) {
3232
benchCases := []struct {
3333
Name string
3434
Roles []string
35+
Groups []string
3536
UserID uuid.UUID
3637
Scope rbac.Scope
3738
}{
@@ -90,7 +91,7 @@ func BenchmarkRBACFilter(b *testing.B) {
9091
b.Run(c.Name, func(b *testing.B) {
9192
objects := benchmarkSetup(orgs, users, b.N)
9293
b.ResetTimer()
93-
allowed, err := rbac.Filter(context.Background(), authorizer, c.UserID.String(), c.Roles, c.Scope, rbac.ActionRead, objects)
94+
allowed, err := rbac.Filter(context.Background(), authorizer, c.UserID.String(), c.Roles, c.Groups, c.Scope, rbac.ActionRead, objects)
9495
require.NoError(b, err)
9596
var _ = allowed
9697
})
@@ -114,6 +115,7 @@ type authSubject struct {
114115
Name string
115116
UserID string
116117
Roles []string
118+
Groups []string
117119
}
118120

119121
func TestRolePermissions(t *testing.T) {
@@ -359,7 +361,7 @@ func TestRolePermissions(t *testing.T) {
359361
delete(remainingSubjs, subj.Name)
360362
msg := fmt.Sprintf("%s as %q doing %q on %q", c.Name, subj.Name, action, c.Resource.Type)
361363
// TODO: scopey
362-
err := auth.ByRoleName(context.Background(), subj.UserID, subj.Roles, rbac.ScopeAll, action, c.Resource)
364+
err := auth.ByRoleName(context.Background(), subj.UserID, subj.Roles, subj.Groups, rbac.ScopeAll, action, c.Resource)
363365
if result {
364366
assert.NoError(t, err, fmt.Sprintf("Should pass: %s", msg))
365367
} else {

‎coderd/rbac/partial.go

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -35,11 +35,11 @@ func (pa *PartialAuthorizer) Authorize(ctx context.Context, object Object) error
3535
return nil
3636
}
3737

38-
func newPartialAuthorizer(ctx context.Context, subjectID string, roles []Role, scope Scope, action Action, objectType string) (*PartialAuthorizer, error) {
38+
func newPartialAuthorizer(ctx context.Context, subjectID string, roles []Role, groups []string, scope Scope, action Action, objectType string) (*PartialAuthorizer, error) {
3939
ctx, span := tracing.StartSpan(ctx)
4040
defer span.End()
4141

42-
pAuth, err := newSubPartialAuthorizer(ctx, subjectID, roles, action, objectType)
42+
pAuth, err := newSubPartialAuthorizer(ctx, subjectID, roles, groups, action, objectType)
4343
if err != nil {
4444
return nil, err
4545
}
@@ -51,7 +51,7 @@ func newPartialAuthorizer(ctx context.Context, subjectID string, roles []Role, s
5151
return nil, xerrors.Errorf("unknown scope %q", scope)
5252
}
5353

54-
scopeAuth, err = newSubPartialAuthorizer(ctx, subjectID, []Role{scopeRole}, action, objectType)
54+
scopeAuth, err = newSubPartialAuthorizer(ctx, subjectID, []Role{scopeRole}, groups, action, objectType)
5555
if err != nil {
5656
return nil, err
5757
}
@@ -78,14 +78,15 @@ type subPartialAuthorizer struct {
7878
alwaysTrue bool
7979
}
8080

81-
func newSubPartialAuthorizer(ctx context.Context, subjectID string, roles []Role, action Action, objectType string) (*subPartialAuthorizer, error) {
81+
func newSubPartialAuthorizer(ctx context.Context, subjectID string, roles []Role, groups []string, action Action, objectType string) (*subPartialAuthorizer, error) {
8282
ctx, span := tracing.StartSpan(ctx)
8383
defer span.End()
8484

8585
input := map[string]interface{}{
8686
"subject": authSubject{
87-
ID: subjectID,
88-
Roles: roles,
87+
ID: subjectID,
88+
Roles: roles,
89+
Groups: groups,
8990
},
9091
"object": map[string]string{
9192
"type": objectType,

‎coderd/roles.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -70,7 +70,7 @@ func (api *API) checkPermissions(rw http.ResponseWriter, r *http.Request) {
7070
if v.Object.OwnerID == "me" {
7171
v.Object.OwnerID = roles.ID.String()
7272
}
73-
err := api.Authorizer.ByRoleName(r.Context(), roles.ID.String(), roles.Roles, apiKey.Scope.ToRBAC(), rbac.Action(v.Action),
73+
err := api.Authorizer.ByRoleName(r.Context(), roles.ID.String(), roles.Roles, roles.Groups, apiKey.Scope.ToRBAC(), rbac.Action(v.Action),
7474
rbac.Object{
7575
Owner: v.Object.OwnerID,
7676
OrgID: v.Object.OrganizationID,

0 commit comments

Comments
 (0)