diff --git a/apps/login/src/lib/server/idp-intent.test.ts b/apps/login/src/lib/server/idp-intent.test.ts index a92ca83fb..a8efa5379 100644 --- a/apps/login/src/lib/server/idp-intent.test.ts +++ b/apps/login/src/lib/server/idp-intent.test.ts @@ -1,4 +1,5 @@ import { AutoLinkingOption } from "@zitadel/proto/zitadel/idp/v2/idp_pb"; +import { AuthenticationMethodType } from "@zitadel/proto/zitadel/user/v2/user_service_pb"; import crypto from "crypto"; import { afterEach, beforeEach, describe, expect, test, vi } from "vitest"; import { processIDPCallback } from "./idp-intent"; @@ -10,6 +11,9 @@ vi.mock("next/headers", () => ({ vi.mock("@zitadel/client", () => ({ create: vi.fn((schema: any, data: any) => data), + // Convert a {seconds} Timestamp-like object into a real Date so the real + // hasVerifiedPrimaryFactor (via the enrollment guard) can evaluate expiry. + timestampDate: vi.fn((ts: { seconds: bigint | number }) => new Date(Number(ts.seconds) * 1000)), ConnectError: class extends Error { code: any; constructor(message: string, code: any) { @@ -39,12 +43,19 @@ vi.mock("../zitadel", () => ({ getUserByID: vi.fn(), getDefaultOrg: vi.fn(), getSession: vi.fn(), + listAuthenticationMethodTypes: vi.fn(), })); vi.mock("./idp", () => ({ createNewSessionFromIdpIntent: vi.fn(), })); +// The linking guard (getEnrollmentAuthorizationError) runs for real; only its +// leaf dependency checkUserVerification (cookie/fingerprint proof) is mocked. +vi.mock("@/lib/verify-helper", () => ({ + checkUserVerification: vi.fn(), +})); + vi.mock("@/lib/cookies", () => ({ getSessionCookieById: vi.fn(), })); @@ -76,6 +87,8 @@ describe("processIDPCallback", () => { let mockCreateNewSessionFromIdpIntent: any; let mockGetSessionCookieById: any; let mockGetFingerprintIdCookie: any; + let mockListAuthenticationMethodTypes: any; + let mockCheckUserVerification: any; const defaultParams = { provider: "google", @@ -156,10 +169,12 @@ describe("processIDPCallback", () => { getUserByID, getDefaultOrg, getSession, + listAuthenticationMethodTypes, } = await import("../zitadel"); const { createNewSessionFromIdpIntent } = await import("./idp"); const { getSessionCookieById } = await import("@/lib/cookies"); const { getFingerprintIdCookie } = await import("../fingerprint"); + const { checkUserVerification } = await import("@/lib/verify-helper"); // Setup mocks mockHeaders = vi.mocked(headers); @@ -180,6 +195,8 @@ describe("processIDPCallback", () => { mockCreateNewSessionFromIdpIntent = vi.mocked(createNewSessionFromIdpIntent); mockGetSessionCookieById = vi.mocked(getSessionCookieById); mockGetFingerprintIdCookie = vi.mocked(getFingerprintIdCookie); + mockListAuthenticationMethodTypes = vi.mocked(listAuthenticationMethodTypes); + mockCheckUserVerification = vi.mocked(checkUserVerification); // Default mock implementations mockHeaders.mockResolvedValue({} as any); @@ -205,6 +222,11 @@ describe("processIDPCallback", () => { mockGetActiveIdentityProviders.mockResolvedValue({ identityProviders: [{ id: "idp123", name: "Test IDP" }], }); + // Default: the linking guard authorizes (no configured auth methods yet + a passed + // user-verification check — the first-authenticator onboarding path). Individual tests + // override these to exercise the identify-only rejection paths. + mockListAuthenticationMethodTypes.mockResolvedValue({ authMethodTypes: [] }); + mockCheckUserVerification.mockResolvedValue(true); }); afterEach(() => { @@ -566,6 +588,117 @@ describe("processIDPCallback", () => { expect(mockAddIDPLink).not.toHaveBeenCalled(); }); + // Security regression: an "identify-only" session (login name submitted, no verified + // primary factor) must never drive linking, even when the IDP allows it and the browser + // fingerprint matches. Otherwise an unauthenticated attacker who only knows a victim's + // login name could bind their own external identity to the victim's account (ATO). + // + // These four cases run the real enrollment guard (getEnrollmentAuthorizationError); only + // its leaf dependencies (listAuthenticationMethodTypes, checkUserVerification) are mocked. + const linkingAllowedIdp = { + ...defaultIdp, + config: { + options: { + ...defaultIdp.config.options, + isLinkingAllowed: true, + }, + }, + }; + + test("should reject linking for an identify-only session when the user already has credentials", async () => { + // Identify-only session (default) + user has a configured auth method → not authorized. + mockListAuthenticationMethodTypes.mockResolvedValue({ + authMethodTypes: [AuthenticationMethodType.PASSWORD], + }); + mockGetIDPByID.mockResolvedValue(linkingAllowedIdp); + + const result = await processIDPCallback(linkParams); + + expect(result.redirect).toContain("/idp/google/linking-failed"); + expect(result.redirect).toContain("error=session_invalid"); + expect(mockAddIDPLink).not.toHaveBeenCalled(); + }); + + test("should reject linking for an identify-only session with no credentials and no verification check", async () => { + // No auth methods yet, but the user-verification check has not been passed → not authorized. + mockListAuthenticationMethodTypes.mockResolvedValue({ authMethodTypes: [] }); + mockCheckUserVerification.mockResolvedValue(false); + mockGetIDPByID.mockResolvedValue(linkingAllowedIdp); + + const result = await processIDPCallback(linkParams); + + expect(result.redirect).toContain("/idp/google/linking-failed"); + expect(result.redirect).toContain("error=session_invalid"); + expect(mockAddIDPLink).not.toHaveBeenCalled(); + }); + + test("should reject linking when the primary factor is expired", async () => { + // A password factor was verified, but the session has expired, so it no longer counts + // as a verified primary factor. Were it still valid, the guard would authorize at the + // primary-factor branch; expired, it falls through and the configured credential + // (PASSWORD) means it is not the credential-less onboarding path → rejected. + const past = { seconds: BigInt(Math.floor(Date.now() / 1000) - 3600), nanos: 0 }; + const factorVerifiedAt = { seconds: BigInt(Math.floor(Date.now() / 1000) - 7200), nanos: 0 }; + mockGetSession.mockResolvedValue({ + session: { + factors: { + user: { id: "user123" }, + password: { verifiedAt: factorVerifiedAt }, + }, + expirationDate: past, + }, + }); + mockListAuthenticationMethodTypes.mockResolvedValue({ + authMethodTypes: [AuthenticationMethodType.PASSWORD], + }); + mockGetIDPByID.mockResolvedValue(linkingAllowedIdp); + + const result = await processIDPCallback(linkParams); + + expect(result.redirect).toContain("/idp/google/linking-failed"); + expect(result.redirect).toContain("error=session_invalid"); + expect(mockAddIDPLink).not.toHaveBeenCalled(); + }); + + test("should allow linking as the first authenticator when the user-verification check passed", async () => { + // Identify-only session, no auth methods yet, but the email/invite-code verification + // check passed → the legitimate "link an IDP as the first authenticator" onboarding path. + mockListAuthenticationMethodTypes.mockResolvedValue({ authMethodTypes: [] }); + mockCheckUserVerification.mockResolvedValue(true); + mockGetIDPByID.mockResolvedValue(linkingAllowedIdp); + + const result = await processIDPCallback(linkParams); + + expect(mockAddIDPLink).toHaveBeenCalled(); + expect(result.redirect).toBe("https://app.example.com/success"); + }); + + test("should allow linking when the session has a verified, non-expired primary factor", async () => { + // A fully authenticated session (verified password, not expired) is authorized at the + // primary-factor branch regardless of configured methods / verification check. This pins + // the other half of the guard's OR so a refactor cannot silently drop it. + const future = { seconds: BigInt(Math.floor(Date.now() / 1000) + 3600), nanos: 0 }; + mockGetSession.mockResolvedValue({ + session: { + factors: { + user: { id: "user123" }, + password: { verifiedAt: future }, + }, + expirationDate: future, + }, + }); + mockListAuthenticationMethodTypes.mockResolvedValue({ + authMethodTypes: [AuthenticationMethodType.PASSWORD], + }); + mockCheckUserVerification.mockResolvedValue(false); + mockGetIDPByID.mockResolvedValue(linkingAllowedIdp); + + const result = await processIDPCallback(linkParams); + + expect(mockAddIDPLink).toHaveBeenCalled(); + expect(result.redirect).toBe("https://app.example.com/success"); + }); + test("should return error redirect when linking fails", async () => { mockGetIDPByID.mockResolvedValue({ ...defaultIdp, diff --git a/apps/login/src/lib/server/idp-intent.ts b/apps/login/src/lib/server/idp-intent.ts index aaab5ddc5..f9c4bb46c 100644 --- a/apps/login/src/lib/server/idp-intent.ts +++ b/apps/login/src/lib/server/idp-intent.ts @@ -33,6 +33,7 @@ import crypto from "crypto"; import { getTranslations } from "next-intl/server"; import { headers } from "next/headers"; import { getFingerprintIdCookie } from "../fingerprint"; +import { getEnrollmentAuthorizationError } from "./enrollment-guard"; import { createNewSessionFromIdpIntent } from "./idp"; import { syncInstanceRolesFromIdpIntent } from "./instance-roles"; @@ -312,6 +313,29 @@ async function resolveUserIdFromSession({ return { redirect: `/idp/${provider}/linking-failed?error=session_invalid` }; } + // Explicit linking must be authorized the same way credential enrollment is. An + // "identify-only" session — produced by only submitting a login name, with no verified + // primary factor (password / passkey / IDP intent) — must NOT be trusted on its own: + // otherwise an unauthenticated attacker who merely knows a victim's login name could bind + // their own external identity to the victim's account (account takeover). The fingerprint + // check above only proves the link request originated from the same browser that started + // the flow, not that the session was ever authenticated. + // + // getEnrollmentAuthorizationError authorizes when the session has a verified, non-expired + // primary factor, OR the user has no authentication methods yet AND passed a + // user-verification check (the email/invite-code proof bound to this browser's + // fingerprint). The latter preserves the legitimate "link an IDP as the first + // authenticator" onboarding flow, which isSessionValid would have rejected. + const enrollmentError = await getEnrollmentAuthorizationError({ + serviceConfig, + session, + userId: session.factors.user.id, + }); + if (enrollmentError) { + logger.warn("Session for linking is not authorized", { enrollmentError }); + return { redirect: `/idp/${provider}/linking-failed?error=session_invalid` }; + } + return { userId: session.factors.user.id }; } catch (error) { logger.warn("Error retrieving session for linking", { error }); diff --git a/internal/api/grpc/user/v2/integration_test/idp_link_test.go b/internal/api/grpc/user/v2/integration_test/idp_link_test.go index 7484a4b54..716a0dc9e 100644 --- a/internal/api/grpc/user/v2/integration_test/idp_link_test.go +++ b/internal/api/grpc/user/v2/integration_test/idp_link_test.go @@ -61,6 +61,27 @@ func TestServer_AddIDPLink(t *testing.T) { want: nil, wantErr: true, }, + { + // Security regression (account takeover): a self-link, an unprivileged + // user binding an external identity to their own account, must now be + // denied. The previous self-link bypass skipped the user-write permission + // check, letting any authenticated user pre-claim an unproven external + // subject on themselves and hijack a victim's future federated logins. + name: "self-link without permission, error", + args: args{ + UserCTX, + &user.AddIDPLinkRequest{ + UserId: Instance.Users.Get(integration.UserTypeNoPermission).ID, + IdpLink: &user.IDPLink{ + IdpId: idpResp.Id, + UserId: "external-selflink", + UserName: "username", + }, + }, + }, + want: nil, + wantErr: true, + }, { name: "add link", args: args{ diff --git a/internal/command/user_idp_link.go b/internal/command/user_idp_link.go index e3484c975..d91b5adc2 100644 --- a/internal/command/user_idp_link.go +++ b/internal/command/user_idp_link.go @@ -26,10 +26,15 @@ func (c *Commands) AddUserIDPLink(ctx context.Context, userID, resourceOwner str if !isUserStateExists(existingUser.UserState) { return nil, zerrors.ThrowPreconditionFailed(nil, "COMMAND-vzktar7b7f", "Errors.User.NotFound") } - if userID != authz.GetCtxData(ctx).UserID { - if err := c.checkPermission(ctx, domain.PermissionUserWrite, existingUser.ResourceOwner, existingUser.AggregateID); err != nil { - return nil, err - } + // Binding an external identity (idpID + externalUserID) to a user must always require a + // user-write permission, even when the caller targets their own account. The external + // subject is caller-supplied and unverified here, and external links are globally unique + // on a first-claim-wins basis, so a self-service link of an unproven subject lets an + // attacker claim a victim's external identity and hijack their future federated logins + // (account takeover). Verified self-service linking is performed through the IDP-intent + // flow, whose service account holds this permission. + if err := c.checkPermission(ctx, domain.PermissionUserWrite, existingUser.ResourceOwner, existingUser.AggregateID); err != nil { + return nil, err } //nolint:staticcheck event, err := addLink(ctx, c.eventstore.Filter, user.NewAggregate(existingUser.AggregateID, existingUser.ResourceOwner), link) @@ -48,6 +53,13 @@ func (c *Commands) AddUserIDPLink(ctx context.Context, userID, resourceOwner str }, nil } +// BulkAddedUserIDPLinks intentionally performs no user-write permission check. +// It is reachable only from the Login V1 auth-request flow, and only after the +// external authentication has already been verified, so the external subjects it +// binds are proven rather than caller-supplied. No API surface exposes this +// command. Do NOT wire it to an API endpoint as-is: without a permission check +// (or verified IDP-intent proof, as AddUserIDPLink requires) it would reintroduce +// the account-takeover via unproven external-identity pre-claim. func (c *Commands) BulkAddedUserIDPLinks(ctx context.Context, userID, resourceOwner string, links []*domain.UserIDPLink) (err error) { if userID == "" { return zerrors.ThrowInvalidArgument(nil, "COMMAND-03j8f", "Errors.IDMissing") diff --git a/internal/command/user_idp_link_test.go b/internal/command/user_idp_link_test.go index 2ff75fadf..85a738e8a 100644 --- a/internal/command/user_idp_link_test.go +++ b/internal/command/user_idp_link_test.go @@ -8,6 +8,7 @@ import ( "github.com/stretchr/testify/assert" "golang.org/x/text/language" + "github.com/zitadel/zitadel/internal/api/authz" "github.com/zitadel/zitadel/internal/domain" "github.com/zitadel/zitadel/internal/eventstore" "github.com/zitadel/zitadel/internal/eventstore/v1/models" @@ -733,6 +734,249 @@ func TestCommandSide_BulkAddUserIDPLinks(t *testing.T) { } } +func TestCommandSide_AddUserIDPLink(t *testing.T) { + type fields struct { + eventstore func(*testing.T) *eventstore.Eventstore + checkPermission domain.PermissionCheck + } + type args struct { + ctx context.Context + userID string + resourceOwner string + link *AddLink + } + type res struct { + err func(error) bool + } + tests := []struct { + name string + fields fields + args args + res res + }{ + { + name: "missing userid, invalid argument error", + fields: fields{ + eventstore: expectEventstore(), + checkPermission: newMockPermissionCheckAllowed(), + }, + args: args{ + ctx: context.Background(), + userID: "", + link: &AddLink{ + IDPID: "config1", + IDPExternalID: "externaluser1", + }, + }, + res: res{ + err: zerrors.IsErrorInvalidArgument, + }, + }, + { + name: "user not existing, precondition failed error", + fields: fields{ + eventstore: expectEventstore(expectFilter()), + checkPermission: newMockPermissionCheckAllowed(), + }, + args: args{ + ctx: context.Background(), + userID: "user1", + resourceOwner: "org1", + link: &AddLink{ + IDPID: "config1", + IDPExternalID: "externaluser1", + }, + }, + res: res{ + err: zerrors.IsPreconditionFailed, + }, + }, + { + // Security regression (account takeover): binding an external identity to a user + // must require an explicit user-write permission even when the caller targets + // their own account (self-link). The previous self-link bypass let any + // authenticated user claim an unproven external subject on themselves and, because + // external links are globally unique, hijack a victim's future federated logins. + name: "self-link without permission, permission denied error", + fields: fields{ + eventstore: expectEventstore( + expectFilter( + eventFromEventPusher( + user.NewHumanAddedEvent( + context.Background(), + &user.NewAggregate("user1", "org1").Aggregate, + "userName", + "firstName", + "lastName", + "nickName", + "displayName", + language.German, + domain.GenderFemale, + "email@Address.ch", + false, + ), + ), + ), + ), + checkPermission: newMockPermissionCheckNotAllowed(), + }, + args: args{ + // ctx acts as the target user itself (self-link). + ctx: authz.SetCtxData(context.Background(), authz.CtxData{UserID: "user1"}), + userID: "user1", + resourceOwner: "org1", + link: &AddLink{ + IDPID: "config1", + DisplayName: "name", + IDPExternalID: "externaluser1", + }, + }, + res: res{ + err: zerrors.IsPermissionDenied, + }, + }, + { + // Self-link is allowed when the caller holds the user-write permission. + // This pins the login-client flow (service user with user.write links a + // verified external subject to its own IDP-intent session) so a future + // refactor cannot silently break it. + name: "self-link with permission, ok", + fields: fields{ + eventstore: expectEventstore( + expectFilter( + eventFromEventPusher( + user.NewHumanAddedEvent( + context.Background(), + &user.NewAggregate("user1", "org1").Aggregate, + "userName", + "firstName", + "lastName", + "nickName", + "displayName", + language.German, + domain.GenderFemale, + "email@Address.ch", + false, + ), + ), + ), + expectFilter( + eventFromEventPusher( + org.NewIDPConfigAddedEvent( + context.Background(), + &org.NewAggregate("org1").Aggregate, + "config1", + "name", + domain.IDPConfigTypeOIDC, + domain.IDPConfigStylingTypeUnspecified, + false, + ), + ), + ), + expectPush( + user.NewUserIDPLinkAddedEvent( + authz.SetCtxData(context.Background(), authz.CtxData{UserID: "user1"}), + &user.NewAggregate("user1", "org1").Aggregate, + "config1", + "name", + "externaluser1", + ), + ), + ), + checkPermission: newMockPermissionCheckAllowed(), + }, + args: args{ + // ctx acts as the target user itself (self-link). + ctx: authz.SetCtxData(context.Background(), authz.CtxData{UserID: "user1"}), + userID: "user1", + resourceOwner: "org1", + link: &AddLink{ + IDPID: "config1", + DisplayName: "name", + IDPExternalID: "externaluser1", + }, + }, + res: res{}, + }, + { + // Linking another user's account with the user-write permission is the + // ordinary managed path and must keep working. + name: "other-user link with permission, ok", + fields: fields{ + eventstore: expectEventstore( + expectFilter( + eventFromEventPusher( + user.NewHumanAddedEvent( + context.Background(), + &user.NewAggregate("user1", "org1").Aggregate, + "userName", + "firstName", + "lastName", + "nickName", + "displayName", + language.German, + domain.GenderFemale, + "email@Address.ch", + false, + ), + ), + ), + expectFilter( + eventFromEventPusher( + org.NewIDPConfigAddedEvent( + context.Background(), + &org.NewAggregate("org1").Aggregate, + "config1", + "name", + domain.IDPConfigTypeOIDC, + domain.IDPConfigStylingTypeUnspecified, + false, + ), + ), + ), + expectPush( + user.NewUserIDPLinkAddedEvent( + authz.SetCtxData(context.Background(), authz.CtxData{UserID: "admin1"}), + &user.NewAggregate("user1", "org1").Aggregate, + "config1", + "name", + "externaluser1", + ), + ), + ), + checkPermission: newMockPermissionCheckAllowed(), + }, + args: args{ + // ctx is a different user (e.g. an admin) than the link target. + ctx: authz.SetCtxData(context.Background(), authz.CtxData{UserID: "admin1"}), + userID: "user1", + resourceOwner: "org1", + link: &AddLink{ + IDPID: "config1", + DisplayName: "name", + IDPExternalID: "externaluser1", + }, + }, + res: res{}, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + r := &Commands{ + eventstore: tt.fields.eventstore(t), + checkPermission: tt.fields.checkPermission, + } + _, err := r.AddUserIDPLink(tt.args.ctx, tt.args.userID, tt.args.resourceOwner, tt.args.link) + if tt.res.err == nil { + assert.NoError(t, err) + } + if tt.res.err != nil && !tt.res.err(err) { + t.Errorf("got wrong err: %v ", err) + } + }) + } +} + func TestCommandSide_RemoveUserIDPLink(t *testing.T) { type fields struct { eventstore func(*testing.T) *eventstore.Eventstore