Skip to content

Commit

Permalink
fix(oidc): roles in userinfo for client credentials token (#7763)
Browse files Browse the repository at this point in the history 
* fix(oidc): roles in userinfo for client credentials token

When tokens were obtained using the client credentials grant,
with audience and role scopes, userinfo would not return the role claims. This had multiple causes:

1. There is no auth request flow, so for legacy userinfo project data was never attached to the token
2. For optimized userinfo, there is no client ID that maps to an application. The client ID for client credentials is the machine user's name. There we can't obtain a project ID. When the project ID remained empty, we always ignored the roleAudience.

This PR fixes situation 2, by always taking the roleAudience into account, even when the projectID is empty. The code responsible for the bug is also refactored to be more readable and understandable, including additional godoc.

The fix only applies to the optimized userinfo code introduced in #7706 and released in v2.50 (currently in RC). Therefore it can't be back-ported to earlier versions.

Fixes #6662

* chore(deps): update all go deps (#7764)

This change updates all go modules, including oidc, a major version of go-jose and the go 1.22 release.

* Revert "chore(deps): update all go deps" (#7772)

Revert "chore(deps): update all go deps (#7764)"

This reverts commit 6893e7d.

---------

Co-authored-by: Livio Spring <livio.a@gmail.com>
  • Loading branch information
@muhlemmer @livio-a
muhlemmer and livio-a committed Apr 16, 2024
1 parent 9bcfa12 commit 9ccbbe0
Show file tree
Hide file tree
Showing 7 changed files with 218 additions and 69 deletions.
2 changes: 1 addition & 1 deletion internal/api/oidc/client_integration_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
2 changes: 1 addition & 1 deletion internal/api/oidc/introspect.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down
2 changes: 1 addition & 1 deletion internal/api/oidc/token_exchange.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down
50 changes: 32 additions & 18 deletions internal/api/oidc/userinfo.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand Down
104 changes: 84 additions & 20 deletions internal/api/oidc/userinfo_integration_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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)
},
},
},
Expand Down Expand Up @@ -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())
Expand Down Expand Up @@ -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 {
Expand Down

0 comments on commit 9ccbbe0

Please sign in to comment.