From 32ea1c1c8452450f735b3a7294e2bf341ec2b6d8 Mon Sep 17 00:00:00 2001 From: Ryan Malloy Date: Thu, 21 May 2026 21:14:08 -0600 Subject: [PATCH] oidc groups: fix post-merge compile and migration issues Bugs found by running the real test suite after merging upstream: - types/types_clone.go, types/types_view.go: extend the regeneration guard struct literals to include the new Groups field, and add a UserView.Groups() accessor. Generated files normally rebuilt via cloner / viewer; touched by hand here pending make generate. - db/db.go: the migration adding the groups column ran after 202505141324, which calls ListUsers() through the User struct that now includes Groups. Move the column-add to 202505141323 so the schema is in place before any migration loads users. Register the new ID in the FK-disabled migration list. - db/db.go: 202507021200 recreates all tables from inline SQL during the SQLite schema migration; add groups to both the CREATE TABLE users statement and the INSERT INTO users ... SELECT FROM users_old so the column survives the recreation. Also fix a copy-paste bug in the Rollback closure that referenced tx instead of db. - db/schema.sql: add the groups column to the canonical schema so squibble.Validate accepts databases produced by the new migration chain. Verified against all 7 historical sqlite dumps in hscontrol/db/testdata/sqlite. - types/users_test.go: the casby-oidc-claim case now exercises group storage; update the want to include the JSON-encoded groups column. - integration/oidc_groups_test.go: replace the aspirational draft (which referenced assertNoErr, scenario.usernames, hsic.WithTLS and other symbols that do not exist) with a focused test that follows the auth_oidc_test.go pattern. Verifies the groups column directly via sqlite3 inside the headscale container since the gRPC User message does not expose Groups. --- hscontrol/db/db.go | 49 ++-- hscontrol/db/schema.sql | 1 + hscontrol/types/types_clone.go | 1 + hscontrol/types/types_view.go | 3 + hscontrol/types/users_test.go | 1 + integration/oidc_groups_test.go | 404 +++++++------------------------- 6 files changed, 119 insertions(+), 340 deletions(-) diff --git a/hscontrol/db/db.go b/hscontrol/db/db.go index a6b7433c..983cac69 100644 --- a/hscontrol/db/db.go +++ b/hscontrol/db/db.go @@ -215,6 +215,27 @@ AND auth_key_id NOT IN ( }, Rollback: func(db *gorm.DB) error { return nil }, }, + // Add groups column to users table for OIDC role mapping. + // Must run before any migration that loads users via the User struct + // (e.g., 202505141324), since the User struct now includes Groups. + { + ID: "202505141323", + Migrate: func(tx *gorm.DB) error { + if !tx.Migrator().HasColumn(&types.User{}, "groups") { + err := tx.Migrator().AddColumn(&types.User{}, "groups") + if err != nil { + return fmt.Errorf("adding groups column to users table: %w", err) + } + } + return nil + }, + Rollback: func(db *gorm.DB) error { + if db.Migrator().HasColumn(&types.User{}, "groups") { + return db.Migrator().DropColumn(&types.User{}, "groups") + } + return nil + }, + }, // Fix the provider identifier for users that have a double slash in the // provider identifier. { @@ -315,6 +336,7 @@ AND auth_key_id NOT IN ( provider_identifier text, provider text, profile_pic_url text, + groups text, created_at datetime, updated_at datetime, deleted_at datetime @@ -381,8 +403,8 @@ AND auth_key_id NOT IN ( // Copy data directly using SQL dataCopySQL := []string{ - `INSERT INTO users (id, name, display_name, email, provider_identifier, provider, profile_pic_url, created_at, updated_at, deleted_at) - SELECT id, name, display_name, email, provider_identifier, provider, profile_pic_url, created_at, updated_at, deleted_at + `INSERT INTO users (id, name, display_name, email, provider_identifier, provider, profile_pic_url, groups, created_at, updated_at, deleted_at) + SELECT id, name, display_name, email, provider_identifier, provider, profile_pic_url, groups, created_at, updated_at, deleted_at FROM users_old`, `INSERT INTO pre_auth_keys (id, key, user_id, reusable, ephemeral, used, tags, expiration, created_at) @@ -447,28 +469,6 @@ AND auth_key_id NOT IN ( }, Rollback: func(db *gorm.DB) error { return nil }, }, - // Add Groups column to users table for OIDC role-based access control - { - ID: "202509161200", - Migrate: func(tx *gorm.DB) error { - // Add Groups column to store OIDC group memberships as JSON - if !tx.Migrator().HasColumn(&types.User{}, "groups") { - err := tx.Migrator().AddColumn(&types.User{}, "groups") - if err != nil { - return fmt.Errorf("adding groups column to users table: %w", err) - } - log.Info().Msg("Added Groups column to users table for OIDC role mapping") - } - return nil - }, - Rollback: func(db *gorm.DB) error { - // Remove Groups column on rollback - if tx.Migrator().HasColumn(&types.User{}, "groups") { - return tx.Migrator().DropColumn(&types.User{}, "groups") - } - return nil - }, - }, // v0.27.1 { // Drop all tables that are no longer in use and has existed. @@ -1000,6 +1000,7 @@ func runMigrations(cfg types.DatabaseConfig, dbConn *gorm.DB, migrations *gormig "202502131714", "202502171819", "202505091439", + "202505141323", "202505141324", // As of 2025-07-02, no new IDs should be added here. diff --git a/hscontrol/db/schema.sql b/hscontrol/db/schema.sql index 781446c0..d6511404 100644 --- a/hscontrol/db/schema.sql +++ b/hscontrol/db/schema.sql @@ -12,6 +12,7 @@ CREATE TABLE users( provider_identifier text, provider text, profile_pic_url text, + groups text, created_at datetime, updated_at datetime, diff --git a/hscontrol/types/types_clone.go b/hscontrol/types/types_clone.go index a14dc6fd..2eacb561 100644 --- a/hscontrol/types/types_clone.go +++ b/hscontrol/types/types_clone.go @@ -35,6 +35,7 @@ var _UserCloneNeedsRegeneration = User(struct { ProviderIdentifier sql.NullString Provider string ProfilePicURL string + Groups string }{}) // Clone makes a deep copy of Node. diff --git a/hscontrol/types/types_view.go b/hscontrol/types/types_view.go index 3a9b3b42..0db6b810 100644 --- a/hscontrol/types/types_view.go +++ b/hscontrol/types/types_view.go @@ -124,8 +124,11 @@ var _UserViewNeedsRegeneration = User(struct { ProviderIdentifier sql.NullString Provider string ProfilePicURL string + Groups string }{}) +func (v UserView) Groups() string { return v.ж.Groups } + // View returns a read-only view of Node. func (p *Node) View() NodeView { return NodeView{ж: p} diff --git a/hscontrol/types/users_test.go b/hscontrol/types/users_test.go index 1218979c..08823a3e 100644 --- a/hscontrol/types/users_test.go +++ b/hscontrol/types/users_test.go @@ -528,6 +528,7 @@ func TestOIDCClaimsJSONToUser(t *testing.T) { Valid: true, }, ProfilePicURL: "https://cdn.casbin.org/img/casbin.svg", + Groups: `["org1/department1","org1/department2"]`, }, }, } diff --git a/integration/oidc_groups_test.go b/integration/oidc_groups_test.go index 7b0f47b0..af7e572f 100644 --- a/integration/oidc_groups_test.go +++ b/integration/oidc_groups_test.go @@ -2,52 +2,47 @@ package integration import ( "encoding/json" + "sort" + "strings" "testing" - "time" - "github.com/google/go-cmp/cmp" - v1 "github.com/juanfont/headscale/gen/go/headscale/v1" "github.com/juanfont/headscale/integration/hsic" - "github.com/juanfont/headscale/integration/tsic" "github.com/oauth2-proxy/mockoidc" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) -// TestOIDCGroupsExtraction tests that OIDC groups are properly extracted and stored -func TestOIDCGroupsExtraction(t *testing.T) { +// TestOIDCGroupsPersisted verifies that the `groups` claim from an OIDC +// provider is persisted into the users.groups column after the user logs in. +// +// The implementation under test: +// - User.Groups column (TEXT, JSON-encoded []string) added by migration +// 202505141323 in hscontrol/db/db.go. +// - User.SetGroups / User.GetGroups in hscontrol/types/users.go. +// - FromClaim() calls SetGroups(claims.Groups) so login populates the column. +// - OIDCClaims.Groups is FlexibleStringSlice so providers like JumpCloud +// that return a single string instead of a one-element array also work. +// +// Verification is done by reading the SQLite database inside the headscale +// container directly, because the gRPC User message does not currently +// expose Groups. Adding groups to the gRPC API is a separate, larger change. +func TestOIDCGroupsPersisted(t *testing.T) { IntegrationSkip(t) - // Create mock users with different group memberships + // mockoidc serves logins in strict queue order, so keep NodesPerUser=1. spec := ScenarioSpec{ NodesPerUser: 1, - Users: []string{"admin", "user", "readonly"}, + Users: []string{"admin", "dev", "solo"}, OIDCUsers: []mockoidc.MockUser{ - // Admin user with multiple groups - { - Subject: "admin@example.com", - Email: "admin@example.com", - PreferredUsername: "admin", - Groups: []string{"admins", "users", "engineering"}, - }, - // Regular user with single group - { - Subject: "user@example.com", - Email: "user@example.com", - PreferredUsername: "user", - Groups: []string{"users"}, - }, - // Readonly user with different groups - { - Subject: "readonly@example.com", - Email: "readonly@example.com", - PreferredUsername: "readonly", - Groups: []string{"readonly", "auditors"}, - }, + oidcMockUserWithGroups("admin", true, []string{"admins", "engineering"}), + oidcMockUserWithGroups("dev", true, []string{"engineering"}), + // User with empty groups — must round-trip as no Groups stored. + oidcMockUserWithGroups("solo", true, nil), }, } scenario, err := NewScenario(spec) - assertNoErr(t, err) + require.NoError(t, err) defer scenario.ShutdownAssertNoPanics(t) oidcMap := map[string]string{ @@ -55,310 +50,87 @@ func TestOIDCGroupsExtraction(t *testing.T) { "HEADSCALE_OIDC_CLIENT_ID": scenario.mockOIDC.ClientID(), "CREDENTIALS_DIRECTORY_TEST": "/tmp", "HEADSCALE_OIDC_CLIENT_SECRET_PATH": "${CREDENTIALS_DIRECTORY_TEST}/hs_client_oidc_secret", - // Enable groups claim extraction - "HEADSCALE_OIDC_EXTRA_PARAMS": `{"groups_claim": "groups"}`, + // Make sure the OIDC scope set includes "groups" so the IdP emits the claim. + "HEADSCALE_OIDC_SCOPE": "openid,profile,email,groups", } err = scenario.CreateHeadscaleEnvWithLoginURL( nil, hsic.WithTestName("oidcgroups"), hsic.WithConfigEnv(oidcMap), - hsic.WithTLS(), - hsic.WithHostnameAsServerURL(), + hsic.WithFileInContainer("/tmp/hs_client_oidc_secret", []byte(scenario.mockOIDC.ClientSecret())), ) - assertNoErr(t, err) + requireNoErrHeadscaleEnv(t, err) - // Perform OIDC logins for all users - allClients, err := scenario.ListTailscaleClients() - assertNoErr(t, err) + // Drive the OIDC login flow for every client. + _, err = scenario.ListTailscaleClients() + requireNoErrListClients(t, err) + err = scenario.WaitForTailscaleSync() + requireNoErrSync(t, err) - for _, client := range allClients { - user, ok := scenario.usernames[client.Hostname()] - assertOK(t, ok) + headscale, err := scenario.Headscale() + require.NoError(t, err) - _ = client.Login(scenario.loginWaitGroup, user) - - // Wait for login to complete - scenario.loginWaitGroup.Wait() - time.Sleep(5 * time.Second) - } - - // Test groups were extracted correctly - t.Run("verify-groups-extracted", func(t *testing.T) { - // Get all users from Headscale - users, err := scenario.ControlServer().ListUsers(&v1.ListUsersRequest{}) - assertNoErr(t, err) - - // Verify each user has correct groups - expectedGroups := map[string][]string{ - "admin": {"admins", "users", "engineering"}, - "user": {"users"}, - "readonly": {"readonly", "auditors"}, - } - - for _, user := range users.GetUsers() { - // Parse groups from user - var userGroups []string - if user.GetGroups() != "" { - err := json.Unmarshal([]byte(user.GetGroups()), &userGroups) - assertNoErr(t, err) - } - - // Check expected groups - expected, exists := expectedGroups[user.GetName()] - assert.True(t, exists, "Unexpected user: %s", user.GetName()) - - // Sort both slices for comparison - assert.ElementsMatch(t, expected, userGroups, - "User %s has incorrect groups. Expected: %v, Got: %v", - user.GetName(), expected, userGroups) - } + // Query the SQLite database inside the headscale container for the groups + // column. CLI/gRPC do not expose it yet; this is the authoritative store. + const dbPath = "/tmp/integration_test_db.sqlite3" + out, err := headscale.Execute([]string{ + "sqlite3", dbPath, + "-cmd", ".mode tabs", + "SELECT name, COALESCE(groups, '') FROM users WHERE provider = 'oidc' ORDER BY name;", }) + require.NoError(t, err, "querying users.groups from sqlite") - // Test groups persist across logins - t.Run("verify-groups-persistence", func(t *testing.T) { - // Get a client and log it out then back in - client := allClients[0] - user := scenario.usernames[client.Hostname()] + got := parseGroupsRows(t, out) - // Logout - err := client.Logout() - assertNoErr(t, err) + want := map[string][]string{ + "admin": {"admins", "engineering"}, + "dev": {"engineering"}, + "solo": nil, + } - // Login again - _ = client.Login(scenario.loginWaitGroup, user) - scenario.loginWaitGroup.Wait() - time.Sleep(3 * time.Second) + for name, wantGroups := range want { + gotGroups, ok := got[name] + assert.True(t, ok, "user %q not present in users table", name) + assert.ElementsMatch(t, wantGroups, gotGroups, + "groups mismatch for user %q (raw rows: %q)", name, out) + } +} - // Verify groups are still there - users, err := scenario.ControlServer().ListUsers(&v1.ListUsersRequest{}) - assertNoErr(t, err) - - // Find our user - var targetUser *v1.User - for _, u := range users.GetUsers() { - if u.GetName() == user { - targetUser = u - break - } +// parseGroupsRows parses the tab-separated output of: +// +// SELECT name, COALESCE(groups, '') FROM users ... +// +// Returns a map of username -> decoded groups slice. An empty groups column +// (stored as "" by SetGroups when the input slice is empty) decodes to nil. +func parseGroupsRows(t *testing.T, raw string) map[string][]string { + t.Helper() + rows := map[string][]string{} + for _, line := range strings.Split(strings.TrimSpace(raw), "\n") { + if line == "" { + continue } - assert.NotNil(t, targetUser, "User not found after re-login") + parts := strings.SplitN(line, "\t", 2) + require.Len(t, parts, 2, "unexpected sqlite row format: %q", line) + name, groupsJSON := parts[0], parts[1] - // Verify groups are preserved - var userGroups []string - if targetUser.GetGroups() != "" { - err := json.Unmarshal([]byte(targetUser.GetGroups()), &userGroups) - assertNoErr(t, err) + if groupsJSON == "" { + rows[name] = nil + continue } - - assert.NotEmpty(t, userGroups, "Groups should persist after re-login") - }) + var gs []string + require.NoError(t, json.Unmarshal([]byte(groupsJSON), &gs), + "groups column for %q is not valid JSON: %q", name, groupsJSON) + sort.Strings(gs) + rows[name] = gs + } + return rows } -// TestOIDCGroupsWithoutClaim tests behavior when groups claim is not configured -func TestOIDCGroupsWithoutClaim(t *testing.T) { - IntegrationSkip(t) - - spec := ScenarioSpec{ - NodesPerUser: 1, - Users: []string{"user1"}, - OIDCUsers: []mockoidc.MockUser{ - { - Subject: "user1@example.com", - Email: "user1@example.com", - PreferredUsername: "user1", - Groups: []string{"admins", "users"}, // Groups present but not requested - }, - }, - } - - scenario, err := NewScenario(spec) - assertNoErr(t, err) - defer scenario.ShutdownAssertNoPanics(t) - - // OIDC config WITHOUT groups claim - oidcMap := map[string]string{ - "HEADSCALE_OIDC_ISSUER": scenario.mockOIDC.Issuer(), - "HEADSCALE_OIDC_CLIENT_ID": scenario.mockOIDC.ClientID(), - "CREDENTIALS_DIRECTORY_TEST": "/tmp", - "HEADSCALE_OIDC_CLIENT_SECRET_PATH": "${CREDENTIALS_DIRECTORY_TEST}/hs_client_oidc_secret", - // No groups claim configured - } - - err = scenario.CreateHeadscaleEnvWithLoginURL( - nil, - hsic.WithTestName("oidcnogroups"), - hsic.WithConfigEnv(oidcMap), - hsic.WithTLS(), - hsic.WithHostnameAsServerURL(), - ) - assertNoErr(t, err) - - // Login user - allClients, err := scenario.ListTailscaleClients() - assertNoErr(t, err) - - client := allClients[0] - user := scenario.usernames[client.Hostname()] - _ = client.Login(scenario.loginWaitGroup, user) - scenario.loginWaitGroup.Wait() - time.Sleep(3 * time.Second) - - // Verify user exists but has no groups - users, err := scenario.ControlServer().ListUsers(&v1.ListUsersRequest{}) - assertNoErr(t, err) - - assert.Len(t, users.GetUsers(), 1, "Should have exactly one user") - - user1 := users.GetUsers()[0] - assert.Empty(t, user1.GetGroups(), "User should have no groups when claim not configured") +// oidcMockUserWithGroups extends [oidcMockUser] with a Groups claim. +// mockoidc populates the id_token / userinfo from this struct verbatim. +func oidcMockUserWithGroups(username string, emailVerified bool, groups []string) mockoidc.MockUser { + u := oidcMockUser(username, emailVerified) + u.Groups = groups + return u } - -// TestOIDCGroupsEmptyGroups tests behavior when user has no groups -func TestOIDCGroupsEmptyGroups(t *testing.T) { - IntegrationSkip(t) - - spec := ScenarioSpec{ - NodesPerUser: 1, - Users: []string{"ungrouped"}, - OIDCUsers: []mockoidc.MockUser{ - { - Subject: "ungrouped@example.com", - Email: "ungrouped@example.com", - PreferredUsername: "ungrouped", - Groups: []string{}, // User has no groups - }, - }, - } - - scenario, err := NewScenario(spec) - assertNoErr(t, err) - defer scenario.ShutdownAssertNoPanics(t) - - oidcMap := map[string]string{ - "HEADSCALE_OIDC_ISSUER": scenario.mockOIDC.Issuer(), - "HEADSCALE_OIDC_CLIENT_ID": scenario.mockOIDC.ClientID(), - "CREDENTIALS_DIRECTORY_TEST": "/tmp", - "HEADSCALE_OIDC_CLIENT_SECRET_PATH": "${CREDENTIALS_DIRECTORY_TEST}/hs_client_oidc_secret", - "HEADSCALE_OIDC_EXTRA_PARAMS": `{"groups_claim": "groups"}`, - } - - err = scenario.CreateHeadscaleEnvWithLoginURL( - nil, - hsic.WithTestName("oidcemptygroups"), - hsic.WithConfigEnv(oidcMap), - hsic.WithTLS(), - hsic.WithHostnameAsServerURL(), - ) - assertNoErr(t, err) - - // Login user with no groups - allClients, err := scenario.ListTailscaleClients() - assertNoErr(t, err) - - client := allClients[0] - user := scenario.usernames[client.Hostname()] - _ = client.Login(scenario.loginWaitGroup, user) - scenario.loginWaitGroup.Wait() - time.Sleep(3 * time.Second) - - // Verify user exists with empty groups - users, err := scenario.ControlServer().ListUsers(&v1.ListUsersRequest{}) - assertNoErr(t, err) - - assert.Len(t, users.GetUsers(), 1, "Should have exactly one user") - - ungroupedUser := users.GetUsers()[0] - assert.Empty(t, ungroupedUser.GetGroups(), "User with no groups should have empty groups field") -} - -// TestOIDCGroupsUpdatesOnLogin tests that groups are updated when user logs in again -func TestOIDCGroupsUpdatesOnLogin(t *testing.T) { - IntegrationSkip(t) - - // Create scenario with user having initial groups - spec := ScenarioSpec{ - NodesPerUser: 1, - Users: []string{"dynamic"}, - OIDCUsers: []mockoidc.MockUser{ - { - Subject: "dynamic@example.com", - Email: "dynamic@example.com", - PreferredUsername: "dynamic", - Groups: []string{"initial-group"}, - }, - }, - } - - scenario, err := NewScenario(spec) - assertNoErr(t, err) - defer scenario.ShutdownAssertNoPanics(t) - - oidcMap := map[string]string{ - "HEADSCALE_OIDC_ISSUER": scenario.mockOIDC.Issuer(), - "HEADSCALE_OIDC_CLIENT_ID": scenario.mockOIDC.ClientID(), - "CREDENTIALS_DIRECTORY_TEST": "/tmp", - "HEADSCALE_OIDC_CLIENT_SECRET_PATH": "${CREDENTIALS_DIRECTORY_TEST}/hs_client_oidc_secret", - "HEADSCALE_OIDC_EXTRA_PARAMS": `{"groups_claim": "groups"}`, - } - - err = scenario.CreateHeadscaleEnvWithLoginURL( - nil, - hsic.WithTestName("oidcdynamicgroups"), - hsic.WithConfigEnv(oidcMap), - hsic.WithTLS(), - hsic.WithHostnameAsServerURL(), - ) - assertNoErr(t, err) - - // Initial login - allClients, err := scenario.ListTailscaleClients() - assertNoErr(t, err) - - client := allClients[0] - user := scenario.usernames[client.Hostname()] - _ = client.Login(scenario.loginWaitGroup, user) - scenario.loginWaitGroup.Wait() - time.Sleep(3 * time.Second) - - // Verify initial groups - users, err := scenario.ControlServer().ListUsers(&v1.ListUsersRequest{}) - assertNoErr(t, err) - - userObj := users.GetUsers()[0] - var initialGroups []string - if userObj.GetGroups() != "" { - err := json.Unmarshal([]byte(userObj.GetGroups()), &initialGroups) - assertNoErr(t, err) - } - assert.Equal(t, []string{"initial-group"}, initialGroups) - - // Update the mock user to have different groups - // Note: In a real test, this would involve updating the OIDC provider - // For this test, we'll simulate the scenario by modifying the mock - - // Logout and login again (simulating groups change in OIDC provider) - err = client.Logout() - assertNoErr(t, err) - - // Update mock user groups (this is test-specific, not production code) - scenario.mockOIDC.SetUserGroups("dynamic@example.com", []string{"updated-group", "admin-group"}) - - _ = client.Login(scenario.loginWaitGroup, user) - scenario.loginWaitGroup.Wait() - time.Sleep(3 * time.Second) - - // Verify groups were updated - users, err = scenario.ControlServer().ListUsers(&v1.ListUsersRequest{}) - assertNoErr(t, err) - - updatedUserObj := users.GetUsers()[0] - var updatedGroups []string - if updatedUserObj.GetGroups() != "" { - err := json.Unmarshal([]byte(updatedUserObj.GetGroups()), &updatedGroups) - assertNoErr(t, err) - } - - assert.ElementsMatch(t, []string{"updated-group", "admin-group"}, updatedGroups, - "Groups should be updated on subsequent login") -} \ No newline at end of file