Fix check for direct rule grant, add unit tests for same

internal/controlplane/handlers_authz.go

@@ -291,6 +291,8 @@ func (s *Server) ListRoleAssignments(
 
 // AssignRole assigns a role to a user on a project.
 // Note that this assumes that the request has already been authorized.
+//
+//nolint:gocyclo  // There's a lot of trivial error handling here
 func (s *Server) AssignRole(ctx context.Context, req *minder.AssignRoleRequest) (*minder.AssignRoleResponse, error) {
 	role := req.GetRoleAssignment().GetRole()
 	sub := req.GetRoleAssignment().GetSubject()
@@ -339,20 +341,27 @@ func (s *Server) AssignRole(ctx context.Context, req *minder.AssignRoleRequest)
 		}
 		return nil, util.UserVisibleError(codes.Unimplemented, "user management is not enabled")
 	} else if sub != "" && inviteeEmail == "" {
-		// Enable one or the other.
-		if flags.Bool(ctx, s.featureFlags, flags.MachineAccounts) || !flags.Bool(ctx, s.featureFlags, flags.UserManagement) {
-			assignment, err := db.WithTransaction(s.store, func(qtx db.ExtendQuerier) (*minder.RoleAssignment, error) {
-				return s.roles.CreateRoleAssignment(ctx, qtx, s.authzClient, s.idClient, targetProject, sub, authzRole)
-			})
-			if err != nil {
-				return nil, err
-			}
-
-			return &minder.AssignRoleResponse{
-				RoleAssignment: assignment,
-			}, nil
+		identity, err := s.idClient.Resolve(ctx, sub)
+		if err != nil || identity == nil {
+			return nil, util.UserVisibleError(codes.NotFound, "could not find identity %q", sub)
+		}
+		isMachine := identity.Provider.String() != ""
+		if !isMachine && flags.Bool(ctx, s.featureFlags, flags.UserManagement) {
+			return nil, util.UserVisibleError(codes.Unimplemented, "human users may only be added by invitation")
 		}
-		return nil, util.UserVisibleError(codes.Unimplemented, "user management is enabled, use invites instead")
+		if isMachine && !flags.Bool(ctx, s.featureFlags, flags.MachineAccounts) {
+			return nil, util.UserVisibleError(codes.Unimplemented, "machine accounts are not enabled")
+		}
+		assignment, err := db.WithTransaction(s.store, func(qtx db.ExtendQuerier) (*minder.RoleAssignment, error) {
+			return s.roles.CreateRoleAssignment(ctx, qtx, s.authzClient, targetProject, *identity, authzRole)
+		})
+		if err != nil {
+			return nil, err
+		}
+
+		return &minder.AssignRoleResponse{
+			RoleAssignment: assignment,
+		}, nil
 	}
 	return nil, util.UserVisibleError(codes.InvalidArgument, "one of subject or email must be specified")
 }

internal/controlplane/handlers_authz_test.go

