From 51fcc4a8c380d84a90ec1897220e196b30f342ae Mon Sep 17 00:00:00 2001 From: Frode Sundby Date: Tue, 20 Oct 2026 15:58:17 +0200 Subject: [PATCH 1/5] feat: support nais.io users in tenant APIs --- .env.example | 2 + charts/nais-api/Feature.yaml | 6 + charts/nais-api/templates/deployment.yaml | 6 + charts/nais-api/values.yaml | 1 + docs/nais_io_users.md | 36 +++ internal/auth/authn/handler.go | 13 +- .../auth/authn/handler_integration_test.go | 130 ++++++++++ internal/auth/middleware/jwt.go | 40 +-- internal/auth/middleware/jwt_test.go | 65 ++++- internal/cmd/api/api.go | 2 +- internal/cmd/api/config.go | 4 + internal/cmd/api/usersync.go | 11 +- internal/usersync/usersyncer/nais.go | 89 +++++++ .../usersyncer/nais_integration_test.go | 235 ++++++++++++++++++ internal/usersync/usersyncer/nais_test.go | 101 ++++++++ internal/usersync/usersyncer/usersyncer.go | 32 ++- 16 files changed, 730 insertions(+), 43 deletions(-) create mode 100644 docs/nais_io_users.md create mode 100644 internal/auth/authn/handler_integration_test.go create mode 100644 internal/usersync/usersyncer/nais.go create mode 100644 internal/usersync/usersyncer/nais_integration_test.go create mode 100644 internal/usersync/usersyncer/nais_test.go diff --git a/.env.example b/.env.example index 3b639e6fa..b11049f5b 100644 --- a/.env.example +++ b/.env.example @@ -38,6 +38,8 @@ ZITADEL_IDP_ID= ZITADEL_KEY= ZITADEL_DOMAIN=https://auth.nais.io ZITADEL_ORGANIZATION_ID= +# Enable nais.io user sync/login; ZITADEL_KEY must have user read access in this organization. +ZITADEL_NAIS_ORGANIZATION_ID= AIVEN_PROJECTS='{"dev": {"id":"nav-dev","vpc":"somevpc","endpoint_id":"some_endpoint_id"}}' diff --git a/charts/nais-api/Feature.yaml b/charts/nais-api/Feature.yaml index 81640646d..41fa3e936 100644 --- a/charts/nais-api/Feature.yaml +++ b/charts/nais-api/Feature.yaml @@ -244,6 +244,12 @@ values: computed: template: "{{ .Env.zitadel_organization_id | quote }}" + zitadel.naisOrganizationID: + displayName: Nais Zitadel organization ID + description: Enables syncing active nais.io users as viewers and allows nais.io login + computed: + template: '{{ .Env.zitadel_nais_organization_id | default "" | quote }}' + aiven.projects: displayName: Aiven projects description: | diff --git a/charts/nais-api/templates/deployment.yaml b/charts/nais-api/templates/deployment.yaml index cb1d0c990..065d72604 100644 --- a/charts/nais-api/templates/deployment.yaml +++ b/charts/nais-api/templates/deployment.yaml @@ -57,7 +57,11 @@ spec: - name: OAUTH_FRONTEND_URL value: "https://{{ .Values.host }}" - name: OAUTH_ADDITIONAL_SCOPES + {{- if .Values.zitadel.naisOrganizationID }} + value: "urn:zitadel:iam:user:resourceowner" + {{- else }} value: "urn:zitadel:iam:org:id:{{ .Values.zitadel.organizationID }}" + {{- end }} - name: LISTEN_ADDRESS value: ":3000" - name: INTERNAL_LISTEN_ADDRESS @@ -98,6 +102,8 @@ spec: value: {{ .Values.zitadel.domain | quote }} - name: ZITADEL_ORGANIZATION_ID value: {{ .Values.zitadel.organizationID | quote }} + - name: ZITADEL_NAIS_ORGANIZATION_ID + value: {{ .Values.zitadel.naisOrganizationID | quote }} - name: CLOUD_SQL_INSTANCE value: {{ .Values.database.instance | quote }} - name: NAIS_AUDIT_LOG_PROJECT_ID diff --git a/charts/nais-api/values.yaml b/charts/nais-api/values.yaml index 853c09193..d81cf3a13 100644 --- a/charts/nais-api/values.yaml +++ b/charts/nais-api/values.yaml @@ -89,6 +89,7 @@ zitadel: idpID: key: organizationID: + naisOrganizationID: "" # Enables nais.io user sync and login when configured aiven: projects: "" diff --git a/docs/nais_io_users.md b/docs/nais_io_users.md new file mode 100644 index 000000000..d921f9f2e --- /dev/null +++ b/docs/nais_io_users.md @@ -0,0 +1,36 @@ +# nais.io users in tenant APIs + +Set `ZITADEL_NAIS_ORGANIZATION_ID` (Helm: `zitadel.naisOrganizationID`) to the +nais.io organization ID to enable user sync and login. An empty value preserves +tenant-only login. Fasit supplies the value through `zitadel_nais_organization_id`. + +The machine user configured by `ZITADEL_KEY` needs read access to users in the +nais.io organization, in addition to its existing tenant user-management access. +Terraform must also grant the nais.io organization access to each tenant's +Console project. These permissions must be deployed before enabling the setting. +The CLI's login restriction and tenant selection are handled separately. + +## User lifecycle and permissions + +- The API imports active human users from the configured Zitadel organization. + Users are not copied into the tenant's Zitadel organization or modified there. +- The Zitadel user ID is stored as `external_id`, matching the token subject. + Existing users are not adopted by email from another identity. +- New nais.io users have the existing authenticated-user read access (Viewer), + without the tenant default `Team creator` role, admin access or team membership. +- Both user snapshots are fetched before database changes. Failure to fetch any + page aborts synchronization without deleting database users. +- Users absent from the next complete snapshot, including inactive, locked and + deleted Zitadel users, are deleted from the API database. Their sessions are + deleted by the database's foreign-key cascade. +- Changes are applied on the existing usersync schedule (every 15 minutes). + First-time users must exist in Zitadel and be synchronized before API login. + +Zitadel is the source of truth for nais.io users. Suspending a user in Google +Workspace alone is not sufficient: deactivation must also reach Zitadel. + +JWT authentication continues to check the issuer, audience and resource-owner +organization, accepting only the tenant organization and the configured nais.io +organization. The subject must match a local user. Console login uses the token +subject rather than email for Zitadel identities and no longer pins the login +flow to the tenant organization when nais.io login is enabled. diff --git a/internal/auth/authn/handler.go b/internal/auth/authn/handler.go index 7fff4389a..19285f68d 100644 --- a/internal/auth/authn/handler.go +++ b/internal/auth/authn/handler.go @@ -155,7 +155,18 @@ func (h *handler) Callback(w http.ResponseWriter, r *http.Request) { return } - u, err := user.GetByEmail(r.Context(), claims.Email) + var u *user.User + if h.oauth2Config.IsZitadel() { + // Zitadel subjects identify the synced user across organizations. An email + // address alone must not allow an identity from another organization in. + if idToken.Subject == "" { + err = fmt.Errorf("missing subject in Zitadel ID token") + } else { + u, err = user.GetByExternalID(r.Context(), idToken.Subject) + } + } else { + u, err = user.GetByEmail(r.Context(), claims.Email) + } if err != nil { h.log.WithError(err).Errorf("get user (%s) from db", claims.Email) http.Redirect(w, r, "/?error=unknown-user", http.StatusFound) diff --git a/internal/auth/authn/handler_integration_test.go b/internal/auth/authn/handler_integration_test.go new file mode 100644 index 000000000..0d617eaa0 --- /dev/null +++ b/internal/auth/authn/handler_integration_test.go @@ -0,0 +1,130 @@ +//go:build integration_test + +package authn + +import ( + "context" + "encoding/base64" + "fmt" + "net/http" + "net/http/httptest" + "testing" + "time" + + "github.com/coreos/go-oidc/v3/oidc" + "github.com/google/uuid" + "github.com/nais/api/internal/database" + "github.com/nais/api/internal/session" + "github.com/nais/api/internal/user" + "github.com/nais/api/internal/usersync/usersyncsql" + logrustest "github.com/sirupsen/logrus/hooks/test" + "github.com/testcontainers/testcontainers-go" + "github.com/testcontainers/testcontainers-go/modules/postgres" + "golang.org/x/oauth2" +) + +// callbackOAuth verifies the claims using the normal OIDC verifier with a test +// key set. Only signature verification is replaced; claims are still decoded by +// go-oidc, including its otherwise private IDToken claims field. +type callbackOAuth struct { + zitadel bool + subject string + email string +} + +func (c callbackOAuth) IsZitadel() bool { return c.zitadel } + +func (c callbackOAuth) AuthCodeURL(string, ...oauth2.AuthCodeOption) string { return "/login" } + +func (c callbackOAuth) Exchange(context.Context, string, ...oauth2.AuthCodeOption) (*oauth2.Token, error) { + return (&oauth2.Token{}).WithExtra(map[string]any{IDTokenKey: "test-token"}), nil +} + +func (c callbackOAuth) Verify(ctx context.Context, _ string) (*oidc.IDToken, error) { + payload := fmt.Sprintf(`{"iss":"https://auth.nais.io","aud":"console","sub":%q,"email":%q,"exp":%d}`, c.subject, c.email, time.Now().Add(time.Hour).Unix()) + raw := base64.RawURLEncoding.EncodeToString([]byte(`{"alg":"RS256"}`)) + "." + base64.RawURLEncoding.EncodeToString([]byte(payload)) + ".c2ln" + return oidc.NewVerifier("https://auth.nais.io", callbackKeySet{payload: []byte(payload)}, &oidc.Config{ClientID: "console"}).Verify(ctx, raw) +} + +type callbackKeySet struct{ payload []byte } + +func (k callbackKeySet) VerifySignature(context.Context, string) ([]byte, error) { + return k.payload, nil +} + +func TestCallbackUsesZitadelSubject(t *testing.T) { + ctx := t.Context() + log, _ := logrustest.NewNullLogger() + container, err := postgres.Run(ctx, "docker.io/postgres:16-alpine", + postgres.WithDatabase("test"), postgres.WithUsername("test"), postgres.WithPassword("test"), + postgres.BasicWaitStrategies()) + if err != nil { + t.Fatal(err) + } + testcontainers.CleanupContainer(t, container) + dsn, err := container.ConnectionString(ctx, "sslmode=disable") + if err != nil { + t.Fatal(err) + } + pool, err := database.NewPool(ctx, dsn, log, true) + if err != nil { + t.Fatal(err) + } + t.Cleanup(pool.Close) + q := usersyncsql.New(pool) + u, err := q.Create(ctx, usersyncsql.CreateParams{Name: "Nais viewer", Email: "viewer@nais.io", ExternalID: "nais-subject"}) + if err != nil { + t.Fatal(err) + } + + for _, tc := range []struct { + name string + config callbackOAuth + wantLogin bool + }{ + {name: "synced Zitadel subject", config: callbackOAuth{zitadel: true, subject: u.ExternalID, email: u.Email}, wantLogin: true}, + {name: "subject remains authoritative after email changes", config: callbackOAuth{zitadel: true, subject: u.ExternalID, email: "renamed@nais.io"}, wantLogin: true}, + {name: "same email from another identity", config: callbackOAuth{zitadel: true, subject: "foreign-subject", email: u.Email}}, + {name: "missing subject cannot fall back to email", config: callbackOAuth{zitadel: true, email: u.Email}}, + {name: "unsynced user", config: callbackOAuth{zitadel: true, subject: "unsynced", email: "unsynced@nais.io"}}, + {name: "non-Zitadel login still uses email", config: callbackOAuth{subject: "google-subject", email: u.Email}, wantLogin: true}, + } { + t.Run(tc.name, func(t *testing.T) { + ctx := database.NewLoaderContext(t.Context(), pool) + ctx = user.NewLoaderContext(ctx, pool) + ctx = session.NewLoaderContext(ctx, pool) + req := httptest.NewRequest(http.MethodGet, "/oauth2/callback?code=code&state=state", nil).WithContext(ctx) + req.AddCookie(&http.Cookie{Name: OAuthStateCookie, Value: "state"}) + rec := httptest.NewRecorder() + New(tc.config, log).Callback(rec, req) + if rec.Code != http.StatusFound { + t.Fatalf("expected redirect, got %d: %s", rec.Code, rec.Body.String()) + } + if !tc.wantLogin { + if got := rec.Header().Get("Location"); got != "/?error=unknown-user" { + t.Fatalf("expected unknown user, got %q", got) + } + for _, cookie := range rec.Result().Cookies() { + if cookie.Name == SessionCookieName { + t.Fatal("unexpected session for an unknown identity") + } + } + return + } + if got := rec.Header().Get("Location"); got != "/" { + t.Fatalf("expected successful login, got %q", got) + } + for _, cookie := range rec.Result().Cookies() { + if cookie.Name != SessionCookieName { + continue + } + sess, err := session.Get(ctx, uuid.MustParse(cookie.Value)) + if err != nil || sess.UserID != u.ID { + t.Fatalf("session belongs to wrong identity: %v, %v", sess, err) + } + return + } + t.Fatal("missing session cookie") + }) + } +} diff --git a/internal/auth/middleware/jwt.go b/internal/auth/middleware/jwt.go index b8f20dcaf..6dcf89d28 100644 --- a/internal/auth/middleware/jwt.go +++ b/internal/auth/middleware/jwt.go @@ -20,13 +20,14 @@ import ( const acceptableClockSkew = 3 * time.Second type jwtAuth struct { - issuer string - audience string - zitadelOrgID string - jwksURL string - jwksCache *jwk.Cache - log logrus.FieldLogger - now clock + issuer string + audience string + zitadelOrgID string + naisZitadelOrgID string + jwksURL string + jwksCache *jwk.Cache + log logrus.FieldLogger + now clock } type clock func() time.Time @@ -65,11 +66,17 @@ func (j *jwtAuth) validate(ctx context.Context, token string) (jwt.Token, error) return nil, fmt.Errorf("provider: parsing token: %w", err) } + orgID := j.zitadelOrgID + var resourceOwner string + if j.naisZitadelOrgID != "" && t.Get("urn:zitadel:iam:user:resourceowner:id", &resourceOwner) == nil && resourceOwner == j.naisZitadelOrgID { + orgID = j.naisZitadelOrgID + } + return t, jwt.Validate(t, jwt.WithAcceptableSkew(acceptableClockSkew), jwt.WithIssuer(j.issuer), jwt.WithAudience(j.audience), - jwt.WithClaimValue("urn:zitadel:iam:user:resourceowner:id", j.zitadelOrgID), + jwt.WithClaimValue("urn:zitadel:iam:user:resourceowner:id", orgID), jwt.WithClock(j.now), ) } @@ -116,7 +123,7 @@ func (j *jwtAuth) handler(next http.Handler) http.Handler { return http.HandlerFunc(fn) } -func JWTAuthentication(ctx context.Context, issuer, audience, zitadelOrgID string, log logrus.FieldLogger) (func(next http.Handler) http.Handler, error) { +func JWTAuthentication(ctx context.Context, issuer, audience, zitadelOrgID, naisZitadelOrgID string, log logrus.FieldLogger) (func(next http.Handler) http.Handler, error) { if issuer == "" { return nil, errors.New("issuer is required") } @@ -143,13 +150,14 @@ func JWTAuthentication(ctx context.Context, issuer, audience, zitadelOrgID strin } auth := jwtAuth{ - jwksURL: client.JwksURI, - jwksCache: cache, - issuer: issuer, - audience: audience, - zitadelOrgID: zitadelOrgID, - log: log, - now: time.Now, + jwksURL: client.JwksURI, + jwksCache: cache, + issuer: issuer, + audience: audience, + zitadelOrgID: zitadelOrgID, + naisZitadelOrgID: naisZitadelOrgID, + log: log, + now: time.Now, } return auth.handler, nil diff --git a/internal/auth/middleware/jwt_test.go b/internal/auth/middleware/jwt_test.go index 160a376cc..fa74d623d 100644 --- a/internal/auth/middleware/jwt_test.go +++ b/internal/auth/middleware/jwt_test.go @@ -30,11 +30,12 @@ func TestJWTAuthorized(t *testing.T) { inputZitadelOrgID string inputTime time.Time - expectedAudience string - expectedIssuer string - expectedZitadelOrgID string - expectedTime time.Time - expectedError error + expectedAudience string + expectedIssuer string + expectedZitadelOrgID string + expectedNaisZitadelOrgID string + expectedTime time.Time + expectedError error }{ "valid token": { inputAudience: "aud", @@ -53,6 +54,47 @@ func TestJWTAuthorized(t *testing.T) { expectedZitadelOrgID: "org-id", expectedError: jwt.InvalidAudienceError(), }, + "nais.io token when enabled": { + inputAudience: "aud", inputIssuer: "http://localhost:1234", inputZitadelOrgID: "nais-org-id", + expectedAudience: "aud", expectedIssuer: "http://localhost:1234", expectedZitadelOrgID: "org-id", + expectedNaisZitadelOrgID: "nais-org-id", + }, + "nais.io token when disabled": { + inputAudience: "aud", inputIssuer: "http://localhost:1234", inputZitadelOrgID: "nais-org-id", + expectedAudience: "aud", expectedIssuer: "http://localhost:1234", expectedZitadelOrgID: "org-id", + expectedError: jwt.ValidateError(), + }, + "tenant token when nais.io is enabled": { + inputAudience: "aud", inputIssuer: "http://localhost:1234", inputZitadelOrgID: "org-id", + expectedAudience: "aud", expectedIssuer: "http://localhost:1234", expectedZitadelOrgID: "org-id", + expectedNaisZitadelOrgID: "nais-org-id", + }, + "another organization when nais.io is enabled": { + inputAudience: "aud", inputIssuer: "http://localhost:1234", inputZitadelOrgID: "other-org-id", + expectedAudience: "aud", expectedIssuer: "http://localhost:1234", expectedZitadelOrgID: "org-id", + expectedNaisZitadelOrgID: "nais-org-id", expectedError: jwt.ValidateError(), + }, + "nais.io token with invalid audience": { + inputAudience: "other-aud", inputIssuer: "http://localhost:1234", inputZitadelOrgID: "nais-org-id", + expectedAudience: "aud", expectedIssuer: "http://localhost:1234", expectedZitadelOrgID: "org-id", + expectedNaisZitadelOrgID: "nais-org-id", expectedError: jwt.InvalidAudienceError(), + }, + "nais.io token with invalid issuer": { + inputAudience: "aud", inputIssuer: "http://other-issuer", inputZitadelOrgID: "nais-org-id", + expectedAudience: "aud", expectedIssuer: "http://localhost:1234", expectedZitadelOrgID: "org-id", + expectedNaisZitadelOrgID: "nais-org-id", expectedError: jwt.InvalidIssuerError(), + }, + "nais.io expired token": { + inputAudience: "aud", inputIssuer: "http://localhost:1234", inputZitadelOrgID: "nais-org-id", + inputTime: now.Add(-time.Hour), + expectedAudience: "aud", expectedIssuer: "http://localhost:1234", expectedZitadelOrgID: "org-id", + expectedNaisZitadelOrgID: "nais-org-id", expectedError: jwt.TokenExpiredError(), + }, + "missing organization when nais.io is enabled": { + inputAudience: "aud", inputIssuer: "http://localhost:1234", + expectedAudience: "aud", expectedIssuer: "http://localhost:1234", expectedZitadelOrgID: "org-id", + expectedNaisZitadelOrgID: "nais-org-id", expectedError: jwt.ValidateError(), + }, "invalid issuer": { inputAudience: "aud", inputIssuer: "http://invalid-issuer", @@ -112,12 +154,13 @@ func TestJWTAuthorized(t *testing.T) { cache.Register(t.Context(), test.expectedIssuer) mw := jwtAuth{ - issuer: test.expectedIssuer, - audience: test.expectedAudience, - zitadelOrgID: test.expectedZitadelOrgID, - jwksURL: test.expectedIssuer, - jwksCache: cache, - log: log, + issuer: test.expectedIssuer, + audience: test.expectedAudience, + zitadelOrgID: test.expectedZitadelOrgID, + naisZitadelOrgID: test.expectedNaisZitadelOrgID, + jwksURL: test.expectedIssuer, + jwksCache: cache, + log: log, now: func() time.Time { return defaultTime(test.expectedTime) }, diff --git a/internal/cmd/api/api.go b/internal/cmd/api/api.go index e33861cea..30436ea22 100644 --- a/internal/cmd/api/api.go +++ b/internal/cmd/api/api.go @@ -278,7 +278,7 @@ func run(ctx context.Context, cfg *Config, log logrus.FieldLogger) error { var jwtMiddleware func(next http.Handler) http.Handler if !cfg.JWT.SkipMiddleware { - jwtMiddleware, err = middleware.JWTAuthentication(ctx, cfg.JWT.Issuer, cfg.JWT.Audience, cfg.Zitadel.OrganizationID, log.WithField("subsystem", "jwt")) + jwtMiddleware, err = middleware.JWTAuthentication(ctx, cfg.JWT.Issuer, cfg.JWT.Audience, cfg.Zitadel.OrganizationID, cfg.Zitadel.NaisOrganizationID, log.WithField("subsystem", "jwt")) if err != nil { return fmt.Errorf("failed to create JWT authentication middleware: %w", err) } diff --git a/internal/cmd/api/config.go b/internal/cmd/api/config.go index cd7b3bc1f..ad87dcc28 100644 --- a/internal/cmd/api/config.go +++ b/internal/cmd/api/config.go @@ -130,6 +130,10 @@ type zitadelConfig struct { // OrganizationID is the ID of the organization to use for the Zitadel API OrganizationID string `env:"ZITADEL_ORGANIZATION_ID"` + + // NaisOrganizationID enables syncing active nais.io users from this Zitadel organization. + // The usersync machine user must have read access to its users. + NaisOrganizationID string `env:"ZITADEL_NAIS_ORGANIZATION_ID"` } func (l loggingConfig) DefaultLogDestinations() []logging.SupportedLogDestination { diff --git a/internal/cmd/api/usersync.go b/internal/cmd/api/usersync.go index 119219ebb..d4a81fe12 100644 --- a/internal/cmd/api/usersync.go +++ b/internal/cmd/api/usersync.go @@ -3,6 +3,7 @@ package api import ( "context" "encoding/base64" + "fmt" "time" "github.com/coreos/go-oidc/v3/oidc" @@ -28,6 +29,9 @@ func runUsersync(ctx context.Context, pool *pgxpool.Pool, cfg *Config, log logru } var zw *usersyncer.ZitadelWrapper + if cfg.Zitadel.NaisOrganizationID != "" && (cfg.Zitadel.Domain == "" || cfg.Zitadel.Key == "" || cfg.Zitadel.IDPID == "") { + return fmt.Errorf("nais.io user sync requires Zitadel domain, key and IDP ID") + } if cfg.Zitadel.Domain != "" && cfg.Zitadel.Key != "" && cfg.Zitadel.IDPID != "" { key, err := base64.StdEncoding.DecodeString(cfg.Zitadel.Key) if err != nil { @@ -42,9 +46,10 @@ func runUsersync(ctx context.Context, pool *pgxpool.Pool, cfg *Config, log logru } zw = &usersyncer.ZitadelWrapper{ - Client: zc, - IDP: cfg.Zitadel.IDPID, - OrganizationID: cfg.Zitadel.OrganizationID, + Client: zc, + IDP: cfg.Zitadel.IDPID, + OrganizationID: cfg.Zitadel.OrganizationID, + NaisOrganizationID: cfg.Zitadel.NaisOrganizationID, } } diff --git a/internal/usersync/usersyncer/nais.go b/internal/usersync/usersyncer/nais.go new file mode 100644 index 000000000..66820e5c4 --- /dev/null +++ b/internal/usersync/usersyncer/nais.go @@ -0,0 +1,89 @@ +package usersyncer + +import ( + "context" + "fmt" + "strings" + + "github.com/nais/api/internal/usersync/usersyncsql" + "github.com/sirupsen/logrus" + "github.com/zitadel/zitadel-go/v3/pkg/client/zitadel/object/v2" + zitadeluser "github.com/zitadel/zitadel-go/v3/pkg/client/zitadel/user/v2" +) + +// activeHumanUsers fetches a complete snapshot before the database is changed. +// A failed page must never be treated as a snapshot from which users can be deleted. +func (z *ZitadelWrapper) activeHumanUsers(ctx context.Context, organizationID string) ([]*zitadeluser.User, error) { + const limit = 250 + var users []*zitadeluser.User + for offset := uint64(0); ; offset += limit { + resp, err := z.ListUsers(ctx, &zitadeluser.ListUsersRequest{ + Query: &object.ListQuery{Offset: offset, Limit: limit}, + Queries: []*zitadeluser.SearchQuery{ + {Query: &zitadeluser.SearchQuery_TypeQuery{TypeQuery: &zitadeluser.TypeQuery{Type: zitadeluser.Type_TYPE_HUMAN}}}, + {Query: &zitadeluser.SearchQuery_OrganizationIdQuery{OrganizationIdQuery: &zitadeluser.OrganizationIdQuery{OrganizationId: organizationID}}}, + {Query: &zitadeluser.SearchQuery_StateQuery{StateQuery: &zitadeluser.StateQuery{State: zitadeluser.UserState_USER_STATE_ACTIVE}}}, + }, + SortingColumn: zitadeluser.UserFieldName_USER_FIELD_NAME_EMAIL, + }) + if err != nil { + return nil, err + } + for _, u := range resp.Result { + if u.GetHuman() == nil || u.GetState() != zitadeluser.UserState_USER_STATE_ACTIVE { + continue + } + if u.GetUserId() == "" || u.GetHuman().GetEmail().GetEmail() == "" { + return nil, fmt.Errorf("Zitadel user is missing ID or email") + } + users = append(users, u) + } + if len(resp.Result) < limit { + return users, nil + } + } +} + +// syncNaisUsers imports existing Zitadel identities without tenant default roles +// (notably Team creator), admin roles or team membership. Reading is available to +// authenticated users without assigning an additional role. +func syncNaisUsers(ctx context.Context, querier *usersyncsql.Queries, incoming []*zitadeluser.User, existing *userMap, log logrus.FieldLogger) error { + for _, remote := range incoming { + name := remote.GetHuman().GetProfile().GetDisplayName() + email := strings.ToLower(remote.GetHuman().GetEmail().GetEmail()) + local, exists := existing.byExternalID[remote.UserId] + if !exists { + // Do not adopt a different identity's roles or sessions by matching on email. + created, err := querier.Create(ctx, usersyncsql.CreateParams{ + Name: name, Email: email, ExternalID: remote.UserId, + }) + if err != nil { + return fmt.Errorf("create user %q: %w", email, err) + } + if err := querier.CreateLogEntry(ctx, usersyncsql.CreateLogEntryParams{ + Action: usersyncsql.UsersyncLogEntryActionCreateUser, + UserID: created.ID, UserName: name, UserEmail: email, + }); err != nil { + log.WithError(err).Error("create user sync log entry") + } + continue + } + + if local.Name != name || local.Email != email { + if err := querier.Update(ctx, usersyncsql.UpdateParams{ + ID: local.ID, Name: name, Email: email, ExternalID: remote.UserId, + }); err != nil { + return fmt.Errorf("update user %q: %w", email, err) + } + if err := querier.CreateLogEntry(ctx, usersyncsql.CreateLogEntryParams{ + Action: usersyncsql.UsersyncLogEntryActionUpdateUser, + UserID: local.ID, UserName: name, UserEmail: email, + OldUserName: &local.Name, OldUserEmail: &local.Email, + }); err != nil { + log.WithError(err).Error("create user sync log entry") + } + } + delete(existing.byID, local.ID) + } + return nil +} diff --git a/internal/usersync/usersyncer/nais_integration_test.go b/internal/usersync/usersyncer/nais_integration_test.go new file mode 100644 index 000000000..a4ed0fc6d --- /dev/null +++ b/internal/usersync/usersyncer/nais_integration_test.go @@ -0,0 +1,235 @@ +//go:build integration_test + +package usersyncer_test + +import ( + "context" + "errors" + "fmt" + "net/http" + "testing" + + "github.com/nais/api/internal/auth/authz" + "github.com/nais/api/internal/database" + "github.com/nais/api/internal/test" + "github.com/nais/api/internal/user" + "github.com/nais/api/internal/usersync/usersyncer" + "github.com/nais/api/internal/usersync/usersyncsql" + logrustest "github.com/sirupsen/logrus/hooks/test" + zitadelclient "github.com/zitadel/zitadel-go/v3/pkg/client/user/v2" + zitadeluser "github.com/zitadel/zitadel-go/v3/pkg/client/zitadel/user/v2" + admindirectoryv1 "google.golang.org/api/admin/directory/v1" + "google.golang.org/api/option" + "google.golang.org/grpc" +) + +type naisUserService struct { + zitadeluser.UserServiceClient + list func(*zitadeluser.ListUsersRequest) (*zitadeluser.ListUsersResponse, error) +} + +type directoryTransport func(*http.Request) *http.Response + +func (f directoryTransport) RoundTrip(req *http.Request) (*http.Response, error) { + return f(req), nil +} + +func (s naisUserService) ListUsers(_ context.Context, req *zitadeluser.ListUsersRequest, _ ...grpc.CallOption) (*zitadeluser.ListUsersResponse, error) { + return s.list(req) +} + +func naisUser(id, email, name string) *zitadeluser.User { + return &zitadeluser.User{ + UserId: id, + State: zitadeluser.UserState_USER_STATE_ACTIVE, + Type: &zitadeluser.User_Human{Human: &zitadeluser.HumanUser{ + Profile: &zitadeluser.HumanProfile{DisplayName: &name}, + Email: &zitadeluser.HumanEmail{Email: email}, + }}, + } +} + +func TestSyncNaisUsers(t *testing.T) { + ctx := t.Context() + log, _ := logrustest.NewNullLogger() + container, dsn, err := startPostgresql(ctx, t, log) + if err != nil { + t.Fatal(err) + } + + t.Run("lifecycle preserves tenant users and grants only viewer access", func(t *testing.T) { + pool := getConnection(ctx, t, container, dsn, log) + ctx := database.NewLoaderContext(t.Context(), pool) + q := usersyncsql.New(pool) + outdated, err := q.Create(ctx, usersyncsql.CreateParams{Name: "Old name", Email: "old@nais.io", ExternalID: "nais-1"}) + if err != nil { + t.Fatal(err) + } + deleted, err := q.Create(ctx, usersyncsql.CreateParams{Name: "Deleted", Email: "deleted@nais.io", ExternalID: "deleted"}) + if err != nil { + t.Fatal(err) + } + if _, err := pool.Exec(ctx, "INSERT INTO sessions (user_id, expires) VALUES ($1, NOW() + INTERVAL '1 hour')", deleted.ID); err != nil { + t.Fatal(err) + } + + snapshot := []*zitadeluser.User{ + naisUser("nais-1", "UPDATED@nais.io", "Updated name"), + naisUser("nais-2", "new@nais.io", "New user"), + } + var reads int + zc := &usersyncer.ZitadelWrapper{ + Client: &zitadelclient.Client{UserServiceClient: naisUserService{list: func(req *zitadeluser.ListUsersRequest) (*zitadeluser.ListUsersResponse, error) { + reads++ + if req.Queries[1].GetOrganizationIdQuery().GetOrganizationId() != "nais-org" { + t.Fatalf("unexpected organization: %v", req.Queries) + } + return &zitadeluser.ListUsersResponse{Result: snapshot}, nil + }}}, + NaisOrganizationID: "nais-org", + } + // The same tenant snapshot is returned on every sync; its admin role and + // normal default role must be unchanged by the additional user source. + httpClient := &http.Client{Transport: directoryTransport(func(req *http.Request) *http.Response { + if req.URL.Path == "/admin/directory/v1/users" { + return test.Response("200 OK", `{"users":[{"id":"tenant-1","primaryEmail":"tenant@example.com","name":{"fullName":"Tenant user"}}]}`) + } + return test.Response("200 OK", `{"members":[{"id":"tenant-1","email":"tenant@example.com","type":"USER","status":"ACTIVE"}]}`) + })} + svc, err := admindirectoryv1.NewService(ctx, option.WithHTTPClient(httpClient)) + if err != nil { + t.Fatal(err) + } + syncer := usersyncer.New(pool, adminGroupPrefix, domain, zc, svc, log) + for range 2 { + if err := syncer.Sync(ctx); err != nil { + t.Fatal(err) + } + } + if reads != 2 { + t.Fatalf("expected one nais.io snapshot per sync, got %d", reads) + } + users, err := q.List(ctx) + if err != nil || len(users) != 3 { + t.Fatalf("expected three users, got %v, %v", users, err) + } + var tenant *usersyncsql.User + for _, u := range users { + if u.ExternalID == "tenant-1" { + tenant = u + continue + } + if u.Admin { + t.Fatalf("nais.io user unexpectedly has admin access: %v", u) + } + if u.ExternalID == "nais-1" && (u.ID != outdated.ID || u.Name != "Updated name" || u.Email != "updated@nais.io") { + t.Fatalf("existing nais.io identity was not updated in place: %v", u) + } + viewerCtx := user.NewLoaderContext(ctx, pool) + viewerCtx = authz.NewLoaderContext(viewerCtx, pool) + viewerCtx = authz.ContextWithActor(viewerCtx, &user.User{UUID: u.ID, Email: u.Email}, nil) + if err := authz.CanCreateTeam(viewerCtx); err == nil { + t.Fatal("nais.io viewer must not be able to create teams") + } + if err := authz.RequireGlobalAdmin(viewerCtx); err == nil { + t.Fatal("nais.io viewer must not be a global admin") + } + } + if tenant == nil || !tenant.Admin { + t.Fatalf("tenant admin was changed: %v", tenant) + } + roles, err := q.ListRoles(ctx) + if err != nil || len(roles) != 1 || roles[0].UserID != tenant.ID || roles[0].RoleName != "Team creator" { + t.Fatalf("only the tenant should receive Team creator; got %v, %v", roles, err) + } + var sessions int + if err := pool.QueryRow(ctx, "SELECT COUNT(*) FROM sessions WHERE user_id = $1", deleted.ID).Scan(&sessions); err != nil || sessions != 0 { + t.Fatalf("deleted user's sessions remain: %d, %v", sessions, err) + } + + // One user disappears; the other becomes inactive. Neither can remain + // in the API, even if they had an active session before this sync. + if _, err := pool.Exec(ctx, "INSERT INTO sessions (user_id, expires) VALUES ($1, NOW() + INTERVAL '1 hour')", outdated.ID); err != nil { + t.Fatal(err) + } + snapshot = []*zitadeluser.User{naisUser("nais-2", "new@nais.io", "New user")} + snapshot[0].State = zitadeluser.UserState_USER_STATE_INACTIVE + if err := syncer.Sync(ctx); err != nil { + t.Fatal(err) + } + users, err = q.List(ctx) + if err != nil || len(users) != 1 || users[0].ExternalID != "tenant-1" || !users[0].Admin { + t.Fatalf("expected only the unchanged tenant admin, got %v, %v", users, err) + } + if err := pool.QueryRow(ctx, "SELECT COUNT(*) FROM sessions").Scan(&sessions); err != nil || sessions != 0 { + t.Fatalf("inactive/deleted users' sessions remain: %d, %v", sessions, err) + } + }) + + t.Run("failed second page does not change database", func(t *testing.T) { + pool := getConnection(ctx, t, container, dsn, log) + q := usersyncsql.New(pool) + before, err := q.Create(ctx, usersyncsql.CreateParams{Name: "Keep", Email: "keep@nais.io", ExternalID: "keep"}) + if err != nil { + t.Fatal(err) + } + wantErr := errors.New("second page failed") + zc := &usersyncer.ZitadelWrapper{ + Client: &zitadelclient.Client{UserServiceClient: naisUserService{list: func(req *zitadeluser.ListUsersRequest) (*zitadeluser.ListUsersResponse, error) { + if req.Query.Offset > 0 { + return nil, wantErr + } + page := make([]*zitadeluser.User, 250) + for i := range page { + page[i] = naisUser(fmt.Sprint(i), fmt.Sprintf("new%d@nais.io", i), "New") + } + return &zitadeluser.ListUsersResponse{Result: page}, nil + }}}, + NaisOrganizationID: "nais-org", + } + svc, err := admindirectoryv1.NewService(ctx, option.WithHTTPClient(test.NewTestHttpClient(func(*http.Request) *http.Response { + return test.Response("200 OK", `{"users":[]}`) + }))) + if err != nil { + t.Fatal(err) + } + if err := usersyncer.New(pool, adminGroupPrefix, domain, zc, svc, log).Sync(ctx); !errors.Is(err, wantErr) { + t.Fatalf("expected source failure, got %v", err) + } + users, err := q.List(ctx) + if err != nil || len(users) != 1 || users[0].ID != before.ID { + t.Fatalf("failed snapshot changed the database: %v, %v", users, err) + } + }) + + t.Run("same email does not transfer another identity's privileges", func(t *testing.T) { + pool := getConnection(ctx, t, container, dsn, log) + q := usersyncsql.New(pool) + before, err := q.Create(ctx, usersyncsql.CreateParams{Name: "Existing", Email: "existing@nais.io", ExternalID: "other-identity"}) + if err != nil { + t.Fatal(err) + } + if err := q.AssignGlobalAdmin(ctx, before.ID); err != nil { + t.Fatal(err) + } + zc := &usersyncer.ZitadelWrapper{ + Client: &zitadelclient.Client{UserServiceClient: naisUserService{list: func(*zitadeluser.ListUsersRequest) (*zitadeluser.ListUsersResponse, error) { + return &zitadeluser.ListUsersResponse{Result: []*zitadeluser.User{naisUser("new-identity", before.Email, "New")}}, nil + }}}, + NaisOrganizationID: "nais-org", + } + svc, err := admindirectoryv1.NewService(ctx, option.WithHTTPClient(test.NewTestHttpClient(func(*http.Request) *http.Response { + return test.Response("200 OK", `{"users":[]}`) + }))) + if err != nil { + t.Fatal(err) + } + if err := usersyncer.New(pool, adminGroupPrefix, domain, zc, svc, log).Sync(ctx); err == nil { + t.Fatal("expected conflicting email to abort sync, not adopt the existing identity") + } + users, err := q.List(ctx) + if err != nil || len(users) != 1 || users[0].ID != before.ID || users[0].ExternalID != before.ExternalID || !users[0].Admin { + t.Fatalf("conflicting identity was changed: %v, %v", users, err) + } + }) +} diff --git a/internal/usersync/usersyncer/nais_test.go b/internal/usersync/usersyncer/nais_test.go new file mode 100644 index 000000000..499c1c858 --- /dev/null +++ b/internal/usersync/usersyncer/nais_test.go @@ -0,0 +1,101 @@ +package usersyncer + +import ( + "context" + "errors" + "fmt" + "testing" + + zitadelclient "github.com/zitadel/zitadel-go/v3/pkg/client/user/v2" + zitadeluser "github.com/zitadel/zitadel-go/v3/pkg/client/zitadel/user/v2" + "google.golang.org/grpc" +) + +type userService struct { + zitadeluser.UserServiceClient + list func(context.Context, *zitadeluser.ListUsersRequest) (*zitadeluser.ListUsersResponse, error) +} + +func (s userService) ListUsers(ctx context.Context, req *zitadeluser.ListUsersRequest, _ ...grpc.CallOption) (*zitadeluser.ListUsersResponse, error) { + return s.list(ctx, req) +} + +func activeUser(id, email string) *zitadeluser.User { + return &zitadeluser.User{ + UserId: id, + State: zitadeluser.UserState_USER_STATE_ACTIVE, + Type: &zitadeluser.User_Human{Human: &zitadeluser.HumanUser{ + Email: &zitadeluser.HumanEmail{Email: email}, + }}, + } +} + +func TestActiveHumanUsers(t *testing.T) { + var offsets []uint64 + z := &ZitadelWrapper{Client: &zitadelclient.Client{UserServiceClient: userService{ + list: func(ctx context.Context, req *zitadeluser.ListUsersRequest) (*zitadeluser.ListUsersResponse, error) { + offsets = append(offsets, req.Query.Offset) + if req.Query.Limit != 250 { + t.Fatalf("unexpected page size: %d", req.Query.Limit) + } + if len(req.Queries) != 3 || req.Queries[0].GetTypeQuery().GetType() != zitadeluser.Type_TYPE_HUMAN || + req.Queries[1].GetOrganizationIdQuery().GetOrganizationId() != "nais-org" || + req.Queries[2].GetStateQuery().GetState() != zitadeluser.UserState_USER_STATE_ACTIVE { + t.Fatalf("unexpected user filters: %v", req.Queries) + } + if req.Query.Offset == 0 { + users := make([]*zitadeluser.User, 250) + for i := range users { + users[i] = activeUser(fmt.Sprint(i), fmt.Sprintf("user%d@nais.io", i)) + } + return &zitadeluser.ListUsersResponse{Result: users}, nil + } + inactive := activeUser("inactive", "inactive@nais.io") + inactive.State = zitadeluser.UserState_USER_STATE_INACTIVE + return &zitadeluser.ListUsersResponse{Result: []*zitadeluser.User{ + activeUser("last", "last@nais.io"), inactive, + {UserId: "machine", State: zitadeluser.UserState_USER_STATE_ACTIVE, Type: &zitadeluser.User_Machine{Machine: &zitadeluser.MachineUser{}}}, + }}, nil + }, + }}} + users, err := z.activeHumanUsers(t.Context(), "nais-org") + if err != nil { + t.Fatal(err) + } + if len(users) != 251 || len(offsets) != 2 || offsets[0] != 0 || offsets[1] != 250 { + t.Fatalf("got %d users, offsets %v", len(users), offsets) + } +} + +func TestActiveHumanUsersDiscardsPartialSnapshot(t *testing.T) { + wantErr := errors.New("Zitadel unavailable") + z := &ZitadelWrapper{Client: &zitadelclient.Client{UserServiceClient: userService{ + list: func(ctx context.Context, req *zitadeluser.ListUsersRequest) (*zitadeluser.ListUsersResponse, error) { + if req.Query.Offset != 0 { + return nil, wantErr + } + users := make([]*zitadeluser.User, 250) + for i := range users { + users[i] = activeUser(fmt.Sprint(i), "user@nais.io") + } + return &zitadeluser.ListUsersResponse{Result: users}, nil + }, + }}} + users, err := z.activeHumanUsers(t.Context(), "nais-org") + if !errors.Is(err, wantErr) || users != nil { + t.Fatalf("expected no partial snapshot and %v, got %v, %v", wantErr, users, err) + } +} + +func TestActiveHumanUsersRejectsIncompleteIdentity(t *testing.T) { + for _, u := range []*zitadeluser.User{activeUser("", "user@nais.io"), activeUser("id", "")} { + z := &ZitadelWrapper{Client: &zitadelclient.Client{UserServiceClient: userService{ + list: func(context.Context, *zitadeluser.ListUsersRequest) (*zitadeluser.ListUsersResponse, error) { + return &zitadeluser.ListUsersResponse{Result: []*zitadeluser.User{u}}, nil + }, + }}} + if _, err := z.activeHumanUsers(t.Context(), "nais-org"); err == nil { + t.Fatal("expected incomplete identity to abort snapshot") + } + } +} diff --git a/internal/usersync/usersyncer/usersyncer.go b/internal/usersync/usersyncer/usersyncer.go index 2176b12a3..c8635fee8 100644 --- a/internal/usersync/usersyncer/usersyncer.go +++ b/internal/usersync/usersyncer/usersyncer.go @@ -24,8 +24,9 @@ import ( type ZitadelWrapper struct { *zitadeluser.Client - IDP string - OrganizationID string + IDP string + OrganizationID string + NaisOrganizationID string } type Usersynchronizer struct { @@ -83,13 +84,10 @@ func NewFromConfig(ctx context.Context, pool *pgxpool.Pool, serviceAccount, subj return New(pool, adminGroupPrefix, tenantDomain, zitadelWrapper, srv, log), nil } -// Sync fetches all users from the Google Directory of the tenant and adds them as users in Nais API. -// -// If a user already exist in Nais API the user will get the name and email potentially updated if it has changed in the -// Google Directory. -// -// After all users have been synced, users that have an email address that matches the tenant domain that no longer -// exist in the Google Directory will be removed. +// Sync fetches tenant users from Google Directory and, when configured, active +// nais.io users from Zitadel. Both complete snapshots are fetched before changing +// the API database. Users are created or updated, and users absent from both +// sources are removed. Only tenant users are synchronized into Zitadel. // // All users present in the admin group in the Google Directory will also be granted the admin role in Nais API, and // existing admins that no longer exist in the admin group will get the admin role revoked. @@ -99,7 +97,15 @@ func (s *Usersynchronizer) Sync(ctx context.Context) error { return fmt.Errorf("get users from Google Directory: %w", err) } - if s.zitadelClient != nil { + var naisUsers []*zitadelgrpcuser.User + if s.zitadelClient != nil && s.zitadelClient.NaisOrganizationID != "" { + naisUsers, err = s.zitadelClient.activeHumanUsers(ctx, s.zitadelClient.NaisOrganizationID) + if err != nil { + return fmt.Errorf("get nais.io users from Zitadel: %w", err) + } + } + + if s.zitadelClient != nil && s.zitadelClient.IDP != "" { wg := &sync.WaitGroup{} wg.Go(func() { s.zitadelUserSync(ctx, googleUsers) @@ -160,6 +166,10 @@ func (s *Usersynchronizer) Sync(ctx context.Context) error { delete(users.byID, user.ID) } + if err := syncNaisUsers(ctx, querier, naisUsers, users, s.log); err != nil { + return fmt.Errorf("sync nais.io users: %w", err) + } + if err := deleteUnknownUsers(ctx, querier, users.byID, s.log); err != nil { return err } @@ -331,7 +341,7 @@ func AssignDefaultPermissionsToUser(ctx context.Context, querier usersyncsql.Que return nil } -// deleteUnknownUsers will delete users from Nais API that does not exist in the Google Directory. +// deleteUnknownUsers deletes API users absent from the complete source snapshots. func deleteUnknownUsers(ctx context.Context, querier usersyncsql.Querier, unknownUsers map[uuid.UUID]*usersyncsql.User, log logrus.FieldLogger) error { for _, user := range unknownUsers { if err := querier.Delete(ctx, user.ID); err != nil { From 17da96227cfdb348cd198c69bafd89e7693f5165 Mon Sep 17 00:00:00 2001 From: Frode Sundby Date: Wed, 21 Oct 2026 13:36:49 +0200 Subject: [PATCH 2/5] fix: reject cross-identity email adoption during user sync Google's email fallback could reuse a nais.io user's UUID and grant tenant admin privileges to its existing sessions. Reject email collisions with different external IDs, including identities missing from the current Zitadel snapshot, and cover the reverse collisions with integration tests. --- docs/nais_io_users.md | 5 +- .../usersyncer/nais_integration_test.go | 67 +++++++++++++++++++ internal/usersync/usersyncer/usersyncer.go | 5 +- 3 files changed, 75 insertions(+), 2 deletions(-) diff --git a/docs/nais_io_users.md b/docs/nais_io_users.md index d921f9f2e..6af6a3cb2 100644 --- a/docs/nais_io_users.md +++ b/docs/nais_io_users.md @@ -15,7 +15,10 @@ The CLI's login restriction and tenant selection are handled separately. - The API imports active human users from the configured Zitadel organization. Users are not copied into the tenant's Zitadel organization or modified there. - The Zitadel user ID is stored as `external_id`, matching the token subject. - Existing users are not adopted by email from another identity. + Neither source adopts existing users by email from another identity. An email + collision with a different external ID aborts the database transaction, + preserving existing identities, roles and sessions. This also applies to + identities absent from the current snapshots. - New nais.io users have the existing authenticated-user read access (Viewer), without the tenant default `Team creator` role, admin access or team membership. - Both user snapshots are fetched before database changes. Failure to fetch any diff --git a/internal/usersync/usersyncer/nais_integration_test.go b/internal/usersync/usersyncer/nais_integration_test.go index a4ed0fc6d..a6f733668 100644 --- a/internal/usersync/usersyncer/nais_integration_test.go +++ b/internal/usersync/usersyncer/nais_integration_test.go @@ -9,6 +9,7 @@ import ( "net/http" "testing" + "github.com/google/uuid" "github.com/nais/api/internal/auth/authz" "github.com/nais/api/internal/database" "github.com/nais/api/internal/test" @@ -232,4 +233,70 @@ func TestSyncNaisUsers(t *testing.T) { t.Fatalf("conflicting identity was changed: %v, %v", users, err) } }) + + for _, tc := range []struct { + name string + remoteName string + absent bool + }{ + {name: "unchanged nais.io display name", remoteName: "Nais viewer"}, + {name: "changed nais.io display name", remoteName: "Renamed nais.io viewer"}, + {name: "nais.io identity absent from current snapshot", absent: true}, + } { + t.Run("Google email collision with "+tc.name, func(t *testing.T) { + pool := getConnection(ctx, t, container, dsn, log) + q := usersyncsql.New(pool) + // Organization membership, not the email domain, determines the + // source. This could also be a previously renamed nais.io account. + before, err := q.Create(ctx, usersyncsql.CreateParams{ + Name: "Nais viewer", Email: "shared@example.com", ExternalID: "nais-subject", + }) + if err != nil { + t.Fatal(err) + } + var sessionID uuid.UUID + if err := pool.QueryRow(ctx, "INSERT INTO sessions (user_id, expires) VALUES ($1, NOW() + INTERVAL '1 hour') RETURNING id", before.ID).Scan(&sessionID); err != nil { + t.Fatal(err) + } + zc := &usersyncer.ZitadelWrapper{ + Client: &zitadelclient.Client{UserServiceClient: naisUserService{list: func(*zitadeluser.ListUsersRequest) (*zitadeluser.ListUsersResponse, error) { + var users []*zitadeluser.User + if !tc.absent { + users = append(users, naisUser(before.ExternalID, before.Email, tc.remoteName)) + } + return &zitadeluser.ListUsersResponse{Result: users}, nil + }}}, + NaisOrganizationID: "nais-org", + } + httpClient := &http.Client{Transport: directoryTransport(func(req *http.Request) *http.Response { + if req.URL.Path == "/admin/directory/v1/users" { + return test.Response("200 OK", `{"users":[{"id":"tenant-admin","primaryEmail":"shared@example.com","name":{"fullName":"Tenant admin"}}]}`) + } + return test.Response("200 OK", `{"members":[{"id":"tenant-admin","email":"shared@example.com","type":"USER","status":"ACTIVE"}]}`) + })} + svc, err := admindirectoryv1.NewService(ctx, option.WithHTTPClient(httpClient)) + if err != nil { + t.Fatal(err) + } + if err := usersyncer.New(pool, adminGroupPrefix, domain, zc, svc, log).Sync(ctx); err == nil { + t.Error("expected conflicting Google identity to abort sync") + } + users, err := q.List(ctx) + if err != nil || len(users) != 1 { + t.Fatalf("unexpected users after collision: %v, %v", users, err) + } + got := users[0] + if got.ID != before.ID || got.ExternalID != before.ExternalID || got.Name != before.Name || got.Email != before.Email || got.Admin { + t.Errorf("Google identity adopted or changed the nais.io viewer: %+v", got) + } + var sessionUserID uuid.UUID + var sessionIsAdmin bool + if err := pool.QueryRow(ctx, "SELECT u.id, u.admin FROM sessions s JOIN users u ON u.id = s.user_id WHERE s.id = $1", sessionID).Scan(&sessionUserID, &sessionIsAdmin); err != nil { + t.Fatal(err) + } + if sessionUserID != before.ID || sessionIsAdmin { + t.Errorf("existing nais.io session received tenant admin privileges: user=%s admin=%v", sessionUserID, sessionIsAdmin) + } + }) + } } diff --git a/internal/usersync/usersyncer/usersyncer.go b/internal/usersync/usersyncer/usersyncer.go index c8635fee8..65ba36f62 100644 --- a/internal/usersync/usersyncer/usersyncer.go +++ b/internal/usersync/usersyncer/usersyncer.go @@ -480,7 +480,10 @@ func getOrCreateUserFromGoogleUser(ctx context.Context, querier usersyncsql.Quer } if existingUser, exists := existingUsers.byEmail[googleUser.Email]; exists { - return existingUser, nil + // Email addresses can be reused by another identity, including a nais.io + // user absent from the current snapshot. Reusing its UUID would transfer + // existing sessions and roles to the Google identity (or vice versa). + return nil, fmt.Errorf("email %q already belongs to external ID %q; refusing to replace it with %q", googleUser.Email, existingUser.ExternalID, googleUser.ID) } createdUser, err := querier.Create(ctx, usersyncsql.CreateParams{ From ce4724fc10233a4fe0822d7a4c62ea41a68e03ae Mon Sep 17 00:00:00 2001 From: Frode Sundby Date: Wed, 21 Oct 2026 15:25:24 +0200 Subject: [PATCH 3/5] refactor: simplify nais.io user sync around unique email addresses Share user synchronization across sources and retain email-based Console login for both tenant and nais.io users. Document global email uniqueness and defer CLI token changes, preserving the existing tenant JWT authentication. --- .env.example | 2 +- charts/nais-api/Feature.yaml | 2 +- charts/nais-api/values.yaml | 2 +- docs/nais_io_users.md | 32 +++--- internal/auth/authn/handler.go | 13 +-- .../auth/authn/handler_integration_test.go | 28 +++--- internal/auth/middleware/jwt.go | 40 +++----- internal/auth/middleware/jwt_test.go | 65 +++---------- internal/cmd/api/api.go | 2 +- internal/usersync/usersyncer/nais.go | 62 +++--------- .../usersyncer/nais_integration_test.go | 97 ++++--------------- internal/usersync/usersyncer/nais_test.go | 8 +- internal/usersync/usersyncer/usersyncer.go | 68 ++++++------- 13 files changed, 143 insertions(+), 278 deletions(-) diff --git a/.env.example b/.env.example index b11049f5b..81f86b28d 100644 --- a/.env.example +++ b/.env.example @@ -38,7 +38,7 @@ ZITADEL_IDP_ID= ZITADEL_KEY= ZITADEL_DOMAIN=https://auth.nais.io ZITADEL_ORGANIZATION_ID= -# Enable nais.io user sync/login; ZITADEL_KEY must have user read access in this organization. +# Enable nais.io user sync/Console login; ZITADEL_KEY must have user read access in this organization. ZITADEL_NAIS_ORGANIZATION_ID= AIVEN_PROJECTS='{"dev": {"id":"nav-dev","vpc":"somevpc","endpoint_id":"some_endpoint_id"}}' diff --git a/charts/nais-api/Feature.yaml b/charts/nais-api/Feature.yaml index 41fa3e936..a69a40e20 100644 --- a/charts/nais-api/Feature.yaml +++ b/charts/nais-api/Feature.yaml @@ -246,7 +246,7 @@ values: zitadel.naisOrganizationID: displayName: Nais Zitadel organization ID - description: Enables syncing active nais.io users as viewers and allows nais.io login + description: Enables syncing active nais.io users as viewers and allows nais.io Console login computed: template: '{{ .Env.zitadel_nais_organization_id | default "" | quote }}' diff --git a/charts/nais-api/values.yaml b/charts/nais-api/values.yaml index d81cf3a13..f64f7be6b 100644 --- a/charts/nais-api/values.yaml +++ b/charts/nais-api/values.yaml @@ -89,7 +89,7 @@ zitadel: idpID: key: organizationID: - naisOrganizationID: "" # Enables nais.io user sync and login when configured + naisOrganizationID: "" # Enables nais.io user sync and Console login when configured aiven: projects: "" diff --git a/docs/nais_io_users.md b/docs/nais_io_users.md index 6af6a3cb2..a6df8efbd 100644 --- a/docs/nais_io_users.md +++ b/docs/nais_io_users.md @@ -1,24 +1,31 @@ # nais.io users in tenant APIs Set `ZITADEL_NAIS_ORGANIZATION_ID` (Helm: `zitadel.naisOrganizationID`) to the -nais.io organization ID to enable user sync and login. An empty value preserves +nais.io organization ID to enable user sync and Console login. An empty value preserves tenant-only login. Fasit supplies the value through `zitadel_nais_organization_id`. The machine user configured by `ZITADEL_KEY` needs read access to users in the nais.io organization, in addition to its existing tenant user-management access. Terraform must also grant the nais.io organization access to each tenant's Console project. These permissions must be deployed before enabling the setting. -The CLI's login restriction and tenant selection are handled separately. +CLI support, including a shared email-based token lookup for tenant and nais.io +users, is handled separately. This change does not modify JWT authentication. + +## User identity + +Email addresses are globally unique across tenants and Zitadel organizations: +the same email identifies the same user. Both user sources use the same +create/update logic, and Console login looks up both tenant and nais.io users by +email. The model does not distinguish separate identities with the same email. + +The existing `external_id` field remains populated for compatibility with the +current tenant CLI and to preserve users when their email changes. It is not +used to distinguish nais.io users from tenant users during Console login. ## User lifecycle and permissions - The API imports active human users from the configured Zitadel organization. Users are not copied into the tenant's Zitadel organization or modified there. -- The Zitadel user ID is stored as `external_id`, matching the token subject. - Neither source adopts existing users by email from another identity. An email - collision with a different external ID aborts the database transaction, - preserving existing identities, roles and sessions. This also applies to - identities absent from the current snapshots. - New nais.io users have the existing authenticated-user read access (Viewer), without the tenant default `Team creator` role, admin access or team membership. - Both user snapshots are fetched before database changes. Failure to fetch any @@ -32,8 +39,9 @@ The CLI's login restriction and tenant selection are handled separately. Zitadel is the source of truth for nais.io users. Suspending a user in Google Workspace alone is not sufficient: deactivation must also reach Zitadel. -JWT authentication continues to check the issuer, audience and resource-owner -organization, accepting only the tenant organization and the configured nais.io -organization. The subject must match a local user. Console login uses the token -subject rather than email for Zitadel identities and no longer pins the login -flow to the tenant organization when nais.io login is enabled. +Console login no longer pins the flow to the tenant organization when nais.io +login is enabled. Users must still authenticate through the configured OIDC +provider and exist in the local API database. The tenant CLI's existing issuer, +audience, organization and subject validation is unchanged. Its access token does +not currently include email; supporting nais.io CLI login will require a shared +token-format change for both tenant and nais.io users. diff --git a/internal/auth/authn/handler.go b/internal/auth/authn/handler.go index 19285f68d..7fff4389a 100644 --- a/internal/auth/authn/handler.go +++ b/internal/auth/authn/handler.go @@ -155,18 +155,7 @@ func (h *handler) Callback(w http.ResponseWriter, r *http.Request) { return } - var u *user.User - if h.oauth2Config.IsZitadel() { - // Zitadel subjects identify the synced user across organizations. An email - // address alone must not allow an identity from another organization in. - if idToken.Subject == "" { - err = fmt.Errorf("missing subject in Zitadel ID token") - } else { - u, err = user.GetByExternalID(r.Context(), idToken.Subject) - } - } else { - u, err = user.GetByEmail(r.Context(), claims.Email) - } + u, err := user.GetByEmail(r.Context(), claims.Email) if err != nil { h.log.WithError(err).Errorf("get user (%s) from db", claims.Email) http.Redirect(w, r, "/?error=unknown-user", http.StatusFound) diff --git a/internal/auth/authn/handler_integration_test.go b/internal/auth/authn/handler_integration_test.go index 0d617eaa0..eefa5ff30 100644 --- a/internal/auth/authn/handler_integration_test.go +++ b/internal/auth/authn/handler_integration_test.go @@ -52,7 +52,7 @@ func (k callbackKeySet) VerifySignature(context.Context, string) ([]byte, error) return k.payload, nil } -func TestCallbackUsesZitadelSubject(t *testing.T) { +func TestCallbackUsesEmail(t *testing.T) { ctx := t.Context() log, _ := logrustest.NewNullLogger() container, err := postgres.Run(ctx, "docker.io/postgres:16-alpine", @@ -76,18 +76,24 @@ func TestCallbackUsesZitadelSubject(t *testing.T) { if err != nil { t.Fatal(err) } + tenant, err := q.Create(ctx, usersyncsql.CreateParams{Name: "Tenant viewer", Email: "viewer@nav.no", ExternalID: "tenant-subject"}) + if err != nil { + t.Fatal(err) + } for _, tc := range []struct { - name string - config callbackOAuth - wantLogin bool + name string + config callbackOAuth + wantUserID uuid.UUID }{ - {name: "synced Zitadel subject", config: callbackOAuth{zitadel: true, subject: u.ExternalID, email: u.Email}, wantLogin: true}, - {name: "subject remains authoritative after email changes", config: callbackOAuth{zitadel: true, subject: u.ExternalID, email: "renamed@nais.io"}, wantLogin: true}, - {name: "same email from another identity", config: callbackOAuth{zitadel: true, subject: "foreign-subject", email: u.Email}}, - {name: "missing subject cannot fall back to email", config: callbackOAuth{zitadel: true, email: u.Email}}, + {name: "nais.io user", config: callbackOAuth{zitadel: true, subject: u.ExternalID, email: u.Email}, wantUserID: u.ID}, + {name: "tenant user", config: callbackOAuth{zitadel: true, subject: tenant.ExternalID, email: tenant.Email}, wantUserID: tenant.ID}, + {name: "nais.io user matched by email regardless of subject", config: callbackOAuth{zitadel: true, subject: "new-nais-subject", email: u.Email}, wantUserID: u.ID}, + {name: "tenant user matched by email regardless of subject", config: callbackOAuth{zitadel: true, subject: "new-tenant-subject", email: tenant.Email}, wantUserID: tenant.ID}, + {name: "known nais.io subject with unknown email", config: callbackOAuth{zitadel: true, subject: u.ExternalID, email: "unknown@nais.io"}}, + {name: "known tenant subject with unknown email", config: callbackOAuth{zitadel: true, subject: tenant.ExternalID, email: "unknown@nav.no"}}, {name: "unsynced user", config: callbackOAuth{zitadel: true, subject: "unsynced", email: "unsynced@nais.io"}}, - {name: "non-Zitadel login still uses email", config: callbackOAuth{subject: "google-subject", email: u.Email}, wantLogin: true}, + {name: "non-Zitadel login still uses email", config: callbackOAuth{subject: "google-subject", email: tenant.Email}, wantUserID: tenant.ID}, } { t.Run(tc.name, func(t *testing.T) { ctx := database.NewLoaderContext(t.Context(), pool) @@ -100,7 +106,7 @@ func TestCallbackUsesZitadelSubject(t *testing.T) { if rec.Code != http.StatusFound { t.Fatalf("expected redirect, got %d: %s", rec.Code, rec.Body.String()) } - if !tc.wantLogin { + if tc.wantUserID == uuid.Nil { if got := rec.Header().Get("Location"); got != "/?error=unknown-user" { t.Fatalf("expected unknown user, got %q", got) } @@ -119,7 +125,7 @@ func TestCallbackUsesZitadelSubject(t *testing.T) { continue } sess, err := session.Get(ctx, uuid.MustParse(cookie.Value)) - if err != nil || sess.UserID != u.ID { + if err != nil || sess.UserID != tc.wantUserID { t.Fatalf("session belongs to wrong identity: %v, %v", sess, err) } return diff --git a/internal/auth/middleware/jwt.go b/internal/auth/middleware/jwt.go index 6dcf89d28..b8f20dcaf 100644 --- a/internal/auth/middleware/jwt.go +++ b/internal/auth/middleware/jwt.go @@ -20,14 +20,13 @@ import ( const acceptableClockSkew = 3 * time.Second type jwtAuth struct { - issuer string - audience string - zitadelOrgID string - naisZitadelOrgID string - jwksURL string - jwksCache *jwk.Cache - log logrus.FieldLogger - now clock + issuer string + audience string + zitadelOrgID string + jwksURL string + jwksCache *jwk.Cache + log logrus.FieldLogger + now clock } type clock func() time.Time @@ -66,17 +65,11 @@ func (j *jwtAuth) validate(ctx context.Context, token string) (jwt.Token, error) return nil, fmt.Errorf("provider: parsing token: %w", err) } - orgID := j.zitadelOrgID - var resourceOwner string - if j.naisZitadelOrgID != "" && t.Get("urn:zitadel:iam:user:resourceowner:id", &resourceOwner) == nil && resourceOwner == j.naisZitadelOrgID { - orgID = j.naisZitadelOrgID - } - return t, jwt.Validate(t, jwt.WithAcceptableSkew(acceptableClockSkew), jwt.WithIssuer(j.issuer), jwt.WithAudience(j.audience), - jwt.WithClaimValue("urn:zitadel:iam:user:resourceowner:id", orgID), + jwt.WithClaimValue("urn:zitadel:iam:user:resourceowner:id", j.zitadelOrgID), jwt.WithClock(j.now), ) } @@ -123,7 +116,7 @@ func (j *jwtAuth) handler(next http.Handler) http.Handler { return http.HandlerFunc(fn) } -func JWTAuthentication(ctx context.Context, issuer, audience, zitadelOrgID, naisZitadelOrgID string, log logrus.FieldLogger) (func(next http.Handler) http.Handler, error) { +func JWTAuthentication(ctx context.Context, issuer, audience, zitadelOrgID string, log logrus.FieldLogger) (func(next http.Handler) http.Handler, error) { if issuer == "" { return nil, errors.New("issuer is required") } @@ -150,14 +143,13 @@ func JWTAuthentication(ctx context.Context, issuer, audience, zitadelOrgID, nais } auth := jwtAuth{ - jwksURL: client.JwksURI, - jwksCache: cache, - issuer: issuer, - audience: audience, - zitadelOrgID: zitadelOrgID, - naisZitadelOrgID: naisZitadelOrgID, - log: log, - now: time.Now, + jwksURL: client.JwksURI, + jwksCache: cache, + issuer: issuer, + audience: audience, + zitadelOrgID: zitadelOrgID, + log: log, + now: time.Now, } return auth.handler, nil diff --git a/internal/auth/middleware/jwt_test.go b/internal/auth/middleware/jwt_test.go index fa74d623d..160a376cc 100644 --- a/internal/auth/middleware/jwt_test.go +++ b/internal/auth/middleware/jwt_test.go @@ -30,12 +30,11 @@ func TestJWTAuthorized(t *testing.T) { inputZitadelOrgID string inputTime time.Time - expectedAudience string - expectedIssuer string - expectedZitadelOrgID string - expectedNaisZitadelOrgID string - expectedTime time.Time - expectedError error + expectedAudience string + expectedIssuer string + expectedZitadelOrgID string + expectedTime time.Time + expectedError error }{ "valid token": { inputAudience: "aud", @@ -54,47 +53,6 @@ func TestJWTAuthorized(t *testing.T) { expectedZitadelOrgID: "org-id", expectedError: jwt.InvalidAudienceError(), }, - "nais.io token when enabled": { - inputAudience: "aud", inputIssuer: "http://localhost:1234", inputZitadelOrgID: "nais-org-id", - expectedAudience: "aud", expectedIssuer: "http://localhost:1234", expectedZitadelOrgID: "org-id", - expectedNaisZitadelOrgID: "nais-org-id", - }, - "nais.io token when disabled": { - inputAudience: "aud", inputIssuer: "http://localhost:1234", inputZitadelOrgID: "nais-org-id", - expectedAudience: "aud", expectedIssuer: "http://localhost:1234", expectedZitadelOrgID: "org-id", - expectedError: jwt.ValidateError(), - }, - "tenant token when nais.io is enabled": { - inputAudience: "aud", inputIssuer: "http://localhost:1234", inputZitadelOrgID: "org-id", - expectedAudience: "aud", expectedIssuer: "http://localhost:1234", expectedZitadelOrgID: "org-id", - expectedNaisZitadelOrgID: "nais-org-id", - }, - "another organization when nais.io is enabled": { - inputAudience: "aud", inputIssuer: "http://localhost:1234", inputZitadelOrgID: "other-org-id", - expectedAudience: "aud", expectedIssuer: "http://localhost:1234", expectedZitadelOrgID: "org-id", - expectedNaisZitadelOrgID: "nais-org-id", expectedError: jwt.ValidateError(), - }, - "nais.io token with invalid audience": { - inputAudience: "other-aud", inputIssuer: "http://localhost:1234", inputZitadelOrgID: "nais-org-id", - expectedAudience: "aud", expectedIssuer: "http://localhost:1234", expectedZitadelOrgID: "org-id", - expectedNaisZitadelOrgID: "nais-org-id", expectedError: jwt.InvalidAudienceError(), - }, - "nais.io token with invalid issuer": { - inputAudience: "aud", inputIssuer: "http://other-issuer", inputZitadelOrgID: "nais-org-id", - expectedAudience: "aud", expectedIssuer: "http://localhost:1234", expectedZitadelOrgID: "org-id", - expectedNaisZitadelOrgID: "nais-org-id", expectedError: jwt.InvalidIssuerError(), - }, - "nais.io expired token": { - inputAudience: "aud", inputIssuer: "http://localhost:1234", inputZitadelOrgID: "nais-org-id", - inputTime: now.Add(-time.Hour), - expectedAudience: "aud", expectedIssuer: "http://localhost:1234", expectedZitadelOrgID: "org-id", - expectedNaisZitadelOrgID: "nais-org-id", expectedError: jwt.TokenExpiredError(), - }, - "missing organization when nais.io is enabled": { - inputAudience: "aud", inputIssuer: "http://localhost:1234", - expectedAudience: "aud", expectedIssuer: "http://localhost:1234", expectedZitadelOrgID: "org-id", - expectedNaisZitadelOrgID: "nais-org-id", expectedError: jwt.ValidateError(), - }, "invalid issuer": { inputAudience: "aud", inputIssuer: "http://invalid-issuer", @@ -154,13 +112,12 @@ func TestJWTAuthorized(t *testing.T) { cache.Register(t.Context(), test.expectedIssuer) mw := jwtAuth{ - issuer: test.expectedIssuer, - audience: test.expectedAudience, - zitadelOrgID: test.expectedZitadelOrgID, - naisZitadelOrgID: test.expectedNaisZitadelOrgID, - jwksURL: test.expectedIssuer, - jwksCache: cache, - log: log, + issuer: test.expectedIssuer, + audience: test.expectedAudience, + zitadelOrgID: test.expectedZitadelOrgID, + jwksURL: test.expectedIssuer, + jwksCache: cache, + log: log, now: func() time.Time { return defaultTime(test.expectedTime) }, diff --git a/internal/cmd/api/api.go b/internal/cmd/api/api.go index 30436ea22..e33861cea 100644 --- a/internal/cmd/api/api.go +++ b/internal/cmd/api/api.go @@ -278,7 +278,7 @@ func run(ctx context.Context, cfg *Config, log logrus.FieldLogger) error { var jwtMiddleware func(next http.Handler) http.Handler if !cfg.JWT.SkipMiddleware { - jwtMiddleware, err = middleware.JWTAuthentication(ctx, cfg.JWT.Issuer, cfg.JWT.Audience, cfg.Zitadel.OrganizationID, cfg.Zitadel.NaisOrganizationID, log.WithField("subsystem", "jwt")) + jwtMiddleware, err = middleware.JWTAuthentication(ctx, cfg.JWT.Issuer, cfg.JWT.Audience, cfg.Zitadel.OrganizationID, log.WithField("subsystem", "jwt")) if err != nil { return fmt.Errorf("failed to create JWT authentication middleware: %w", err) } diff --git a/internal/usersync/usersyncer/nais.go b/internal/usersync/usersyncer/nais.go index 66820e5c4..72313df4c 100644 --- a/internal/usersync/usersyncer/nais.go +++ b/internal/usersync/usersyncer/nais.go @@ -5,17 +5,16 @@ import ( "fmt" "strings" - "github.com/nais/api/internal/usersync/usersyncsql" - "github.com/sirupsen/logrus" "github.com/zitadel/zitadel-go/v3/pkg/client/zitadel/object/v2" zitadeluser "github.com/zitadel/zitadel-go/v3/pkg/client/zitadel/user/v2" + admindirectoryv1 "google.golang.org/api/admin/directory/v1" ) // activeHumanUsers fetches a complete snapshot before the database is changed. // A failed page must never be treated as a snapshot from which users can be deleted. -func (z *ZitadelWrapper) activeHumanUsers(ctx context.Context, organizationID string) ([]*zitadeluser.User, error) { +func (z *ZitadelWrapper) activeHumanUsers(ctx context.Context, organizationID string) ([]*sourceUser, error) { const limit = 250 - var users []*zitadeluser.User + var users []*sourceUser for offset := uint64(0); ; offset += limit { resp, err := z.ListUsers(ctx, &zitadeluser.ListUsersRequest{ Query: &object.ListQuery{Offset: offset, Limit: limit}, @@ -36,54 +35,19 @@ func (z *ZitadelWrapper) activeHumanUsers(ctx context.Context, organizationID st if u.GetUserId() == "" || u.GetHuman().GetEmail().GetEmail() == "" { return nil, fmt.Errorf("Zitadel user is missing ID or email") } - users = append(users, u) + profile := u.GetHuman().GetProfile() + users = append(users, &sourceUser{ + ID: u.UserId, + Email: strings.ToLower(u.GetHuman().GetEmail().GetEmail()), + Name: admindirectoryv1.UserName{ + FullName: profile.GetDisplayName(), + GivenName: profile.GetGivenName(), + FamilyName: profile.GetFamilyName(), + }, + }) } if len(resp.Result) < limit { return users, nil } } } - -// syncNaisUsers imports existing Zitadel identities without tenant default roles -// (notably Team creator), admin roles or team membership. Reading is available to -// authenticated users without assigning an additional role. -func syncNaisUsers(ctx context.Context, querier *usersyncsql.Queries, incoming []*zitadeluser.User, existing *userMap, log logrus.FieldLogger) error { - for _, remote := range incoming { - name := remote.GetHuman().GetProfile().GetDisplayName() - email := strings.ToLower(remote.GetHuman().GetEmail().GetEmail()) - local, exists := existing.byExternalID[remote.UserId] - if !exists { - // Do not adopt a different identity's roles or sessions by matching on email. - created, err := querier.Create(ctx, usersyncsql.CreateParams{ - Name: name, Email: email, ExternalID: remote.UserId, - }) - if err != nil { - return fmt.Errorf("create user %q: %w", email, err) - } - if err := querier.CreateLogEntry(ctx, usersyncsql.CreateLogEntryParams{ - Action: usersyncsql.UsersyncLogEntryActionCreateUser, - UserID: created.ID, UserName: name, UserEmail: email, - }); err != nil { - log.WithError(err).Error("create user sync log entry") - } - continue - } - - if local.Name != name || local.Email != email { - if err := querier.Update(ctx, usersyncsql.UpdateParams{ - ID: local.ID, Name: name, Email: email, ExternalID: remote.UserId, - }); err != nil { - return fmt.Errorf("update user %q: %w", email, err) - } - if err := querier.CreateLogEntry(ctx, usersyncsql.CreateLogEntryParams{ - Action: usersyncsql.UsersyncLogEntryActionUpdateUser, - UserID: local.ID, UserName: name, UserEmail: email, - OldUserName: &local.Name, OldUserEmail: &local.Email, - }); err != nil { - log.WithError(err).Error("create user sync log entry") - } - } - delete(existing.byID, local.ID) - } - return nil -} diff --git a/internal/usersync/usersyncer/nais_integration_test.go b/internal/usersync/usersyncer/nais_integration_test.go index a6f733668..0dde86984 100644 --- a/internal/usersync/usersyncer/nais_integration_test.go +++ b/internal/usersync/usersyncer/nais_integration_test.go @@ -203,100 +203,43 @@ func TestSyncNaisUsers(t *testing.T) { } }) - t.Run("same email does not transfer another identity's privileges", func(t *testing.T) { + t.Run("existing user is matched by email", func(t *testing.T) { pool := getConnection(ctx, t, container, dsn, log) q := usersyncsql.New(pool) - before, err := q.Create(ctx, usersyncsql.CreateParams{Name: "Existing", Email: "existing@nais.io", ExternalID: "other-identity"}) + before, err := q.Create(ctx, usersyncsql.CreateParams{Name: "Existing", Email: "existing@nais.io", ExternalID: "old-zitadel-id"}) if err != nil { t.Fatal(err) } - if err := q.AssignGlobalAdmin(ctx, before.ID); err != nil { + var sessionID uuid.UUID + if err := pool.QueryRow(ctx, "INSERT INTO sessions (user_id, expires) VALUES ($1, NOW() + INTERVAL '1 hour') RETURNING id", before.ID).Scan(&sessionID); err != nil { t.Fatal(err) } zc := &usersyncer.ZitadelWrapper{ Client: &zitadelclient.Client{UserServiceClient: naisUserService{list: func(*zitadeluser.ListUsersRequest) (*zitadeluser.ListUsersResponse, error) { - return &zitadeluser.ListUsersResponse{Result: []*zitadeluser.User{naisUser("new-identity", before.Email, "New")}}, nil + return &zitadeluser.ListUsersResponse{Result: []*zitadeluser.User{naisUser("new-zitadel-id", "EXISTING@nais.io", before.Name)}}, nil }}}, NaisOrganizationID: "nais-org", } - svc, err := admindirectoryv1.NewService(ctx, option.WithHTTPClient(test.NewTestHttpClient(func(*http.Request) *http.Response { - return test.Response("200 OK", `{"users":[]}`) - }))) + svc, err := admindirectoryv1.NewService(ctx, option.WithHTTPClient(&http.Client{Transport: directoryTransport(func(*http.Request) *http.Response { + return test.Response("200 OK", `{"users":[],"members":[]}`) + })})) if err != nil { t.Fatal(err) } - if err := usersyncer.New(pool, adminGroupPrefix, domain, zc, svc, log).Sync(ctx); err == nil { - t.Fatal("expected conflicting email to abort sync, not adopt the existing identity") + if err := usersyncer.New(pool, adminGroupPrefix, domain, zc, svc, log).Sync(ctx); err != nil { + t.Fatal(err) } users, err := q.List(ctx) - if err != nil || len(users) != 1 || users[0].ID != before.ID || users[0].ExternalID != before.ExternalID || !users[0].Admin { - t.Fatalf("conflicting identity was changed: %v, %v", users, err) + if err != nil || len(users) != 1 || users[0].ID != before.ID || users[0].ExternalID != "new-zitadel-id" || users[0].Email != before.Email || users[0].Admin { + t.Fatalf("expected existing viewer to be updated in place: %v, %v", users, err) + } + roles, err := q.ListRoles(ctx) + if err != nil || len(roles) != 0 { + t.Fatalf("expected no extra roles: %v, %v", roles, err) + } + var sessionUserID uuid.UUID + if err := pool.QueryRow(ctx, "SELECT user_id FROM sessions WHERE id = $1", sessionID).Scan(&sessionUserID); err != nil || sessionUserID != before.ID { + t.Fatalf("existing session was not preserved: %v, %v", sessionUserID, err) } }) - - for _, tc := range []struct { - name string - remoteName string - absent bool - }{ - {name: "unchanged nais.io display name", remoteName: "Nais viewer"}, - {name: "changed nais.io display name", remoteName: "Renamed nais.io viewer"}, - {name: "nais.io identity absent from current snapshot", absent: true}, - } { - t.Run("Google email collision with "+tc.name, func(t *testing.T) { - pool := getConnection(ctx, t, container, dsn, log) - q := usersyncsql.New(pool) - // Organization membership, not the email domain, determines the - // source. This could also be a previously renamed nais.io account. - before, err := q.Create(ctx, usersyncsql.CreateParams{ - Name: "Nais viewer", Email: "shared@example.com", ExternalID: "nais-subject", - }) - if err != nil { - t.Fatal(err) - } - var sessionID uuid.UUID - if err := pool.QueryRow(ctx, "INSERT INTO sessions (user_id, expires) VALUES ($1, NOW() + INTERVAL '1 hour') RETURNING id", before.ID).Scan(&sessionID); err != nil { - t.Fatal(err) - } - zc := &usersyncer.ZitadelWrapper{ - Client: &zitadelclient.Client{UserServiceClient: naisUserService{list: func(*zitadeluser.ListUsersRequest) (*zitadeluser.ListUsersResponse, error) { - var users []*zitadeluser.User - if !tc.absent { - users = append(users, naisUser(before.ExternalID, before.Email, tc.remoteName)) - } - return &zitadeluser.ListUsersResponse{Result: users}, nil - }}}, - NaisOrganizationID: "nais-org", - } - httpClient := &http.Client{Transport: directoryTransport(func(req *http.Request) *http.Response { - if req.URL.Path == "/admin/directory/v1/users" { - return test.Response("200 OK", `{"users":[{"id":"tenant-admin","primaryEmail":"shared@example.com","name":{"fullName":"Tenant admin"}}]}`) - } - return test.Response("200 OK", `{"members":[{"id":"tenant-admin","email":"shared@example.com","type":"USER","status":"ACTIVE"}]}`) - })} - svc, err := admindirectoryv1.NewService(ctx, option.WithHTTPClient(httpClient)) - if err != nil { - t.Fatal(err) - } - if err := usersyncer.New(pool, adminGroupPrefix, domain, zc, svc, log).Sync(ctx); err == nil { - t.Error("expected conflicting Google identity to abort sync") - } - users, err := q.List(ctx) - if err != nil || len(users) != 1 { - t.Fatalf("unexpected users after collision: %v, %v", users, err) - } - got := users[0] - if got.ID != before.ID || got.ExternalID != before.ExternalID || got.Name != before.Name || got.Email != before.Email || got.Admin { - t.Errorf("Google identity adopted or changed the nais.io viewer: %+v", got) - } - var sessionUserID uuid.UUID - var sessionIsAdmin bool - if err := pool.QueryRow(ctx, "SELECT u.id, u.admin FROM sessions s JOIN users u ON u.id = s.user_id WHERE s.id = $1", sessionID).Scan(&sessionUserID, &sessionIsAdmin); err != nil { - t.Fatal(err) - } - if sessionUserID != before.ID || sessionIsAdmin { - t.Errorf("existing nais.io session received tenant admin privileges: user=%s admin=%v", sessionUserID, sessionIsAdmin) - } - }) - } } diff --git a/internal/usersync/usersyncer/nais_test.go b/internal/usersync/usersyncer/nais_test.go index 499c1c858..08e6c8c4d 100644 --- a/internal/usersync/usersyncer/nais_test.go +++ b/internal/usersync/usersyncer/nais_test.go @@ -52,8 +52,10 @@ func TestActiveHumanUsers(t *testing.T) { } inactive := activeUser("inactive", "inactive@nais.io") inactive.State = zitadeluser.UserState_USER_STATE_INACTIVE + last := activeUser("last", "LAST@nais.io") + last.GetHuman().Profile = &zitadeluser.HumanProfile{DisplayName: new("Last user")} return &zitadeluser.ListUsersResponse{Result: []*zitadeluser.User{ - activeUser("last", "last@nais.io"), inactive, + last, inactive, {UserId: "machine", State: zitadeluser.UserState_USER_STATE_ACTIVE, Type: &zitadeluser.User_Machine{Machine: &zitadeluser.MachineUser{}}}, }}, nil }, @@ -65,6 +67,10 @@ func TestActiveHumanUsers(t *testing.T) { if len(users) != 251 || len(offsets) != 2 || offsets[0] != 0 || offsets[1] != 250 { t.Fatalf("got %d users, offsets %v", len(users), offsets) } + last := users[len(users)-1] + if last.ID != "last" || last.Email != "last@nais.io" || last.Name.FullName != "Last user" || last.isTenantUser { + t.Fatalf("unexpected mapped nais.io user: %+v", last) + } } func TestActiveHumanUsersDiscardsPartialSnapshot(t *testing.T) { diff --git a/internal/usersync/usersyncer/usersyncer.go b/internal/usersync/usersyncer/usersyncer.go index 65ba36f62..4eaff4ef0 100644 --- a/internal/usersync/usersyncer/usersyncer.go +++ b/internal/usersync/usersyncer/usersyncer.go @@ -45,10 +45,11 @@ type userMap struct { byEmail map[string]*usersyncsql.User } -type googleUser struct { - ID string - Email string - Name admindirectoryv1.UserName +type sourceUser struct { + ID string + Email string + Name admindirectoryv1.UserName + isTenantUser bool } func New(pool *pgxpool.Pool, adminGroupPrefix, tenantDomain string, zitadelWrapper *ZitadelWrapper, service *admindirectoryv1.Service, log logrus.FieldLogger) *Usersynchronizer { @@ -97,7 +98,7 @@ func (s *Usersynchronizer) Sync(ctx context.Context) error { return fmt.Errorf("get users from Google Directory: %w", err) } - var naisUsers []*zitadelgrpcuser.User + var naisUsers []*sourceUser if s.zitadelClient != nil && s.zitadelClient.NaisOrganizationID != "" { naisUsers, err = s.zitadelClient.activeHumanUsers(ctx, s.zitadelClient.NaisOrganizationID) if err != nil { @@ -132,8 +133,8 @@ func (s *Usersynchronizer) Sync(ctx context.Context) error { } googleUserMap := make(map[string]*usersyncsql.User) - for _, gu := range googleUsers { - user, err := getOrCreateUserFromGoogleUser(ctx, querier, gu, users, s.log) + for _, gu := range append(googleUsers, naisUsers...) { + user, err := getOrCreateUser(ctx, querier, gu, users, s.log) if err != nil { return fmt.Errorf("get or create user %q: %w", gu.Email, err) } @@ -160,16 +161,14 @@ func (s *Usersynchronizer) Sync(ctx context.Context) error { } } - googleUserMap[gu.ID] = user + if gu.isTenantUser { + googleUserMap[gu.ID] = user + } - // remove user from map to keep track of users that no longer exist in the Google Directory + // Keep track of users absent from both source snapshots. delete(users.byID, user.ID) } - if err := syncNaisUsers(ctx, querier, naisUsers, users, s.log); err != nil { - return fmt.Errorf("sync nais.io users: %w", err) - } - if err := deleteUnknownUsers(ctx, querier, users.byID, s.log); err != nil { return err } @@ -181,7 +180,7 @@ func (s *Usersynchronizer) Sync(ctx context.Context) error { return tx.Commit(ctx) } -func (s *Usersynchronizer) zitadelUserSync(ctx context.Context, googleUsers []*googleUser) { +func (s *Usersynchronizer) zitadelUserSync(ctx context.Context, googleUsers []*sourceUser) { start := time.Now() defer func() { s.log.WithField("duration", time.Since(start).String()).Debugf("Zitadel user sync done") @@ -457,7 +456,7 @@ func getAdminGroupMembers(ctx context.Context, membersService *admindirectoryv1. } // userIsOutdated checks if a user needs to get its name or its email address updated. -func userIsOutdated(user *usersyncsql.User, gu *googleUser) bool { +func userIsOutdated(user *usersyncsql.User, gu *sourceUser) bool { if user.Name != gu.Name.FullName { return true } @@ -473,30 +472,30 @@ func userIsOutdated(user *usersyncsql.User, gu *googleUser) bool { return false } -// getOrCreateUserFromGoogleUser will return a user for a Google user, creating it first if needed. -func getOrCreateUserFromGoogleUser(ctx context.Context, querier usersyncsql.Querier, googleUser *googleUser, existingUsers *userMap, log logrus.FieldLogger) (*usersyncsql.User, error) { - if existingUser, exists := existingUsers.byExternalID[googleUser.ID]; exists { +// getOrCreateUser matches globally unique email addresses across user sources. +func getOrCreateUser(ctx context.Context, querier usersyncsql.Querier, remote *sourceUser, existingUsers *userMap, log logrus.FieldLogger) (*usersyncsql.User, error) { + if existingUser, exists := existingUsers.byEmail[remote.Email]; exists { return existingUser, nil } - if existingUser, exists := existingUsers.byEmail[googleUser.Email]; exists { - // Email addresses can be reused by another identity, including a nais.io - // user absent from the current snapshot. Reusing its UUID would transfer - // existing sessions and roles to the Google identity (or vice versa). - return nil, fmt.Errorf("email %q already belongs to external ID %q; refusing to replace it with %q", googleUser.Email, existingUser.ExternalID, googleUser.ID) + // Preserve the existing support for email changes without losing roles or sessions. + if existingUser, exists := existingUsers.byExternalID[remote.ID]; exists { + return existingUser, nil } createdUser, err := querier.Create(ctx, usersyncsql.CreateParams{ - Name: googleUser.Name.FullName, - Email: googleUser.Email, - ExternalID: googleUser.ID, + Name: remote.Name.FullName, + Email: remote.Email, + ExternalID: remote.ID, }) if err != nil { return nil, err } - if err := AssignDefaultPermissionsToUser(ctx, querier, createdUser.ID); err != nil { - return nil, err + if remote.isTenantUser { + if err := AssignDefaultPermissionsToUser(ctx, querier, createdUser.ID); err != nil { + return nil, err + } } if err := querier.CreateLogEntry(ctx, usersyncsql.CreateLogEntryParams{ @@ -512,15 +511,16 @@ func getOrCreateUserFromGoogleUser(ctx context.Context, querier usersyncsql.Quer } // getGoogleUsers fetches all users from the Google Directory. -func getGoogleUsers(ctx context.Context, svc *admindirectoryv1.UsersService, tenantDomain string, log logrus.FieldLogger) ([]*googleUser, error) { - users := make([]*googleUser, 0) +func getGoogleUsers(ctx context.Context, svc *admindirectoryv1.UsersService, tenantDomain string, log logrus.FieldLogger) ([]*sourceUser, error) { + users := make([]*sourceUser, 0) callback := func(fragments *admindirectoryv1.Users) error { log.WithField("num", len(fragments.Users)).Debugf("fetched batch of users from from Google Directory") for _, user := range fragments.Users { - users = append(users, &googleUser{ - ID: user.Id, - Email: strings.ToLower(user.PrimaryEmail), - Name: *user.Name, + users = append(users, &sourceUser{ + ID: user.Id, + Email: strings.ToLower(user.PrimaryEmail), + Name: *user.Name, + isTenantUser: true, }) } return nil From fe5311f427b3a5bfff9273cd26338db63c6c4365 Mon Sep 17 00:00:00 2001 From: Frode Sundby Date: Wed, 21 Oct 2026 15:40:37 +0200 Subject: [PATCH 4/5] fix: lowercase zitadel user sync error message --- internal/usersync/usersyncer/nais.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/usersync/usersyncer/nais.go b/internal/usersync/usersyncer/nais.go index 72313df4c..c8ed45949 100644 --- a/internal/usersync/usersyncer/nais.go +++ b/internal/usersync/usersyncer/nais.go @@ -33,7 +33,7 @@ func (z *ZitadelWrapper) activeHumanUsers(ctx context.Context, organizationID st continue } if u.GetUserId() == "" || u.GetHuman().GetEmail().GetEmail() == "" { - return nil, fmt.Errorf("Zitadel user is missing ID or email") + return nil, fmt.Errorf("zitadel user is missing ID or email") } profile := u.GetHuman().GetProfile() users = append(users, &sourceUser{ From f02000e14ac849bbf19914338822ef8b2698810e Mon Sep 17 00:00:00 2001 From: Frode Sundby Date: Wed, 21 Oct 2026 15:56:56 +0200 Subject: [PATCH 5/5] feat: accept nais.io organization in CLI JWT authentication --- docs/nais_io_users.md | 18 ++- internal/auth/middleware/jwt.go | 51 ++++--- .../auth/middleware/jwt_integration_test.go | 128 ++++++++++++++++++ internal/auth/middleware/jwt_test.go | 116 ++++++++++++++-- internal/cmd/api/api.go | 6 +- internal/cmd/api/config.go | 2 +- 6 files changed, 282 insertions(+), 39 deletions(-) create mode 100644 internal/auth/middleware/jwt_integration_test.go diff --git a/docs/nais_io_users.md b/docs/nais_io_users.md index a6df8efbd..8214a1c81 100644 --- a/docs/nais_io_users.md +++ b/docs/nais_io_users.md @@ -8,8 +8,8 @@ The machine user configured by `ZITADEL_KEY` needs read access to users in the nais.io organization, in addition to its existing tenant user-management access. Terraform must also grant the nais.io organization access to each tenant's Console project. These permissions must be deployed before enabling the setting. -CLI support, including a shared email-based token lookup for tenant and nais.io -users, is handled separately. This change does not modify JWT authentication. +The API also accepts CLI JWTs from the configured nais.io organization. Removing +the CLI's login restriction and adding tenant selection are handled separately. ## User identity @@ -41,7 +41,13 @@ Workspace alone is not sufficient: deactivation must also reach Zitadel. Console login no longer pins the flow to the tenant organization when nais.io login is enabled. Users must still authenticate through the configured OIDC -provider and exist in the local API database. The tenant CLI's existing issuer, -audience, organization and subject validation is unchanged. Its access token does -not currently include email; supporting nais.io CLI login will require a shared -token-format change for both tenant and nais.io users. +provider and exist in the local API database. + +CLI JWT authentication uses the same validation and user lookup for tenant and +nais.io users: the signature, issuer, audience and expiry must be valid, the +resource-owner organization must be either the tenant organization or the +configured nais.io organization, and the subject must match a synced user's +`external_id`. With no nais.io organization configured, only the tenant +organization is accepted. No token-format change is needed for this support. +Moving CLI authentication to email lookup later would require including email in +access tokens for both tenant and nais.io users. diff --git a/internal/auth/middleware/jwt.go b/internal/auth/middleware/jwt.go index b8f20dcaf..4186cfa19 100644 --- a/internal/auth/middleware/jwt.go +++ b/internal/auth/middleware/jwt.go @@ -5,6 +5,7 @@ import ( "errors" "fmt" "net/http" + "slices" "time" "github.com/lestrrat-go/httprc/v3" @@ -20,13 +21,13 @@ import ( const acceptableClockSkew = 3 * time.Second type jwtAuth struct { - issuer string - audience string - zitadelOrgID string - jwksURL string - jwksCache *jwk.Cache - log logrus.FieldLogger - now clock + issuer string + audience string + zitadelOrgIDs []string + jwksURL string + jwksCache *jwk.Cache + log logrus.FieldLogger + now clock } type clock func() time.Time @@ -69,7 +70,16 @@ func (j *jwtAuth) validate(ctx context.Context, token string) (jwt.Token, error) jwt.WithAcceptableSkew(acceptableClockSkew), jwt.WithIssuer(j.issuer), jwt.WithAudience(j.audience), - jwt.WithClaimValue("urn:zitadel:iam:user:resourceowner:id", j.zitadelOrgID), + jwt.WithValidator(jwt.ValidatorFunc(func(_ context.Context, token jwt.Token) error { + var orgID string + if err := token.Get("urn:zitadel:iam:user:resourceowner:id", &orgID); err != nil { + return fmt.Errorf("get token organization: %w", err) + } + if orgID == "" || !slices.Contains(j.zitadelOrgIDs, orgID) { + return errors.New("token organization is not allowed") + } + return nil + })), jwt.WithClock(j.now), ) } @@ -116,15 +126,20 @@ func (j *jwtAuth) handler(next http.Handler) http.Handler { return http.HandlerFunc(fn) } -func JWTAuthentication(ctx context.Context, issuer, audience, zitadelOrgID string, log logrus.FieldLogger) (func(next http.Handler) http.Handler, error) { +func JWTAuthentication(ctx context.Context, issuer, audience string, zitadelOrgIDs []string, log logrus.FieldLogger) (func(next http.Handler) http.Handler, error) { if issuer == "" { return nil, errors.New("issuer is required") } if audience == "" { return nil, errors.New("audience is required") } - if zitadelOrgID == "" { - return nil, errors.New("zitadelOrgID is required") + if len(zitadelOrgIDs) == 0 { + return nil, errors.New("at least one zitadel organization ID is required") + } + for _, orgID := range zitadelOrgIDs { + if orgID == "" { + return nil, errors.New("zitadel organization ID cannot be empty") + } } client, err := client.Discover(ctx, issuer, http.DefaultClient) @@ -143,13 +158,13 @@ func JWTAuthentication(ctx context.Context, issuer, audience, zitadelOrgID strin } auth := jwtAuth{ - jwksURL: client.JwksURI, - jwksCache: cache, - issuer: issuer, - audience: audience, - zitadelOrgID: zitadelOrgID, - log: log, - now: time.Now, + jwksURL: client.JwksURI, + jwksCache: cache, + issuer: issuer, + audience: audience, + zitadelOrgIDs: zitadelOrgIDs, + log: log, + now: time.Now, } return auth.handler, nil diff --git a/internal/auth/middleware/jwt_integration_test.go b/internal/auth/middleware/jwt_integration_test.go new file mode 100644 index 000000000..298dfcbf5 --- /dev/null +++ b/internal/auth/middleware/jwt_integration_test.go @@ -0,0 +1,128 @@ +//go:build integration_test + +package middleware + +import ( + "net/http" + "net/http/httptest" + "testing" + "time" + + "github.com/google/uuid" + "github.com/lestrrat-go/httprc/v3" + "github.com/lestrrat-go/jwx/v3/jwk" + "github.com/nais/api/internal/auth/authz" + "github.com/nais/api/internal/database" + "github.com/nais/api/internal/user" + "github.com/nais/api/internal/usersync/usersyncsql" + logrustest "github.com/sirupsen/logrus/hooks/test" + "github.com/testcontainers/testcontainers-go" + "github.com/testcontainers/testcontainers-go/modules/postgres" +) + +func TestJWTAuthenticationUsers(t *testing.T) { + ctx := t.Context() + log, _ := logrustest.NewNullLogger() + container, err := postgres.Run(ctx, "docker.io/postgres:16-alpine", + postgres.WithDatabase("test"), postgres.WithUsername("test"), postgres.WithPassword("test"), + postgres.BasicWaitStrategies()) + if err != nil { + t.Fatal(err) + } + testcontainers.CleanupContainer(t, container) + dsn, err := container.ConnectionString(ctx, "sslmode=disable") + if err != nil { + t.Fatal(err) + } + pool, err := database.NewPool(ctx, dsn, log, true) + if err != nil { + t.Fatal(err) + } + t.Cleanup(pool.Close) + q := usersyncsql.New(pool) + tenant, err := q.Create(ctx, usersyncsql.CreateParams{Name: "Tenant user", Email: "user@nav.no", ExternalID: "tenant-subject"}) + if err != nil { + t.Fatal(err) + } + if err := q.AssignGlobalRole(ctx, usersyncsql.AssignGlobalRoleParams{UserID: tenant.ID, RoleName: "Team creator"}); err != nil { + t.Fatal(err) + } + nais, err := q.Create(ctx, usersyncsql.CreateParams{Name: "Nais viewer", Email: "user@nais.io", ExternalID: "nais-subject"}) + if err != nil { + t.Fatal(err) + } + + const issuer = "https://auth.nais.io" + keys, err := newJwkSet("key") + if err != nil { + t.Fatal(err) + } + cache, err := jwk.NewCache(ctx, httprc.NewClient(httprc.WithHTTPClient(mockJWKSetClient(keys)))) + if err != nil { + t.Fatal(err) + } + if err := cache.Register(ctx, issuer); err != nil { + t.Fatal(err) + } + now := time.Now() + for _, tc := range []struct { + name string + orgID string + subject string + tenantOnly bool + wantUserID uuid.UUID + wantRoles int + }{ + {name: "tenant user", orgID: "tenant-org", subject: tenant.ExternalID, wantUserID: tenant.ID, wantRoles: 1}, + {name: "nais.io viewer", orgID: "nais-org", subject: nais.ExternalID, wantUserID: nais.ID}, + {name: "nais.io disabled", orgID: "nais-org", subject: nais.ExternalID, tenantOnly: true}, + {name: "unsynced nais.io user", orgID: "nais-org", subject: "unknown-subject"}, + {name: "unrelated organization", orgID: "other-org", subject: nais.ExternalID}, + {name: "missing subject", orgID: "nais-org"}, + } { + t.Run(tc.name, func(t *testing.T) { + ctx := database.NewLoaderContext(t.Context(), pool) + ctx = user.NewLoaderContext(ctx, pool) + ctx = authz.NewLoaderContext(ctx, pool) + orgIDs := []string{"tenant-org", "nais-org"} + if tc.tenantOnly { + orgIDs = orgIDs[:1] + } + mw := jwtAuth{ + issuer: issuer, audience: "cli", zitadelOrgIDs: orgIDs, + jwksURL: issuer, jwksCache: cache, log: log, + now: func() time.Time { return now }, + } + // Like the current CLI access token, this token has no email claim. + tok := token(now, time.Minute) + tok.Set("iss", issuer) + tok.Set("aud", "cli") + tok.Set("urn:zitadel:iam:user:resourceowner:id", tc.orgID) + if tc.subject != "" { + tok.Set("sub", tc.subject) + } + signed, err := tok.sign(keys) + if err != nil { + t.Fatal(err) + } + req := httptest.NewRequest(http.MethodPost, "/graphql", nil).WithContext(ctx) + req.Header.Set("Authorization", "Bearer "+signed) + next := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + actor := authz.ActorFromContext(r.Context()) + if tc.wantUserID == uuid.Nil { + if actor != nil { + t.Fatalf("unexpected authenticated actor: %v", actor) + } + return + } + if actor == nil || actor.User.GetID() != tc.wantUserID { + t.Fatalf("expected user %s, got %v", tc.wantUserID, actor) + } + if actor.User.IsAdmin() || len(actor.Roles) != tc.wantRoles { + t.Fatalf("unexpected privileges: admin=%v roles=%v", actor.User.IsAdmin(), actor.Roles) + } + }) + mw.handler(next).ServeHTTP(httptest.NewRecorder(), req) + }) + } +} diff --git a/internal/auth/middleware/jwt_test.go b/internal/auth/middleware/jwt_test.go index 160a376cc..f7e6edd83 100644 --- a/internal/auth/middleware/jwt_test.go +++ b/internal/auth/middleware/jwt_test.go @@ -112,12 +112,12 @@ func TestJWTAuthorized(t *testing.T) { cache.Register(t.Context(), test.expectedIssuer) mw := jwtAuth{ - issuer: test.expectedIssuer, - audience: test.expectedAudience, - zitadelOrgID: test.expectedZitadelOrgID, - jwksURL: test.expectedIssuer, - jwksCache: cache, - log: log, + issuer: test.expectedIssuer, + audience: test.expectedAudience, + zitadelOrgIDs: []string{test.expectedZitadelOrgID}, + jwksURL: test.expectedIssuer, + jwksCache: cache, + log: log, now: func() time.Time { return defaultTime(test.expectedTime) }, @@ -174,13 +174,13 @@ func TestJWTAuthorizedSkipsUnrelatedIssuersBeforeJWKSLookup(t *testing.T) { } mw := jwtAuth{ - issuer: issuer, - audience: "aud", - zitadelOrgID: "org-id", - jwksURL: issuer, - jwksCache: cache, - log: logrus.New(), - now: func() time.Time { return now }, + issuer: issuer, + audience: "aud", + zitadelOrgIDs: []string{"org-id"}, + jwksURL: issuer, + jwksCache: cache, + log: logrus.New(), + now: func() time.Time { return now }, } signed := func(set jwk.Set, iss string) string { @@ -223,6 +223,96 @@ func TestJWTAuthorizedSkipsUnrelatedIssuersBeforeJWKSLookup(t *testing.T) { } } +func TestJWTAuthorizedWithMultipleOrganizations(t *testing.T) { + const issuer = "https://auth.nais.io" + now := time.Date(2026, 10, 21, 12, 0, 0, 0, time.UTC) + keys, err := newJwkSet("key") + if err != nil { + t.Fatal(err) + } + foreignKeys, err := newJwkSet("key") + if err != nil { + t.Fatal(err) + } + cache, err := jwk.NewCache(t.Context(), httprc.NewClient(httprc.WithHTTPClient(mockJWKSetClient(keys)))) + if err != nil { + t.Fatal(err) + } + if err := cache.Register(t.Context(), issuer); err != nil { + t.Fatal(err) + } + for _, tc := range []struct { + name string + organization any + tenantOnly bool + wrongIssuer bool + wrongAudience bool + expired bool + wrongKey bool + wantError bool + }{ + {name: "tenant organization", organization: "tenant-org"}, + {name: "nais.io organization", organization: "nais-org"}, + {name: "nais.io organization disabled", organization: "nais-org", tenantOnly: true, wantError: true}, + {name: "unrelated organization", organization: "other-org", wantError: true}, + {name: "missing organization", wantError: true}, + {name: "empty organization", organization: "", wantError: true}, + {name: "non-string organization", organization: []string{"nais-org"}, wantError: true}, + {name: "wrong issuer", organization: "nais-org", wrongIssuer: true, wantError: true}, + {name: "wrong audience", organization: "nais-org", wrongAudience: true, wantError: true}, + {name: "expired token", organization: "nais-org", expired: true, wantError: true}, + {name: "wrong signature", organization: "nais-org", wrongKey: true, wantError: true}, + } { + t.Run(tc.name, func(t *testing.T) { + orgIDs := []string{"tenant-org", "nais-org"} + if tc.tenantOnly { + orgIDs = orgIDs[:1] + } + mw := jwtAuth{ + issuer: issuer, audience: "cli", zitadelOrgIDs: orgIDs, + jwksURL: issuer, jwksCache: cache, log: logrus.New(), + now: func() time.Time { return now }, + } + issuedAt := now + if tc.expired { + issuedAt = now.Add(-time.Hour) + } + tok := token(issuedAt, time.Minute) + tokenIssuer, audience := issuer, "cli" + if tc.wrongIssuer { + tokenIssuer = "https://other-issuer.example.com" + } + if tc.wrongAudience { + audience = "other-client" + } + tok.Set("iss", tokenIssuer) + tok.Set("aud", audience) + if tc.organization != nil { + tok.Set("urn:zitadel:iam:user:resourceowner:id", tc.organization) + } + signingKeys := keys + if tc.wrongKey { + signingKeys = foreignKeys + } + signed, err := tok.sign(signingKeys) + if err != nil { + t.Fatal(err) + } + if _, err := mw.validate(t.Context(), signed); (err != nil) != tc.wantError { + t.Fatalf("validate error = %v, want error = %v", err, tc.wantError) + } + }) + } +} + +func TestJWTAuthenticationRejectsEmptyOrganizations(t *testing.T) { + for _, orgIDs := range [][]string{nil, {}, {""}, {"tenant-org", ""}, {"", "nais-org"}} { + if _, err := JWTAuthentication(t.Context(), "https://auth.nais.io", "cli", orgIDs, logrus.New()); err == nil { + t.Errorf("expected invalid organization list %v to be rejected", orgIDs) + } + } +} + type RoundTripFn func(req *http.Request) *http.Response func (f RoundTripFn) RoundTrip(req *http.Request) (*http.Response, error) { return f(req), nil } diff --git a/internal/cmd/api/api.go b/internal/cmd/api/api.go index e33861cea..4d6411f71 100644 --- a/internal/cmd/api/api.go +++ b/internal/cmd/api/api.go @@ -278,7 +278,11 @@ func run(ctx context.Context, cfg *Config, log logrus.FieldLogger) error { var jwtMiddleware func(next http.Handler) http.Handler if !cfg.JWT.SkipMiddleware { - jwtMiddleware, err = middleware.JWTAuthentication(ctx, cfg.JWT.Issuer, cfg.JWT.Audience, cfg.Zitadel.OrganizationID, log.WithField("subsystem", "jwt")) + orgIDs := []string{cfg.Zitadel.OrganizationID} + if cfg.Zitadel.NaisOrganizationID != "" { + orgIDs = append(orgIDs, cfg.Zitadel.NaisOrganizationID) + } + jwtMiddleware, err = middleware.JWTAuthentication(ctx, cfg.JWT.Issuer, cfg.JWT.Audience, orgIDs, log.WithField("subsystem", "jwt")) if err != nil { return fmt.Errorf("failed to create JWT authentication middleware: %w", err) } diff --git a/internal/cmd/api/config.go b/internal/cmd/api/config.go index ad87dcc28..d9dbee180 100644 --- a/internal/cmd/api/config.go +++ b/internal/cmd/api/config.go @@ -131,7 +131,7 @@ type zitadelConfig struct { // OrganizationID is the ID of the organization to use for the Zitadel API OrganizationID string `env:"ZITADEL_ORGANIZATION_ID"` - // NaisOrganizationID enables syncing active nais.io users from this Zitadel organization. + // NaisOrganizationID enables syncing and authenticating nais.io users from this Zitadel organization. // The usersync machine user must have read access to its users. NaisOrganizationID string `env:"ZITADEL_NAIS_ORGANIZATION_ID"` }