-
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 2 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 |
---|---|---|
|
@@ -50,6 +50,13 @@ func TestCreateUser_gRPC(t *testing.T) { | |
projectID := uuid.New() | ||
keyCloakUserToken := openid.New() | ||
require.NoError(t, keyCloakUserToken.Set("gh_id", "31337")) | ||
require.NoError(t, keyCloakUserToken.Set("realm_access", map[string]interface{}{ | ||
"roles": []interface{}{ | ||
"default-roles-stacklok", | ||
"offline_access", | ||
"uma_authorization", | ||
}, | ||
})) | ||
|
||
testCases := []struct { | ||
name string | ||
|
@@ -62,7 +69,7 @@ func TestCreateUser_gRPC(t *testing.T) { | |
{ | ||
name: "Success", | ||
req: &pb.CreateUserRequest{}, | ||
buildStubs: func(ctx context.Context, store *mockdb.MockStore, jwt *mockjwt.MockValidator, | ||
buildStubs: func(ctx context.Context, store *mockdb.MockStore, mockJwt *mockjwt.MockValidator, | ||
_ *mockprov.MockGitHubProviderService) context.Context { | ||
tx := sql.Tx{} | ||
store.EXPECT().BeginTransaction().Return(&tx, nil) | ||
|
@@ -83,10 +90,14 @@ func TestCreateUser_gRPC(t *testing.T) { | |
store.EXPECT(). | ||
CreateUser(gomock.Any(), gomock.Any()). | ||
Return(returnedUser, nil) | ||
store.EXPECT(). | ||
GetUnclaimedInstallationsByUser(gomock.Any(), sql.NullString{String: "31337", Valid: true}). | ||
Return([]db.ProviderGithubAppInstallation{}, nil) | ||
store.EXPECT().Commit(gomock.Any()) | ||
store.EXPECT().Rollback(gomock.Any()) | ||
tokenResult, _ := openid.NewBuilder().GivenName("Foo").FamilyName("Bar").Email("[email protected]").Subject("subject1").Build() | ||
jwt.EXPECT().ParseAndValidate(gomock.Any()).Return(tokenResult, nil) | ||
mockJwt.EXPECT().ParseAndValidate(gomock.Any()).Return(tokenResult, nil) | ||
ctx = jwt.WithAuthTokenContext(ctx, keyCloakUserToken) | ||
|
||
return ctx | ||
}, | ||
|
@@ -262,6 +273,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 |
---|---|---|
|
@@ -9,10 +9,13 @@ import ( | |
"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" | ||
|
@@ -35,8 +38,17 @@ func TestProvisionSelfEnrolledProject(t *testing.T) { | |
}, nil) | ||
|
||
ctx := context.Background() | ||
|
||
creator := projects.NewProjectCreator(authzClient, marketplaces.NewNoopMarketplace(), &server.DefaultProfilesConfig{}) | ||
keyCloakUserToken := openid.New() | ||
require.NoError(t, keyCloakUserToken.Set("realm_access", map[string]interface{}{ | ||
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. You can spell |
||
"roles": []interface{}{ | ||
"default-roles-stacklok", | ||
"offline_access", | ||
"uma_authorization", | ||
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. Let's use some non-stacklok-specific examples, like "companyA" or "teamB" |
||
}, | ||
})) | ||
ctx = jwt.WithAuthTokenContext(ctx, keyCloakUserToken) | ||
|
||
creator := projects.NewProjectCreator(authzClient, marketplaces.NewNoopMarketplace(), &server.DefaultProfilesConfig{}, &server.FeaturesConfig{}) | ||
_, err := creator.ProvisionSelfEnrolledProject( | ||
ctx, | ||
mockStore, | ||
|
@@ -62,8 +74,17 @@ 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{}) | ||
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) | ||
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. If you're doing this multiple times, it may be worth having a helper function. 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. Alternatively, if you avoid the errors in |
||
|
||
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( | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
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. If this is simply taking a |
||
func CreateEntitlements(ctx context.Context, qtx db.Querier, projectID uuid.UUID, features []string) error { | ||
for _, feature := range features { | ||
err := qtx.CreateEntitlement(ctx, db.CreateEntitlementParams{ | ||
ProjectID: projectID, | ||
Feature: feature, | ||
}) | ||
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. 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. 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. Yes, that is another thing I also had in mind, the new query cleverly handles this. |
||
if err != nil { | ||
return err | ||
} | ||
} | ||
|
||
return nil | ||
} | ||
|
||
// Is a simple helper function to check if a feature is enabled for a project. | ||
// This does not check the feature's configuration, if any, just that it's enabled. | ||
func featureEnabled(ctx context.Context, store db.Store, projectID uuid.UUID, featureFlag string) bool { | ||
|
Original file line number | Diff line number | Diff line change | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,55 @@ | ||||||||||||||||||||
// SPDX-FileCopyrightText: Copyright 2024 The Minder Authors | ||||||||||||||||||||
// SPDX-License-Identifier: Apache-2.0 | ||||||||||||||||||||
|
||||||||||||||||||||
package server | ||||||||||||||||||||
|
||||||||||||||||||||
import ( | ||||||||||||||||||||
"context" | ||||||||||||||||||||
"fmt" | ||||||||||||||||||||
|
||||||||||||||||||||
"github.com/mindersec/minder/internal/auth/jwt" | ||||||||||||||||||||
) | ||||||||||||||||||||
|
||||||||||||||||||||
// FeaturesConfig is the configuration for the features | ||||||||||||||||||||
type FeaturesConfig struct { | ||||||||||||||||||||
RoleFeatureMapping map[string]string `mapstructure:"role_feature_mapping"` | ||||||||||||||||||||
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. Can you add a comment on what the key and value of the map are? 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. Also, I'm wondering if this should be a |
||||||||||||||||||||
} | ||||||||||||||||||||
|
||||||||||||||||||||
// GetFeaturesForRoles returns the features associated with the roles in the context | ||||||||||||||||||||
func (fc *FeaturesConfig) GetFeaturesForRoles(ctx context.Context) ([]string, error) { | ||||||||||||||||||||
roles, err := extractRolesFromContext(ctx) | ||||||||||||||||||||
if err != nil { | ||||||||||||||||||||
return nil, fmt.Errorf("error extracting roles: %v", err) | ||||||||||||||||||||
} | ||||||||||||||||||||
|
||||||||||||||||||||
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 _, role := range roles { | ||||||||||||||||||||
if feature, ok := fc.RoleFeatureMapping[role]; ok { | ||||||||||||||||||||
features = append(features, feature) | ||||||||||||||||||||
} | ||||||||||||||||||||
} | ||||||||||||||||||||
return features, nil | ||||||||||||||||||||
} | ||||||||||||||||||||
|
||||||||||||||||||||
// extractRolesFromContext extracts roles from the JWT in the context | ||||||||||||||||||||
func extractRolesFromContext(ctx context.Context) ([]string, error) { | ||||||||||||||||||||
var realmAccess map[string]interface{} | ||||||||||||||||||||
if claim, ok := jwt.GetUserClaimFromContext[map[string]interface{}](ctx, "realm_access"); ok { | ||||||||||||||||||||
realmAccess = claim | ||||||||||||||||||||
} else { | ||||||||||||||||||||
return nil, fmt.Errorf("realm_access claim not found") | ||||||||||||||||||||
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. Does this need to be an error, or could a JWT have no 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. Removed all errors returned from this func, I had the same thought. |
||||||||||||||||||||
} | ||||||||||||||||||||
|
||||||||||||||||||||
var roles []string | ||||||||||||||||||||
if rolesInterface, ok := realmAccess["roles"].([]interface{}); ok { | ||||||||||||||||||||
for _, role := range rolesInterface { | ||||||||||||||||||||
if roleStr, ok := role.(string); ok { | ||||||||||||||||||||
roles = append(roles, roleStr) | ||||||||||||||||||||
} | ||||||||||||||||||||
} | ||||||||||||||||||||
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. Does this work as:
Suggested change
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 type assertion was not possible --tested at runtime locally. |
||||||||||||||||||||
} else { | ||||||||||||||||||||
return nil, fmt.Errorf("roles not found in realm_access") | ||||||||||||||||||||
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. Does this need to be an error, or is it okay for this to simply return 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. No, it was removed too. |
||||||||||||||||||||
} | ||||||||||||||||||||
|
||||||||||||||||||||
return roles, nil | ||||||||||||||||||||
} |
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.
Why do this extraction up here, rather than next to the usage?