From cea0ef340d0d183ee56a2c338e7df198405ff0a5 Mon Sep 17 00:00:00 2001 From: Kristoffer Dalby Date: Sat, 17 Oct 2026 10:21:48 +0000 Subject: [PATCH] state, oidc: persist user changes and refresh nodes and clients Write cleared user columns, discard OIDC email the provider no longer vouches for, refresh NodeStore users, and notify clients from the callback. Fixes #3471 --- docs/ref/oidc.md | 9 +- hscontrol/oidc.go | 20 +- hscontrol/oidc_test.go | 80 +++++++- hscontrol/servertest/issues_test.go | 31 +++ hscontrol/state/persist_test.go | 300 ++++++++++++++++++++++++++++ hscontrol/state/state.go | 68 ++++++- hscontrol/types/users.go | 2 + hscontrol/types/users_test.go | 28 +++ 8 files changed, 522 insertions(+), 16 deletions(-) diff --git a/docs/ref/oidc.md b/docs/ref/oidc.md index 3d2e8764a..c3013dc6c 100644 --- a/docs/ref/oidc.md +++ b/docs/ref/oidc.md @@ -127,12 +127,13 @@ are configured, a user needs to pass all of them. ### Control email verification -Headscale uses the `email` claim from the identity provider to synchronize the email address to its user profile. By -default, a user's email address is only synchronized when the identity provider reports the email address as verified -via the `email_verified: true` claim. +Headscale synchronizes the `email` claim from the identity provider to its user profile on each OIDC login. By default, +the profile uses the email only when the latest login reports `email_verified: true`. An absent, unverified, or +malformed email clears the previously stored address. Unverified emails may be allowed in case an identity provider does not send the `email_verified` claim or email -verification is not required. In that case, a user's email address is always synchronized to the user profile. +verification is not required. With `email_verified_required: false`, the latest login's email is stored without +requiring verification. An absent or malformed email still clears the previously stored address. ```yaml hl_lines="5" oidc: diff --git a/hscontrol/oidc.go b/hscontrol/oidc.go index e79b9e4d7..e31265899 100644 --- a/hscontrol/oidc.go +++ b/hscontrol/oidc.go @@ -19,6 +19,7 @@ import ( "github.com/juanfont/headscale/hscontrol/db" "github.com/juanfont/headscale/hscontrol/templates" "github.com/juanfont/headscale/hscontrol/types" + "github.com/juanfont/headscale/hscontrol/types/change" "github.com/juanfont/headscale/hscontrol/util" "github.com/rs/zerolog/log" "golang.org/x/oauth2" @@ -377,7 +378,7 @@ func (a *AuthProviderOIDC) OIDCCallbackHandler( return } - user, err := a.createOrUpdateUserFromClaim(&claims) + user, userChange, err := a.createOrUpdateUserFromClaim(&claims) if err != nil { httpUserError(writer, NewHTTPError( http.StatusInternalServerError, @@ -388,6 +389,8 @@ func (a *AuthProviderOIDC) OIDCCallbackHandler( return } + a.h.Change(userChange) + // If this is a registration flow, render the confirmation // interstitial instead of finalising the registration immediately. // Without an explicit user click, a single GET to @@ -713,16 +716,17 @@ func (a *AuthProviderOIDC) getAuthInfoFromState(state string) *AuthInfo { func (a *AuthProviderOIDC) createOrUpdateUserFromClaim( claims *types.OIDCClaims, -) (*types.User, error) { +) (*types.User, change.Change, error) { var ( user *types.User err error newUser bool + c change.Change ) user, err = a.h.state.GetUserByOIDCIdentifier(claims.Identifier()) if err != nil && !errors.Is(err, db.ErrUserNotFound) { - return nil, fmt.Errorf("creating or updating user: %w", err) + return nil, change.Change{}, fmt.Errorf("creating or updating user: %w", err) } // if the user is still not found, create a new empty user. @@ -736,21 +740,21 @@ func (a *AuthProviderOIDC) createOrUpdateUserFromClaim( user.FromClaim(claims, a.cfg.EmailVerifiedRequired) if newUser { - user, _, err = a.h.state.CreateUser(*user) + user, c, err = a.h.state.CreateUser(*user) if err != nil { - return nil, fmt.Errorf("creating user: %w", err) + return nil, change.Change{}, fmt.Errorf("creating user: %w", err) } } else { - _, _, err = a.h.state.UpdateUser(types.UserID(user.ID), func(u *types.User) error { + _, c, err = a.h.state.UpdateUser(types.UserID(user.ID), func(u *types.User) error { *u = *user return nil }) if err != nil { - return nil, fmt.Errorf("updating user: %w", err) + return nil, change.Change{}, fmt.Errorf("updating user: %w", err) } } - return user, nil + return user, c, nil } // registerConfirmCSRFCookie is the cookie name used to bind the diff --git a/hscontrol/oidc_test.go b/hscontrol/oidc_test.go index 94372f9b9..32024f66a 100644 --- a/hscontrol/oidc_test.go +++ b/hscontrol/oidc_test.go @@ -926,7 +926,7 @@ func TestOIDCPendingCallbackPreservesSSHVerdict(t *testing.T) { require.True(t, ok) principal := mockoidc.DefaultUser() - user, err := a.createOrUpdateUserFromClaim(&types.OIDCClaims{ + user, _, err := a.createOrUpdateUserFromClaim(&types.OIDCClaims{ Iss: b.idp.Issuer(), Sub: principal.Subject, Username: principal.PreferredUsername, @@ -1470,6 +1470,84 @@ func TestOIDCRefusesAlteredSubject(t *testing.T) { assert.Equal(t, *before, users[0], "a refused subject must not update an existing row") } +func TestOIDCLoginClearsUnverifiedEmail(t *testing.T) { + app := createTestApp(t) + provider := &AuthProviderOIDC{ + h: app, + cfg: &types.OIDCConfig{ + EmailVerifiedRequired: true, + }, + } + claims := &types.OIDCClaims{ + Iss: "https://idp.example.com", Sub: "alice", Username: "alice", + Email: "alice@example.com", EmailVerified: true, + } + first, _, err := provider.createOrUpdateUserFromClaim(claims) + require.NoError(t, err) + require.Equal(t, "alice@example.com", first.Email) + + claims.EmailVerified = false + second, _, err := provider.createOrUpdateUserFromClaim(claims) + require.NoError(t, err) + assert.Equal(t, first.ID, second.ID) + assert.Empty(t, second.Email) + + stored, err := app.state.GetUserByID(types.UserID(first.ID)) + require.NoError(t, err) + assert.Empty(t, stored.Email) +} + +func TestOIDCCallbackDispatchesUserIdentityChange(t *testing.T) { + b := newOIDCBrowser(t) + provider, ok := b.app.authProvider.(*AuthProviderOIDC) + require.True(t, ok) + + provider.cfg.EmailVerifiedRequired = true + user, _, err := provider.createOrUpdateUserFromClaim(&types.OIDCClaims{ + Iss: b.idp.Issuer(), Sub: "identity-change", Username: "alice", + Email: "alice@old.example", EmailVerified: true, + }) + require.NoError(t, err) + + node := b.app.state.CreateRegisteredNodeForTest(user, "identity-node") + node.User = user.Clone() + b.app.state.PutNodeInStoreForTest(*node) + require.NoError(t, b.app.state.UpdatePolicyManagerNodesForTest()) + _, err = b.app.state.SetPolicy([]byte(`{ + "acls": [{"action": "accept", "src": ["alice@old.example"], "dst": ["*:*"]}] + }`)) + require.NoError(t, err) + + responses := make(chan *tailcfg.MapResponse, 8) + require.NoError(t, b.app.mapBatcher.AddNode(node.ID, responses, tailcfg.CurrentCapabilityVersion, nil)) + t.Cleanup(func() { b.app.mapBatcher.RemoveNode(node.ID, responses) }) + + initial := <-responses + require.NotEmpty(t, initial.PacketFilters["base"]) + + b.idp.QueueUser(&mockoidc.MockUser{ + Subject: "identity-change", PreferredUsername: "alice", + Email: "alice@new.example", EmailVerified: true, + }) + _, registerURL := b.pendingNode(t) + status, _, _ := b.get(t, registerURL) + require.Equal(t, http.StatusOK, status) + + stored, err := b.app.state.GetUserByID(types.UserID(user.ID)) + require.NoError(t, err) + require.Equal(t, "alice@new.example", stored.Email) + + assert.Eventually(t, func() bool { + select { + case response := <-responses: + filters, present := response.PacketFilters["base"] + return present && len(filters) == 0 + default: + return false + } + }, 3*time.Second, 20*time.Millisecond, "the connected node must receive the updated filter before registration confirmation") +} + func TestOIDCRegistrationRejectsOversizedMergedHostinfo(t *testing.T) { t.Parallel() diff --git a/hscontrol/servertest/issues_test.go b/hscontrol/servertest/issues_test.go index ee5e3e7c1..abd554a63 100644 --- a/hscontrol/servertest/issues_test.go +++ b/hscontrol/servertest/issues_test.go @@ -1239,3 +1239,34 @@ func peerByHostname(nm *netmap.NetworkMap, hostname string) (tailcfg.NodeView, b return tailcfg.NodeView{}, false } + +func TestUserProfileFollowsRename(t *testing.T) { + srv := servertest.NewServer(t) + owner := srv.CreateUser(t, "profile-original") + profileID := owner.TailscaleUserProfile().ID + peer := srv.CreateUser(t, "profile-peer") + ownerClient := servertest.NewClient(t, srv, "owner", servertest.WithUser(owner)) + peerClient := servertest.NewClient(t, srv, "peer", servertest.WithUser(peer)) + ownerClient.WaitForPeers(t, 1, 5*time.Second) + peerClient.WaitForPeers(t, 1, 5*time.Second) + + _, c, err := srv.State().RenameUser(types.UserID(owner.ID), "profile-renamed") + require.NoError(t, err) + + srv.App.Change(c) + + for _, client := range []*servertest.TestClient{ownerClient, peerClient} { + client.WaitForCondition(t, "renamed user profile", 5*time.Second, + func(nm *netmap.NetworkMap) bool { + profile, ok := nm.UserProfiles[profileID] + return ok && profile.LoginName() == "profile-renamed" + }) + } + + late := servertest.NewClient(t, srv, "late", servertest.WithUser(peer)) + late.WaitForCondition(t, "renamed user profile on join", 5*time.Second, + func(nm *netmap.NetworkMap) bool { + profile, ok := nm.UserProfiles[profileID] + return ok && profile.LoginName() == "profile-renamed" + }) +} diff --git a/hscontrol/state/persist_test.go b/hscontrol/state/persist_test.go index 7bd910b4d..57c06adbc 100644 --- a/hscontrol/state/persist_test.go +++ b/hscontrol/state/persist_test.go @@ -2475,3 +2475,303 @@ func TestReauthReplacesReportedTags(t *testing.T) { assert.Equal(t, want, row.Hostinfo.RequestTags) } } + +func TestUpdateUserClearsColumns(t *testing.T) { + _, s, nodeID := persistTestSetup(t) + t.Cleanup(func() { _ = s.Close() }) + + node, ok := s.GetNodeByID(nodeID) + require.True(t, ok) + + userID := types.UserID(node.UserID().Get()) + _, _, err := s.UpdateUser(userID, func(user *types.User) error { + user.DisplayName = "Display" + user.Email = "persist@example.com" + user.ProfilePicURL = "https://example.com/picture" + + return nil + }) + require.NoError(t, err) + _, _, err = s.RenameUser(userID, "renamed") + require.NoError(t, err) + user, err := s.GetUserByID(userID) + require.NoError(t, err) + assert.Equal(t, "Display", user.DisplayName) + assert.Equal(t, "persist@example.com", user.Email) + assert.Equal(t, "https://example.com/picture", user.ProfilePicURL) + _, _, err = s.UpdateUser(userID, func(user *types.User) error { + user.DisplayName = "" + user.Email = "" + user.ProfilePicURL = "" + + return nil + }) + require.NoError(t, err) + user, err = s.GetUserByID(userID) + require.NoError(t, err) + assert.Empty(t, user.DisplayName) + assert.Empty(t, user.Email) + assert.Empty(t, user.ProfilePicURL) + assert.Equal(t, "renamed", user.Name) +} + +func TestUpdateUserRefreshesNodeCopies(t *testing.T) { + _, s, nodeID := persistTestSetup(t) + t.Cleanup(func() { _ = s.Close() }) + + original, ok := s.GetNodeByID(nodeID) + require.True(t, ok) + + userID := types.UserID(original.UserID().Get()) + user, err := s.GetUserByID(userID) + require.NoError(t, err) + + _, ok = s.nodeStore.UpdateNode(nodeID, func(n *types.Node) { + n.AuthKey = &types.Credential{ID: 1, UserID: new(user.ID), User: user.Clone()} + }) + require.True(t, ok) + + tagged := s.nodeStore.PutNode(types.Node{ + ID: nodeID + 1, Hostname: "tagged", Tags: []string{"tag:server"}, + MachineKey: key.NewMachine().Public(), NodeKey: key.NewNode().Public(), + AuthKey: &types.Credential{ID: 2, UserID: new(user.ID), User: user.Clone()}, + }) + other, _, err := s.CreateUser(types.User{Name: "other"}) + require.NoError(t, err) + + otherNode := s.nodeStore.PutNode(types.Node{ + ID: nodeID + 2, Hostname: "other", UserID: new(other.ID), User: other.Clone(), + MachineKey: key.NewMachine().Public(), NodeKey: key.NewNode().Public(), + AuthKey: &types.Credential{ID: 3, UserID: new(other.ID), User: other.Clone()}, + }) + before, ok := s.GetNodeByID(nodeID) + require.True(t, ok) + + _, c, err := s.RenameUser(userID, "renamed-user") + require.NoError(t, err) + assert.True(t, c.IncludeSelf) + assert.True(t, c.SendAllPeers) + + updated, ok := s.GetNodeByID(nodeID) + require.True(t, ok) + assert.Equal(t, "renamed-user", updated.User().Name()) + assert.Equal(t, "renamed-user", updated.AuthKey().User().Name()) + assert.Equal(t, "persist-user", before.User().Name()) + assert.Equal(t, "persist-user", before.AuthKey().User().Name()) + + updatedTagged, ok := s.GetNodeByID(tagged.ID()) + require.True(t, ok) + assert.True(t, updatedTagged.IsTagged()) + assert.Equal(t, "renamed-user", updatedTagged.AuthKey().User().Name()) + assert.Equal(t, "persist-user", tagged.AuthKey().User().Name()) + + unchanged, ok := s.GetNodeByID(otherNode.ID()) + require.True(t, ok) + assert.Equal(t, "other", unchanged.User().Name()) + assert.Equal(t, "other", unchanged.AuthKey().User().Name()) + + snapshot := s.nodeStore.data.Load() + _, c, err = s.RenameUser(userID, "renamed-user") + require.NoError(t, err) + assert.True(t, c.IsEmpty()) + assert.Same(t, snapshot, s.nodeStore.data.Load(), "an unchanged user must not rebuild the snapshot") + + _, c, err = s.UpdateUser(userID, func(user *types.User) error { + user.Provider = "updated-provider" + return nil + }) + require.NoError(t, err) + assert.True(t, c.IsEmpty(), "a non-profile column does not need a full map update") + + updated, ok = s.GetNodeByID(nodeID) + require.True(t, ok) + assert.Equal(t, "updated-provider", updated.User().Provider()) + assert.Equal(t, "updated-provider", updated.AuthKey().User().Provider()) +} + +type blockingSetUsersPolicyManager struct { + policy.PolicyManager + + blocked atomic.Bool + reached chan struct{} + resume <-chan struct{} + last []types.User +} + +func (pm *blockingSetUsersPolicyManager) SetUsers(users []types.User) (bool, bool, error) { + if !pm.blocked.Swap(true) { + close(pm.reached) + <-pm.resume + } + + pm.last = users + + return pm.PolicyManager.SetUsers(users) +} + +func TestUpdateUserPublishesInOrder(t *testing.T) { + _, s, nodeID := persistTestSetup(t) + t.Cleanup(func() { _ = s.Close() }) + + node, ok := s.GetNodeByID(nodeID) + require.True(t, ok) + + user := node.User().AsStruct() + userID := types.UserID(user.ID) + _, ok = s.nodeStore.UpdateNode(nodeID, func(n *types.Node) { + n.AuthKey = &types.Credential{ID: 1, UserID: new(user.ID), User: user.Clone()} + }) + require.True(t, ok) + + resume := make(chan struct{}) + + releaseFirst := sync.OnceFunc(func() { close(resume) }) + defer releaseFirst() + + pm := &blockingSetUsersPolicyManager{ + PolicyManager: s.polMan, + reached: make(chan struct{}), + resume: resume, + } + s.polMan = pm + + update := func(name string) error { + _, _, err := s.UpdateUser(userID, func(u *types.User) error { + u.Name = name + u.Email = name + "@example.com" + u.DisplayName = name + " display" + u.ProfilePicURL = "https://example.com/" + name + + return nil + }) + + return err + } + + firstDone := make(chan error, 1) + go func() { firstDone <- update("first-user") }() + + // The first update pauses after its commit, inside the policy user update. + <-pm.reached + + var secondErr error + + secondDone := make(chan struct{}) + + go func() { + secondErr = update("latest-user") + + close(secondDone) + }() + + // Give the second update time to overtake the paused first one. + select { + case <-secondDone: + case <-time.After(100 * time.Millisecond): + } + + releaseFirst() + require.NoError(t, <-firstDone) + <-secondDone + require.NoError(t, secondErr) + + stored, err := s.GetUserByID(userID) + require.NoError(t, err) + assert.Equal(t, "latest-user", stored.Name) + + require.Len(t, pm.last, 1) + assert.Equal(t, stored.Name, pm.last[0].Name, "policy users must follow the database") + + updated, ok := s.GetNodeByID(nodeID) + require.True(t, ok) + assert.Equal(t, stored.Name, updated.User().Name()) + assert.Equal(t, stored.Name, updated.AuthKey().User().Name()) + assert.Equal(t, stored.TailscaleUserProfile(), updated.User().TailscaleUserProfile()) + assert.Equal(t, stored.TailscaleUserProfile(), updated.AuthKey().User().TailscaleUserProfile()) +} + +func TestUserListPublishesInOrder(t *testing.T) { + _, s, nodeID := persistTestSetup(t) + t.Cleanup(func() { _ = s.Close() }) + + node, ok := s.GetNodeByID(nodeID) + require.True(t, ok) + + resume := make(chan struct{}) + + releaseFirst := sync.OnceFunc(func() { close(resume) }) + defer releaseFirst() + + pm := &blockingSetUsersPolicyManager{ + PolicyManager: s.polMan, + reached: make(chan struct{}), + resume: resume, + } + s.polMan = pm + + createDone := make(chan error, 1) + + go func() { + _, _, err := s.CreateUser(types.User{Name: "created-user"}) + createDone <- err + }() + + // The creation pauses holding a user list read before the rename commits. + <-pm.reached + + var renameErr error + + renameDone := make(chan struct{}) + + go func() { + _, _, renameErr = s.RenameUser(types.UserID(node.User().ID()), "renamed-user") + + close(renameDone) + }() + + select { + case <-renameDone: + case <-time.After(100 * time.Millisecond): + } + + releaseFirst() + require.NoError(t, <-createDone) + <-renameDone + require.NoError(t, renameErr) + + names := make([]string, 0, len(pm.last)) + for _, user := range pm.last { + names = append(names, user.Name) + } + + assert.ElementsMatch(t, []string{"renamed-user", "created-user"}, names, + "policy users must follow the database") +} + +var errSetUsersTest = errors.New("set users failed") + +type failingSetUsersPolicyManager struct { + policy.PolicyManager +} + +func (failingSetUsersPolicyManager) SetUsers([]types.User) (bool, bool, error) { + return false, false, errSetUsersTest +} + +func TestUpdateUserRefreshesNodeCopiesWhenPolicyUpdateFails(t *testing.T) { + _, s, nodeID := persistTestSetup(t) + t.Cleanup(func() { _ = s.Close() }) + + node, ok := s.GetNodeByID(nodeID) + require.True(t, ok) + + s.polMan = failingSetUsersPolicyManager{PolicyManager: s.polMan} + + _, _, err := s.RenameUser(types.UserID(node.User().ID()), "renamed-user") + require.ErrorIs(t, err, errSetUsersTest) + + updated, ok := s.GetNodeByID(nodeID) + require.True(t, ok) + assert.Equal(t, "renamed-user", updated.User().Name(), + "the committed row and its cached copy must agree") +} diff --git a/hscontrol/state/state.go b/hscontrol/state/state.go index 7a5a8211a..29680e0f3 100644 --- a/hscontrol/state/state.go +++ b/hscontrol/state/state.go @@ -244,6 +244,13 @@ type State struct { // nothing. persistMu sync.Mutex + // userUpdateMu keeps committed user changes and cached copies in the same order. + userUpdateMu sync.Mutex + + // policyUsersMu orders user-list reads with SetUsers, so a list read + // before a commit cannot replace one read after it. + policyUsersMu sync.Mutex + // selfRefresh holds nodes whose own DNS config inputs changed (NextDNS // device metadata, given name), drained by [State.DrainSelfRefreshes]. selfRefresh []types.NodeID @@ -583,18 +590,26 @@ func (s *State) CreateUser(user types.User) (*types.User, change.Change, error) // UpdateUser modifies an existing user using the provided update function within a transaction. // Returns the updated user, change set, and any error. func (s *State) UpdateUser(userID types.UserID, updateFn func(*types.User) error) (*types.User, change.Change, error) { + s.userUpdateMu.Lock() + defer s.userUpdateMu.Unlock() + + var previous types.User + user, err := hsdb.Write(s.db.DB, func(tx *gorm.DB) (*types.User, error) { user, err := hsdb.GetUserByID(tx, userID) if err != nil { return nil, err } + previous = *user + if err := updateFn(user); err != nil { //nolint:noinlineerr return nil, err } - // Use Updates() to only update modified fields, preserving unchanged values. - err = tx.Updates(user).Error + // Select every column so cleared fields are written. Save would also + // insert the row again if a delete removed it meanwhile. + err = tx.Select("*").Updates(user).Error //nolint:unqueryvet // UPDATE, not a read if err != nil { return nil, fmt.Errorf("updating user: %w", err) } @@ -605,13 +620,57 @@ func (s *State) UpdateUser(userID types.UserID, updateFn func(*types.User) error return nil, change.Change{}, err } + var profileChanged bool + + previous.UpdatedAt = user.UpdatedAt + if previous != *user { + updates := make(map[types.NodeID]UpdateNodeFunc) + + var ownerRefreshed bool + + for _, node := range s.nodeStore.ListNodes().All() { + owner := node.User() + credential := node.AuthKey() + owns := owner.Valid() && owner.ID() == user.ID + createdKey := credential.Valid() && credential.User().Valid() && + credential.User().ID() == user.ID + + if !owns && !createdKey { + continue + } + + updates[node.ID()] = func(n *types.Node) { + // Recheck ownership after waiting in the writer queue, and replace + // shared records so earlier snapshots keep their user data. + if n.User != nil && n.User.ID == user.ID { + n.User = user.Clone() + ownerRefreshed = ownerRefreshed || !n.IsTagged() + } + + if n.AuthKey != nil && n.AuthKey.User != nil && n.AuthKey.User.ID == user.ID { + n.AuthKey = n.AuthKey.Clone() + n.AuthKey.User = user.Clone() + } + } + } + + s.nodeStore.UpdateNodes(updates) + + previousProfile := previous.TailscaleUserProfile() + profile := user.TailscaleUserProfile() + + profileChanged = ownerRefreshed && !previousProfile.Equal(&profile) + } + // Check if policy manager needs updating c, err := s.updatePolicyManagerUsers() if err != nil { return user, change.Change{}, fmt.Errorf("updating policy manager after user update: %w", err) } - // TODO(kradalby): We might want to update nodestore with the user data + if profileChanged { + c = change.UserAdded() + } return user, c, nil } @@ -3820,6 +3879,9 @@ func reloginChanged(before, after types.NodeView, now time.Time) relogin { // manager, rebuilds peer adjacency when user identity changed, and returns // a PolicyChange when clients need a refresh. func (s *State) updatePolicyManagerUsers() (change.Change, error) { + s.policyUsersMu.Lock() + defer s.policyUsersMu.Unlock() + users, err := s.ListAllUsers() if err != nil { return change.Change{}, fmt.Errorf("listing users for policy update: %w", err) diff --git a/hscontrol/types/users.go b/hscontrol/types/users.go index b8dbbc4be..1dc55732b 100644 --- a/hscontrol/types/users.go +++ b/hscontrol/types/users.go @@ -440,6 +440,8 @@ func (u *User) FromClaim(claims *OIDCClaims, emailVerifiedRequired bool) { log.Debug().Caller().Err(err).Msgf("username %s is not valid", claims.Username) } + u.Email = "" + if claims.EmailVerified || !FlexibleBoolean(emailVerifiedRequired) { _, err = mail.ParseAddress(claims.Email) if err == nil { diff --git a/hscontrol/types/users_test.go b/hscontrol/types/users_test.go index 80fb19237..05d166d9e 100644 --- a/hscontrol/types/users_test.go +++ b/hscontrol/types/users_test.go @@ -591,3 +591,31 @@ func TestOIDCClaimsIdentifierKeepsSubject(t *testing.T) { }) } } + +func TestFromClaimReplacesEmail(t *testing.T) { + for _, tt := range []struct { + name string + email string + verified FlexibleBoolean + required bool + want string + }{ + {"verified", "current@example.com", true, true, "current@example.com"}, + {"unverified", "current@example.com", false, true, ""}, + {"absent", "", false, true, ""}, + {"malformed", "not-an-address", true, true, ""}, + {"verification optional", "current@example.com", false, false, "current@example.com"}, + {"optional but absent", "", false, false, ""}, + {"optional but malformed", "not-an-address", false, false, ""}, + } { + t.Run(tt.name, func(t *testing.T) { + user := User{Name: "alice", Email: "previous@example.com"} + user.FromClaim(&OIDCClaims{ + Iss: "https://idp.example.com", Sub: "alice", + Email: tt.email, EmailVerified: tt.verified, + }, tt.required) + assert.Equal(t, tt.want, user.Email) + assert.Equal(t, "alice", user.Name) + }) + } +}