diff --git a/internal/api/oidc/client_integration_test.go b/internal/api/oidc/client_integration_test.go index c7ace3c097..2ad88cf096 100644 --- a/internal/api/oidc/client_integration_test.go +++ b/internal/api/oidc/client_integration_test.go @@ -348,7 +348,7 @@ func createInvalidKeyData(t testing.TB, client *management.AddOIDCAppResponse) [ } func TestServer_CreateAccessToken_ClientCredentials(t *testing.T) { - clientID, clientSecret, err := Tester.CreateOIDCCredentialsClient(CTX) + _, clientID, clientSecret, err := Tester.CreateOIDCCredentialsClient(CTX) require.NoError(t, err) type clientDetails struct { diff --git a/internal/api/oidc/introspect.go b/internal/api/oidc/introspect.go index 0615193b03..a2e59c9a45 100644 --- a/internal/api/oidc/introspect.go +++ b/internal/api/oidc/introspect.go @@ -99,7 +99,7 @@ func (s *Server) Introspect(ctx context.Context, r *op.Request[op.IntrospectionR if err = validateIntrospectionAudience(token.audience, client.clientID, client.projectID); err != nil { return nil, err } - userInfo, err := s.userInfo(ctx, token.userID, client.projectID, client.projectRoleAssertion, token.scope, []string{client.projectID}) + userInfo, err := s.userInfo(ctx, token.userID, token.scope, client.projectID, client.projectRoleAssertion, true) if err != nil { return nil, err } diff --git a/internal/api/oidc/token_exchange.go b/internal/api/oidc/token_exchange.go index c50cf5859d..e9c6ed27d4 100644 --- a/internal/api/oidc/token_exchange.go +++ b/internal/api/oidc/token_exchange.go @@ -216,7 +216,7 @@ func (s *Server) createExchangeTokens(ctx context.Context, tokenType oidc.TokenT ) if slices.Contains(scopes, oidc.ScopeOpenID) || tokenType == oidc.JWTTokenType || tokenType == oidc.IDTokenType { projectID := client.client.ProjectID - userInfo, err = s.userInfo(ctx, subjectToken.userID, projectID, client.client.ProjectRoleAssertion, scopes, []string{projectID}) + userInfo, err = s.userInfo(ctx, subjectToken.userID, scopes, projectID, client.client.ProjectRoleAssertion, false) if err != nil { return nil, err } diff --git a/internal/api/oidc/userinfo.go b/internal/api/oidc/userinfo.go index e05a1a9f5d..c59e9c0952 100644 --- a/internal/api/oidc/userinfo.go +++ b/internal/api/oidc/userinfo.go @@ -53,18 +53,28 @@ func (s *Server) UserInfo(ctx context.Context, r *op.Request[oidc.UserInfoReques } } - userInfo, err := s.userInfo(ctx, token.userID, projectID, assertion, token.scope, nil) + userInfo, err := s.userInfo(ctx, token.userID, token.scope, projectID, assertion, false) if err != nil { return nil, err } return op.NewResponse(userInfo), nil } -func (s *Server) userInfo(ctx context.Context, userID, projectID string, projectRoleAssertion bool, scope, roleAudience []string) (_ *oidc.UserInfo, err error) { +// userInfo gets the user's data based on the scope. +// The returned UserInfo contains standard and reserved claims, documented +// here: https://zitadel.com/docs/apis/openidoauth/claims. +// +// projectID is an optional parameter which defines the default audience when there are any (or all) role claims requested. +// projectRoleAssertion sets the default of returning all project roles, only if no specific roles were requested in the scope. +// +// currentProjectOnly can be set to use the current project ID only and ignore the audience from the scope. +// It should be set in cases where the client doesn't need to know roles outside its own project, +// for example an introspection client. +func (s *Server) userInfo(ctx context.Context, userID string, scope []string, projectID string, projectRoleAssertion, currentProjectOnly bool) (_ *oidc.UserInfo, err error) { ctx, span := tracing.NewSpan(ctx) defer func() { span.EndWithError(err) }() - roleAudience, requestedRoles := prepareRoles(ctx, projectID, projectRoleAssertion, scope, roleAudience) + roleAudience, requestedRoles := prepareRoles(ctx, scope, projectID, projectRoleAssertion, currentProjectOnly) qu, err := s.query.GetOIDCUserInfo(ctx, userID, roleAudience) if err != nil { return nil, err @@ -74,31 +84,35 @@ func (s *Server) userInfo(ctx context.Context, userID, projectID string, project return userInfo, s.userinfoFlows(ctx, qu, userInfo) } -// prepareRoles scans the requested scopes, appends to roleAudience and returns the requestedRoles. +// prepareRoles scans the requested scopes and builds the requested roles +// and the audience for which roles need to be asserted. // // Scopes with [ScopeProjectRolePrefix] are added to requestedRoles. -// When [ScopeProjectsRoles] is present and roleAudience was empty, -// project IDs with the [domain.ProjectIDScope] prefix are added to the roleAudience. +// When [ScopeProjectsRoles] is present project IDs with the [domain.ProjectIDScope] +// prefix are added to the returned audience. // -// If projectRoleAssertion is true and the resulting requestedRoles or roleAudience are not empty, -// the current projectID will always be parts or roleAudience. -// Else nil, nil is returned. -func prepareRoles(ctx context.Context, projectID string, projectRoleAssertion bool, scope, roleAudience []string) (ra, requestedRoles []string) { - // if all roles are requested take the audience for those from the scopes - if slices.Contains(scope, ScopeProjectsRoles) && len(roleAudience) == 0 { - roleAudience = domain.AddAudScopeToAudience(ctx, roleAudience, scope) - } - requestedRoles = make([]string, 0, len(scope)) +// If projectRoleAssertion is true and there were no specific roles requested, +// the current projectID will always be parts of the returned audience. +func prepareRoles(ctx context.Context, scope []string, projectID string, projectRoleAssertion, currentProjectOnly bool) (roleAudience, requestedRoles []string) { for _, s := range scope { if role, ok := strings.CutPrefix(s, ScopeProjectRolePrefix); ok { requestedRoles = append(requestedRoles, role) } } - if !projectRoleAssertion && len(requestedRoles) == 0 && len(roleAudience) == 0 { - return nil, nil + + // If roles are requested take the audience for those from the scopes, + // when currentProjectOnly is not set. + if !currentProjectOnly && (len(requestedRoles) > 0 || slices.Contains(scope, ScopeProjectsRoles)) { + roleAudience = domain.AddAudScopeToAudience(ctx, roleAudience, scope) } - if projectID != "" && !slices.Contains(roleAudience, projectID) { + // When either: + // - Project role assertion is set; + // - Roles for the current project (only) are requested; + // - There is already a roleAudience requested through scope; + // - There are requested roles through the scope; + // and the projectID is not empty, projectID must be part of the roleAudience. + if (projectRoleAssertion || currentProjectOnly || len(roleAudience) > 0 || len(requestedRoles) > 0) && projectID != "" && !slices.Contains(roleAudience, projectID) { roleAudience = append(roleAudience, projectID) } return roleAudience, requestedRoles diff --git a/internal/api/oidc/userinfo_integration_test.go b/internal/api/oidc/userinfo_integration_test.go index 78cd5479ed..22e688ff4b 100644 --- a/internal/api/oidc/userinfo_integration_test.go +++ b/internal/api/oidc/userinfo_integration_test.go @@ -15,6 +15,7 @@ import ( "golang.org/x/oauth2" oidc_api "github.com/zitadel/zitadel/internal/api/oidc" + "github.com/zitadel/zitadel/internal/domain" "github.com/zitadel/zitadel/internal/integration" feature "github.com/zitadel/zitadel/pkg/grpc/feature/v2beta" "github.com/zitadel/zitadel/pkg/grpc/management" @@ -66,20 +67,13 @@ func TestServer_UserInfo(t *testing.T) { // testServer_UserInfo is the actual userinfo integration test, // which calls the userinfo endpoint with different client configurations, roles and token scopes. func testServer_UserInfo(t *testing.T) { - const role = "testUserRole" + const ( + roleFoo = "foo" + roleBar = "bar" + ) + clientID, projectID := createClient(t) - _, err := Tester.Client.Mgmt.AddProjectRole(CTX, &management.AddProjectRoleRequest{ - ProjectId: projectID, - RoleKey: role, - DisplayName: "test", - }) - require.NoError(t, err) - _, err = Tester.Client.Mgmt.AddUserGrant(CTX, &management.AddUserGrantRequest{ - UserId: User.GetUserId(), - ProjectId: projectID, - RoleKeys: []string{role}, - }) - require.NoError(t, err) + addProjectRolesGrants(t, User.GetUserId(), projectID, roleFoo, roleBar) tests := []struct { name string @@ -149,18 +143,34 @@ func testServer_UserInfo(t *testing.T) { assertions: []func(*testing.T, *oidc.UserInfo){ assertUserinfo, func(t *testing.T, ui *oidc.UserInfo) { - assertProjectRoleClaims(t, projectID, ui.Claims, role) + assertProjectRoleClaims(t, projectID, ui.Claims, true, roleFoo, roleBar) }, }, }, { - name: "projects roles scope", + name: "project role scope", prepare: getTokens, - scope: []string{oidc.ScopeProfile, oidc.ScopeOpenID, oidc.ScopeEmail, oidc.ScopeOfflineAccess, oidc_api.ScopeProjectRolePrefix + role}, + scope: []string{oidc.ScopeProfile, oidc.ScopeOpenID, oidc.ScopeEmail, oidc.ScopeOfflineAccess, + oidc_api.ScopeProjectRolePrefix + roleFoo, + }, assertions: []func(*testing.T, *oidc.UserInfo){ assertUserinfo, func(t *testing.T, ui *oidc.UserInfo) { - assertProjectRoleClaims(t, projectID, ui.Claims, role) + assertProjectRoleClaims(t, projectID, ui.Claims, true, roleFoo) + }, + }, + }, + { + name: "project role and audience scope", + prepare: getTokens, + scope: []string{oidc.ScopeProfile, oidc.ScopeOpenID, oidc.ScopeEmail, oidc.ScopeOfflineAccess, + oidc_api.ScopeProjectRolePrefix + roleFoo, + domain.ProjectIDScope + projectID + domain.AudSuffix, + }, + assertions: []func(*testing.T, *oidc.UserInfo){ + assertUserinfo, + func(t *testing.T, ui *oidc.UserInfo) { + assertProjectRoleClaims(t, projectID, ui.Claims, true, roleFoo) }, }, }, @@ -211,6 +221,57 @@ func testServer_UserInfo(t *testing.T) { } } +// https://github.com/zitadel/zitadel/issues/6662 +func TestServer_UserInfo_Issue6662(t *testing.T) { + const ( + roleFoo = "foo" + roleBar = "bar" + ) + + project, err := Tester.CreateProject(CTX) + projectID := project.GetId() + require.NoError(t, err) + userID, clientID, clientSecret, err := Tester.CreateOIDCCredentialsClient(CTX) + require.NoError(t, err) + addProjectRolesGrants(t, userID, projectID, roleFoo, roleBar) + + scope := []string{oidc.ScopeProfile, oidc.ScopeOpenID, oidc.ScopeEmail, oidc.ScopeOfflineAccess, + oidc_api.ScopeProjectRolePrefix + roleFoo, + domain.ProjectIDScope + projectID + domain.AudSuffix, + } + + provider, err := rp.NewRelyingPartyOIDC(CTX, Tester.OIDCIssuer(), clientID, clientSecret, redirectURI, scope) + require.NoError(t, err) + tokens, err := rp.ClientCredentials(CTX, provider, nil) + require.NoError(t, err) + + userinfo, err := rp.Userinfo[*oidc.UserInfo](CTX, tokens.AccessToken, tokens.TokenType, userID, provider) + require.NoError(t, err) + assertProjectRoleClaims(t, projectID, userinfo.Claims, false, roleFoo) +} + +func addProjectRolesGrants(t *testing.T, userID, projectID string, roles ...string) { + t.Helper() + bulkRoles := make([]*management.BulkAddProjectRolesRequest_Role, len(roles)) + for i, role := range roles { + bulkRoles[i] = &management.BulkAddProjectRolesRequest_Role{ + Key: role, + DisplayName: role, + } + } + _, err := Tester.Client.Mgmt.BulkAddProjectRoles(CTX, &management.BulkAddProjectRolesRequest{ + ProjectId: projectID, + Roles: bulkRoles, + }) + require.NoError(t, err) + _, err = Tester.Client.Mgmt.AddUserGrant(CTX, &management.AddUserGrantRequest{ + UserId: userID, + ProjectId: projectID, + RoleKeys: roles, + }) + require.NoError(t, err) +} + func getTokens(t *testing.T, clientID string, scope []string) *oidc.Tokens[*oidc.IDTokenClaims] { authRequestID := createAuthRequest(t, clientID, redirectURI, scope...) sessionID, sessionToken, startTime, changeTime := Tester.CreateVerifiedWebAuthNSession(t, CTXLOGIN, User.GetUserId()) @@ -255,10 +316,13 @@ func assertNoReservedScopes(t *testing.T, claims map[string]any) { } } -func assertProjectRoleClaims(t *testing.T, projectID string, claims map[string]any, roles ...string) { +func assertProjectRoleClaims(t *testing.T, projectID string, claims map[string]any, claimProjectRole bool, roles ...string) { t.Helper() - projectIDRoleClaim := fmt.Sprintf(oidc_api.ClaimProjectRolesFormat, projectID) - for _, claim := range []string{oidc_api.ClaimProjectRoles, projectIDRoleClaim} { + projectRoleClaims := []string{fmt.Sprintf(oidc_api.ClaimProjectRolesFormat, projectID)} + if claimProjectRole { + projectRoleClaims = append(projectRoleClaims, oidc_api.ClaimProjectRoles) + } + for _, claim := range projectRoleClaims { roleMap, ok := claims[claim].(map[string]any) require.Truef(t, ok, "claim %s not found or wrong type %T", claim, claims[claim]) for _, roleKey := range roles { diff --git a/internal/api/oidc/userinfo_test.go b/internal/api/oidc/userinfo_test.go index 21e06e21c4..65354a4040 100644 --- a/internal/api/oidc/userinfo_test.go +++ b/internal/api/oidc/userinfo_test.go @@ -18,25 +18,25 @@ import ( func Test_prepareRoles(t *testing.T) { type args struct { projectID string - projectRoleAssertion bool scope []string - roleAudience []string + projectRoleAssertion bool + currentProjectOnly bool } tests := []struct { name string args args - wantRa []string + wantRoleAudience []string wantRequestedRoles []string }{ { - name: "empty scope and roleAudience", + name: "empty scope", args: args{ projectID: "projID", - projectRoleAssertion: false, scope: nil, - roleAudience: nil, + projectRoleAssertion: false, + currentProjectOnly: false, }, - wantRa: nil, + wantRoleAudience: nil, wantRequestedRoles: nil, }, { @@ -45,50 +45,121 @@ func Test_prepareRoles(t *testing.T) { projectID: "projID", projectRoleAssertion: true, scope: nil, - roleAudience: nil, + currentProjectOnly: false, }, - wantRa: []string{"projID"}, - wantRequestedRoles: []string{}, + wantRoleAudience: []string{"projID"}, + wantRequestedRoles: nil, }, { - name: "some scope and roleAudience", + name: "some scope, current project only", args: args{ projectID: "projID", projectRoleAssertion: false, scope: []string{"openid", "profile"}, - roleAudience: []string{"project2"}, + currentProjectOnly: true, }, - wantRa: []string{"project2", "projID"}, - wantRequestedRoles: []string{}, + wantRoleAudience: []string{"projID"}, + wantRequestedRoles: nil, }, { name: "scope projects roles", args: args{ projectID: "projID", projectRoleAssertion: false, - scope: []string{ScopeProjectsRoles, domain.ProjectIDScope + "project2" + domain.AudSuffix}, - roleAudience: nil, + scope: []string{ + "openid", "profile", + ScopeProjectsRoles, + domain.ProjectIDScope + "project2" + domain.AudSuffix, + }, + currentProjectOnly: false, }, - wantRa: []string{"project2", "projID"}, - wantRequestedRoles: []string{}, + wantRoleAudience: []string{"project2", "projID"}, + wantRequestedRoles: nil, + }, + { + name: "scope projects roles ignored, current project only", + args: args{ + projectID: "projID", + projectRoleAssertion: false, + scope: []string{ + "openid", "profile", + ScopeProjectsRoles, + domain.ProjectIDScope + "project2" + domain.AudSuffix, + }, + currentProjectOnly: true, + }, + wantRoleAudience: []string{"projID"}, + wantRequestedRoles: nil, }, { name: "scope project role prefix", args: args{ projectID: "projID", projectRoleAssertion: false, - scope: []string{"openid", "profile", ScopeProjectRolePrefix + "foo", ScopeProjectRolePrefix + "bar"}, - roleAudience: nil, + scope: []string{ + "openid", "profile", + ScopeProjectRolePrefix + "foo", + ScopeProjectRolePrefix + "bar", + }, + currentProjectOnly: false, }, - wantRa: []string{"projID"}, + wantRoleAudience: []string{"projID"}, + wantRequestedRoles: []string{"foo", "bar"}, + }, + { + name: "scope project role prefix and audience", + args: args{ + projectID: "projID", + projectRoleAssertion: false, + scope: []string{ + "openid", "profile", + ScopeProjectRolePrefix + "foo", + ScopeProjectRolePrefix + "bar", + domain.ProjectIDScope + "project2" + domain.AudSuffix, + }, + currentProjectOnly: false, + }, + wantRoleAudience: []string{"projID", "project2"}, + wantRequestedRoles: []string{"foo", "bar"}, + }, + { + name: "scope project role prefix and audience ignored, current project only", + args: args{ + projectID: "projID", + projectRoleAssertion: false, + scope: []string{ + "openid", "profile", + ScopeProjectRolePrefix + "foo", + ScopeProjectRolePrefix + "bar", + domain.ProjectIDScope + "project2" + domain.AudSuffix, + }, + currentProjectOnly: true, + }, + wantRoleAudience: []string{"projID"}, + wantRequestedRoles: []string{"foo", "bar"}, + }, + { + name: "no projectID, scope project role prefix and audience", + args: args{ + projectID: "", + projectRoleAssertion: false, + scope: []string{ + "openid", "profile", + ScopeProjectRolePrefix + "foo", + ScopeProjectRolePrefix + "bar", + domain.ProjectIDScope + "project2" + domain.AudSuffix, + }, + currentProjectOnly: false, + }, + wantRoleAudience: []string{"project2"}, wantRequestedRoles: []string{"foo", "bar"}, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - gotRa, gotRequestedRoles := prepareRoles(context.Background(), tt.args.projectID, tt.args.projectRoleAssertion, tt.args.scope, tt.args.roleAudience) - assert.Equal(t, tt.wantRa, gotRa, "roleAudience") - assert.Equal(t, tt.wantRequestedRoles, gotRequestedRoles, "requestedRoles") + gotRoleAudience, gotRequestedRoles := prepareRoles(context.Background(), tt.args.scope, tt.args.projectID, tt.args.projectRoleAssertion, tt.args.currentProjectOnly) + assert.ElementsMatch(t, tt.wantRoleAudience, gotRoleAudience, "roleAudience") + assert.ElementsMatch(t, tt.wantRequestedRoles, gotRequestedRoles, "requestedRoles") }) } } diff --git a/internal/integration/oidc.go b/internal/integration/oidc.go index 765da68b4d..d928c3e004 100644 --- a/internal/integration/oidc.go +++ b/internal/integration/oidc.go @@ -281,7 +281,7 @@ func CheckRedirect(req *http.Request) (*url.URL, error) { return resp.Location() } -func (s *Tester) CreateOIDCCredentialsClient(ctx context.Context) (string, string, error) { +func (s *Tester) CreateOIDCCredentialsClient(ctx context.Context) (userID, clientID, clientSecret string, err error) { name := gofakeit.Username() user, err := s.Client.Mgmt.AddMachineUser(ctx, &management.AddMachineUserRequest{ Name: name, @@ -289,13 +289,13 @@ func (s *Tester) CreateOIDCCredentialsClient(ctx context.Context) (string, strin AccessTokenType: user.AccessTokenType_ACCESS_TOKEN_TYPE_JWT, }) if err != nil { - return "", "", err + return "", "", "", err } secret, err := s.Client.Mgmt.GenerateMachineSecret(ctx, &management.GenerateMachineSecretRequest{ UserId: user.GetUserId(), }) if err != nil { - return "", "", err + return "", "", "", err } - return secret.GetClientId(), secret.GetClientSecret(), nil + return user.GetUserId(), secret.GetClientId(), secret.GetClientSecret(), nil }