Test service account RBAC too

Broadly, these are the same as users, but ServiceAccounts can only
have permissions in their parent org, so that's checked explicitly.

Creating service accounts needs an issuer, and this is a faff to set
up; so I have recreated what the handler does, by using
`conversion.NewObjectMetadata(...)`. This means the accounts get a
fresh UID every time, so the IDs get wrapped in a hold-all
fixture. (Usually this is a good idea anyway, because it means you can
generate names and IDs, and check everything still works.)

Author: squaremo

pkg/rbac/groups_test.go

@@ -19,12 +19,14 @@ package rbac_test
 import (
 	"context"
 	"testing"
+	"time"
 
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 
 	"github.com/unikorn-cloud/core/pkg/constants"
 	coreopenapi "github.com/unikorn-cloud/core/pkg/openapi"
+	"github.com/unikorn-cloud/core/pkg/server/conversion"
 	unikornv1 "github.com/unikorn-cloud/identity/pkg/apis/unikorn/v1alpha1"
 	handlercommon "github.com/unikorn-cloud/identity/pkg/handler/common"
 	"github.com/unikorn-cloud/identity/pkg/handler/users"
@@ -36,16 +38,29 @@ import (
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/utils/ptr"
 
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/client/fake"
 )
 
+type fixture struct {
+	// Because of the way service account creation works, we can't specify the ID
+	// for a service account when creating it; we have to create it then look at
+	// the ID it was given.
+	serviceAccountAlphaID, serviceAccountBetaID, serviceAccountAltAlphaID string
+
+	rbac *rbac.RBAC
+}
+
 const (
 	testNamespace = "test-namespace"
 	testOrgID     = "test-org"
 	testOrgNS     = "test-org-ns"
 
+	altOrgID = "alt-org-id"
+	altOrgNS = "alt-namespace"
+
 	userAliceSubject = "alice@example.com"
 	userAliceID      = "user-alice"
 
@@ -55,9 +70,13 @@ const (
 	userCharlieSubject = "charlie@example.com"
 	userCharlieID      = "user-charlie"
 
+	serviceAccountAlphaName = "sa-alpha"
+	serviceAccountBetaName  = "sa-beta"
+
 	groupAdminsID   = "group-admins"
 	groupDevelopers = "group-developers"
 	groupReaders    = "group-readers"
+	groupServices   = "group-services"
 
 	roleAdminID     = "role-admin"
 	roleDeveloperID = "role-developer"
@@ -129,10 +148,35 @@ func newOrganization(objectNamespace, name, orgNamespace string) *unikornv1.Orga
 	}
 }
 
+func createServiceAccount(t *testing.T, c client.Client, name, orgID, orgNamespace string) string {
+	t.Helper()
+
+	// It would be better to use the API, but creating service accounts needs a token issuer,
+	// and that is a pain to set up. So: fake it by creating a record in Kubernetes the way
+	// the handler would, rather than going through the API.
+	meta := &coreopenapi.ResourceWriteMetadata{
+		Name: name,
+	}
+
+	objectMeta := conversion.NewObjectMetadata(meta, orgNamespace).WithOrganization(orgID).Get()
+	sa := unikornv1.ServiceAccount{
+		ObjectMeta: objectMeta,
+		Spec: unikornv1.ServiceAccountSpec{
+			Expiry: ptr.To(metav1.NewTime(time.Now().Add(2 * time.Hour))),
+		},
+	}
+
+	require.NoError(t, c.Create(t.Context(), &sa))
+
+	return sa.Name // hereafter used as the service account ID
+}
+
 // setupTestEnvironment creates a comprehensive RBAC test environment with users, groups, roles, and projects.
-func setupTestEnvironment(t *testing.T) (client.Client, *rbac.RBAC) {
+func setupTestEnvironment(t *testing.T) fixture {
 	t.Helper()
 
+	var f fixture
+
 	scheme := runtime.NewScheme()
 	require.NoError(t, corev1.AddToScheme(scheme))
 	require.NoError(t, unikornv1.AddToScheme(scheme))
@@ -149,7 +193,18 @@ func setupTestEnvironment(t *testing.T) (client.Client, *rbac.RBAC) {
 
 	organization := newOrganization(testNamespace, testOrgID, testOrgNS)
 
-	createObjects(organization)
+	altOrganization := &unikornv1.Organization{
+		ObjectMeta: metav1.ObjectMeta{
+			Namespace: testNamespace,
+			Name:      altOrgID,
+		},
+		Spec: unikornv1.OrganizationSpec{},
+		Status: unikornv1.OrganizationStatus{
+			Namespace: altOrgNS,
+		},
+	}
+
+	createObjects(organization, altOrganization)
 	require.NoError(t, c.Update(t.Context(), organization)) // to update the status with a namespace.
 
 	// Create Roles with different permission scopes.
@@ -255,7 +310,36 @@ func setupTestEnvironment(t *testing.T) (client.Client, *rbac.RBAC) {
 		},
 	}
 
-	createObjects(groupAdminsObj, groupReadersObj, groupDevelopersObj)
+	f.serviceAccountAlphaID = createServiceAccount(t, c, serviceAccountAlphaName, testOrgID, testOrgNS)
+	f.serviceAccountBetaID = createServiceAccount(t, c, serviceAccountBetaName, testOrgID, testOrgNS)
+	f.serviceAccountAltAlphaID = createServiceAccount(t, c, serviceAccountAlphaName, altOrgID, altOrgNS)
+
+	// Group for service accounts
+	groupServicesObj := &unikornv1.Group{
+		ObjectMeta: metav1.ObjectMeta{
+			Namespace: testOrgNS,
+			Name:      groupServices,
+		},
+		Spec: unikornv1.GroupSpec{
+			RoleIDs:           []string{roleDeveloperID},
+			ServiceAccountIDs: []string{f.serviceAccountAlphaID},
+		},
+	}
+
+	// Group in alternate org for the service account there, to check the calculation for Org A does not
+	// include permissions from Org B.
+	groupAltServicesObj := &unikornv1.Group{
+		ObjectMeta: metav1.ObjectMeta{
+			Namespace: altOrgNS,
+			Name:      groupServices,
+		},
+		Spec: unikornv1.GroupSpec{
+			RoleIDs:           []string{roleDeveloperID},
+			ServiceAccountIDs: []string{f.serviceAccountAltAlphaID},
+		},
+	}
+
+	createObjects(groupAdminsObj, groupReadersObj, groupDevelopersObj, groupServicesObj, groupAltServicesObj)
 
 	projectAlpha := &unikornv1.Project{
 		ObjectMeta: metav1.ObjectMeta{
@@ -266,7 +350,7 @@ func setupTestEnvironment(t *testing.T) (client.Client, *rbac.RBAC) {
 			},
 		},
 		Spec: unikornv1.ProjectSpec{
-			GroupIDs: []string{groupDevelopers}, // Only developers have access
+			GroupIDs: []string{groupDevelopers, groupServices}, // Developers and services have access
 		},
 	}
 
@@ -279,7 +363,7 @@ func setupTestEnvironment(t *testing.T) (client.Client, *rbac.RBAC) {
 			},
 		},
 		Spec: unikornv1.ProjectSpec{
-			GroupIDs: []string{groupDevelopers, groupReaders}, // Developers and readers
+			GroupIDs: []string{groupDevelopers, groupReaders, groupServices}, // Developers, readers, and services
 		},
 	}
 
@@ -290,8 +374,9 @@ func setupTestEnvironment(t *testing.T) (client.Client, *rbac.RBAC) {
 	createUser(t, c, userCharlieID, userCharlieSubject, []*unikornv1.Group{groupReadersObj, groupDevelopersObj})
 
 	rbacClient := rbac.New(c, testNamespace, &rbac.Options{})
+	f.rbac = rbacClient
 
-	return c, rbacClient
+	return f
 }
 
 // getACLForUser is a helper to get the ACL for a given user subject.
@@ -320,16 +405,16 @@ func getACLForUser(t *testing.T, rbacClient *rbac.RBAC, subject string) *openapi
 func TestGroupACLContent(t *testing.T) {
 	t.Parallel()
 
-	_, rbacClient := setupTestEnvironment(t)
+	f := setupTestEnvironment(t)
 
 	// Test Alice (Admin) - should have organization permissions.
-	aclAlice := getACLForUser(t, rbacClient, userAliceSubject)
+	aclAlice := getACLForUser(t, f.rbac, userAliceSubject)
 	assert.Nil(t, aclAlice.Global, "Alice should not have global permissions")
 	assert.NotNil(t, aclAlice.Organization, "Alice should have organization permissions")
 	assert.Nil(t, aclAlice.Projects, "Alice should not have project-specific permissions")
 
 	// Test Bob (Developer) - should have organization read and project permissions.
-	aclBob := getACLForUser(t, rbacClient, userBobSubject)
+	aclBob := getACLForUser(t, f.rbac, userBobSubject)
 	assert.Nil(t, aclBob.Global, "Bob should not have global permissions")
 	assert.NotNil(t, aclBob.Organization, "Bob should have organization permissions")
 	assert.NotNil(t, aclBob.Projects, "Bob should have project permissions")
@@ -349,7 +434,7 @@ func TestGroupACLContent(t *testing.T) {
 	assert.True(t, hasOrgRead, "Bob should have org:read permission")
 
 	// Test Charlie (Developer + Reader) - should have merged permissions.
-	aclCharlie := getACLForUser(t, rbacClient, userCharlieSubject)
+	aclCharlie := getACLForUser(t, f.rbac, userCharlieSubject)
 	assert.Nil(t, aclCharlie.Global, "Charlie should not have global permissions")
 	assert.NotNil(t, aclCharlie.Organization, "Charlie should have organization permissions")
 	assert.NotNil(t, aclCharlie.Projects, "Charlie should have project permissions")
@@ -390,3 +475,68 @@ func TestGroupACLContent(t *testing.T) {
 		}
 	}
 }
+
+// getACLForServiceAccount is a helper to get the ACL for a given service account.
+func getACLForServiceAccount(t *testing.T, rbacClient *rbac.RBAC, subject string) *openapi.Acl {
+	t.Helper()
+
+	// Create authorization info for service account
+	info := &authorization.Info{
+		Userinfo: &openapi.Userinfo{
+			Sub: subject,
+		},
+		ServiceAccount: true,
+	}
+
+	ctx := authorization.NewContext(t.Context(), info)
+
+	acl, err := rbacClient.GetACL(ctx, testOrgID)
+	require.NoError(t, err)
+	require.NotNil(t, acl)
+
+	return acl
+}
+
+// TestServiceAccountACL verifies that service accounts get correct permissions via groups.
+func TestServiceAccountACL(t *testing.T) {
+	t.Parallel()
+
+	f := setupTestEnvironment(t)
+
+	// Test service account that's a member of the services group
+	aclAlpha := getACLForServiceAccount(t, f.rbac, f.serviceAccountAlphaID)
+	assert.Nil(t, aclAlpha.Global, "Service account should not have global permissions")
+	assert.NotNil(t, aclAlpha.Organization, "Service account should have organization permissions")
+	assert.NotNil(t, aclAlpha.Projects, "Service account should have project permissions")
+
+	// Verify service account has org:read (from developer role)
+	hasOrgRead := false
+
+	for _, endpoint := range aclAlpha.Organization.Endpoints {
+		if endpoint.Name == "org:read" {
+			hasOrgRead = true
+
+			require.Contains(t, endpoint.Operations, openapi.Read)
+		}
+	}
+
+	assert.True(t, hasOrgRead, "Service account should have org:read permission")
+
+	// Service account should have access to projects (from developer role)
+	assert.Len(t, *aclAlpha.Projects, 2, "Service account should have access to 2 projects")
+
+	// Test service account not in any group
+	aclBeta := getACLForServiceAccount(t, f.rbac, f.serviceAccountBetaID)
+	assert.Nil(t, aclBeta.Global, "Service account not in groups should not have global permissions")
+	assert.Nil(t, aclBeta.Organization, "Service account not in groups should not have organization permissions")
+	assert.Nil(t, aclBeta.Projects, "Service account not in groups should not have project permissions")
+}
+
+func TestServiceAccount_WrongOrganization(t *testing.T) {
+	t.Parallel()
+
+	f := setupTestEnvironment(t)
+
+	aclAlpha := getACLForServiceAccount(t, f.rbac, f.serviceAccountAltAlphaID)
+	assert.Empty(t, aclAlpha, "Service account bound to org B should have no permissions in org A")
+}