andwati/zitadel
1
0
Fork 0
mirror of https://github.com/andwati/zitadel.git synced 2026-10-18 05:52:51 +00:00
Code Issues Packages Projects Releases Wiki Activity

Merge commit from fork

Browse Source 
* fix: client_id verification during code exchange and refresh token flows

* add clientID check for device token flow

* Trigger Build
This commit is contained in:
Gayathri Vijayan
2026-06-16 15:19:56 +02:00
committed by GitHub
parent e94d4c3986
commit 0973b074b4
8 changed files with 257 additions and 9 deletions
Download Patch File Download Diff File Expand all files Collapse all files
internal/api/oidc/integration_test/auth_request_test.go
+118
View File
@@ -117,6 +117,62 @@ func TestOPStorage_CreateAccessToken_code(t *testing.T) {
}
}
func TestOPStorage_CreateAccessToken_code_invalid_client(t *testing.T) {
tests := []struct {
name string
authorizeClientID string
codeExchangeClientID string
authRequestID func(t testing.TB, instance *integration.Instance, clientID, redirectURI string, scope ...string) string
}{
{
name: "login header",
authorizeClientID: func() string {
clientID, _ := createClient(t, Instance)
return clientID
}(),
codeExchangeClientID: func() string {
clientID, _ := createClient(t, Instance)
return clientID
}(),
authRequestID: createAuthRequest,
},
{
name: "login v2 config",
authorizeClientID: func() string {
clientID, _ := createClientLoginV2(t, Instance)
return clientID
}(),
codeExchangeClientID: func() string {
clientID, _ := createClientLoginV2(t, Instance)
return clientID
}(),
authRequestID: createAuthRequestNoLoginClientHeader,
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
authRequestID := createAuthRequest(t, Instance, tt.authorizeClientID, redirectURI)
sessionID, sessionToken, _, _ := Instance.CreateVerifiedWebAuthNSession(t, CTXLOGIN, User.GetUserId())
linkResp, err := Instance.Client.OIDCv2.CreateCallback(CTXLOGIN, &oidc_pb.CreateCallbackRequest{
AuthRequestId: authRequestID,
CallbackKind: &oidc_pb.CreateCallbackRequest_Session{
Session: &oidc_pb.Session{
SessionId: sessionID,
SessionToken: sessionToken,
},
},
})
require.NoError(t, err)
code := assertCodeResponse(t, linkResp.GetCallbackUrl())
// the client used for code exchange does not match the client in the auth request -> must fail
tokens, err := exchangeTokens(t, Instance, tt.codeExchangeClientID, code, redirectURI)
require.ErrorContains(t, err, "client_id does not correspond to the client_id in the authorization request")
assert.Nil(t, tokens)
})
}
}
func TestOPStorage_CreateAccessToken_implicit(t *testing.T) {
tests := []struct {
name string
@@ -295,6 +351,68 @@ func TestOPStorage_CreateAccessAndRefreshTokens_refresh(t *testing.T) {
}
}
func TestOPStorage_CreateAccessAndRefreshTokens_refresh_invalid_client(t *testing.T) {
tests := []struct {
name string
clientID string
otherClientID string
authRequestID func(t testing.TB, instance *integration.Instance, clientID, redirectURI string, scope ...string) string
}{
{
name: "login header",
clientID: func() string {
clientID, _ := createClient(t, Instance)
return clientID
}(),
otherClientID: func() string {
clientID, _ := createClient(t, Instance)
return clientID
}(),
authRequestID: createAuthRequest,
},
{
name: "login v2 config",
clientID: func() string {
clientID, _ := createClientLoginV2(t, Instance)
return clientID
}(),
otherClientID: func() string {
clientID, _ := createClient(t, Instance)
return clientID
}(),
authRequestID: createAuthRequestNoLoginClientHeader,
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
authRequestID := tt.authRequestID(t, Instance, tt.clientID, redirectURI, oidc.ScopeOpenID, oidc.ScopeOfflineAccess)
sessionID, sessionToken, startTime, changeTime := Instance.CreateVerifiedWebAuthNSession(t, CTXLOGIN, User.GetUserId())
linkResp, err := Instance.Client.OIDCv2.CreateCallback(CTXLOGIN, &oidc_pb.CreateCallbackRequest{
AuthRequestId: authRequestID,
CallbackKind: &oidc_pb.CreateCallbackRequest_Session{
Session: &oidc_pb.Session{
SessionId: sessionID,
SessionToken: sessionToken,
},
},
})
require.NoError(t, err)
// code exchange
code := assertCodeResponse(t, linkResp.GetCallbackUrl())
tokens, err := exchangeTokens(t, Instance, tt.clientID, code, redirectURI)
require.NoError(t, err)
assertTokens(t, tokens, true)
assertIDTokenClaims(t, tokens.IDTokenClaims, User.GetUserId(), armPasskey, startTime, changeTime, sessionID)
// refresh grant with a different client -> must fail
newTokens, err := refreshTokens(t, tt.otherClientID, tokens.RefreshToken)
require.ErrorContains(t, err, "client_id does not correspond to the client_id in the refresh token")
assert.Nil(t, newTokens)
})
}
}
func TestOPStorage_RevokeToken_access_token(t *testing.T) {
clientID, _ := createClient(t, Instance)
provider, err := Instance.CreateRelyingParty(CTX, clientID, redirectURI)
internal/api/oidc/integration_test/token_device_test.go
+55
View File
@@ -124,3 +124,58 @@ func TestServer_DeviceAuth(t *testing.T) {
})
}
}
func TestServer_DeviceAuth_invalid_client(t *testing.T) {
project := Instance.CreateProject(CTX, t, "", integration.ProjectName(), false, false)
client, err := Instance.CreateOIDCClient(CTX, redirectURI, logoutRedirectURI, project.GetId(), app.OIDCAppType_OIDC_APP_TYPE_NATIVE, app.OIDCAuthMethodType_OIDC_AUTH_METHOD_TYPE_NONE, false, app.OIDCGrantType_OIDC_GRANT_TYPE_DEVICE_CODE)
require.NoError(t, err)
scope := []string{oidc.ScopeOpenID, oidc.ScopeProfile, oidc.ScopeEmail}
provider, err := rp.NewRelyingPartyOIDC(CTX, Instance.OIDCIssuer(), client.GetClientId(), "", "", scope)
require.NoError(t, err)
deviceAuthorization, err := rp.DeviceAuthorization(CTX, scope, provider, nil)
require.NoError(t, err)
relyingPartyDone := make(chan struct{})
go func() {
ctx, cancel := context.WithTimeout(CTX, 1*time.Minute)
defer func() {
cancel()
relyingPartyDone <- struct{}{}
}()
var req *oidc_pb.GetDeviceAuthorizationRequestResponse
retryDuration, tick := integration.WaitForAndTickWithMaxDuration(CTX, time.Minute)
assert.EventuallyWithT(t, func(collectT *assert.CollectT) {
req, err = Instance.Client.OIDCv2.GetDeviceAuthorizationRequest(CTX, &oidc_pb.GetDeviceAuthorizationRequestRequest{
UserCode: deviceAuthorization.UserCode,
})
assert.NoError(collectT, err)
}, retryDuration, tick)
sessionID, sessionToken, _, _ := Instance.CreateVerifiedWebAuthNSession(t, CTXLOGIN, User.GetUserId())
_, err = Instance.Client.OIDCv2.AuthorizeOrDenyDeviceAuthorization(CTXLOGIN, &oidc_pb.AuthorizeOrDenyDeviceAuthorizationRequest{
DeviceAuthorizationId: req.GetDeviceAuthorizationRequest().GetId(),
Decision: &oidc_pb.AuthorizeOrDenyDeviceAuthorizationRequest_Session{
Session: &oidc_pb.Session{
SessionId: sessionID,
SessionToken: sessionToken,
},
},
})
require.NoError(t, err)
otherClient, err := Instance.CreateOIDCClient(CTX, redirectURI, logoutRedirectURI, project.GetId(), app.OIDCAppType_OIDC_APP_TYPE_NATIVE, app.OIDCAuthMethodType_OIDC_AUTH_METHOD_TYPE_NONE, false, app.OIDCGrantType_OIDC_GRANT_TYPE_DEVICE_CODE)
require.NoError(t, err)
otherProvider, err := rp.NewRelyingPartyOIDC(CTX, Instance.OIDCIssuer(), otherClient.GetClientId(), "", "", scope)
require.NoError(t, err)
tokens, err := rp.DeviceAccessToken(ctx, deviceAuthorization.DeviceCode, time.Duration(deviceAuthorization.Interval)*time.Second, otherProvider)
require.ErrorIs(t, err, oidc.ErrInvalidClient())
require.ErrorContains(t, err, "client_id does not correspond to the client_id in the authorization request")
require.Nil(t, tokens)
}()
<-relyingPartyDone
}
internal/api/oidc/token_code.go
+7
View File
@@ -62,6 +62,10 @@ func (s *Server) codeExchangeV1(ctx context.Context, client *Client, req *oidc.A
return nil, err
}
if authReq.GetClientID() != client.client.ClientID {
return nil, oidc.ErrInvalidClient().WithDescription("client_id does not correspond to the client_id in the authorization request")
}
if challenge := authReq.GetCodeChallenge(); challenge != nil || client.AuthMethod() == oidc.AuthMethodNone {
if err = op.AuthorizeCodeChallenge(req.CodeVerifier, challenge); err != nil {
return nil, err
@@ -121,6 +125,9 @@ func (s *Server) getAuthRequestV1ByID(ctx context.Context, id string) (*AuthRequ
func codeExchangeComplianceChecker(client *Client, req *oidc.AccessTokenRequest) command.AuthRequestComplianceChecker {
return func(ctx context.Context, authReq *command.AuthRequestWriteModel) error {
if authReq.ClientID != client.client.ClientID {
return oidc.ErrInvalidClient().WithDescription("client_id does not correspond to the client_id in the authorization request")
}
if authReq.CodeChallenge != nil || client.AuthMethod() == oidc.AuthMethodNone {
err := op.AuthorizeCodeChallenge(req.CodeVerifier, CodeChallengeToOIDC(authReq.CodeChallenge))
if err != nil {
internal/api/oidc/token_device.go
+4 -1
View File
@@ -25,13 +25,16 @@ func (s *Server) DeviceToken(ctx context.Context, r *op.ClientRequest[oidc.Devic
if !ok {
return nil, zerrors.ThrowInternal(nil, "OIDC-Ae2ph", "Error.Internal")
}
session, err := s.command.CreateOIDCSessionFromDeviceAuth(ctx, r.Data.DeviceCode, client.client.BackChannelLogoutURI)
session, err := s.command.CreateOIDCSessionFromDeviceAuth(ctx, r.Data.DeviceCode, client.client.BackChannelLogoutURI, client.client.ClientID)
if err == nil {
return response(s.accessTokenResponseFromSession(ctx, client, session, "", client.client.ProjectID, client.client.ProjectRoleAssertion, client.client.AccessTokenRoleAssertion, client.client.IDTokenRoleAssertion, client.client.IDTokenUserinfoAssertion))
}
if errors.Is(err, context.DeadlineExceeded) {
return nil, oidc.ErrSlowDown().WithParent(err).WithReturnParentToClient(authz.GetFeatures(ctx).DebugOIDCParentError)
}
if errors.Is(err, oidc.ErrInvalidClient()) {
return nil, err
}
var target command.DeviceAuthStateError
if errors.As(err, &target) {
internal/api/oidc/token_refresh.go
+5 -2
View File
@@ -26,7 +26,7 @@ func (s *Server) RefreshToken(ctx context.Context, r *op.ClientRequest[oidc.Refr
return nil, zerrors.ThrowInternal(nil, "OIDC-ga0EP", "Error.Internal")
}
session, err := s.command.ExchangeOIDCSessionRefreshAndAccessToken(ctx, r.Data.RefreshToken, r.Data.Scopes, refreshTokenComplianceChecker())
session, err := s.command.ExchangeOIDCSessionRefreshAndAccessToken(ctx, r.Data.RefreshToken, r.Data.Scopes, client.client.ClientID, refreshTokenComplianceChecker())
if err == nil {
return response(s.accessTokenResponseFromSession(ctx, client, session, "", client.client.ProjectID, client.client.ProjectRoleAssertion, client.client.AccessTokenRoleAssertion, client.client.IDTokenRoleAssertion, client.client.IDTokenUserinfoAssertion))
} else if errors.Is(err, zerrors.ThrowPreconditionFailed(nil, "OIDCS-JOI23", "Errors.OIDCSession.RefreshTokenInvalid")) {
@@ -86,7 +86,10 @@ func (s *Server) refreshTokenV1(ctx context.Context, client *Client, r *op.Clien
// refreshTokenComplianceChecker validates that the requested scope is a subset of the original auth request scope.
func refreshTokenComplianceChecker() command.RefreshTokenComplianceChecker {
return func(_ context.Context, model *command.OIDCSessionWriteModel, requestedScope []string) ([]string, error) {
return func(_ context.Context, model *command.OIDCSessionWriteModel, requestedScope []string, reqClientID string) ([]string, error) {
if model.ClientID != reqClientID {
return nil, oidc.ErrInvalidClient().WithDescription("client_id does not correspond to the client_id in the refresh token")
}
return validateRefreshTokenScopes(model.Scope, requestedScope)
}
}
internal/command/device_auth.go
+6 -1
View File
@@ -5,6 +5,7 @@ import (
"fmt"
"time"
"github.com/zitadel/oidc/v3/pkg/oidc"
"golang.org/x/text/language"
"github.com/zitadel/zitadel/internal/api/authz"
@@ -184,7 +185,7 @@ func (e DeviceAuthStateError) Error() string {
// As devices can poll at various intervals, an explicit state takes precedence over expiry.
// This is to prevent cases where users might approve or deny the authorization on time, but the next poll
// happens after expiry.
func (c *Commands) CreateOIDCSessionFromDeviceAuth(ctx context.Context, deviceCode, backChannelLogoutURI string) (_ *OIDCSession, err error) {
func (c *Commands) CreateOIDCSessionFromDeviceAuth(ctx context.Context, deviceCode, backChannelLogoutURI, clientID string) (_ *OIDCSession, err error) {
ctx, span := tracing.NewSpan(ctx)
defer func() { span.EndWithError(err) }()
@@ -193,6 +194,10 @@ func (c *Commands) CreateOIDCSessionFromDeviceAuth(ctx context.Context, deviceCo
return nil, err
}
if deviceAuthModel.ClientID != clientID {
return nil, oidc.ErrInvalidClient().WithDescription("client_id does not correspond to the client_id in the authorization request")
}
switch deviceAuthModel.State {
case domain.DeviceAuthStateApproved:
break
internal/command/oidc_session.go
+3 -3
View File
@@ -192,11 +192,11 @@ func (c *Commands) CreateOIDCSession(ctx context.Context,
return session, nil
}
type RefreshTokenComplianceChecker func(ctx context.Context, wm *OIDCSessionWriteModel, requestedScope []string) (scope []string, err error)
type RefreshTokenComplianceChecker func(ctx context.Context, wm *OIDCSessionWriteModel, requestedScope []string, reqClientID string) (scope []string, err error)
// ExchangeOIDCSessionRefreshAndAccessToken updates an existing OIDC Session, creates a new access and refresh token.
// It returns the access token id and expiration and the new refresh token.
func (c *Commands) ExchangeOIDCSessionRefreshAndAccessToken(ctx context.Context, refreshToken string, scope []string, complianceCheck RefreshTokenComplianceChecker) (_ *OIDCSession, err error) {
func (c *Commands) ExchangeOIDCSessionRefreshAndAccessToken(ctx context.Context, refreshToken string, scope []string, reqClientID string, complianceCheck RefreshTokenComplianceChecker) (_ *OIDCSession, err error) {
ctx, span := tracing.NewSpan(ctx)
defer func() { span.EndWithError(err) }()
@@ -204,7 +204,7 @@ func (c *Commands) ExchangeOIDCSessionRefreshAndAccessToken(ctx context.Context,
if err != nil {
return nil, err
}
scope, err = complianceCheck(ctx, cmd.oidcSessionWriteModel, scope)
scope, err = complianceCheck(ctx, cmd.oidcSessionWriteModel, scope, reqClientID)
if err != nil {
return nil, err
}
internal/command/oidc_session_test.go
+59 -2
View File
@@ -1644,10 +1644,13 @@ func TestCommands_CreateOIDCSession(t *testing.T) {
}
func mockRefreshTokenComplianceChecker(returnErr error) RefreshTokenComplianceChecker {
return func(_ context.Context, wm *OIDCSessionWriteModel, scope []string) ([]string, error) {
return func(_ context.Context, wm *OIDCSessionWriteModel, scope []string, clientID string) ([]string, error) {
if returnErr != nil {
return nil, returnErr
}
if wm.ClientID != clientID {
return nil, zerrors.ThrowInvalidArgument(nil, "test", "invalid clientID")
}
if len(scope) > 0 {
return scope, nil
}
@@ -1668,6 +1671,7 @@ func TestCommands_ExchangeOIDCSessionRefreshAndAccessToken(t *testing.T) {
ctx context.Context
refreshToken string
scope []string
reqClientID string
complianceCheck RefreshTokenComplianceChecker
}
type res struct {
@@ -1831,6 +1835,58 @@ func TestCommands_ExchangeOIDCSessionRefreshAndAccessToken(t *testing.T) {
err: zerrors.ThrowPreconditionFailed(nil, "OIDCS-J39h2", "Errors.User.NotActive"),
},
},
{
"refresh with an invalid client id fails",
fields{
eventstore: expectEventstore(
expectFilter(
eventFromEventPusherWithCreationDateNow(
oidcsession.NewAddedEvent(context.Background(), &oidcsession.NewAggregate("V2_oidcSessionID", "org1").Aggregate,
"userID", "org1", "sessionID", "clientID", []string{"audience"}, []string{"openid", "profile", "offline_access"},
[]domain.UserAuthMethodType{domain.UserAuthMethodTypePassword}, testNow, "nonce", &language.Afrikaans,
&domain.UserAgent{FingerprintID: gu.Ptr("browserFP")},
),
),
eventFromEventPusherWithCreationDateNow(
oidcsession.NewAccessTokenAddedEvent(context.Background(), &oidcsession.NewAggregate("V2_oidcSessionID", "org1").Aggregate,
"at_accessTokenID", []string{"openid", "profile", "offline_access"}, time.Hour, domain.TokenReasonAuthRequest, nil),
),
eventFromEventPusherWithCreationDateNow(
oidcsession.NewRefreshTokenAddedEvent(context.Background(), &oidcsession.NewAggregate("V2_oidcSessionID", "org1").Aggregate,
"rt_refreshTokenID", 7*24*time.Hour, 24*time.Hour),
),
),
expectFilter(
user.NewHumanAddedEvent(
context.Background(),
&user.NewAggregate("userID", "org1").Aggregate,
"username",
"firstname",
"lastname",
"nickname",
"displayname",
language.Afrikaans,
domain.GenderUnspecified,
"email",
false,
),
),
expectFilter(),
),
idGenerator: mock.NewIDGeneratorExpectIDs(t),
keyAlgorithm: crypto.CreateMockEncryptionAlg(gomock.NewController(t)),
},
args{
ctx: authz.WithInstanceID(context.Background(), "instanceID"),
refreshToken: "V2_oidcSessionID-rt_refreshTokenID:userID", //V2_oidcSessionID:rt_refreshTokenID:userID
scope: []string{"openid", "offline_access"},
reqClientID: "differentClientID",
complianceCheck: mockRefreshTokenComplianceChecker(nil),
},
res{
err: zerrors.ThrowInvalidArgument(nil, "test", "invalid clientID"),
},
},
{
"refresh successful",
fields{
@@ -1885,6 +1941,7 @@ func TestCommands_ExchangeOIDCSessionRefreshAndAccessToken(t *testing.T) {
ctx: authz.WithInstanceID(context.Background(), "instanceID"),
refreshToken: "V2_oidcSessionID-rt_refreshTokenID:userID", //V2_oidcSessionID:rt_refreshTokenID:userID
scope: []string{"openid", "offline_access"},
reqClientID: "clientID",
complianceCheck: mockRefreshTokenComplianceChecker(nil),
},
res{
@@ -1918,7 +1975,7 @@ func TestCommands_ExchangeOIDCSessionRefreshAndAccessToken(t *testing.T) {
keyAlgorithm: tt.fields.keyAlgorithm,
authAlgorithm: &mockAuthCrypto{},
}
got, err := c.ExchangeOIDCSessionRefreshAndAccessToken(tt.args.ctx, tt.args.refreshToken, tt.args.scope, tt.args.complianceCheck)
got, err := c.ExchangeOIDCSessionRefreshAndAccessToken(tt.args.ctx, tt.args.refreshToken, tt.args.scope, tt.args.reqClientID, tt.args.complianceCheck)
require.ErrorIs(t, err, tt.res.err)
if got != nil {
assert.WithinRange(t, got.AuthTime, tt.res.session.AuthTime.Add(-time.Second), tt.res.session.AuthTime.Add(time.Second))