@@ -4,6 +4,7 @@
 package controlplane
 
 import (
+	"cmp"
 	"context"
 	"crypto/rand"
 	"crypto/rsa"
@@ -29,9 +30,11 @@ import (
 
 	mockdb "github.com/mindersec/minder/database/mock"
 	"github.com/mindersec/minder/internal/auth"
+	"github.com/mindersec/minder/internal/auth/githubactions"
 	authjwt "github.com/mindersec/minder/internal/auth/jwt"
 	"github.com/mindersec/minder/internal/auth/jwt/noop"
 	"github.com/mindersec/minder/internal/auth/keycloak"
+	mockauth "github.com/mindersec/minder/internal/auth/mock"
 	"github.com/mindersec/minder/internal/authz"
 	"github.com/mindersec/minder/internal/authz/mock"
 	"github.com/mindersec/minder/internal/db"
@@ -658,61 +661,62 @@ func TestAssignRole(t *testing.T) {
 	userEmail := "[email protected]"
 	authzRole := authz.RoleAdmin
 	projectId := uuid.New()
-	projectIdString := projectId.String()
+	otherProject := uuid.New()
 
 	tests := []struct {
-		name           string
-		inviteeEmail   string
-		subject        string
-		buildStubs     func(t *testing.T, store *mockdb.MockStore)
-		expectedError  string
-		inviteByEmail  bool
-		inviteByUserId bool
+		name          string
+		project       uuid.UUID
+		inviteeEmail  string
+		subject       string
+		buildStubs    func(t *testing.T, store *mockdb.MockStore)
+		expectedError string
+		userIdentity  *auth.Identity
+		flagData      map[string]any
 	}{
+		{
+			name:          "error with no subject or email",
+			expectedError: "one of subject or email must be specified",
+		},
 		{
 			name:          "error when self enroll",
 			inviteeEmail:  userEmail,
 			expectedError: "cannot update your own role",
 		},
 		{
-			name: "error when project ID is not found",
-			buildStubs: func(t *testing.T, store *mockdb.MockStore) {
-				t.Helper()
-				store.EXPECT().GetProjectByID(gomock.Any(), gomock.Any()).Return(db.Project{}, sql.ErrNoRows)
-			},
-			expectedError: fmt.Sprintf("target project with ID %s not found", projectIdString),
-		},
-		{
-			name: "error with no subject or email",
-			buildStubs: func(t *testing.T, store *mockdb.MockStore) {
-				t.Helper()
-				store.EXPECT().GetProjectByID(gomock.Any(), gomock.Any()).Return(db.Project{
-					ID: projectId,
-				}, nil)
-			},
-			expectedError: "one of subject or email must be specified",
+			name:          "error when project ID is not found",
+			project:       otherProject,
+			subject:       "user",
+			expectedError: fmt.Sprintf("target project with ID %s not found", otherProject.String()),
 		},
 		{
 			name:         "request with email creates invite",
 			inviteeEmail: "[email protected]",
-			buildStubs: func(t *testing.T, store *mockdb.MockStore) {
-				t.Helper()
-				store.EXPECT().GetProjectByID(gomock.Any(), gomock.Any()).Return(db.Project{
-					ID: projectId,
-				}, nil)
-			},
-			inviteByEmail: true,
+			flagData:     map[string]any{"user_management": true},
 		},
 		{
 			name:    "request with subject creates role assignment",
 			subject: "user",
-			buildStubs: func(t *testing.T, store *mockdb.MockStore) {
-				t.Helper()
-				store.EXPECT().GetProjectByID(gomock.Any(), gomock.Any()).Return(db.Project{
-					ID: projectId,
-				}, nil)
+			userIdentity: &auth.Identity{
+				UserID:   "user",
+				Provider: &keycloak.KeyCloak{},
+			},
+		}, {
+			name:    "machine accounts disabled",
+			subject: "githubactions/repo:mindersec/community:ref:refs/heads/main",
+			userIdentity: &auth.Identity{
+				UserID:   "repo:mindersec/community:ref:refs/heads/main",
+				Provider: &githubactions.GitHubActions{},
+			},
+			expectedError: "Description: Unimplemented\nDetails: machine accounts are not enabled",
+		},
+		{
+			name:    "grant permission to GitHub Action",
+			subject: "githubactions/repo:mindersec/community:ref:refs/heads/main",
+			userIdentity: &auth.Identity{
+				UserID:   "repo:mindersec/community:ref:refs/heads/main",
+				Provider: &githubactions.GitHubActions{},
 			},
-			inviteByUserId: true,
+			flagData: map[string]any{"machine_accounts": true},
 		},
 	}
 
@@ -723,41 +727,46 @@ func TestAssignRole(t *testing.T) {
 			ctrl := gomock.NewController(t)
 			defer ctrl.Finish()
 
+			projectIdString := cmp.Or(tc.project, projectId).String()
 			user := openid.New()
 			assert.NoError(t, user.Set("email", userEmail))
 
 			ctx := context.Background()
 			ctx = authjwt.WithAuthTokenContext(ctx, user)
 			ctx = engcontext.WithEntityContext(ctx, &engcontext.EntityContext{
-				Project: engcontext.Project{ID: projectId},
+				Project: engcontext.Project{ID: cmp.Or(tc.project, projectId)},
 			})
 
-			featureClient := &flags.FakeClient{}
-
-			// Enable user management feature flag if inviting by email
-			if tc.inviteByEmail {
-				featureClient.Data = map[string]any{
-					"user_management": true,
-				}
+			featureClient := &flags.FakeClient{
+				Data: tc.flagData,
 			}
+			idClient := mockauth.NewMockResolver(ctrl)
+			idClient.EXPECT().Resolve(gomock.Any(), tc.subject).Return(tc.userIdentity, nil).MaxTimes(1)
 
 			mockInviteService := mockinvites.NewMockInviteService(ctrl)
-			if tc.inviteByEmail {
+			mockRoleService := mockroles.NewMockRoleService(ctrl)
+			if tc.expectedError == "" && tc.userIdentity != nil {
+				mockRoleService.EXPECT().CreateRoleAssignment(gomock.Any(), gomock.Any(), gomock.Any(),
+					gomock.Any(), *tc.userIdentity, authzRole).Return(&minder.RoleAssignment{
+					Role:    authzRole.String(),
+					Project: &projectIdString,
+				}, nil)
+			} else if tc.expectedError == "" {
 				mockInviteService.EXPECT().CreateInvite(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(),
 					gomock.Any(), authzRole, tc.inviteeEmail).Return(&minder.Invitation{
 					Role:    authzRole.String(),
 					Project: projectIdString,
 				}, nil)
 			}
-			mockRoleService := mockroles.NewMockRoleService(ctrl)
-			if tc.inviteByUserId {
-				mockRoleService.EXPECT().CreateRoleAssignment(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(),
-					gomock.Any(), tc.subject, authzRole).Return(&minder.RoleAssignment{
-					Role:    authzRole.String(),
-					Project: &projectIdString,
-				}, nil)
-			}
+
 			mockStore := mockdb.NewMockStore(ctrl)
+			// Most tests will call GetProjectByID with the correct ID, but some will
+			// deliberately choose a different ID; return an error for those.
+			mockStore.EXPECT().GetProjectByID(gomock.Any(), projectId).Return(db.Project{
+				ID: projectId,
+			}, nil).MaxTimes(1)
+			mockStore.EXPECT().GetProjectByID(gomock.Any(), gomock.Not(gomock.Eq(projectId))).
+				Return(db.Project{}, sql.ErrNoRows).MaxTimes(1)
 			mockStore.EXPECT().BeginTransaction().AnyTimes()
 			mockStore.EXPECT().GetQuerierWithTransaction(gomock.Any()).AnyTimes()
 			mockStore.EXPECT().Commit(gomock.Any()).AnyTimes()
@@ -771,6 +780,7 @@ func TestAssignRole(t *testing.T) {
 				invites:      mockInviteService,
 				roles:        mockRoleService,
 				store:        mockStore,
+				idClient:     idClient,
 				cfg:          &serverconfig.Config{Email: serverconfig.EmailConfig{}},
 			}
 
@@ -792,13 +802,12 @@ func TestAssignRole(t *testing.T) {
 			}
 
 			require.NoError(t, err)
-			if tc.inviteByEmail {
+			if tc.userIdentity != nil {
+				require.Equal(t, authzRole.String(), response.RoleAssignment.Role)
+			} else {
 				require.Equal(t, authzRole.String(), response.Invitation.Role)
 				require.Equal(t, projectIdString, response.Invitation.Project)
 			}
-			if tc.inviteByUserId {
-				require.Equal(t, authzRole.String(), response.RoleAssignment.Role)
-			}
 		})
 	}
 }

internal/roles/service.go

@@ -26,8 +26,8 @@ import (
 // RoleService encapsulates the methods to manage user role assignments
 type RoleService interface {
 	// CreateRoleAssignment assigns a user a role on a project
-	CreateRoleAssignment(ctx context.Context, qtx db.Querier, authzClient authz.Client, idClient auth.Resolver,
-		targetProject uuid.UUID, subject string, authzRole authz.Role) (*pb.RoleAssignment, error)
+	CreateRoleAssignment(ctx context.Context, qtx db.Querier, authzClient authz.Client,
+		targetProject uuid.UUID, subject auth.Identity, authzRole authz.Role) (*pb.RoleAssignment, error)
 
 	// UpdateRoleAssignment updates the users role on a project
 	UpdateRoleAssignment(ctx context.Context, qtx db.Querier, authzClient authz.Client, idClient auth.Resolver,
@@ -47,13 +47,7 @@ func NewRoleService() RoleService {
 }
 
 func (_ *roleService) CreateRoleAssignment(ctx context.Context, qtx db.Querier, authzClient authz.Client,
-	idClient auth.Resolver, targetProject uuid.UUID, subject string, authzRole authz.Role) (*pb.RoleAssignment, error) {
-	// Resolve the subject to an identity
-	identity, err := idClient.Resolve(ctx, subject)
-	if err != nil {
-		zerolog.Ctx(ctx).Error().Err(err).Msg("error resolving identity")
-		return nil, util.UserVisibleError(codes.NotFound, "could not find identity %q", subject)
-	}
+	targetProject uuid.UUID, identity auth.Identity, authzRole authz.Role) (*pb.RoleAssignment, error) {
 
 	// For users in the primary (human) identity store, verify if user exists in our database.
 	// TODO: this assumes that we store human users in the Minder database, in addition to the

internal/roles/service_test.go

@@ -80,14 +80,13 @@ func TestCreateRoleAssignment(t *testing.T) {
 				},
 			}
 
-			idClient := mockauth.NewMockResolver(ctrl)
-			idClient.EXPECT().Resolve(ctx, subject).Return(&auth.Identity{
+			identitySub := auth.Identity{
 				UserID:   subject,
 				Provider: &keycloak.KeyCloak{},
-			}, nil)
+			}
 
 			service := NewRoleService()
-			_, err := service.CreateRoleAssignment(ctx, store, authzClient, idClient, project, subject, userRole)
+			_, err := service.CreateRoleAssignment(ctx, store, authzClient, project, identitySub, userRole)
 
 			if scenario.expectedError != "" {
 				require.ErrorContains(t, err, scenario.expectedError)