Sort the authz roles in ListRoles response (#3857)

* Sort the authz roles in ListRoles response

Signed-off-by: Radoslav Dimitrov <radoslav@stacklok.com>

* Rename AllRoles and remove the authz prefix

Signed-off-by: Radoslav Dimitrov <radoslav@stacklok.com>

---------

Signed-off-by: Radoslav Dimitrov <radoslav@stacklok.com>

internal/authz/authz.go

@@ -338,7 +338,7 @@ func (a *ClientWrapper) doDelete(ctx context.Context, user string, role string,
 
 // DeleteUser removes all tuples for the given user
 func (a *ClientWrapper) DeleteUser(ctx context.Context, user string) error {
-	for role := range AllRoles {
+	for role := range AllRolesDescriptions {
 		listresp, err := a.cli.ListObjects(ctx).Body(fgaclient.ClientListObjectsRequest{
 			Type:     "project",
 			Relation: role.String(),

internal/authz/authz_test.go

@@ -70,7 +70,7 @@ func TestAllRolesExistInFGAModel(t *testing.T) {
 
 	t.Logf("relations: %v", projectTypeDef.Relations)
 
-	for r := range authz.AllRoles {
+	for r := range authz.AllRolesDescriptions {
 		assert.Contains(t, *projectTypeDef.Relations, r.String(), "role %s not found in authz model", r)
 	}
 }
@@ -101,7 +101,7 @@ func TestVerifyOneProject(t *testing.T) {
 
 	// create a project
 	prj := uuid.New()
-	assert.NoError(t, c.Write(ctx, "user-1", authz.AuthzRoleAdmin, prj), "failed to write project")
+	assert.NoError(t, c.Write(ctx, "user-1", authz.RoleAdmin, prj), "failed to write project")
 
 	userJWT := openid.New()
 	assert.NoError(t, userJWT.Set("sub", "user-1"))
@@ -123,7 +123,7 @@ func TestVerifyOneProject(t *testing.T) {
 	assert.Equal(t, "user-1", assignments[0].Subject, "expected user to be assigned to project")
 
 	// delete the project
-	assert.NoError(t, c.Delete(ctx, "user-1", authz.AuthzRoleAdmin, prj), "failed to delete project")
+	assert.NoError(t, c.Delete(ctx, "user-1", authz.RoleAdmin, prj), "failed to delete project")
 
 	// verify the project is gone
 	assert.Error(t, c.Check(userctx, "get", prj), "expected project to be gone")
@@ -155,7 +155,7 @@ func TestVerifyMultipleProjects(t *testing.T) {
 
 	// create a project
 	prj1 := uuid.New()
-	assert.NoError(t, c.Write(ctx, "user-1", authz.AuthzRoleAdmin, prj1), "failed to write project")
+	assert.NoError(t, c.Write(ctx, "user-1", authz.RoleAdmin, prj1), "failed to write project")
 
 	user1JWT := openid.New()
 	assert.NoError(t, user1JWT.Set("sub", "user-1"))
@@ -166,14 +166,14 @@ func TestVerifyMultipleProjects(t *testing.T) {
 
 	// create another project
 	prj2 := uuid.New()
-	assert.NoError(t, c.Write(ctx, "user-1", authz.AuthzRoleViewer, prj2), "failed to write project")
+	assert.NoError(t, c.Write(ctx, "user-1", authz.RoleViewer, prj2), "failed to write project")
 
 	// verify the project
 	assert.NoError(t, c.Check(userctx, "get", prj2), "failed to check project")
 
 	// create an unrelated project
 	prj3 := uuid.New()
-	assert.NoError(t, c.Write(ctx, "user-2", authz.AuthzRoleAdmin, prj3), "failed to write project")
+	assert.NoError(t, c.Write(ctx, "user-2", authz.RoleAdmin, prj3), "failed to write project")
 
 	// verify the project
 	user2JWT := openid.New()

internal/authz/fuzz_test.go

@@ -43,7 +43,7 @@ func FuzzAllAuthzApis(f *testing.F) {
 		}
 		prj := uuid.New()
 
-		c.Write(ctx, str1, authz.AuthzRoleAdmin, prj)
+		c.Write(ctx, str1, authz.RoleAdmin, prj)
 
 		userJWT := openid.New()
 
@@ -58,7 +58,7 @@ func FuzzAllAuthzApis(f *testing.F) {
 
 		c.ProjectsForUser(userctx, str4)
 		c.AssignmentsToProject(userctx, prj)
-		c.Delete(ctx, str5, authz.AuthzRoleAdmin, prj)
+		c.Delete(ctx, str5, authz.RoleAdmin, prj)
 		c.Check(userctx, "get", prj)
 		c.ProjectsForUser(userctx, str6)
 		c.AssignmentsToProject(userctx, prj)

internal/authz/interface.go

@@ -32,39 +32,41 @@ var ErrNotAuthorized = fmt.Errorf("not authorized")
 type Role string
 
 const (
-	// AuthzRoleAdmin is the admin role
-	AuthzRoleAdmin Role = "admin"
-	// AuthzRoleEditor is the editor role
-	AuthzRoleEditor Role = "editor"
-	// AuthzRoleViewer is the viewer role
-	AuthzRoleViewer Role = "viewer"
-	// AuthzRolePolicyWriter is the `policy_writer` role
-	AuthzRolePolicyWriter Role = "policy_writer"
-	// AuthzRolePermissionsManager is the `permissions_manager` role
-	AuthzRolePermissionsManager Role = "permissions_manager"
+	// RoleAdmin is the admin role
+	RoleAdmin Role = "admin"
+	// RoleEditor is the editor role
+	RoleEditor Role = "editor"
+	// RoleViewer is the viewer role
+	RoleViewer Role = "viewer"
+	// RolePolicyWriter is the `policy_writer` role
+	RolePolicyWriter Role = "policy_writer"
+	// RolePermissionsManager is the `permissions_manager` role
+	RolePermissionsManager Role = "permissions_manager"
 )
 
 var (
-	// AllRoles is a list of all roles
-	AllRoles = map[Role]string{
-		AuthzRoleAdmin: "The Admin role allows the user to perform all actions on the project and " +
+	// AllRolesDescriptions is a list of all roles
+	AllRolesDescriptions = map[Role]string{
+		RoleAdmin: "The Admin role allows the user to perform all actions on the project and " +
 			"sub-projects.",
-		AuthzRoleEditor: "The Editor role allows for more write and read actions on the project and " +
+		RoleEditor: "The Editor role allows for more write and read actions on the project and " +
 			"sub-projects except for project administration.",
-		AuthzRoleViewer: "The Viewer role allows for read actions on the project and sub-projects.",
-		AuthzRolePolicyWriter: "The Policy Writer role allows for writing policies (rule types and " +
+		RoleViewer: "The Viewer role allows for read actions on the project and sub-projects.",
+		RolePolicyWriter: "The Policy Writer role allows for writing policies (rule types and " +
 			"profiles) on the project and sub-projects. This is handy for CI jobs.",
-		AuthzRolePermissionsManager: "The Permissions Manager role allows for managing permissions " +
+		RolePermissionsManager: "The Permissions Manager role allows for managing permissions " +
 			"on the project and sub-projects.",
 	}
 	// AllRolesDisplayName is a list of all roles with their display names
 	AllRolesDisplayName = map[Role]string{
-		AuthzRoleAdmin:              "Admin",
-		AuthzRoleEditor:             "Editor",
-		AuthzRoleViewer:             "Viewer",
-		AuthzRolePolicyWriter:       "Policy Writer",
-		AuthzRolePermissionsManager: "Permissions Manager",
+		RoleAdmin:              "Admin",
+		RoleEditor:             "Editor",
+		RoleViewer:             "Viewer",
+		RolePolicyWriter:       "Policy Writer",
+		RolePermissionsManager: "Permissions Manager",
 	}
+	// AllRolesSorted is a list of all roles sorted
+	AllRolesSorted = []Role{RoleAdmin, RoleEditor, RoleViewer, RolePolicyWriter, RolePermissionsManager}
 )
 
 func (r Role) String() string {
@@ -77,7 +79,7 @@ func ParseRole(r string) (Role, error) {
 		return "", fmt.Errorf("role cannot be empty")
 	}
 	rr := Role(r)
-	if _, ok := AllRoles[rr]; !ok {
+	if _, ok := AllRolesDescriptions[rr]; !ok {
 		return "", fmt.Errorf("invalid role %s", r)
 	}
 

internal/controlplane/handlers_authz.go

@@ -220,16 +220,22 @@ var _ minder.PermissionsServiceServer = (*Server)(nil)
 // ListRoles returns the list of available roles for the minder instance
 func (*Server) ListRoles(_ context.Context, _ *minder.ListRolesRequest) (*minder.ListRolesResponse, error) {
 	resp := minder.ListRolesResponse{
-		Roles: make([]*minder.Role, 0, len(authz.AllRoles)),
+		Roles: make([]*minder.Role, 0, len(authz.AllRolesDescriptions)),
 	}
-	for role, desc := range authz.AllRoles {
+	// Iterate over all roles and add them to the response if they have a description. Skip if they don't.
+	// The roles are sorted by the order in which they are defined in the authz package, i.e. admin, editor, viewer, etc.
+	for _, role := range authz.AllRolesSorted {
+		// Skip roles that don't have a description
+		if authz.AllRolesDescriptions[role] == "" {
+			continue
+		}
+		// Add the role to the response
 		resp.Roles = append(resp.Roles, &minder.Role{
 			Name:        role.String(),
 			DisplayName: authz.AllRolesDisplayName[role],
-			Description: desc,
+			Description: authz.AllRolesDescriptions[role],
 		})
 	}
-
 	return &resp, nil
 }
 
@@ -661,7 +667,7 @@ func (s *Server) removeRole(
 	}
 
 	// Validate in case there's only one admin for the project and the user is trying to remove themselves
-	if roleToRemove == authz.AuthzRoleAdmin {
+	if roleToRemove == authz.RoleAdmin {
 		// Get all role assignments for the project
 		as, err := s.authzClient.AssignmentsToProject(ctx, targetProject)
 		if err != nil {
@@ -670,7 +676,7 @@ func (s *Server) removeRole(
 		// Count the number of admin roles
 		adminRolesCnt := 0
 		for _, existing := range as {
-			if existing.Role == authz.AuthzRoleAdmin.String() {
+			if existing.Role == authz.RoleAdmin.String() {
 				adminRolesCnt++
 			}
 		}

internal/controlplane/handlers_authz_test.go

@@ -317,50 +317,50 @@ func TestRoleManagement(t *testing.T) {
 	}{{
 		name: "simple adds",
 		adds: []*minder.RoleAssignment{{
-			Role:    authz.AuthzRoleAdmin.String(),
+			Role:    authz.RoleAdmin.String(),
 			Subject: user1.String(),
 		}, {
-			Role:    authz.AuthzRoleAdmin.String(),
+			Role:    authz.RoleAdmin.String(),
 			Subject: user2.String(),
 		}},
 		result: &minder.ListRoleAssignmentsResponse{
 			RoleAssignments: []*minder.RoleAssignment{{
-				Role:        authz.AuthzRoleAdmin.String(),
+				Role:        authz.RoleAdmin.String(),
 				Subject:     user1.String(),
 				DisplayName: "user1",
 				Project:     proto.String(project.String()),
 			}, {
-				Role:        authz.AuthzRoleAdmin.String(),
+				Role:        authz.RoleAdmin.String(),
 				DisplayName: "user2",
 				Subject:     user2.String(),
 				Project:     proto.String(project.String()),
 			}},
 		},
 		stored: []*minder.RoleAssignment{{
-			Role:    authz.AuthzRoleAdmin.String(),
+			Role:    authz.RoleAdmin.String(),
 			Subject: user1.String(),
 			Project: proto.String(project.String()),
 		}, {
-			Role:    authz.AuthzRoleAdmin.String(),
+			Role:    authz.RoleAdmin.String(),
 			Subject: user2.String(),
 			Project: proto.String(project.String()),
 		}},
 	}, {
 		name: "add and remove",
 		adds: []*minder.RoleAssignment{{
-			Role:    authz.AuthzRoleAdmin.String(),
+			Role:    authz.RoleAdmin.String(),
 			Subject: user1.String(),
 		}, {
-			Role:    authz.AuthzRoleAdmin.String(),
+			Role:    authz.RoleAdmin.String(),
 			Subject: user2.String(),
 		}},
 		removes: []*minder.RoleAssignment{{
-			Role:    authz.AuthzRoleAdmin.String(),
+			Role:    authz.RoleAdmin.String(),
 			Subject: user2.String(),
 		}},
 		result: &minder.ListRoleAssignmentsResponse{
 			RoleAssignments: []*minder.RoleAssignment{{
-				Role:        authz.AuthzRoleAdmin.String(),
+				Role:        authz.RoleAdmin.String(),
 				DisplayName: "user1",
 				Subject:     user1.String(),
 				Project:     proto.String(project.String()),
@@ -370,31 +370,31 @@ func TestRoleManagement(t *testing.T) {
 		name:    "IDP resolution",
 		idpFlag: true,
 		adds: []*minder.RoleAssignment{{
-			Role:    authz.AuthzRoleAdmin.String(),
+			Role:    authz.RoleAdmin.String(),
 			Subject: "user1",
 		}, {
-			Role:    authz.AuthzRoleAdmin.String(),
+			Role:    authz.RoleAdmin.String(),
 			Subject: user2.String(),
 		}},
 		result: &minder.ListRoleAssignmentsResponse{
 			RoleAssignments: []*minder.RoleAssignment{{
-				Role:        authz.AuthzRoleAdmin.String(),
+				Role:        authz.RoleAdmin.String(),
 				Subject:     user1.String(),
 				DisplayName: "user1",
 				Project:     proto.String(project.String()),
 			}, {
-				Role:        authz.AuthzRoleAdmin.String(),
+				Role:        authz.RoleAdmin.String(),
 				Subject:     user2.String(),
 				DisplayName: "user2",
 				Project:     proto.String(project.String()),
 			}},
 		},
 		stored: []*minder.RoleAssignment{{
-			Role:    authz.AuthzRoleAdmin.String(),
+			Role:    authz.RoleAdmin.String(),
 			Subject: user1.String(),
 			Project: proto.String(project.String()),
 		}, {
-			Role:    authz.AuthzRoleAdmin.String(),
+			Role:    authz.RoleAdmin.String(),
 			Subject: user2.String(),
 			Project: proto.String(project.String()),
 		}},
@@ -403,23 +403,23 @@ func TestRoleManagement(t *testing.T) {
 		// NOTE: we don't have a way to create invitations yet.
 		userManagementFlag: true,
 		adds: []*minder.RoleAssignment{{
-			Role:    authz.AuthzRoleAdmin.String(),
+			Role:    authz.RoleAdmin.String(),
 			Subject: user1.String(),
 		}, {
-			Role:    authz.AuthzRoleAdmin.String(),
+			Role:    authz.RoleAdmin.String(),
 			Subject: user2.String(),
 		}},
 		invites: []db.ListInvitationsForProjectRow{{
 			Email:           "george@happyplace.dev",
-			Role:            authz.AuthzRoleEditor.String(),
+			Role:            authz.RoleEditor.String(),
 			IdentitySubject: user1.String(),
 			CreatedAt:       time.Time{},
 			UpdatedAt:       time.Time{},
 		}},
 		result: &minder.ListRoleAssignmentsResponse{
 			RoleAssignments: []*minder.RoleAssignment{},
 			Invitations: []*minder.Invitation{{
-				Role:      authz.AuthzRoleEditor.String(),
+				Role:      authz.RoleEditor.String(),
 				Email:     "george@happyplace.dev",
 				Project:   project.String(),
 				Sponsor:   user1.String(),

internal/controlplane/handlers_user.go

@@ -254,7 +254,7 @@ func (s *Server) getUserDependencies(ctx context.Context, user db.User) ([]*pb.P
 			projectRole = &pb.Role{
 				Name:        authzRole.String(),
 				DisplayName: authz.AllRolesDisplayName[authzRole],
-				Description: authz.AllRoles[authzRole],
+				Description: authz.AllRolesDescriptions[authzRole],
 			}
 		}
 

internal/controlplane/identity_events_test.go

@@ -127,7 +127,7 @@ func TestDeleteUserOneProject(t *testing.T) {
 			p1.ID: {
 				{
 					Subject: u1.IdentitySubject,
-					Role:    authz.AuthzRoleAdmin.String(),
+					Role:    authz.RoleAdmin.String(),
 					Project: &pID,
 				},
 			},
@@ -196,19 +196,19 @@ func TestDeleteUserMultiProjectMembership(t *testing.T) {
 			p1.ID: {
 				{
 					Subject: u1.IdentitySubject,
-					Role:    authz.AuthzRoleAdmin.String(),
+					Role:    authz.RoleAdmin.String(),
 					Project: &p1ID,
 				},
 			},
 			p2.ID: {
 				{
 					Subject: u2.IdentitySubject,
-					Role:    authz.AuthzRoleAdmin.String(),
+					Role:    authz.RoleAdmin.String(),
 					Project: &p2ID,
 				},
 				{
 					Subject: u1.IdentitySubject,
-					Role:    authz.AuthzRoleViewer.String(),
+					Role:    authz.RoleViewer.String(),
 					Project: &p2ID,
 				},
 			},
@@ -247,7 +247,7 @@ func TestDeleteUserMultiProjectMembership(t *testing.T) {
 	assert.NotEmpty(t, assignments, "Assignments are empty")
 	assert.Len(t, assignments, 1, "Assignments length is not 1")
 	assert.Equal(t, u2.IdentitySubject, assignments[0].Subject, "User2 is not a member of project2")
-	assert.Equal(t, authz.AuthzRoleAdmin.String(), assignments[0].Role, "User2 is not an admin of project2")
+	assert.Equal(t, authz.RoleAdmin.String(), assignments[0].Role, "User2 is not an admin of project2")
 	assert.Equal(t, p2ID, *assignments[0].Project, "User2 is not a member of project2")
 }
 

internal/projects/creator.go

@@ -124,14 +124,14 @@ func (p *projectCreator) ProvisionSelfEnrolledProject(
 	projectID := uuid.New()
 
 	// Create authorization tuple
-	if err := p.authzClient.Write(ctx, userSub, authz.AuthzRoleAdmin, projectID); err != nil {
+	if err := p.authzClient.Write(ctx, userSub, authz.RoleAdmin, projectID); err != nil {
 		return nil, fmt.Errorf("failed to create authorization tuple: %w", err)
 	}
 	defer func() {
 		// TODO: this can't be part of a transaction, so we should probably find a saga-ish
 		// way to reverse this operation if the transaction fails.
 		if outproj == nil && projerr != nil {
-			if err := p.authzClient.Delete(ctx, userSub, authz.AuthzRoleAdmin, projectID); err != nil {
+			if err := p.authzClient.Delete(ctx, userSub, authz.RoleAdmin, projectID); err != nil {
 				log.Ctx(ctx).Error().Err(err).Msg("failed to delete authorization tuple")
 			}
 		}

internal/projects/deleter.go

@@ -167,6 +167,6 @@ func (p *projectDeleter) DeleteProject(
 
 func hasOtherRoleAssignments(as []*v1.RoleAssignment, subject string) bool {
 	return slices.ContainsFunc(as, func(a *v1.RoleAssignment) bool {
-		return a.GetRole() == authz.AuthzRoleAdmin.String() && a.Subject != subject
+		return a.GetRole() == authz.RoleAdmin.String() && a.Subject != subject
 	})
 }