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()