Skip to content
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

fix: improve token OIDC logging #1606

Merged
merged 2 commits into from
Jun 3, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 15 additions & 10 deletions internal/api/token_oidc.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ type IdTokenGrantParams struct {
Issuer string `json:"issuer"`
}

func (p *IdTokenGrantParams) getProvider(ctx context.Context, config *conf.GlobalConfiguration, r *http.Request) (*oidc.Provider, *conf.OAuthProviderConfiguration, string, []string, error) {
func (p *IdTokenGrantParams) getProvider(ctx context.Context, config *conf.GlobalConfiguration, r *http.Request) (*oidc.Provider, bool, string, []string, error) {
log := observability.GetLogEntry(r).Entry

var cfg *conf.OAuthProviderConfiguration
Expand Down Expand Up @@ -54,7 +54,7 @@ func (p *IdTokenGrantParams) getProvider(ctx context.Context, config *conf.Globa
if issuer == "" || !provider.IsAzureIssuer(issuer) {
detectedIssuer, err := provider.DetectAzureIDTokenIssuer(ctx, p.IdToken)
if err != nil {
return nil, nil, "", nil, badRequestError(ErrorCodeValidationFailed, "Unable to detect issuer in ID token for Azure provider").WithInternalError(err)
return nil, false, "", nil, badRequestError(ErrorCodeValidationFailed, "Unable to detect issuer in ID token for Azure provider").WithInternalError(err)
}
issuer = detectedIssuer
}
Expand Down Expand Up @@ -95,20 +95,25 @@ func (p *IdTokenGrantParams) getProvider(ctx context.Context, config *conf.Globa
}

if !allowed {
return nil, nil, "", nil, badRequestError(ErrorCodeValidationFailed, fmt.Sprintf("Custom OIDC provider %q not allowed", p.Provider))
return nil, false, "", nil, badRequestError(ErrorCodeValidationFailed, fmt.Sprintf("Custom OIDC provider %q not allowed", p.Provider))
}

cfg = &conf.OAuthProviderConfiguration{
Enabled: true,
SkipNonceCheck: false,
}
}

if cfg != nil && !cfg.Enabled {
return nil, nil, "", nil, badRequestError(ErrorCodeProviderDisabled, fmt.Sprintf("Provider (issuer %q) is not enabled", issuer))
if !cfg.Enabled {
return nil, false, "", nil, badRequestError(ErrorCodeProviderDisabled, fmt.Sprintf("Provider (issuer %q) is not enabled", issuer))
}

oidcProvider, err := oidc.NewProvider(ctx, issuer)
if err != nil {
return nil, nil, "", nil, err
return nil, false, "", nil, err
}

return oidcProvider, cfg, providerType, acceptableClientIDs, nil
return oidcProvider, cfg.SkipNonceCheck, providerType, acceptableClientIDs, nil
}

// IdTokenGrant implements the id_token grant type flow
Expand All @@ -131,7 +136,7 @@ func (a *API) IdTokenGrant(ctx context.Context, w http.ResponseWriter, r *http.R
return oauthError("invalid request", "provider or client_id and issuer required")
}

oidcProvider, oauthConfig, providerType, acceptableClientIDs, err := params.getProvider(ctx, config, r)
oidcProvider, skipNonceCheck, providerType, acceptableClientIDs, err := params.getProvider(ctx, config, r)
if err != nil {
return err
}
Expand Down Expand Up @@ -179,10 +184,10 @@ func (a *API) IdTokenGrant(ctx context.Context, w http.ResponseWriter, r *http.R
}

if !correctAudience {
return oauthError("invalid request", "Unacceptable audience in id_token")
return oauthError("invalid request", fmt.Sprintf("Unacceptable audience in id_token: %v", idToken.Audience))
J0 marked this conversation as resolved.
Show resolved Hide resolved
}

if !oauthConfig.SkipNonceCheck {
kangmingtay marked this conversation as resolved.
Show resolved Hide resolved
if !skipNonceCheck {
tokenHasNonce := idToken.Nonce != ""
paramsHasNonce := params.Nonce != ""

Expand Down
69 changes: 69 additions & 0 deletions internal/api/token_oidc_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
package api

import (
"context"
"net/http"
"net/http/httptest"
"testing"

"github.com/stretchr/testify/require"
"github.com/stretchr/testify/suite"
"github.com/supabase/auth/internal/conf"
)

type TokenOIDCTestSuite struct {
suite.Suite
API *API
Config *conf.GlobalConfiguration
}

func TestTokenOIDC(t *testing.T) {
api, config, err := setupAPIForTest()
require.NoError(t, err)

ts := &TokenOIDCTestSuite{
API: api,
Config: config,
}
defer api.db.Close()

suite.Run(t, ts)
}

func SetupTestOIDCProvider(ts *TokenOIDCTestSuite) *httptest.Server {
var server *httptest.Server
server = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
switch r.URL.Path {
case "/.well-known/openid-configuration":
w.WriteHeader(http.StatusOK)
w.Write([]byte(`{"issuer":"` + server.URL + `","authorization_endpoint":"` + server.URL + `/authorize","token_endpoint":"` + server.URL + `/token","jwks_uri":"` + server.URL + `/jwks"}`))
default:
w.WriteHeader(http.StatusNotFound)
}
}))
return server
}

func (ts *TokenOIDCTestSuite) TestGetProvider() {
server := SetupTestOIDCProvider(ts)
defer server.Close()

params := &IdTokenGrantParams{
IdToken: "test-id-token",
AccessToken: "test-access-token",
Nonce: "test-nonce",
Provider: server.URL,
ClientID: "test-client-id",
Issuer: server.URL,
}

ts.Config.External.AllowedIdTokenIssuers = []string{server.URL}

req := httptest.NewRequest(http.MethodPost, "http://localhost", nil)
oidcProvider, skipNonceCheck, providerType, acceptableClientIds, err := params.getProvider(context.Background(), ts.Config, req)
require.NoError(ts.T(), err)
require.NotNil(ts.T(), oidcProvider)
require.False(ts.T(), skipNonceCheck)
require.Equal(ts.T(), params.Provider, providerType)
require.NotEmpty(ts.T(), acceptableClientIds)
}
Loading