-
Notifications
You must be signed in to change notification settings - Fork 43
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add entitlements assignment at the time of project creation #4963
Changes from all commits
442fe73
85fe297
81b6754
b66e6ae
7095bc5
91fc68a
25b4ec5
f1dd858
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -83,6 +83,8 @@ func TestCreateUser_gRPC(t *testing.T) { | |
store.EXPECT(). | ||
CreateUser(gomock.Any(), gomock.Any()). | ||
Return(returnedUser, nil) | ||
store.EXPECT().CreateEntitlements(gomock.Any(), gomock.Any()). | ||
Return(nil) | ||
store.EXPECT().Commit(gomock.Any()) | ||
store.EXPECT().Rollback(gomock.Any()) | ||
tokenResult, _ := openid.NewBuilder().GivenName("Foo").FamilyName("Bar").Email("[email protected]").Subject("subject1").Build() | ||
|
@@ -262,6 +264,7 @@ func TestCreateUser_gRPC(t *testing.T) { | |
authz, | ||
marketplaces.NewNoopMarketplace(), | ||
&serverconfig.DefaultProfilesConfig{}, | ||
&serverconfig.FeaturesConfig{}, | ||
), | ||
} | ||
|
||
|
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,13 +6,17 @@ package projects_test | |
import ( | ||
"context" | ||
"fmt" | ||
"reflect" | ||
"testing" | ||
|
||
"github.com/google/uuid" | ||
"github.com/lestrrat-go/jwx/v2/jwt/openid" | ||
"github.com/stretchr/testify/assert" | ||
"github.com/stretchr/testify/require" | ||
"go.uber.org/mock/gomock" | ||
|
||
mockdb "github.com/mindersec/minder/database/mock" | ||
"github.com/mindersec/minder/internal/auth/jwt" | ||
"github.com/mindersec/minder/internal/authz/mock" | ||
"github.com/mindersec/minder/internal/db" | ||
"github.com/mindersec/minder/internal/marketplaces" | ||
|
@@ -33,10 +37,28 @@ func TestProvisionSelfEnrolledProject(t *testing.T) { | |
Return(db.Project{ | ||
ID: uuid.New(), | ||
}, nil) | ||
mockStore.EXPECT().CreateEntitlements(gomock.Any(), gomock.Any()). | ||
DoAndReturn(func(_ context.Context, params db.CreateEntitlementsParams) error { | ||
expectedFeatures := []string{"featureA", "featureB"} | ||
if !reflect.DeepEqual(params.Features, expectedFeatures) { | ||
t.Errorf("expected features %v, got %v", expectedFeatures, params.Features) | ||
} | ||
return nil | ||
}) | ||
|
||
ctx := prepareTestToken(context.Background(), t, []any{ | ||
"teamA", | ||
"teamB", | ||
"teamC", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice addition to have a membership that doesn't convert to a feature. |
||
}) | ||
|
||
creator := projects.NewProjectCreator(authzClient, marketplaces.NewNoopMarketplace(), &server.DefaultProfilesConfig{}, &server.FeaturesConfig{ | ||
MembershipFeatureMapping: map[string]string{ | ||
"teamA": "featureA", | ||
"teamB": "featureB", | ||
}, | ||
}) | ||
|
||
ctx := context.Background() | ||
|
||
creator := projects.NewProjectCreator(authzClient, marketplaces.NewNoopMarketplace(), &server.DefaultProfilesConfig{}) | ||
_, err := creator.ProvisionSelfEnrolledProject( | ||
ctx, | ||
mockStore, | ||
|
@@ -62,8 +84,7 @@ func TestProvisionSelfEnrolledProjectFailsWritingProjectToDB(t *testing.T) { | |
Return(db.Project{}, fmt.Errorf("failed to create project")) | ||
|
||
ctx := context.Background() | ||
|
||
creator := projects.NewProjectCreator(authzClient, marketplaces.NewNoopMarketplace(), &server.DefaultProfilesConfig{}) | ||
creator := projects.NewProjectCreator(authzClient, marketplaces.NewNoopMarketplace(), &server.DefaultProfilesConfig{}, &server.FeaturesConfig{}) | ||
_, err := creator.ProvisionSelfEnrolledProject( | ||
ctx, | ||
mockStore, | ||
|
@@ -94,7 +115,7 @@ func TestProvisionSelfEnrolledProjectInvalidName(t *testing.T) { | |
|
||
mockStore := mockdb.NewMockStore(ctrl) | ||
ctx := context.Background() | ||
creator := projects.NewProjectCreator(authzClient, marketplaces.NewNoopMarketplace(), &server.DefaultProfilesConfig{}) | ||
creator := projects.NewProjectCreator(authzClient, marketplaces.NewNoopMarketplace(), &server.DefaultProfilesConfig{}, &server.FeaturesConfig{}) | ||
|
||
for _, tc := range testCases { | ||
_, err := creator.ProvisionSelfEnrolledProject( | ||
|
@@ -107,3 +128,15 @@ func TestProvisionSelfEnrolledProjectInvalidName(t *testing.T) { | |
} | ||
|
||
} | ||
|
||
// prepareTestToken creates a JWT token with the specified roles and returns the context with the token. | ||
func prepareTestToken(ctx context.Context, t *testing.T, roles []any) context.Context { | ||
t.Helper() | ||
|
||
token := openid.New() | ||
require.NoError(t, token.Set("realm_access", map[string]any{ | ||
"roles": roles, | ||
})) | ||
|
||
return jwt.WithAuthTokenContext(ctx, token) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,53 @@ | ||
// SPDX-FileCopyrightText: Copyright 2024 The Minder Authors | ||
// SPDX-License-Identifier: Apache-2.0 | ||
|
||
package server | ||
|
||
import ( | ||
"context" | ||
|
||
"github.com/mindersec/minder/internal/auth/jwt" | ||
) | ||
|
||
// FeaturesConfig is the configuration for the features | ||
type FeaturesConfig struct { | ||
// MembershipFeatureMapping maps a membership to a feature | ||
MembershipFeatureMapping map[string]string `mapstructure:"membership_feature_mapping"` | ||
} | ||
|
||
// GetFeaturesForMemberships returns the features associated with the memberships in the context | ||
func (fc *FeaturesConfig) GetFeaturesForMemberships(ctx context.Context) []string { | ||
memberships := extractMembershipsFromContext(ctx) | ||
|
||
features := make([]string, 0, len(memberships)) | ||
for _, m := range memberships { | ||
if feature := fc.MembershipFeatureMapping[m]; feature != "" { | ||
features = append(features, feature) | ||
} | ||
} | ||
|
||
return features | ||
} | ||
|
||
// extractMembershipsFromContext extracts memberships from the JWT in the context. | ||
// Returns empty slice if no memberships are found. | ||
func extractMembershipsFromContext(ctx context.Context) []string { | ||
realmAccess, ok := jwt.GetUserClaimFromContext[map[string]any](ctx, "realm_access") | ||
if !ok { | ||
return nil | ||
} | ||
|
||
rawMemberships, ok := realmAccess["roles"].([]any) | ||
if !ok { | ||
return nil | ||
} | ||
|
||
memberships := make([]string, 0, len(rawMemberships)) | ||
for _, membership := range rawMemberships { | ||
if membershipStr, ok := membership.(string); ok { | ||
memberships = append(memberships, membershipStr) | ||
} | ||
} | ||
|
||
return memberships | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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: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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, file a bug to move this code to
internal/projects
, so all project creation happens in that one module, andinternal/controlplane
gets a bit smaller.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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