From 7901f3700203bb782220d6bf9cc3eaab52abfbde Mon Sep 17 00:00:00 2001 From: Aarnav Tale Date: Sat, 30 May 2026 17:14:28 -0400 Subject: [PATCH] fix: correctly handle user id passthrough on headscale actions --- app/routes/users/components/menu.tsx | 6 +- app/routes/users/dialogs/delete-user.tsx | 2 +- app/routes/users/dialogs/link-user.tsx | 6 +- app/routes/users/dialogs/reassign-user.tsx | 6 +- app/routes/users/dialogs/rename-user.tsx | 2 +- .../users/dialogs/transfer-ownership.tsx | 6 +- app/routes/users/user-actions.ts | 81 +++++-------------- .../headscale/api/resources/pre-auth-keys.ts | 6 +- app/server/web/auth.ts | 59 +++++--------- tests/unit/auth/auth-service.test.ts | 45 +++++------ 10 files changed, 76 insertions(+), 143 deletions(-) diff --git a/app/routes/users/components/menu.tsx b/app/routes/users/components/menu.tsx index 195d29f..f75963b 100644 --- a/app/routes/users/components/menu.tsx +++ b/app/routes/users/components/menu.tsx @@ -54,24 +54,24 @@ export default function UserMenu({ {modal === "reassign" && ( { if (!isOpen) setModal(null); }} - userId={user.linkedHeadscaleUser?.id ?? user.id} /> )} {modal === "link" && ( { if (!isOpen) setModal(null); }} - userId={user.linkedHeadscaleUser?.id ?? user.id} /> )} {modal === "transfer" && ( @@ -81,7 +81,7 @@ export default function UserMenu({ if (!isOpen) setModal(null); }} targetDisplayName={displayName} - targetUserId={user.linkedHeadscaleUser?.id ?? user.id} + targetHeadplaneUserId={user.id} /> )} diff --git a/app/routes/users/dialogs/delete-user.tsx b/app/routes/users/dialogs/delete-user.tsx index ac8bcb9..815ec0a 100644 --- a/app/routes/users/dialogs/delete-user.tsx +++ b/app/routes/users/dialogs/delete-user.tsx @@ -34,7 +34,7 @@ export default function DeleteUser({ user, machines, isOpen, setIsOpen }: Delete )} - + ); diff --git a/app/routes/users/dialogs/link-user.tsx b/app/routes/users/dialogs/link-user.tsx index ac1510b..61e30ec 100644 --- a/app/routes/users/dialogs/link-user.tsx +++ b/app/routes/users/dialogs/link-user.tsx @@ -5,7 +5,7 @@ import Title from "~/components/title"; import cn from "~/utils/cn"; interface LinkUserProps { - userId: string; + headplaneUserId: string; displayName: string; headscaleUsers: { id: string; name: string }[]; currentLink?: string; @@ -14,7 +14,7 @@ interface LinkUserProps { } export default function LinkUser({ - userId, + headplaneUserId, displayName, headscaleUsers, currentLink, @@ -34,7 +34,7 @@ export default function LinkUser({ ) : ( <> - + - + {Object.keys(Roles) .filter((r) => r !== "owner") diff --git a/app/routes/users/dialogs/rename-user.tsx b/app/routes/users/dialogs/rename-user.tsx index 2d67910..96f8976 100644 --- a/app/routes/users/dialogs/rename-user.tsx +++ b/app/routes/users/dialogs/rename-user.tsx @@ -21,7 +21,7 @@ export default function RenameUser({ user, isOpen, setIsOpen }: RenameProps) { update any ACL policies that may refer to this user by their old username. - + void; } export default function TransferOwnership({ - targetUserId, + targetHeadplaneUserId, targetDisplayName, isOpen, setIsOpen, @@ -29,7 +29,7 @@ export default function TransferOwnership({ ownership. - + ); diff --git a/app/routes/users/user-actions.ts b/app/routes/users/user-actions.ts index 5e3153b..9f0869f 100644 --- a/app/routes/users/user-actions.ts +++ b/app/routes/users/user-actions.ts @@ -1,7 +1,6 @@ import { data } from "react-router"; import { usersResource } from "~/server/headscale/live-store"; -import { getOidcSubject } from "~/server/web/headscale-identity"; import { Capabilities } from "~/server/web/roles"; import type { Role } from "~/server/web/roles"; @@ -42,28 +41,28 @@ export async function userAction({ request, context }: Route.ActionArgs) { return { message: "User created successfully" }; } case "delete_user": { - const userId = formData.get("user_id")?.toString(); - if (!userId) { - throw data("Missing `user_id` in the form data.", { + const headscaleUserId = formData.get("headscale_user_id")?.toString(); + if (!headscaleUserId) { + throw data("Missing `headscale_user_id` in the form data.", { status: 400, }); } - await api.users.delete(userId); + await api.users.delete(headscaleUserId); await context.hsLive.refresh(usersResource, api); return { message: "User deleted successfully" }; } case "rename_user": { - const userId = formData.get("user_id")?.toString(); + const headscaleUserId = formData.get("headscale_user_id")?.toString(); const newName = formData.get("new_name")?.toString(); - if (!userId || !newName) { + if (!headscaleUserId || !newName) { return data({ success: false }, 400); } - const users = await api.users.list({ id: userId }); - const user = users.find((user) => user.id === userId); + const users = await api.users.list({ id: headscaleUserId }); + const user = users.find((user) => user.id === headscaleUserId); if (!user) { - throw data(`No user found with id: ${userId}`, { status: 400 }); + throw data(`No user found with id: ${headscaleUserId}`, { status: 400 }); } if (user.provider === "oidc") { @@ -73,34 +72,20 @@ export async function userAction({ request, context }: Route.ActionArgs) { }); } - await api.users.rename(userId, newName); + await api.users.rename(headscaleUserId, newName); await context.hsLive.refresh(usersResource, api); return { message: "User renamed successfully" }; } case "reassign_user": { - const userId = formData.get("user_id")?.toString(); + const headplaneUserId = formData.get("headplane_user_id")?.toString(); const newRole = formData.get("new_role")?.toString(); - if (!userId || !newRole) { - throw data("Missing `user_id` or `new_role` in the form data.", { + if (!headplaneUserId || !newRole) { + throw data("Missing `headplane_user_id` or `new_role` in the form data.", { status: 400, }); } - const users = await api.users.list({ id: userId }); - const user = users.find((user) => user.id === userId); - if (!user) { - throw data("Specified user not found", { - status: 400, - }); - } - - const subject = getOidcSubject(user); - if (!subject) { - throw data("Specified user is not an OIDC user or has no subject.", { status: 400 }); - } - - const result = await context.auth.reassignSubject(subject, newRole as Role); - + const result = await context.auth.reassignUser(headplaneUserId, newRole as Role); if (!result) { throw data("Failed to reassign user role.", { status: 500 }); } @@ -112,23 +97,12 @@ export async function userAction({ request, context }: Route.ActionArgs) { throw data("Only the owner can transfer ownership.", { status: 403 }); } - const userId = formData.get("user_id")?.toString(); - if (!userId) { - throw data("Missing `user_id` in the form data.", { status: 400 }); + const headplaneUserId = formData.get("headplane_user_id")?.toString(); + if (!headplaneUserId) { + throw data("Missing `headplane_user_id` in the form data.", { status: 400 }); } - const users = await api.users.list({ id: userId }); - const user = users.find((user) => user.id === userId); - if (!user) { - throw data("Specified user not found", { status: 400 }); - } - - const targetSubject = getOidcSubject(user); - if (!targetSubject) { - throw data("Target user is not an OIDC user or has no subject.", { status: 400 }); - } - - const result = await context.auth.transferOwnership(principal.user.subject, targetSubject); + const result = await context.auth.transferOwnership(principal.user.id, headplaneUserId); if (!result) { throw data("Failed to transfer ownership.", { status: 500 }); } @@ -136,26 +110,15 @@ export async function userAction({ request, context }: Route.ActionArgs) { return { message: "Ownership transferred successfully" }; } case "link_user": { - const userId = formData.get("user_id")?.toString(); + const headplaneUserId = formData.get("headplane_user_id")?.toString(); const headscaleUserId = formData.get("headscale_user_id")?.toString(); - if (!userId || !headscaleUserId) { - throw data("Missing `user_id` or `headscale_user_id` in the form data.", { + if (!headplaneUserId || !headscaleUserId) { + throw data("Missing `headplane_user_id` or `headscale_user_id` in the form data.", { status: 400, }); } - const users = await api.users.list({ id: userId }); - const user = users.find((user) => user.id === userId); - if (!user) { - throw data("Specified user not found", { status: 400 }); - } - - const subject = getOidcSubject(user); - if (!subject) { - throw data("Specified user is not an OIDC user or has no subject.", { status: 400 }); - } - - const linked = await context.auth.linkHeadscaleUserBySubject(subject, headscaleUserId); + const linked = await context.auth.linkHeadscaleUser(headplaneUserId, headscaleUserId); if (!linked) { throw data("That Headscale user is already linked to another account.", { status: 409 }); } diff --git a/app/server/headscale/api/resources/pre-auth-keys.ts b/app/server/headscale/api/resources/pre-auth-keys.ts index 127961e..28a33eb 100644 --- a/app/server/headscale/api/resources/pre-auth-keys.ts +++ b/app/server/headscale/api/resources/pre-auth-keys.ts @@ -65,12 +65,14 @@ export function makePreAuthKeyApi( }); return; } - // Pre-0.28: expire takes user + key string. + // Pre-0.28: expire takes the owning user's ID (a uint64 — Headscale + // rejects names with `proto: invalid value for uint64 field user`) + // plus the key string. await transport.request({ method: "POST", path: "v1/preauthkey/expire", apiKey, - body: { user: key.user?.name ?? "", key: key.key }, + body: { user: key.user?.id ?? "", key: key.key }, }); }, }; diff --git a/app/server/web/auth.ts b/app/server/web/auth.ts index 2c49c26..d194e4d 100644 --- a/app/server/web/auth.ts +++ b/app/server/web/auth.ts @@ -77,13 +77,12 @@ export interface AuthService { linkHeadscaleUser(userId: string, headscaleUserId: string): Promise; unlinkHeadscaleUser(userId: string): Promise; - linkHeadscaleUserBySubject(subject: string, headscaleUserId: string): Promise; listUsers(): Promise; claimedHeadscaleUserIds(): Promise>; roleForSubject(subject: string): Promise; roleForHeadscaleUser(headscaleUserId: string): Promise; - transferOwnership(currentOwnerSubject: string, newOwnerSubject: string): Promise; - reassignSubject(subject: string, role: Role): Promise; + transferOwnership(currentOwnerUserId: string, newOwnerUserId: string): Promise; + reassignUser(userId: string, role: Role): Promise; pruneExpiredSessions(): Promise; start(): void; stop(): void; @@ -370,23 +369,6 @@ export function createAuthService(opts: AuthServiceOptions): AuthService { .where(eq(users.id, userId)); } - async function linkHeadscaleUserBySubject( - subject: string, - headscaleUserId: string, - ): Promise { - const [user] = await opts.db - .select({ id: users.id }) - .from(users) - .where(eq(users.sub, subject)) - .limit(1); - - if (!user) { - return false; - } - - return linkHeadscaleUser(user.id, headscaleUserId); - } - async function listUsers(): Promise { return opts.db.select().from(users); } @@ -428,13 +410,17 @@ export function createAuthService(opts: AuthServiceOptions): AuthService { } async function transferOwnership( - currentOwnerSubject: string, - newOwnerSubject: string, + currentOwnerUserId: string, + newOwnerUserId: string, ): Promise { + if (currentOwnerUserId === newOwnerUserId) { + return false; + } + const [current] = await opts.db .select() .from(users) - .where(eq(users.sub, currentOwnerSubject)) + .where(eq(users.id, currentOwnerUserId)) .limit(1); if (!current || current.role !== "owner") { @@ -444,10 +430,10 @@ export function createAuthService(opts: AuthServiceOptions): AuthService { const [target] = await opts.db .select() .from(users) - .where(eq(users.sub, newOwnerSubject)) + .where(eq(users.id, newOwnerUserId)) .limit(1); - if (!target || target.id === current.id) { + if (!target) { return false; } @@ -464,24 +450,16 @@ export function createAuthService(opts: AuthServiceOptions): AuthService { return true; } - async function reassignSubject(subject: string, role: Role): Promise { - const currentRole = await roleForSubject(subject); - if (currentRole === "owner") { + async function reassignUser(userId: string, role: Role): Promise { + const [user] = await opts.db.select().from(users).where(eq(users.id, userId)).limit(1); + if (!user || user.role === "owner") { return false; } await opts.db - .insert(users) - .values({ - id: ulid(), - sub: subject, - role, - caps: capsForRole(role), - }) - .onConflictDoUpdate({ - target: users.sub, - set: { role, caps: capsForRole(role), updated_at: new Date() }, - }); + .update(users) + .set({ role, caps: capsForRole(role), updated_at: new Date() }) + .where(eq(users.id, userId)); return true; } @@ -512,13 +490,12 @@ export function createAuthService(opts: AuthServiceOptions): AuthService { findOrCreateUser, linkHeadscaleUser, unlinkHeadscaleUser, - linkHeadscaleUserBySubject, listUsers, claimedHeadscaleUserIds, roleForSubject, roleForHeadscaleUser, transferOwnership, - reassignSubject, + reassignUser, pruneExpiredSessions, start, stop, diff --git a/tests/unit/auth/auth-service.test.ts b/tests/unit/auth/auth-service.test.ts index d23671e..18a685b 100644 --- a/tests/unit/auth/auth-service.test.ts +++ b/tests/unit/auth/auth-service.test.ts @@ -78,15 +78,9 @@ describe("linkHeadscaleUser", () => { const user = users.find((u) => u.id === userId); expect(user?.headscale_user_id).toBeNull(); }); - - test("linkHeadscaleUserBySubject works through subject lookup", async () => { - await auth.findOrCreateUser("sub-1"); - const result = await auth.linkHeadscaleUserBySubject("sub-1", "hs-1"); - expect(result).toBe(true); - }); }); -describe("reassignSubject", () => { +describe("reassignUser", () => { let auth: AuthService; beforeEach(() => { @@ -95,9 +89,9 @@ describe("reassignSubject", () => { test("updates an existing non-owner role", async () => { await auth.findOrCreateUser("sub-owner"); - await auth.findOrCreateUser("sub-user"); + const userId = await auth.findOrCreateUser("sub-user"); - const result = await auth.reassignSubject("sub-user", "admin"); + const result = await auth.reassignUser(userId, "admin"); expect(result).toBe(true); const role = await auth.roleForSubject("sub-user"); @@ -105,18 +99,15 @@ describe("reassignSubject", () => { }); test("returns false for owner demotion attempt", async () => { - await auth.findOrCreateUser("sub-owner"); + const ownerId = await auth.findOrCreateUser("sub-owner"); - const result = await auth.reassignSubject("sub-owner", "member"); + const result = await auth.reassignUser(ownerId, "member"); expect(result).toBe(false); }); - test("creates user with role if subject doesn't exist yet (upsert behavior)", async () => { - const result = await auth.reassignSubject("sub-new", "auditor"); - expect(result).toBe(true); - - const role = await auth.roleForSubject("sub-new"); - expect(role).toBe("auditor"); + test("returns false when user does not exist", async () => { + const result = await auth.reassignUser("01ARZ3NDEKTSV4RRFFQ69G5FAV", "auditor"); + expect(result).toBe(false); }); }); @@ -128,10 +119,10 @@ describe("transferOwnership", () => { }); test("swaps roles: owner→admin, target→owner", async () => { - await auth.findOrCreateUser("sub-owner"); - await auth.findOrCreateUser("sub-target"); + const ownerId = await auth.findOrCreateUser("sub-owner"); + const targetId = await auth.findOrCreateUser("sub-target"); - const result = await auth.transferOwnership("sub-owner", "sub-target"); + const result = await auth.transferOwnership(ownerId, targetId); expect(result).toBe(true); expect(await auth.roleForSubject("sub-owner")).toBe("admin"); @@ -139,24 +130,24 @@ describe("transferOwnership", () => { }); test("returns false if caller is not owner", async () => { - await auth.findOrCreateUser("sub-owner"); - await auth.findOrCreateUser("sub-other"); + const ownerId = await auth.findOrCreateUser("sub-owner"); + const otherId = await auth.findOrCreateUser("sub-other"); - const result = await auth.transferOwnership("sub-other", "sub-owner"); + const result = await auth.transferOwnership(otherId, ownerId); expect(result).toBe(false); }); test("returns false if target doesn't exist", async () => { - await auth.findOrCreateUser("sub-owner"); + const ownerId = await auth.findOrCreateUser("sub-owner"); - const result = await auth.transferOwnership("sub-owner", "sub-ghost"); + const result = await auth.transferOwnership(ownerId, "01ARZ3NDEKTSV4RRFFQ69G5FAV"); expect(result).toBe(false); }); test("returns false if target is same user as owner", async () => { - await auth.findOrCreateUser("sub-owner"); + const ownerId = await auth.findOrCreateUser("sub-owner"); - const result = await auth.transferOwnership("sub-owner", "sub-owner"); + const result = await auth.transferOwnership(ownerId, ownerId); expect(result).toBe(false); }); });