Add entitlements assignment at the time of project creation #4963

teodor-yanev · 2024-11-13T12:44:31Z

Summary

Provide a brief overview of the changes and the issue being addressed.
Explain the rationale and any background necessary for understanding the changes.
List dependencies required by this change, if any.

This PR adds the functionality of automatically creating entitlements records for new projects. This will be done for both project creation paths - parent and sub-projects.

The process is as follows:

In the config we have (e.g.:)

features:
  role_feature_mapping:
    stacklok-employees: "stacklok"
    default-roles-stacklok: "default"
    offline_access: "offline"

From the user's JWT, we get Keycloak-related data in the form of:

  "realm_access": {
    "roles": [
      "stacklok-employees",
      "default-roles-stacklok",
      "offline_access",
      "uma_authorization"
    ]
  },

For each of the three features (stacklok, default, offline), a new entitlement record will be created, mapping the ID of the newly created project.

This not only introduces flexibility in setting up Minder by simply adjusting a config mapping but also helps have separate configurations for each environment and adequate telemetry data at the time of creation instead of aggregating and analysing batched data periodically.

Change Type

Mark the type of change your PR introduces:

Bug fix (resolves an issue without affecting existing features)
Feature (adds new functionality without breaking changes)
Breaking change (may impact existing functionalities or require documentation updates)
Documentation (updates or additions to documentation)
Refactoring or test improvements (no bug fixes or new functionality)

Testing

Outline how the changes were tested, including steps to reproduce and any relevant configurations.
Attach screenshots if helpful.

Tested locally and with updated Unit tests.

Review Checklist:

Reviewed my own code for quality and clarity.
Added comments to complex or tricky code sections.
Updated any affected documentation.
Included tests that validate the fix or feature.
Checked that related changes are merged.

coveralls · 2024-11-13T13:42:30Z

coverage: 54.763% (-0.002%) from 54.765%
when pulling f1dd858 on add-automatic-entitlements-assignment
into 84d517a on main.

evankanderson

A few top-level comments:

I'm not sure I'd use the terms "Role" and "Feature" given that we also have OpenFGA roles and OpenFeature features. Maybe "Claim" or "membership" and "Entitlement"?
This needs a test which covers the new code you've added to actually add some entitlements to a project.
I'd tend to prefer "return empty" over "return error" for cases where fields are not found unless we can't proceed without that information.

evankanderson · 2024-11-13T17:18:03Z

pkg/config/server/features.go

