fix: Group ACL list fixed

coder · sreya · Oct 10, 2022 · Sep 13, 2022 · Sep 13, 2022 · Sep 13, 2022
commit 7544e373c301c5b2b43977cf57a3710c242e241c
diff --git a/coderd/authorize.go b/coderd/authorize.go
@@ -12,7 +12,7 @@ import (
 
 func AuthorizeFilter[O rbac.Objecter](h *HTTPAuthorizer, r *http.Request, action rbac.Action, objects []O) ([]O, error) {
 	roles := httpmw.UserAuthorization(r)
-	objects, err := rbac.Filter(r.Context(), h.Authorizer, roles.ID.String(), roles.Roles, roles.Scope.ToRBAC(), action, objects)
+	objects, err := rbac.Filter(r.Context(), h.Authorizer, roles.ID.String(), roles.Roles, roles.Groups, roles.Scope.ToRBAC(), action, objects)
 	if err != nil {
 		// Log the error as Filter should not be erroring.
 		h.Logger.Error(r.Context(), "filter failed",
@@ -57,7 +57,7 @@ func (api *API) Authorize(r *http.Request, action rbac.Action, object rbac.Objec
 //	}
 func (h *HTTPAuthorizer) Authorize(r *http.Request, action rbac.Action, object rbac.Objecter) bool {
 	roles := httpmw.UserAuthorization(r)
-	err := h.Authorizer.ByRoleName(r.Context(), roles.ID.String(), roles.Roles, roles.Scope.ToRBAC(), action, object.RBACObject())
+	err := h.Authorizer.ByRoleName(r.Context(), roles.ID.String(), roles.Roles, roles.Groups, roles.Scope.ToRBAC(), action, object.RBACObject())
 	if err != nil {
 		// Log the errors for debugging
 		internalError := new(rbac.UnauthorizedError)

diff --git a/coderd/coderdtest/authorize.go b/coderd/coderdtest/authorize.go
@@ -498,6 +498,7 @@ func (a *AuthTester) Test(ctx context.Context, assertRoute map[string]RouteCheck
 type authCall struct {
 	SubjectID string
 	Roles     []string
+	Groups    []string
 	Scope     rbac.Scope
 	Action    rbac.Action
 	Object    rbac.Object
@@ -510,24 +511,26 @@ type RecordingAuthorizer struct {
 
 var _ rbac.Authorizer = (*RecordingAuthorizer)(nil)
 
-func (r *RecordingAuthorizer) ByRoleName(_ context.Context, subjectID string, roleNames []string, scope rbac.Scope, action rbac.Action, object rbac.Object) error {
+func (r *RecordingAuthorizer) ByRoleName(_ context.Context, subjectID string, roleNames []string, groups []string, scope rbac.Scope, action rbac.Action, object rbac.Object) error {
 	r.Called = &authCall{
 		SubjectID: subjectID,
 		Roles:     roleNames,
+		Groups:    groups,
 		Scope:     scope,
 		Action:    action,
 		Object:    object,
 	}
 	return r.AlwaysReturn
 }
 
-func (r *RecordingAuthorizer) PrepareByRoleName(_ context.Context, subjectID string, roles []string, scope rbac.Scope, action rbac.Action, _ string) (rbac.PreparedAuthorized, error) {
+func (r *RecordingAuthorizer) PrepareByRoleName(_ context.Context, subjectID string, roles []string, groups []string, scope rbac.Scope, action rbac.Action, _ string) (rbac.PreparedAuthorized, error) {
 	return &fakePreparedAuthorizer{
 		Original:  r,
 		SubjectID: subjectID,
 		Roles:     roles,
 		Scope:     scope,
 		Action:    action,
+		Groups:    groups,
 	}, nil
 }
 
@@ -539,10 +542,11 @@ type fakePreparedAuthorizer struct {
 	Original  *RecordingAuthorizer
 	SubjectID string
 	Roles     []string
+	Groups    []string
 	Scope     rbac.Scope
 	Action    rbac.Action
 }
 
 func (f *fakePreparedAuthorizer) Authorize(ctx context.Context, object rbac.Object) error {
-	return f.Original.ByRoleName(ctx, f.SubjectID, f.Roles, f.Scope, f.Action, object)
+	return f.Original.ByRoleName(ctx, f.SubjectID, f.Roles, f.Groups, f.Scope, f.Action, object)
 }
diff --git a/coderd/httpmw/apikey.go b/coderd/httpmw/apikey.go
@@ -41,6 +41,7 @@ type Authorization struct {
 	ID       uuid.UUID
 	Username string
 	Roles    []string
+	Groups   []string
 	Scope    database.APIKeyScope
 }
 
@@ -336,6 +337,7 @@ func ExtractAPIKey(db database.Store, oauth *OAuth2Configs, redirectToLogin bool
 				Username: roles.Username,
 				Roles:    roles.Roles,
 				Scope:    key.Scope,
+				Groups:   roles.Groups,
 			})
 
 			next.ServeHTTP(rw, r.WithContext(ctx))

diff --git a/coderd/rbac/authz.go b/coderd/rbac/authz.go
@@ -13,8 +13,8 @@ import (
 )
 
 type Authorizer interface {
-	ByRoleName(ctx context.Context, subjectID string, roleNames []string, scope Scope, action Action, object Object) error
-	PrepareByRoleName(ctx context.Context, subjectID string, roleNames []string, scope Scope, action Action, objectType string) (PreparedAuthorized, error)
+	ByRoleName(ctx context.Context, subjectID string, roleNames []string, groups []string, scope Scope, action Action, object Object) error
+	PrepareByRoleName(ctx context.Context, subjectID string, roleNames []string, groups []string, scope Scope, action Action, objectType string) (PreparedAuthorized, error)
 }
 
 type PreparedAuthorized interface {
@@ -25,7 +25,7 @@ type PreparedAuthorized interface {
 // the elements the subject does not have permission for. This function slows
 // down if the list contains objects of multiple types. Attempt to only
 // filter objects of the same type for faster performance.
-func Filter[O Objecter](ctx context.Context, auth Authorizer, subjID string, subjRoles []string, scope Scope, action Action, objects []O) ([]O, error) {
+func Filter[O Objecter](ctx context.Context, auth Authorizer, subjID string, subjRoles []string, groups []string, scope Scope, action Action, objects []O) ([]O, error) {
 	ctx, span := tracing.StartSpan(ctx, trace.WithAttributes(
 		attribute.String("subject_id", subjID),
 		attribute.StringSlice("subject_roles", subjRoles),
@@ -48,7 +48,7 @@ func Filter[O Objecter](ctx context.Context, auth Authorizer, subjID string, sub
 		objectAuth, ok := prepared[object.RBACObject().Type]
 		if !ok {
 			var err error
-			objectAuth, err = auth.PrepareByRoleName(ctx, subjID, subjRoles, scope, action, objectType)
+			objectAuth, err = auth.PrepareByRoleName(ctx, subjID, subjRoles, groups, scope, action, objectType)
 			if err != nil {
 				return nil, xerrors.Errorf("prepare: %w", err)
 			}
@@ -158,19 +158,19 @@ func (a RegoAuthorizer) Authorize(ctx context.Context, subjectID string, roles [
 
 // Prepare will partially execute the rego policy leaving the object fields unknown (except for the type).
 // This will vastly speed up performance if batch authorization on the same type of objects is needed.
-func (RegoAuthorizer) Prepare(ctx context.Context, subjectID string, roles []Role, scope Scope, action Action, objectType string) (*PartialAuthorizer, error) {
+func (RegoAuthorizer) Prepare(ctx context.Context, subjectID string, roles []Role, groups []string, scope Scope, action Action, objectType string) (*PartialAuthorizer, error) {
 	ctx, span := tracing.StartSpan(ctx)
 	defer span.End()
 
-	auth, err := newPartialAuthorizer(ctx, subjectID, roles, scope, action, objectType)
+	auth, err := newPartialAuthorizer(ctx, subjectID, roles, groups, scope, action, objectType)
 	if err != nil {
 		return nil, xerrors.Errorf("new partial authorizer: %w", err)
 	}
 
 	return auth, nil
 }
 
-func (a RegoAuthorizer) PrepareByRoleName(ctx context.Context, subjectID string, roleNames []string, scope Scope, action Action, objectType string) (PreparedAuthorized, error) {
+func (a RegoAuthorizer) PrepareByRoleName(ctx context.Context, subjectID string, roleNames []string, groups []string, scope Scope, action Action, objectType string) (PreparedAuthorized, error) {
 	ctx, span := tracing.StartSpan(ctx)
 	defer span.End()
 
@@ -179,5 +179,5 @@ func (a RegoAuthorizer) PrepareByRoleName(ctx context.Context, subjectID string,
 		return nil, err
 	}
 
-	return a.Prepare(ctx, subjectID, roles, scope, action, objectType)
+	return a.Prepare(ctx, subjectID, roles, groups, scope, action, objectType)
 }
diff --git a/coderd/rbac/authz_internal_test.go b/coderd/rbac/authz_internal_test.go
@@ -19,7 +19,8 @@ type subject struct {
 	// For the unit test we want to pass in the roles directly, instead of just
 	// by name. This allows us to test custom roles that do not exist in the product,
 	// but test edge cases of the implementation.
-	Roles []Role `json:"roles"`
+	Roles  []Role   `json:"roles"`
+	Groups []string `json:"groups"`
 }
 
 type fakeObject struct {
@@ -162,7 +163,7 @@ func TestFilter(t *testing.T) {
 			var allowedCount int
 			for i, obj := range localObjects {
 				obj.Type = tc.ObjectType
-				err := auth.ByRoleName(ctx, tc.SubjectID, tc.Roles, scope, ActionRead, obj.RBACObject())
+				err := auth.ByRoleName(ctx, tc.SubjectID, tc.Roles, []string{}, scope, ActionRead, obj.RBACObject())
 				obj.Allowed = err == nil
 				if err == nil {
 					allowedCount++
@@ -171,7 +172,7 @@ func TestFilter(t *testing.T) {
 			}
 
 			// Run by filter
-			list, err := Filter(ctx, auth, tc.SubjectID, tc.Roles, scope, tc.Action, localObjects)
+			list, err := Filter(ctx, auth, tc.SubjectID, tc.Roles, []string{}, scope, tc.Action, localObjects)
 			require.NoError(t, err)
 			require.Equal(t, allowedCount, len(list), "expected number of allowed")
 			for _, obj := range list {
@@ -714,7 +715,7 @@ func testAuthorize(t *testing.T, name string, subject subject, sets ...[]authTes
 					ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitShort)
 					t.Cleanup(cancel)
 
-					authError := authorizer.Authorize(ctx, subject.UserID, subject.Roles, a, c.resource)
+					authError := authorizer.Authorize(ctx, subject.UserID, subject.Roles, subject.Groups, a, c.resource)
 
 					// Logging only
 					if authError != nil {
@@ -739,7 +740,7 @@ func testAuthorize(t *testing.T, name string, subject subject, sets ...[]authTes
 						assert.Error(t, authError, "expected unauthorized")
 					}
 
-					partialAuthz, err := authorizer.Prepare(ctx, subject.UserID, subject.Roles, ScopeAll, a, c.resource.Type)
+					partialAuthz, err := authorizer.Prepare(ctx, subject.UserID, subject.Roles, subject.Groups, ScopeAll, a, c.resource.Type)
 					require.NoError(t, err, "make prepared authorizer")
 
 					// Also check the rego policy can form a valid partial query result.

diff --git a/coderd/rbac/builtin_test.go b/coderd/rbac/builtin_test.go
@@ -32,6 +32,7 @@ func BenchmarkRBACFilter(b *testing.B) {
 	benchCases := []struct {
 		Name   string
 		Roles  []string
+		Groups []string
 		UserID uuid.UUID
 		Scope  rbac.Scope
 	}{
@@ -90,7 +91,7 @@ func BenchmarkRBACFilter(b *testing.B) {
 		b.Run(c.Name, func(b *testing.B) {
 			objects := benchmarkSetup(orgs, users, b.N)
 			b.ResetTimer()
-			allowed, err := rbac.Filter(context.Background(), authorizer, c.UserID.String(), c.Roles, c.Scope, rbac.ActionRead, objects)
+			allowed, err := rbac.Filter(context.Background(), authorizer, c.UserID.String(), c.Roles, c.Groups, c.Scope, rbac.ActionRead, objects)
 			require.NoError(b, err)
 			var _ = allowed
 		})
@@ -114,6 +115,7 @@ type authSubject struct {
 	Name   string
 	UserID string
 	Roles  []string
+	Groups []string
 }
 
 func TestRolePermissions(t *testing.T) {
@@ -359,7 +361,7 @@ func TestRolePermissions(t *testing.T) {
 						delete(remainingSubjs, subj.Name)
 						msg := fmt.Sprintf("%s as %q doing %q on %q", c.Name, subj.Name, action, c.Resource.Type)
 						// TODO: scopey
-						err := auth.ByRoleName(context.Background(), subj.UserID, subj.Roles, rbac.ScopeAll, action, c.Resource)
+						err := auth.ByRoleName(context.Background(), subj.UserID, subj.Roles, subj.Groups, rbac.ScopeAll, action, c.Resource)
 						if result {
 							assert.NoError(t, err, fmt.Sprintf("Should pass: %s", msg))
 						} else {

diff --git a/coderd/rbac/partial.go b/coderd/rbac/partial.go
@@ -35,11 +35,11 @@ func (pa *PartialAuthorizer) Authorize(ctx context.Context, object Object) error
 	return nil
 }
 
-func newPartialAuthorizer(ctx context.Context, subjectID string, roles []Role, scope Scope, action Action, objectType string) (*PartialAuthorizer, error) {
+func newPartialAuthorizer(ctx context.Context, subjectID string, roles []Role, groups []string, scope Scope, action Action, objectType string) (*PartialAuthorizer, error) {
 	ctx, span := tracing.StartSpan(ctx)
 	defer span.End()
 
-	pAuth, err := newSubPartialAuthorizer(ctx, subjectID, roles, action, objectType)
+	pAuth, err := newSubPartialAuthorizer(ctx, subjectID, roles, groups, action, objectType)
 	if err != nil {
 		return nil, err
 	}
@@ -51,7 +51,7 @@ func newPartialAuthorizer(ctx context.Context, subjectID string, roles []Role, s
 			return nil, xerrors.Errorf("unknown scope %q", scope)
 		}
 
-		scopeAuth, err = newSubPartialAuthorizer(ctx, subjectID, []Role{scopeRole}, action, objectType)
+		scopeAuth, err = newSubPartialAuthorizer(ctx, subjectID, []Role{scopeRole}, groups, action, objectType)
 		if err != nil {
 			return nil, err
 		}
@@ -78,14 +78,15 @@ type subPartialAuthorizer struct {
 	alwaysTrue bool
 }
 
-func newSubPartialAuthorizer(ctx context.Context, subjectID string, roles []Role, action Action, objectType string) (*subPartialAuthorizer, error) {
+func newSubPartialAuthorizer(ctx context.Context, subjectID string, roles []Role, groups []string, action Action, objectType string) (*subPartialAuthorizer, error) {
 	ctx, span := tracing.StartSpan(ctx)
 	defer span.End()
 
 	input := map[string]interface{}{
 		"subject": authSubject{
-			ID:    subjectID,
-			Roles: roles,
+			ID:     subjectID,
+			Roles:  roles,
+			Groups: groups,
 		},
 		"object": map[string]string{
 			"type": objectType,

diff --git a/coderd/roles.go b/coderd/roles.go
@@ -70,7 +70,7 @@ func (api *API) checkPermissions(rw http.ResponseWriter, r *http.Request) {
 		if v.Object.OwnerID == "me" {
 			v.Object.OwnerID = roles.ID.String()
 		}
-		err := api.Authorizer.ByRoleName(r.Context(), roles.ID.String(), roles.Roles, apiKey.Scope.ToRBAC(), rbac.Action(v.Action),
+		err := api.Authorizer.ByRoleName(r.Context(), roles.ID.String(), roles.Roles, roles.Groups, apiKey.Scope.ToRBAC(), rbac.Action(v.Action),
 			rbac.Object{
 				Owner: v.Object.OwnerID,
 				OrgID: v.Object.OrganizationID,