From b254b5d23c334d1d9f9c4431e5b2e4551cdc1f03 Mon Sep 17 00:00:00 2001 From: Kristoffer Dalby Date: Fri, 20 Feb 2026 09:26:54 +0000 Subject: [PATCH 1/4] hscontrol/db: add migration to clear user_id on tagged nodes Tagged nodes are owned by their tags, not a user. Previously user_id was kept as "created by" tracking, but this prevents deleting users whose nodes have all been tagged, and the ON DELETE CASCADE FK would destroy the tagged nodes. Add a migration that sets user_id = NULL on all existing tagged nodes. Subsequent commits enforce this invariant at write time. Updates #3077 --- hscontrol/db/db.go | 23 +++++++++++++++++++++++ hscontrol/db/schema.sql | 2 ++ 2 files changed, 25 insertions(+) diff --git a/hscontrol/db/db.go b/hscontrol/db/db.go index 6841f446..478614fb 100644 --- a/hscontrol/db/db.go +++ b/hscontrol/db/db.go @@ -704,6 +704,29 @@ AND auth_key_id NOT IN ( }, Rollback: func(db *gorm.DB) error { return nil }, }, + { + // Clear user_id on tagged nodes. + // Tagged nodes are owned by their tags, not a user. + // Previously user_id was kept as "created by" tracking, + // but this prevents deleting users whose nodes have been + // tagged, and the ON DELETE CASCADE FK would destroy the + // tagged nodes if the user were deleted. + // Fixes: https://github.com/juanfont/headscale/issues/3077 + ID: "202602201200-clear-tagged-node-user-id", + Migrate: func(tx *gorm.DB) error { + err := tx.Exec(` +UPDATE nodes +SET user_id = NULL +WHERE tags IS NOT NULL AND tags != '[]' AND tags != ''; + `).Error + if err != nil { + return fmt.Errorf("clearing user_id on tagged nodes: %w", err) + } + + return nil + }, + Rollback: func(db *gorm.DB) error { return nil }, + }, }, ) diff --git a/hscontrol/db/schema.sql b/hscontrol/db/schema.sql index 41e817ee..781446c0 100644 --- a/hscontrol/db/schema.sql +++ b/hscontrol/db/schema.sql @@ -79,6 +79,8 @@ CREATE TABLE nodes( ipv6 text, hostname text, given_name varchar(63), + -- user_id is NULL for tagged nodes (owned by tags, not a user). + -- Only set for user-owned nodes (no tags). user_id integer, register_method text, tags text, From 3c75b5e94b47db21f02145e0c9779f6e1c51c532 Mon Sep 17 00:00:00 2001 From: Kristoffer Dalby Date: Fri, 20 Feb 2026 09:27:11 +0000 Subject: [PATCH 2/4] hscontrol: enforce that tagged nodes never have user_id Tagged nodes are owned by their tags, not a user. Enforce this invariant at every write path: - createAndSaveNewNode: do not set UserID for tagged PreAuthKey registration; clear UserID when advertise-tags are applied during OIDC/CLI registration - SetNodeTags: clear UserID/User when tags are assigned - processReauthTags: clear UserID/User when tags are applied during re-authentication - validateNodeOwnership: reject tagged nodes with non-nil UserID - NodeStore: skip nodesByUser indexing for tagged nodes since they have no owning user - HandleNodeFromPreAuthKey: add fallback lookup for tagged PAK re-registration (tagged nodes indexed under UserID(0)); guard against nil User deref for tagged nodes in different-user check Since tagged nodes now have user_id = NULL, ListNodesByUser will not return them and DestroyUser naturally allows deleting users whose nodes have all been tagged. The ON DELETE CASCADE FK cannot reach tagged nodes through a NULL foreign key. Also tone down shouty comments throughout state.go. Fixes #3077 --- hscontrol/db/users.go | 3 +- hscontrol/state/node_store.go | 22 ++++----- hscontrol/state/state.go | 84 ++++++++++++++++++++--------------- hscontrol/state/tags.go | 24 +++++----- hscontrol/types/node.go | 6 +-- 5 files changed, 73 insertions(+), 66 deletions(-) diff --git a/hscontrol/db/users.go b/hscontrol/db/users.go index 36ea50e5..b9e6bb3f 100644 --- a/hscontrol/db/users.go +++ b/hscontrol/db/users.go @@ -48,7 +48,8 @@ func (hsdb *HSDatabase) DestroyUser(uid types.UserID) error { } // DestroyUser destroys a User. Returns error if the User does -// not exist or if there are nodes associated with it. +// not exist or if there are user-owned nodes associated with it. +// Tagged nodes have user_id = NULL so they do not block deletion. func DestroyUser(tx *gorm.DB, uid types.UserID) error { user, err := GetUserByID(tx, uid) if err != nil { diff --git a/hscontrol/state/node_store.go b/hscontrol/state/node_store.go index 1c921d6d..460808c1 100644 --- a/hscontrol/state/node_store.go +++ b/hscontrol/state/node_store.go @@ -425,7 +425,12 @@ func snapshotFromNodes(nodes map[types.NodeID]types.Node, peersFunc PeersFunc) S nodeView := n.View() userID := n.TypedUserID() - newSnap.nodesByUser[userID] = append(newSnap.nodesByUser[userID], nodeView) + // Tagged nodes are owned by their tags, not a user, + // so they are not indexed by user. + if !n.IsTagged() { + newSnap.nodesByUser[userID] = append(newSnap.nodesByUser[userID], nodeView) + } + newSnap.nodesByNodeKey[n.NodeKey] = nodeView // Build machine key index @@ -531,23 +536,12 @@ func (s *NodeStore) DebugString() string { for userID, nodes := range snapshot.nodesByUser { if len(nodes) > 0 { userName := "unknown" - taggedCount := 0 - if len(nodes) > 0 && nodes[0].Valid() { + if nodes[0].Valid() && nodes[0].User().Valid() { userName = nodes[0].User().Name() - // Count tagged nodes (which have UserID set but are owned by "tagged-devices") - for _, n := range nodes { - if n.IsTagged() { - taggedCount++ - } - } } - if taggedCount > 0 { - sb.WriteString(fmt.Sprintf(" - User %d (%s): %d nodes (%d tagged)\n", userID, userName, len(nodes), taggedCount)) - } else { - sb.WriteString(fmt.Sprintf(" - User %d (%s): %d nodes\n", userID, userName, len(nodes))) - } + sb.WriteString(fmt.Sprintf(" - User %d (%s): %d nodes\n", userID, userName, len(nodes))) } } diff --git a/hscontrol/state/state.go b/hscontrol/state/state.go index e421d5bd..0b1b7bfa 100644 --- a/hscontrol/state/state.go +++ b/hscontrol/state/state.go @@ -472,10 +472,9 @@ func (s *State) DeleteNode(node types.NodeView) (change.Change, error) { // Connect marks a node as connected and updates its primary routes in the state. func (s *State) Connect(id types.NodeID) []change.Change { - // CRITICAL FIX: Update the online status in NodeStore BEFORE creating change notification - // This ensures that when the NodeCameOnline change is distributed and processed by other nodes, - // the NodeStore already reflects the correct online status for full map generation. - // now := time.Now() + // Update online status in NodeStore before creating change notification + // so the NodeStore already reflects the correct state when other nodes + // process the NodeCameOnline change for full map generation. node, ok := s.nodeStore.UpdateNode(id, func(n *types.Node) { n.IsOnline = new(true) // n.LastSeen = ptr.To(now) @@ -488,9 +487,8 @@ func (s *State) Connect(id types.NodeID) []change.Change { log.Info().EmbedObject(node).Msg("node connected") - // Use the node's current routes for primary route update - // AllApprovedRoutes() returns only the intersection of announced AND approved routes - // We MUST use AllApprovedRoutes() to maintain the security model + // Use the node's current routes for primary route update. + // AllApprovedRoutes() returns only the intersection of announced and approved routes. routeChange := s.primaryRoutes.SetRoutes(id, node.AllApprovedRoutes()...) if routeChange { @@ -658,9 +656,8 @@ func (s *State) SetNodeExpiry(nodeID types.NodeID, expiry time.Time) (types.Node // SetNodeTags assigns tags to a node, making it a "tagged node". // Once a node is tagged, it cannot be un-tagged (only tags can be changed). -// The UserID is preserved as "created by" information. +// Setting tags clears UserID since tagged nodes are owned by their tags. func (s *State) SetNodeTags(nodeID types.NodeID, tags []string) (types.NodeView, change.Change, error) { - // CANNOT REMOVE ALL TAGS if len(tags) == 0 { return types.NodeView{}, change.Change{}, types.ErrCannotRemoveAllTags } @@ -700,7 +697,9 @@ func (s *State) SetNodeTags(nodeID types.NodeID, tags []string) (types.NodeView, // make the exact same change. n, ok := s.nodeStore.UpdateNode(nodeID, func(node *types.Node) { node.Tags = validatedTags - // UserID is preserved as "created by" - do NOT set to nil + // Tagged nodes are owned by their tags, not a user. + node.UserID = nil + node.User = nil }) if !ok { @@ -1291,17 +1290,10 @@ func (s *State) createAndSaveNewNode(params newNodeParams) (types.NodeView, erro // Assign ownership based on PreAuthKey if params.PreAuthKey != nil { if params.PreAuthKey.IsTagged() { - // TAGGED NODE - // Tags from PreAuthKey are assigned ONLY during initial authentication + // Tagged nodes are owned by their tags, not a user. + // UserID is intentionally left nil. nodeToRegister.Tags = params.PreAuthKey.Proto().GetAclTags() - // Set UserID to track "created by" (who created the PreAuthKey) - if params.PreAuthKey.UserID != nil { - nodeToRegister.UserID = params.PreAuthKey.UserID - nodeToRegister.User = params.PreAuthKey.User - } - // If PreAuthKey.UserID is nil, the node is "orphaned" (system-created) - // Tagged nodes have key expiry disabled. nodeToRegister.Expiry = nil } else { @@ -1342,6 +1334,11 @@ func (s *State) createAndSaveNewNode(params newNodeParams) (types.NodeView, erro slices.Sort(nodeToRegister.Tags) nodeToRegister.Tags = slices.Compact(nodeToRegister.Tags) + // Node is now tagged, so clear user ownership. + // Tagged nodes are owned by their tags, not a user. + nodeToRegister.UserID = nil + nodeToRegister.User = nil + // Tagged nodes have key expiry disabled. nodeToRegister.Expiry = nil @@ -1488,7 +1485,10 @@ func (s *State) processReauthTags( wasTagged := node.IsTagged() node.Tags = approvedTags - // Note: UserID is preserved as "created by" tracking, consistent with SetNodeTags + // Tagged nodes are owned by their tags, not a user. + node.UserID = nil + node.User = nil + if !wasTagged { log.Info(). Uint64(zf.NodeID, uint64(node.ID)). @@ -1713,9 +1713,15 @@ func (s *State) HandleNodeFromPreAuthKey( existingNodeSameUser, existsSameUser = s.nodeStore.GetNodeByMachineKey(machineKey, types.UserID(pak.User.ID)) } + // Tagged nodes have nil UserID, so they are indexed under UserID(0) + // in nodesByMachineKey. Check there too for tagged PAK re-registration. + if !existsSameUser && pak.IsTagged() { + existingNodeSameUser, existsSameUser = s.nodeStore.GetNodeByMachineKey(machineKey, 0) + } + // For existing nodes, skip validation if: // 1. MachineKey matches (cryptographic proof of machine identity) - // 2. User matches (from the PAK being used) + // 2. User/tag ownership matches (from the PAK being used) // 3. Not a NodeKey rotation (rotation requires fresh validation) // // Security: MachineKey is the cryptographic identity. If someone has the MachineKey, @@ -1723,9 +1729,7 @@ func (s *State) HandleNodeFromPreAuthKey( // We don't check which specific PAK was used originally because: // - Container restarts may use different PAKs (e.g., env var changed) // - Original PAK may be deleted - // - MachineKey + User is sufficient to prove this is the same node - // - // Note: For tags-only keys, existsSameUser is always false, so we always validate. + // - MachineKey + ownership is sufficient to prove this is the same node isExistingNodeReregistering := existsSameUser && existingNodeSameUser.Valid() // Check if this is a NodeKey rotation (different NodeKey) @@ -1812,11 +1816,9 @@ func (s *State) HandleNodeFromPreAuthKey( node.RegisterMethod = util.RegisterMethodAuthKey - // CRITICAL: Tags from PreAuthKey are ONLY applied during initial authentication - // On re-registration, we MUST NOT change tags or node ownership - // The node keeps whatever tags/user ownership it already has - // - // Only update AuthKey reference + // Tags from PreAuthKey are only applied during initial registration. + // On re-registration the node keeps its existing tags and ownership. + // Only update AuthKey reference. node.AuthKey = pak node.AuthKeyID = &pak.ID node.IsOnline = new(false) @@ -1869,19 +1871,27 @@ func (s *State) HandleNodeFromPreAuthKey( // Check if node exists with this machine key for a different user existingNodeAnyUser, existsAnyUser := s.nodeStore.GetNodeByMachineKeyAnyUser(machineKey) - // For user-owned keys, check if node exists for a different user - // For tags-only keys (pak.User == nil), this check is skipped - if pak.User != nil && existsAnyUser && existingNodeAnyUser.Valid() && existingNodeAnyUser.UserID().Get() != pak.User.ID { - // Node exists but belongs to a different user - // Create a NEW node for the new user (do not transfer) - // This allows the same machine to have separate node identities per user - oldUser := existingNodeAnyUser.User() + // For user-owned keys, check if node exists for a different user. + // Tags-only keys (pak.User == nil) skip this check. + // Tagged nodes are also skipped since they have no owning user. + existingIsUserOwned := existsAnyUser && + existingNodeAnyUser.Valid() && + !existingNodeAnyUser.IsTagged() + belongsToDifferentUser := pak.User != nil && + existingIsUserOwned && + existingNodeAnyUser.UserID().Get() != pak.User.ID + + if belongsToDifferentUser { + // Node exists but belongs to a different user. + // Create a new node for the new user (do not transfer). + oldUserName := existingNodeAnyUser.User().Name() + log.Info(). Caller(). Str(zf.ExistingNodeName, existingNodeAnyUser.Hostname()). Uint64(zf.ExistingNodeID, existingNodeAnyUser.ID().Uint64()). Str(zf.MachineKey, machineKey.ShortString()). - Str(zf.OldUser, oldUser.Name()). + Str(zf.OldUser, oldUserName). Str(zf.NewUser, pakUsername()). Msg("Creating new node for different user (same machine key exists for another user)") } diff --git a/hscontrol/state/tags.go b/hscontrol/state/tags.go index 56dff16e..fbc05d9b 100644 --- a/hscontrol/state/tags.go +++ b/hscontrol/state/tags.go @@ -20,22 +20,26 @@ var ( ErrRequestedTagsInvalidOrNotPermitted = errors.New("requested tags") ) -// validateNodeOwnership ensures proper node ownership model. -// A node must be EITHER user-owned OR tagged (mutually exclusive by behavior). -// Tagged nodes CAN have a UserID for "created by" tracking, but the tag is the owner. -func validateNodeOwnership(node *types.Node) error { - isTagged := node.IsTagged() +// ErrTaggedNodeHasUser is returned when a tagged node has a UserID set. +var ErrTaggedNodeHasUser = errors.New("tagged node must not have user_id set") - // Tagged nodes: Must have tags, UserID is optional (just "created by") - if isTagged { +// validateNodeOwnership ensures proper node ownership model. +// A node must be either user-owned or tagged, and these are mutually exclusive: +// tagged nodes must not have a UserID, and user-owned nodes must not have tags. +func validateNodeOwnership(node *types.Node) error { + if node.IsTagged() { if len(node.Tags) == 0 { return fmt.Errorf("%w: %q", ErrNodeMarkedTaggedButHasNoTags, node.Hostname) } - // UserID can be set (created by) or nil (orphaned), both valid for tagged nodes + + if node.UserID != nil { + return fmt.Errorf("%w: %q", ErrTaggedNodeHasUser, node.Hostname) + } + return nil } - // User-owned nodes: Must have UserID, must NOT have tags + // User-owned nodes must have a UserID. if node.UserID == nil { return fmt.Errorf("%w: %q", ErrNodeHasNeitherUserNorTags, node.Hostname) } @@ -59,7 +63,7 @@ func logTagOperation(existingNode types.NodeView, newTags []string) { log.Info(). EmbedObject(existingNode). - Uint("created.by.user", userID). + Uint("previous.user", userID). Strs("new.tags", newTags). Msg("Converting user-owned node to tagged node") } diff --git a/hscontrol/types/node.go b/hscontrol/types/node.go index e705a33a..05b27c19 100644 --- a/hscontrol/types/node.go +++ b/hscontrol/types/node.go @@ -105,10 +105,8 @@ type Node struct { // parts of headscale. GivenName string `gorm:"type:varchar(63);unique_index"` - // UserID is set for ALL nodes (tagged and user-owned) to track "created by". - // For tagged nodes, this is informational only - the tag is the owner. - // For user-owned nodes, this identifies the owner. - // Only nil for orphaned nodes (should not happen in normal operation). + // UserID identifies the owning user for user-owned nodes. + // Nil for tagged nodes, which are owned by their tags. UserID *uint User *User `gorm:"constraint:OnDelete:CASCADE;"` From 89b23d75e4a6641743d093e1b138397c22cbdb50 Mon Sep 17 00:00:00 2001 From: Kristoffer Dalby Date: Fri, 20 Feb 2026 09:27:23 +0000 Subject: [PATCH 3/4] hscontrol/types: regenerate types_view.go make generate Updates #3077 --- hscontrol/types/types_view.go | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/hscontrol/types/types_view.go b/hscontrol/types/types_view.go index e48dd029..cd648d9d 100644 --- a/hscontrol/types/types_view.go +++ b/hscontrol/types/types_view.go @@ -215,10 +215,8 @@ func (v NodeView) Hostname() string { return v.ж.Hostname } // parts of headscale. func (v NodeView) GivenName() string { return v.ж.GivenName } -// UserID is set for ALL nodes (tagged and user-owned) to track "created by". -// For tagged nodes, this is informational only - the tag is the owner. -// For user-owned nodes, this identifies the owner. -// Only nil for orphaned nodes (should not happen in normal operation). +// UserID identifies the owning user for user-owned nodes. +// Nil for tagged nodes, which are owned by their tags. func (v NodeView) UserID() views.ValuePointer[uint] { return views.ValuePointerOf(v.ж.UserID) } func (v NodeView) User() UserView { return v.ж.User.View() } From d62786b06ea6727aa8529f9e76b352ed4da3f561 Mon Sep 17 00:00:00 2001 From: Kristoffer Dalby Date: Fri, 20 Feb 2026 09:27:42 +0000 Subject: [PATCH 4/4] hscontrol: add tests for deleting users with tagged nodes Test the tagged-node-survives-user-deletion scenario at two layers: DB layer (users_test.go): - success_user_only_has_tagged_nodes: tagged nodes with nil user_id do not block user deletion and survive it - error_user_has_tagged_and_owned_nodes: user-owned nodes still block deletion even when tagged nodes coexist App layer (grpcv1_test.go): - TestDeleteUser_TaggedNodeSurvives: full registration flow with tagged PreAuthKey verifies nil UserID after registration, absence from nodesByUser index, user deletion succeeds, and tagged node remains in global node list Also update auth_tags_test.go assertions to expect nil UserID on tagged nodes, consistent with the new invariant. Updates #3077 --- hscontrol/auth_tags_test.go | 32 +++++++-------- hscontrol/db/users_test.go | 71 ++++++++++++++++++++++++++++++++ hscontrol/grpcv1_test.go | 81 +++++++++++++++++++++++++++++++++++-- 3 files changed, 165 insertions(+), 19 deletions(-) diff --git a/hscontrol/auth_tags_test.go b/hscontrol/auth_tags_test.go index e7b74b75..2564154c 100644 --- a/hscontrol/auth_tags_test.go +++ b/hscontrol/auth_tags_test.go @@ -14,7 +14,7 @@ import ( // TestTaggedPreAuthKeyCreatesTaggedNode tests that a PreAuthKey with tags creates // a tagged node with: // - Tags from the PreAuthKey -// - UserID tracking who created the key (informational "created by") +// - Nil UserID (tagged nodes are owned by tags, not a user) // - IsTagged() returns true. func TestTaggedPreAuthKeyCreatesTaggedNode(t *testing.T) { app := createTestApp(t) @@ -51,11 +51,10 @@ func TestTaggedPreAuthKeyCreatesTaggedNode(t *testing.T) { node, found := app.state.GetNodeByNodeKey(nodeKey.Public()) require.True(t, found) - // Critical assertions for tags-as-identity model + // Tagged nodes are owned by their tags, not a user. assert.True(t, node.IsTagged(), "Node should be tagged") assert.ElementsMatch(t, tags, node.Tags().AsSlice(), "Node should have tags from PreAuthKey") - assert.True(t, node.UserID().Valid(), "Node should have UserID tracking creator") - assert.Equal(t, user.ID, node.UserID().Get(), "UserID should track PreAuthKey creator") + assert.False(t, node.UserID().Valid(), "Tagged node should not have UserID") // Verify node is identified correctly assert.True(t, node.IsTagged(), "Tagged node is not user-owned") @@ -129,9 +128,10 @@ func TestReAuthDoesNotReapplyTags(t *testing.T) { assert.True(t, nodeAfterReauth.IsTagged(), "Node should still be tagged") assert.ElementsMatch(t, initialTags, nodeAfterReauth.Tags().AsSlice(), "Tags should remain unchanged on re-auth") - // Verify only one node was created (no duplicates) - nodes := app.state.ListNodesByUser(types.UserID(user.ID)) - assert.Equal(t, 1, nodes.Len(), "Should have exactly one node") + // Verify only one node was created (no duplicates). + // Tagged nodes are not indexed by user, so check the global list. + allNodes := app.state.ListNodes() + assert.Equal(t, 1, allNodes.Len(), "Should have exactly one node") } // NOTE: TestSetTagsOnUserOwnedNode functionality is covered by gRPC tests in grpcv1_test.go @@ -294,13 +294,13 @@ func TestMultipleNodesWithSameReusableTaggedPreAuthKey(t *testing.T) { assert.ElementsMatch(t, tags, node1.Tags().AsSlice(), "First node should have PreAuthKey tags") assert.ElementsMatch(t, tags, node2.Tags().AsSlice(), "Second node should have PreAuthKey tags") - // Both nodes should track the same creator - assert.Equal(t, user.ID, node1.UserID().Get(), "First node should track creator") - assert.Equal(t, user.ID, node2.UserID().Get(), "Second node should track creator") + // Tagged nodes should not have UserID set. + assert.False(t, node1.UserID().Valid(), "First node should not have UserID") + assert.False(t, node2.UserID().Valid(), "Second node should not have UserID") - // Verify we have exactly 2 nodes - nodes := app.state.ListNodesByUser(types.UserID(user.ID)) - assert.Equal(t, 2, nodes.Len(), "Should have exactly two nodes") + // Verify we have exactly 2 nodes. + allNodes := app.state.ListNodes() + assert.Equal(t, 2, allNodes.Len(), "Should have exactly two nodes") } // TestNonReusableTaggedPreAuthKey tests that a non-reusable PreAuthKey with tags @@ -359,9 +359,9 @@ func TestNonReusableTaggedPreAuthKey(t *testing.T) { _, err = app.handleRegisterWithAuthKey(regReq2, machineKey2.Public()) require.Error(t, err, "Should not be able to reuse non-reusable PreAuthKey") - // Verify only one node was created - nodes := app.state.ListNodesByUser(types.UserID(user.ID)) - assert.Equal(t, 1, nodes.Len(), "Should have exactly one node") + // Verify only one node was created. + allNodes := app.state.ListNodes() + assert.Equal(t, 1, allNodes.Len(), "Should have exactly one node") } // TestExpiredTaggedPreAuthKey tests that an expired PreAuthKey with tags diff --git a/hscontrol/db/users_test.go b/hscontrol/db/users_test.go index 284ca639..f13d89dd 100644 --- a/hscontrol/db/users_test.go +++ b/hscontrol/db/users_test.go @@ -89,6 +89,77 @@ func TestDestroyUserErrors(t *testing.T) { assert.ErrorIs(t, err, ErrUserStillHasNodes) }, }, + { + // https://github.com/juanfont/headscale/issues/3077 + // Tagged nodes have user_id = NULL, so they do not block + // user deletion and are unaffected by ON DELETE CASCADE. + name: "success_user_only_has_tagged_nodes", + test: func(t *testing.T, db *HSDatabase) { + t.Helper() + + user, err := db.CreateUser(types.User{Name: "test"}) + require.NoError(t, err) + + // Create a tagged node with no user_id (the invariant). + node := types.Node{ + ID: 0, + Hostname: "tagged-node", + RegisterMethod: util.RegisterMethodAuthKey, + Tags: []string{"tag:server"}, + } + trx := db.DB.Save(&node) + require.NoError(t, trx.Error) + + err = db.DestroyUser(types.UserID(user.ID)) + require.NoError(t, err) + + // User is gone. + _, err = db.GetUserByID(types.UserID(user.ID)) + require.ErrorIs(t, err, ErrUserNotFound) + + // Tagged node survives. + var survivingNode types.Node + + result := db.DB.First(&survivingNode, "id = ?", node.ID) + require.NoError(t, result.Error) + assert.Nil(t, survivingNode.UserID) + assert.Equal(t, []string{"tag:server"}, survivingNode.Tags) + }, + }, + { + // A user who has both tagged and user-owned nodes cannot + // be deleted; the user-owned nodes still block deletion. + name: "error_user_has_tagged_and_owned_nodes", + test: func(t *testing.T, db *HSDatabase) { + t.Helper() + + user, err := db.CreateUser(types.User{Name: "test"}) + require.NoError(t, err) + + // Tagged node: no user_id. + taggedNode := types.Node{ + ID: 0, + Hostname: "tagged-node", + RegisterMethod: util.RegisterMethodAuthKey, + Tags: []string{"tag:server"}, + } + trx := db.DB.Save(&taggedNode) + require.NoError(t, trx.Error) + + // User-owned node: has user_id. + ownedNode := types.Node{ + ID: 0, + Hostname: "owned-node", + UserID: &user.ID, + RegisterMethod: util.RegisterMethodAuthKey, + } + trx = db.DB.Save(&ownedNode) + require.NoError(t, trx.Error) + + err = db.DestroyUser(types.UserID(user.ID)) + require.ErrorIs(t, err, ErrUserStillHasNodes) + }, + }, } for _, tt := range tests { diff --git a/hscontrol/grpcv1_test.go b/hscontrol/grpcv1_test.go index 626204ec..aa584681 100644 --- a/hscontrol/grpcv1_test.go +++ b/hscontrol/grpcv1_test.go @@ -3,8 +3,10 @@ package hscontrol import ( "context" "testing" + "time" v1 "github.com/juanfont/headscale/gen/go/headscale/v1" + "github.com/juanfont/headscale/hscontrol/types" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "google.golang.org/grpc/codes" @@ -186,13 +188,12 @@ func TestSetTags_TaggedNode(t *testing.T) { taggedNode, found := app.state.GetNodeByNodeKey(nodeKey.Public()) require.True(t, found) assert.True(t, taggedNode.IsTagged(), "Node should be tagged") - assert.True(t, taggedNode.UserID().Valid(), "Tagged node should have UserID for tracking") + assert.False(t, taggedNode.UserID().Valid(), "Tagged node should not have UserID") // Create API server instance apiServer := newHeadscaleV1APIServer(app) - // Test: SetTags should NOT reject tagged nodes with "user-owned" error - // (Even though they have UserID set, IsTagged() identifies them correctly) + // Test: SetTags should work on tagged nodes. resp, err := apiServer.SetTags(context.Background(), &v1.SetTagsRequest{ NodeId: uint64(taggedNode.ID()), Tags: []string{"tag:initial"}, // Keep existing tag to avoid ACL validation issues @@ -283,6 +284,80 @@ func TestDeleteUser_ReturnsProperChangeSignal(t *testing.T) { assert.False(t, changeSignal.IsEmpty(), "DeleteUser should return a non-empty change signal (issue #2967)") } +// TestDeleteUser_TaggedNodeSurvives tests that deleting a user succeeds when +// the user's only nodes are tagged, and that those nodes remain in the +// NodeStore with nil UserID. +// https://github.com/juanfont/headscale/issues/3077 +func TestDeleteUser_TaggedNodeSurvives(t *testing.T) { + t.Parallel() + + app := createTestApp(t) + + user := app.state.CreateUserForTest("legacy-user") + + // Register a tagged node via the full auth flow. + tags := []string{"tag:server"} + pak, err := app.state.CreatePreAuthKey(user.TypedID(), true, false, nil, tags) + require.NoError(t, err) + + machineKey := key.NewMachine() + nodeKey := key.NewNode() + + regReq := tailcfg.RegisterRequest{ + Auth: &tailcfg.RegisterResponseAuth{ + AuthKey: pak.Key, + }, + NodeKey: nodeKey.Public(), + Hostinfo: &tailcfg.Hostinfo{ + Hostname: "tagged-server", + }, + Expiry: time.Now().Add(24 * time.Hour), + } + + resp, err := app.handleRegisterWithAuthKey(regReq, machineKey.Public()) + require.NoError(t, err) + require.True(t, resp.MachineAuthorized) + + // Verify the registered node has nil UserID (enforced invariant). + node, found := app.state.GetNodeByNodeKey(nodeKey.Public()) + require.True(t, found) + require.True(t, node.IsTagged()) + assert.False(t, node.UserID().Valid(), + "tagged node should have nil UserID after registration") + + nodeID := node.ID() + + // NodeStore should not list the tagged node under any user. + nodesForUser := app.state.ListNodesByUser(types.UserID(user.ID)) + assert.Equal(t, 0, nodesForUser.Len(), + "tagged nodes should not appear in nodesByUser index") + + // Delete the user. + changeSignal, err := app.state.DeleteUser(*user.TypedID()) + require.NoError(t, err) + assert.False(t, changeSignal.IsEmpty()) + + // Tagged node survives in the NodeStore. + nodeAfter, found := app.state.GetNodeByID(nodeID) + require.True(t, found, "tagged node should survive user deletion") + assert.True(t, nodeAfter.IsTagged()) + assert.False(t, nodeAfter.UserID().Valid()) + + // Tagged node appears in the global list. + allNodes := app.state.ListNodes() + foundInAll := false + + for _, n := range allNodes.All() { + if n.ID() == nodeID { + foundInAll = true + + break + } + } + + assert.True(t, foundInAll, "tagged node should appear in the global node list") +} + // TestExpireApiKey_ByID tests that API keys can be expired by ID. func TestExpireApiKey_ByID(t *testing.T) { t.Parallel()