-
Notifications
You must be signed in to change notification settings - Fork 42
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 7 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.Column1, expectedFeatures) { | ||
t.Errorf("expected features %v, got %v", expectedFeatures, params.Column1) | ||
} | ||
return nil | ||
}) | ||
|
||
ctx := prepareTestToken(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,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 { | ||
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. This should probably take an existing context, so you can use it as a wrapper. (Just a standard pattern, like |
||
t.Helper() | ||
|
||
token := openid.New() | ||
require.NoError(t, token.Set("realm_access", map[string]any{ | ||
"roles": roles, | ||
})) | ||
|
||
ctx := jwt.WithAuthTokenContext(context.Background(), token) | ||
return ctx | ||
} |
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) | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
var features []string | ||||||||||||||||||||||||||
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. On line 45, you pre-allocate the slice. Do the same thing here?
Suggested change
|
||||||||||||||||||||||||||
for _, m := range memberships { | ||||||||||||||||||||||||||
if feature, ok := fc.MembershipFeatureMapping[m]; ok { | ||||||||||||||||||||||||||
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. What a about:
Suggested change
Which will cover both "not found" and "was an empty string", which we probably don't want. |
||||||||||||||||||||||||||
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, len(rawMemberships)) | ||||||||||||||||||||||||||
for i, membership := range rawMemberships { | ||||||||||||||||||||||||||
if membershipStr, ok := membership.(string); ok { | ||||||||||||||||||||||||||
memberships[i] = membershipStr | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
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. 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
|
||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
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.
Can we rename these parameters from
Column1
andColumn2
? 😁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.
Absolutely 😄