+
+// FeaturesConfig is the configuration for the features
+type FeaturesConfig struct {
+	RoleFeatureMapping map[string]string `mapstructure:"role_feature_mapping"`


Can you add a comment on what the key and value of the map are?

Also, I'm wondering if this should be a map[string][]string, to allow multiple features (entitlements) associated with a particular role.

evankanderson · 2024-11-13T17:20:41Z

pkg/config/server/features.go

+	if claim, ok := jwt.GetUserClaimFromContext[map[string]interface{}](ctx, "realm_access"); ok {
+		realmAccess = claim
+	} else {
+		return nil, fmt.Errorf("realm_access claim not found")


Does this need to be an error, or could a JWT have no realm_access field?

Removed all errors returned from this func, I had the same thought.

evankanderson · 2024-11-13T17:27:10Z

pkg/config/server/features.go

+	if rolesInterface, ok := realmAccess["roles"].([]interface{}); ok {
+		for _, role := range rolesInterface {
+			if roleStr, ok := role.(string); ok {
+				roles = append(roles, roleStr)
+			}
+		}


Does this work as:

Suggested change

if rolesInterface, ok := realmAccess["roles"].([]interface{}); ok {

for _, role := range rolesInterface {

if roleStr, ok := role.(string); ok {

roles = append(roles, roleStr)

}

}

if roles, ok := realmAccess["roles"].([]string); ok {

return roles, nil

}

This type assertion was not possible --tested at runtime locally.
Still, refactored this part.

evankanderson · 2024-11-13T17:27:15Z

pkg/config/server/features.go

+			}
+		}
+	} else {
+		return nil, fmt.Errorf("roles not found in realm_access")


Does this need to be an error, or is it okay for this to simply return nil (a valid empty slice).

No, it was removed too.

evankanderson · 2024-11-13T17:28:39Z

internal/projects/features/features.go

@@ -30,6 +30,22 @@ func ProjectAllowsProjectHierarchyOperations(ctx context.Context, store db.Store
 	return featureEnabled(ctx, store, projectID, projectHierarchyOperationsEnabledFlag)
 }

+// CreateEntitlements creates entitlements for a project
+// It takes a 'qtx' because it is usually called within a transaction


If this is simply taking a db.Querier, I don't think the name needs to be explained.

evankanderson · 2024-11-13T17:29:22Z

internal/projects/features/features.go

+		err := qtx.CreateEntitlement(ctx, db.CreateEntitlementParams{
+			ProjectID: projectID,
+			Feature:   feature,
+		})


Can we write a single query that inserts multiple rows at once? I'm not sure it matters, this is more a matter of curiousity.

Yes, that is another thing I also had in mind, the new query cleverly handles this.

evankanderson · 2024-11-13T17:30:26Z

internal/projects/creator_test.go

-
-	creator := projects.NewProjectCreator(authzClient, marketplaces.NewNoopMarketplace(), &server.DefaultProfilesConfig{})
+	keyCloakUserToken := openid.New()
+	require.NoError(t, keyCloakUserToken.Set("realm_access", map[string]interface{}{


You can spell interface{} as any as of Go 1.20 or so.

evankanderson · 2024-11-13T17:36:42Z

internal/projects/creator_test.go

+			"default-roles-stacklok",
+			"offline_access",
+			"uma_authorization",


Let's use some non-stacklok-specific examples, like "companyA" or "teamB"

evankanderson · 2024-11-13T17:37:12Z

internal/projects/creator_test.go

+	keyCloakUserToken := openid.New()
+	require.NoError(t, keyCloakUserToken.Set("realm_access", map[string]interface{}{
+		"roles": []interface{}{
+			"default-roles-stacklok",
+			"offline_access",
+			"uma_authorization",
+		},
+	}))
+	ctx = jwt.WithAuthTokenContext(ctx, keyCloakUserToken)


If you're doing this multiple times, it may be worth having a helper function.

Alternatively, if you avoid the errors in features.go, you might not need to change this.

evankanderson · 2024-11-13T17:38:40Z

internal/projects/creator.go

+	// Retrieve the role-to-feature mapping from the configuration
+	projectFeatures, err := p.featuresCfg.GetFeaturesForRoles(ctx)
+	if err != nil {
+		return nil, fmt.Errorf("error getting features for roles: %w", err)
+	}


Why do this extraction up here, rather than next to the usage?

JAORMX · 2024-11-14T06:39:28Z

I'm not sure I'd use the terms "Role" and "Feature" given that we also have OpenFGA roles and OpenFeature features. Maybe "Claim" or "membership" and "Entitlement"?

We do have the term feature within Minder and it's a database table which is linked to projects via entitlements. So the term feature might not be too bad after all in this context. I certainly would advice against using "role" here, though.

teodor-yanev · 2024-11-14T14:00:44Z

Thanks for the review @evankanderson, I've addressed all the concerns we had.

evankanderson

The only item I feel strongly enough to not approve is the names of the db.CreateEntitlementsParams -- I'd like names that help us avoid swapping parameters.

evankanderson · 2024-11-14T17:05:10Z

pkg/config/server/features.go

+
+	var features []string
+	for _, m := range memberships {
+		if feature, ok := fc.MembershipFeatureMapping[m]; ok {


What a about:

Suggested change

if feature, ok := fc.MembershipFeatureMapping[m]; ok {

if feature := fc.MembershipFeatureMapping[m]; feature != "" {

Which will cover both "not found" and "was an empty string", which we probably don't want.

evankanderson · 2024-11-14T17:18:47Z

pkg/config/server/features.go

+	memberships := make([]string, len(rawMemberships))
+	for i, membership := range rawMemberships {
+		if membershipStr, ok := membership.(string); ok {
+			memberships[i] = membershipStr
+		}
+	}


This will leave empty-string memberships if fields can't be converted to string. (Which shouldn't happen, but if we're checking, we should assume it could fail.)

Suggested change

memberships := make([]string, len(rawMemberships))

for i, membership := range rawMemberships {

if membershipStr, ok := membership.(string); ok {

memberships[i] = membershipStr

}

}

memberships := make([]string, 0, len(rawMemberships))

for i, membership := range rawMemberships {

if membershipStr, ok := membership.(string); ok {

memberships = append(memberships, membershipStr)

}

}

evankanderson · 2024-11-14T17:19:41Z

pkg/config/server/features.go

+func (fc *FeaturesConfig) GetFeaturesForMemberships(ctx context.Context) []string {
+	memberships := extractMembershipsFromContext(ctx)
+
+	var features []string


On line 45, you pre-allocate the slice. Do the same thing here?

Suggested change

var features []string

features := make([]string, 0, len(memberships))

evankanderson · 2024-11-14T17:23:12Z

internal/projects/creator_test.go

@@ -107,3 +128,16 @@ func TestProvisionSelfEnrolledProjectInvalidName(t *testing.T) {
 	}

 }
+
+// prepareTestToken creates a JWT token with the specified roles and returns the context with the token.
+func prepareTestToken(t *testing.T, roles []any) context.Context {


This should probably take an existing context, so you can use it as a wrapper. (Just a standard pattern, like context.WithValue())

evankanderson · 2024-11-14T17:28:40Z

internal/projects/creator.go

+		Column1: projectFeatures,
+		Column2: project.ID,


Can we rename these parameters from Column1 and Column2? 😁

Absolutely 😄

evankanderson · 2024-11-18T19:01:37Z

internal/controlplane/handlers_projects.go

+	// Retrieve the membership-to-feature mapping from the configuration
+	projectFeatures := s.cfg.Features.GetFeaturesForMemberships(ctx)
+	if err := qtx.CreateEntitlements(ctx, db.CreateEntitlementsParams{
+		Features:  projectFeatures,
+		ProjectID: subProject.ID,
+	}); err != nil {
+		return nil, status.Errorf(codes.Internal, "error creating entitlements: %v", err)
+	}


This is sort-of-weird: this is creating a child project free-form here, separate from the internal/projects code. It's not clear to me whether these child projects should:

Copy entitlements from the parent project

Use entitlements based on the specific parent project member who creates them

Have an empty set of entitlements, and the entitlement queries are extended to check parents.

Right now, it looks like were doing option 2, but it's not clear that is correct.

Let's file an issue and assign it to @ethomson to figure out the desired way to handle entitlements in sub-projects, because those are currently behind an entitlement, and we probably know most of the users of that feature.

Also, file a bug to move this code to internal/projects, so all project creation happens in that one module, and internal/controlplane gets a bit smaller.

I did find it weird that we have two separate paths for project creation, especially one of them being exclusive to the handler and was going to follow-up on that, thanks for noting it as well.

Here are the new issues:

#5001
#5002

evankanderson · 2024-11-18T19:03:32Z

internal/projects/creator_test.go

+	ctx := prepareTestToken(context.Background(), t, []any{
+		"teamA",
+		"teamB",
+		"teamC",


Nice addition to have a membership that doesn't convert to a feature.

add: automatic-entitlements-assignment

442fe73

teodor-yanev requested a review from a team as a code owner November 13, 2024 12:44

fix: lint order imports

85fe297

teodor-yanev self-assigned this Nov 13, 2024

evankanderson reviewed Nov 13, 2024

View reviewed changes

update: refactoring + updated query

81b6754

teodor-yanev added 4 commits November 14, 2024 13:21

update: revert and update user_test

b66e6ae

add: test; refactoring

7095bc5

update: refactoring

91fc68a

update: var name

25b4ec5

teodor-yanev requested a review from evankanderson November 14, 2024 14:22

evankanderson reviewed Nov 14, 2024

View reviewed changes

update: slight refactoring

f1dd858

teodor-yanev requested a review from evankanderson November 18, 2024 10:14

evankanderson approved these changes Nov 18, 2024

View reviewed changes

teodor-yanev merged commit 6bd6f55 into main Nov 19, 2024
25 checks passed

teodor-yanev deleted the add-automatic-entitlements-assignment branch November 19, 2024 07:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add entitlements assignment at the time of project creation #4963

Add entitlements assignment at the time of project creation #4963

teodor-yanev commented Nov 13, 2024 •

edited

Loading

coveralls commented Nov 13, 2024 •

edited

Loading

evankanderson left a comment

evankanderson Nov 13, 2024

evankanderson Nov 13, 2024

evankanderson Nov 13, 2024

teodor-yanev Nov 14, 2024

evankanderson Nov 13, 2024

teodor-yanev Nov 14, 2024 •

edited

Loading

evankanderson Nov 13, 2024

teodor-yanev Nov 14, 2024

evankanderson Nov 13, 2024

evankanderson Nov 13, 2024

teodor-yanev Nov 14, 2024

evankanderson Nov 13, 2024

evankanderson Nov 13, 2024

evankanderson Nov 13, 2024

evankanderson Nov 13, 2024

evankanderson Nov 13, 2024

JAORMX commented Nov 14, 2024

teodor-yanev commented Nov 14, 2024

evankanderson left a comment

evankanderson Nov 14, 2024

evankanderson Nov 14, 2024

evankanderson Nov 14, 2024

evankanderson Nov 14, 2024

evankanderson Nov 14, 2024

teodor-yanev Nov 18, 2024

evankanderson Nov 18, 2024

evankanderson Nov 18, 2024

teodor-yanev Nov 19, 2024

evankanderson Nov 18, 2024

	if feature, ok := fc.MembershipFeatureMapping[m]; ok {
	if feature := fc.MembershipFeatureMapping[m]; feature != "" {

	var features []string
	features := make([]string, 0, len(memberships))

Add entitlements assignment at the time of project creation #4963

Add entitlements assignment at the time of project creation #4963

Conversation

teodor-yanev commented Nov 13, 2024 • edited Loading

Summary

Change Type

Testing

Review Checklist:

coveralls commented Nov 13, 2024 • edited Loading

evankanderson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

teodor-yanev Nov 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JAORMX commented Nov 14, 2024

teodor-yanev commented Nov 14, 2024

evankanderson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

teodor-yanev commented Nov 13, 2024 •

edited

Loading

coveralls commented Nov 13, 2024 •

edited

Loading

teodor-yanev Nov 14, 2024 •

edited

Loading