mirror of
https://github.com/andwati/zitadel.git
synced 2026-11-08 07:05:12 +00:00
fix(idp): apply PKCE when building OAuth and OIDC providers (#12247)
# Which Problems Are Solved
- IDP intent authorization redirects used by Login v2 ignored the
configured `UsePKCE` setting for Generic OAuth providers
- This caused providers such as X/Twitter OAuth2 to receive
authorization requests without `code_challenge` and
`code_challenge_method`
- OIDC provider construction had the same gap, even though PKCE is
already part of the provider configuration model
- Existing IDP intent redirect tests expected non-PKCE OAuth URLs and
failed once PKCE was applied correctly
# How the Problems Are Solved
- Updated `OAuthIDPWriteModel.ToProvider` in
internal/command/idp_model.go to pass `rp.WithPKCE(nil)` when `UsePKCE`
is enabled
- Updated `OIDCIDPWriteModel.ToProvider` to apply the same PKCE
relying-party option for OIDC providers
# Additional Changes
- Added focused provider-construction tests covering:
- OAuth provider redirects include code_challenge
- OIDC provider redirects include code_challenge
- both providers use code_challenge_method=S256
- both persist the generated codeVerifier for token exchange
- Updated `TestCommands_AuthFromProvider` in
internal/command/idp_intent_test.go so OAuth redirect assertions verify
PKCE structurally instead of hard-coding the generated challenge value
# Additional Context
- Reproduced with X/Twitter OAuth2 where the generated authorization URL
was missing PKCE parameters despite `usePkce: true`
- Verified both the focused PKCE tests and the full unit test suite run
successfully with the fix
- Closes #12036
- Closes/supersedes #12054:
- Tests are included
- No slice re-allocation on `opts` append
# Result
### Before
`authUrl` in `StartIdentityProviderIntent` response is missing
`code_challenge` and `code_challenge_method` for Generic OAuth IDP with
PKCE enabled:
```json
{
"details": {
"sequence": "1",
"changeDate": "2026-06-07T18:48:16.921317Z",
"resourceOwner": "376298239768395779"
},
"authUrl": "https://x.com/i/oauth2/authorize?client_id=<REDACTED>&prompt=select_account&redirect_uri=http%3A%2F%2Flocalhost%3A8080%2Fidps%2Fcallback&response_type=code&scope=tweet.read+users.email+users.read+offline.access&state=376417136207200259"
}
```
### After
`code_challenge` and `code_challenge_method` are correctly included into
`authUrl`:
```json
{
"details": {
"sequence": "1",
"changeDate": "2026-06-07T18:49:39.488941Z",
"resourceOwner": "376298239768395779"
},
"authUrl": "https://x.com/i/oauth2/authorize?client_id=<REDACTED>&code_challenge=8G4vN8QNgSsbvGSHKwYPEc2qUYU2BK5L0fsr992duTA&code_challenge_method=S256&prompt=select_account&redirect_uri=http%3A%2F%2Flocalhost%3A8080%2Fidps%2Fcallback&response_type=code&scope=tweet.read+users.email+users.read+offline.access&state=376417274736672771"
}
```
This commit is contained in:
@@ -1,12 +1,22 @@
|
||||
package command
|
||||
|
||||
import (
|
||||
"context"
|
||||
"encoding/json"
|
||||
"net/http"
|
||||
"net/http/httptest"
|
||||
"net/url"
|
||||
"sync/atomic"
|
||||
"testing"
|
||||
|
||||
"github.com/stretchr/testify/assert"
|
||||
"github.com/stretchr/testify/require"
|
||||
oidc_pkg "github.com/zitadel/oidc/v3/pkg/oidc"
|
||||
|
||||
"github.com/zitadel/zitadel/internal/crypto"
|
||||
"github.com/zitadel/zitadel/internal/domain"
|
||||
providers "github.com/zitadel/zitadel/internal/idp"
|
||||
"github.com/zitadel/zitadel/internal/idp/providers/oauth"
|
||||
"github.com/zitadel/zitadel/internal/zerrors"
|
||||
)
|
||||
|
||||
@@ -353,3 +363,92 @@ func TestCommands_AllIDPWriteModel(t *testing.T) {
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
func TestOAuthIDPWriteModel_ToProvider_WithPKCE(t *testing.T) {
|
||||
wm := &OAuthIDPWriteModel{
|
||||
Name: "OAuth",
|
||||
ClientID: "clientID",
|
||||
ClientSecret: &crypto.CryptoValue{Algorithm: "plain", Crypted: []byte("clientSecret"), KeyID: "keyID"},
|
||||
AuthorizationEndpoint: "https://idp.example.com/authorize",
|
||||
TokenEndpoint: "https://idp.example.com/token",
|
||||
UserEndpoint: "https://idp.example.com/user",
|
||||
Scopes: []string{"user"},
|
||||
IDAttribute: "id",
|
||||
UsePKCE: true,
|
||||
}
|
||||
|
||||
provider, err := wm.ToProvider("https://zitadel.example.com/idps/callback", plainTextEncryption{})
|
||||
require.NoError(t, err)
|
||||
|
||||
assertProviderUsesPKCE(t, provider)
|
||||
}
|
||||
|
||||
func TestOIDCIDPWriteModel_ToProvider_WithPKCE(t *testing.T) {
|
||||
var (
|
||||
issuer string
|
||||
discoveryRequested atomic.Bool
|
||||
)
|
||||
handler := func(w http.ResponseWriter, r *http.Request) {
|
||||
if r.URL.Path != oidc_pkg.DiscoveryEndpoint {
|
||||
http.NotFound(w, r)
|
||||
return
|
||||
}
|
||||
discoveryRequested.Store(true)
|
||||
w.Header().Set("Content-Type", "application/json")
|
||||
// assert, not require: FailNow must not be called from the server's handler goroutine
|
||||
assert.NoError(t, json.NewEncoder(w).Encode(&oidc_pkg.DiscoveryConfiguration{
|
||||
Issuer: issuer,
|
||||
AuthorizationEndpoint: issuer + "/authorize",
|
||||
TokenEndpoint: issuer + "/token",
|
||||
UserinfoEndpoint: issuer + "/userinfo",
|
||||
}))
|
||||
}
|
||||
|
||||
server := httptest.NewServer(http.HandlerFunc(handler))
|
||||
t.Cleanup(server.Close)
|
||||
issuer = server.URL
|
||||
|
||||
wm := &OIDCIDPWriteModel{
|
||||
Name: "OIDC",
|
||||
Issuer: issuer,
|
||||
ClientID: "clientID",
|
||||
ClientSecret: &crypto.CryptoValue{Algorithm: "plain", Crypted: []byte("clientSecret"), KeyID: "keyID"},
|
||||
Scopes: []string{"openid"},
|
||||
UsePKCE: true,
|
||||
}
|
||||
|
||||
provider, err := wm.ToProvider("https://zitadel.example.com/idps/callback", plainTextEncryption{})
|
||||
require.NoError(t, err)
|
||||
require.True(t, discoveryRequested.Load(), "expected OIDC discovery to be requested")
|
||||
|
||||
assertProviderUsesPKCE(t, provider)
|
||||
}
|
||||
|
||||
func assertProviderUsesPKCE(t *testing.T, provider providers.Provider) {
|
||||
t.Helper()
|
||||
|
||||
session, err := provider.BeginAuth(context.Background(), "state")
|
||||
require.NoError(t, err)
|
||||
auth, err := session.GetAuth(context.Background())
|
||||
require.NoError(t, err)
|
||||
redirect, ok := auth.(*providers.RedirectAuth)
|
||||
require.True(t, ok)
|
||||
authURL, err := url.Parse(redirect.RedirectURL)
|
||||
require.NoError(t, err)
|
||||
|
||||
query := authURL.Query()
|
||||
assert.NotEmpty(t, query.Get("code_challenge"))
|
||||
assert.Equal(t, "S256", query.Get("code_challenge_method"))
|
||||
assert.NotEmpty(t, session.PersistentParameters()[oauth.CodeVerifier])
|
||||
}
|
||||
|
||||
type plainTextEncryption struct{}
|
||||
|
||||
func (plainTextEncryption) Algorithm() string { return "plain" }
|
||||
func (plainTextEncryption) EncryptionKeyID() string { return "keyID" }
|
||||
func (plainTextEncryption) DecryptionKeyIDs() []string { return []string{"keyID"} }
|
||||
func (plainTextEncryption) Encrypt(value []byte) ([]byte, error) { return value, nil }
|
||||
func (plainTextEncryption) Decrypt(hashed []byte, _ string) ([]byte, error) { return hashed, nil }
|
||||
func (plainTextEncryption) DecryptString(hashed []byte, _ string) (string, error) {
|
||||
return string(hashed), nil
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user