diff --git a/pkg/authserver/server/handlers/callback.go b/pkg/authserver/server/handlers/callback.go index 1b110c2283..727cc36769 100644 --- a/pkg/authserver/server/handlers/callback.go +++ b/pkg/authserver/server/handlers/callback.go @@ -122,7 +122,15 @@ func (h *Handler) CallbackHandler(w http.ResponseWriter, req *http.Request) { user, err := h.userResolver.ResolveUser(ctx, providerID, providerSubject) if err != nil { slog.Error("failed to resolve user", "error", err) - h.provider.WriteAuthorizeError(ctx, w, ar, fosite.ErrServerError.WithHint("failed to resolve user")) + // A UserStorage that deliberately refuses to auto-provision this + // identity (e.g. SCIM-only provisioning) is a client-facing denial, + // not a server fault — every other resolution failure keeps the + // generic server_error mapping. + resolveErr := fosite.ErrServerError.WithHint("failed to resolve user") + if errors.Is(err, storage.ErrUserNotProvisioned) { + resolveErr = fosite.ErrAccessDenied.WithHint("user not provisioned") + } + h.provider.WriteAuthorizeError(ctx, w, ar, resolveErr) return } subject = user.ID diff --git a/pkg/authserver/server/handlers/callback_test.go b/pkg/authserver/server/handlers/callback_test.go index a4efe103d8..add96cf56a 100644 --- a/pkg/authserver/server/handlers/callback_test.go +++ b/pkg/authserver/server/handlers/callback_test.go @@ -427,6 +427,84 @@ func TestCallbackHandler_IdentityResolutionFailure(t *testing.T) { // --- Multi-upstream chain tests --- +func TestCallbackHandler_UserResolutionFailure_UserNotProvisioned_DeniesAccess(t *testing.T) { + t.Parallel() + handler, storState, mockUpstream := handlerTestSetup(t, withCreateUserError(storage.ErrUserNotProvisioned)) + + // A subject with no existing provider identity takes UserResolver's create + // path, where the configured storage refuses to auto-provision it. + mockUpstream.exchangeResult = &upstream.Identity{ + Tokens: &upstream.Tokens{ + AccessToken: "upstream-access-token", + ExpiresAt: time.Now().Add(time.Hour), + }, + Subject: "unprovisioned-user", + } + + internalState := testInternalState + pending := &storage.PendingAuthorization{ + ClientID: testAuthClientID, + RedirectURI: testAuthRedirectURI, + State: "client-state", + PKCEChallenge: "challenge123", + PKCEMethod: "S256", + Scopes: []string{"openid"}, + InternalState: internalState, + SessionID: "session-not-provisioned", + UpstreamProviderName: "test-upstream", + CreatedAt: time.Now(), + } + storState.pendingAuths[internalState] = pending + + req := httptest.NewRequest(http.MethodGet, "/oauth/callback?code=upstream-code&state="+internalState, nil) + rec := httptest.NewRecorder() + + handler.CallbackHandler(rec, req) + + assert.Equal(t, http.StatusSeeOther, rec.Code) + location := rec.Header().Get("Location") + assert.Contains(t, location, "error=access_denied", + "a deliberate provisioning refusal must deny the login, not surface as a server error") +} + +func TestCallbackHandler_UserResolutionFailure_OtherError_ServerError(t *testing.T) { + t.Parallel() + handler, storState, mockUpstream := handlerTestSetup(t, withCreateUserError(assert.AnError)) + + mockUpstream.exchangeResult = &upstream.Identity{ + Tokens: &upstream.Tokens{ + AccessToken: "upstream-access-token", + ExpiresAt: time.Now().Add(time.Hour), + }, + Subject: "some-user", + } + + internalState := testInternalState + pending := &storage.PendingAuthorization{ + ClientID: testAuthClientID, + RedirectURI: testAuthRedirectURI, + State: "client-state", + PKCEChallenge: "challenge123", + PKCEMethod: "S256", + Scopes: []string{"openid"}, + InternalState: internalState, + SessionID: "session-other-error", + UpstreamProviderName: "test-upstream", + CreatedAt: time.Now(), + } + storState.pendingAuths[internalState] = pending + + req := httptest.NewRequest(http.MethodGet, "/oauth/callback?code=upstream-code&state="+internalState, nil) + rec := httptest.NewRecorder() + + handler.CallbackHandler(rec, req) + + assert.Equal(t, http.StatusSeeOther, rec.Code) + location := rec.Header().Get("Location") + assert.Contains(t, location, "error=server_error", + "an internal storage failure unrelated to provisioning refusal keeps mapping to server_error") +} + func TestCallbackHandler_TwoUpstreams_FirstLeg_RedirectsToSecond(t *testing.T) { t.Parallel() handler, storState, provider1, _ := multiUpstreamTestSetup(t) diff --git a/pkg/authserver/server/handlers/helpers_test.go b/pkg/authserver/server/handlers/helpers_test.go index a391a2a0b9..1f87ed1b39 100644 --- a/pkg/authserver/server/handlers/helpers_test.go +++ b/pkg/authserver/server/handlers/helpers_test.go @@ -107,6 +107,7 @@ type baseTestSetupOption func(*baseTestSetupConfig) type baseTestSetupConfig struct { storePendingErr error // if non-nil, StorePendingAuthorization always returns this error getLatestUpstreamTokensErr error // if non-nil, GetLatestUpstreamTokensForUser always returns this error + createUserErr error // if non-nil, CreateUser always returns this error } func withStorePendingError(err error) baseTestSetupOption { @@ -121,6 +122,12 @@ func withGetLatestUpstreamTokensError(err error) baseTestSetupOption { } } +func withCreateUserError(err error) baseTestSetupOption { + return func(c *baseTestSetupConfig) { + c.createUserErr = err + } +} + // baseTestSetup creates the shared test infrastructure (RSA keys, fosite provider, mock storage // with all expectations wired, including upstream token mocks). Callers create the Handler. func baseTestSetup(t *testing.T, opts ...baseTestSetupOption) (fosite.OAuth2Provider, *server.AuthorizationServerConfig, *mocks.MockStorage, *testStorageState) { @@ -289,11 +296,16 @@ func baseTestSetup(t *testing.T, opts ...baseTestSetupOption) (fosite.OAuth2Prov stor.EXPECT().RevokeRefreshToken(gomock.Any(), gomock.Any()).Return(nil).AnyTimes() // Setup mock expectations for user storage (needed by UserResolver) - stor.EXPECT().CreateUser(gomock.Any(), gomock.Any()).DoAndReturn( - func(_ context.Context, user *storage.User) error { - storState.users[user.ID] = user - return nil - }).AnyTimes() + if setupCfg.createUserErr != nil { + // CreateUser always fails with the configured error + stor.EXPECT().CreateUser(gomock.Any(), gomock.Any()).Return(setupCfg.createUserErr).AnyTimes() + } else { + stor.EXPECT().CreateUser(gomock.Any(), gomock.Any()).DoAndReturn( + func(_ context.Context, user *storage.User) error { + storState.users[user.ID] = user + return nil + }).AnyTimes() + } stor.EXPECT().GetUser(gomock.Any(), gomock.Any()).DoAndReturn( func(_ context.Context, id string) (*storage.User, error) { diff --git a/pkg/authserver/storage/types.go b/pkg/authserver/storage/types.go index 2d80d062b8..e33665edd2 100644 --- a/pkg/authserver/storage/types.go +++ b/pkg/authserver/storage/types.go @@ -57,6 +57,14 @@ var ( // ErrReservedClientID is returned when a caller attempts to register a // real client whose ID collides with SyntheticClientIDPrefix. ErrReservedClientID = errors.New("storage: client id uses reserved synthetic prefix") + + // ErrUserNotProvisioned is returned by a UserStorage.CreateUser implementation + // that deliberately refuses to auto-provision an upstream identity (e.g. a + // deployment where user accounts are only created out-of-band, such as via + // SCIM). It signals the authorization callback to deny the login with + // fosite.ErrAccessDenied instead of treating the refusal as an internal + // server error. + ErrUserNotProvisioned = errors.New("storage: user not provisioned") ) // DefaultPendingAuthorizationTTL is the default TTL for pending authorization requests. @@ -748,6 +756,10 @@ type UpstreamTokenRefresher interface { type UserStorage interface { // CreateUser creates a new user account. // Returns ErrAlreadyExists if a user with the same ID already exists. + // A deployment that provisions users out-of-band (e.g. via SCIM) and never + // auto-creates them here may return ErrUserNotProvisioned to deny the login + // instead; the caller (the authorization callback) maps that specifically to + // an OAuth access_denied response rather than a server error. CreateUser(ctx context.Context, user *User) error // GetUser retrieves a user by their internal ID.