refactor: restrict provider configuration to super admins only
This commit is contained in:
parent
efc112820d
commit
5e4f3fca62
12 changed files with 202 additions and 25 deletions
|
|
@ -134,7 +134,7 @@ export function UserManagement({ currentUser }: { currentUser: { id: string } |
|
|||
</TableRow>
|
||||
</TableHeader>
|
||||
<TableBody>
|
||||
<TableRow className={cn({ hidden: search.length === 0 })}>
|
||||
<TableRow className={cn({ hidden: filteredUsers.length > 0 })}>
|
||||
<TableCell colSpan={4} className="h-24 text-center">
|
||||
No users found.
|
||||
</TableCell>
|
||||
|
|
|
|||
|
|
@ -50,7 +50,7 @@ export function CreateSsoProviderPage() {
|
|||
clientId: "",
|
||||
clientSecret: "",
|
||||
discoveryEndpoint: "",
|
||||
linkMatchingEmails: true,
|
||||
linkMatchingEmails: false,
|
||||
},
|
||||
});
|
||||
|
||||
|
|
|
|||
|
|
@ -1,8 +1,18 @@
|
|||
import { createFileRoute } from "@tanstack/react-router";
|
||||
import { createFileRoute, redirect } from "@tanstack/react-router";
|
||||
import { CreateSsoProviderPage } from "~/client/modules/settings/routes/create-sso-provider";
|
||||
import { fetchUser } from "../../route";
|
||||
|
||||
export const Route = createFileRoute("/(dashboard)/settings/sso/new")({
|
||||
component: RouteComponent,
|
||||
loader: async () => {
|
||||
const authContext = await fetchUser();
|
||||
|
||||
if (authContext.user?.role !== "admin") {
|
||||
throw redirect({ to: "/settings" });
|
||||
}
|
||||
|
||||
return authContext;
|
||||
},
|
||||
staticData: {
|
||||
breadcrumb: () => [{ label: "Settings", href: "/settings" }, { label: "Register SSO Provider" }],
|
||||
},
|
||||
|
|
|
|||
|
|
@ -8,11 +8,7 @@ const handle = ({ request }: { request: Request }) => app.fetch(request.clone())
|
|||
export const Route = createFileRoute("/api/$")({
|
||||
server: {
|
||||
handlers: {
|
||||
GET: handle,
|
||||
POST: handle,
|
||||
DELETE: handle,
|
||||
PUT: handle,
|
||||
PATCH: handle,
|
||||
ANY: handle,
|
||||
},
|
||||
},
|
||||
});
|
||||
|
|
|
|||
|
|
@ -184,7 +184,7 @@ export const ssoProvider = sqliteTable(
|
|||
userId: text("user_id").references(() => usersTable.id, { onDelete: "set null" }),
|
||||
issuer: text("issuer").notNull(),
|
||||
domain: text("domain").notNull(),
|
||||
autoLinkMatchingEmails: int("auto_link_matching_emails", { mode: "boolean" }).notNull().default(true),
|
||||
autoLinkMatchingEmails: int("auto_link_matching_emails", { mode: "boolean" }).notNull().default(false),
|
||||
oidcConfig: text("oidc_config", { mode: "json" }).$type<Record<string, unknown> | null>(),
|
||||
samlConfig: text("saml_config", { mode: "json" }).$type<Record<string, unknown> | null>(),
|
||||
createdAt: integer("created_at", { mode: "timestamp_ms" })
|
||||
|
|
|
|||
|
|
@ -4,6 +4,7 @@ import {
|
|||
type BetterAuthOptions,
|
||||
type MiddlewareContext,
|
||||
type MiddlewareOptions,
|
||||
type User,
|
||||
} from "better-auth";
|
||||
import { drizzleAdapter } from "better-auth/adapters/drizzle";
|
||||
import { admin, createAuthMiddleware, twoFactor, username, organization } from "better-auth/plugins";
|
||||
|
|
@ -16,10 +17,10 @@ import { tanstackStartCookies } from "better-auth/tanstack-start";
|
|||
import { isValidUsername, normalizeUsername } from "~/lib/username";
|
||||
import { ensureOnlyOneUser } from "./auth/middlewares/only-one-user";
|
||||
import { convertLegacyUserOnFirstLogin } from "./auth/middlewares/convert-legacy-user";
|
||||
import { validateSsoCallbackUrls } from "./auth/middlewares/validate-sso-callback-urls";
|
||||
import { createUserDefaultOrg } from "./auth/helpers/create-default-org";
|
||||
import { isSsoCallbackRequest, requireSsoInvitation } from "./auth/middlewares/require-sso-invitation";
|
||||
import { ssoTrustedProviderLinkingPlugin } from "./auth/plugins/sso-trusted-provider-linking";
|
||||
import { logger } from "../utils/logger";
|
||||
|
||||
export type AuthMiddlewareContext = MiddlewareContext<MiddlewareOptions, AuthContext<BetterAuthOptions>>;
|
||||
|
||||
|
|
@ -36,6 +37,7 @@ export const auth = betterAuth({
|
|||
},
|
||||
hooks: {
|
||||
before: createAuthMiddleware(async (ctx) => {
|
||||
await validateSsoCallbackUrls(ctx);
|
||||
await ensureOnlyOneUser(ctx);
|
||||
await convertLegacyUserOnFirstLogin(ctx);
|
||||
}),
|
||||
|
|
@ -53,7 +55,6 @@ export const auth = betterAuth({
|
|||
create: {
|
||||
before: async (user, ctx) => {
|
||||
if (isSsoCallbackRequest(ctx)) {
|
||||
logger.debug("SSO callback detected, checking for pending invitations for user", user.email);
|
||||
await requireSsoInvitation(user.email, ctx);
|
||||
user.hasDownloadedResticPassword = true;
|
||||
}
|
||||
|
|
@ -119,7 +120,15 @@ export const auth = betterAuth({
|
|||
allowUserToCreateOrganization: false,
|
||||
}),
|
||||
sso({
|
||||
trustEmailVerified: true,
|
||||
trustEmailVerified: false,
|
||||
providersLimit: async (user: User) => {
|
||||
const existingUser = await db.query.usersTable.findFirst({
|
||||
columns: { role: true },
|
||||
where: { id: user.id },
|
||||
});
|
||||
|
||||
return existingUser?.role === "admin" ? 10 : 0;
|
||||
},
|
||||
organizationProvisioning: {
|
||||
disabled: false,
|
||||
defaultRole: "member",
|
||||
|
|
|
|||
|
|
@ -69,17 +69,17 @@ describe("requireSsoInvitation", () => {
|
|||
await db.delete(usersTable);
|
||||
});
|
||||
|
||||
test("returns early when context is null", async () => {
|
||||
test("throws when context is null", async () => {
|
||||
await createSsoProvider("oidc-acme");
|
||||
|
||||
expect(requireSsoInvitation("user@example.com", null)).resolves.toBeUndefined();
|
||||
expect(requireSsoInvitation("user@example.com", null)).rejects.toThrow("Missing SSO context");
|
||||
});
|
||||
|
||||
test("returns early when request is not an SSO callback", async () => {
|
||||
test("throws when request is not an SSO callback", async () => {
|
||||
await createSsoProvider("oidc-acme");
|
||||
|
||||
const ctx = createMockContext("/sign-up/email");
|
||||
expect(requireSsoInvitation("user@example.com", ctx)).resolves.toBeUndefined();
|
||||
expect(requireSsoInvitation("user@example.com", ctx)).rejects.toThrow("Missing providerId");
|
||||
});
|
||||
|
||||
test("detects whether current request is an SSO callback", async () => {
|
||||
|
|
@ -105,7 +105,7 @@ describe("requireSsoInvitation", () => {
|
|||
await db.insert(invitation).values({
|
||||
id: randomId(),
|
||||
organizationId,
|
||||
email: "User@Example.com",
|
||||
email: "user@example.com",
|
||||
role: "member",
|
||||
status: "pending",
|
||||
expiresAt: new Date(Date.now() + 60 * 60 * 1000),
|
||||
|
|
@ -117,8 +117,8 @@ describe("requireSsoInvitation", () => {
|
|||
expect(requireSsoInvitation("user@example.com", ctx)).resolves.toBeUndefined();
|
||||
});
|
||||
|
||||
test("returns early when provider is not registered", async () => {
|
||||
test("throws when provider is not registered", async () => {
|
||||
const ctx = createMockSsoCallbackContext("missing-provider");
|
||||
expect(requireSsoInvitation("user@example.com", ctx)).resolves.toBeUndefined();
|
||||
expect(requireSsoInvitation("user@example.com", ctx)).rejects.toThrow("SSO provider not found");
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -1,5 +1,6 @@
|
|||
import { beforeEach, describe, expect, test } from "bun:test";
|
||||
import type { GenericEndpointContext } from "@better-auth/core";
|
||||
import { eq } from "drizzle-orm";
|
||||
import { db } from "~/server/db/db";
|
||||
import { account, member, organization, ssoProvider, usersTable } from "~/server/db/schema";
|
||||
import { isSsoCallbackPath, trustSsoProviderForLinking } from "../trust-sso-provider-for-linking";
|
||||
|
|
@ -146,6 +147,23 @@ describe("trustSsoProviderForLinking", () => {
|
|||
expect(ctx.context.options.account?.accountLinking?.trustedProviders).toEqual(["pocket-id"]);
|
||||
});
|
||||
|
||||
test("removes provider from trusted providers when auto-linking is disabled", async () => {
|
||||
await createSsoProviderRecord("pocket-id", true);
|
||||
|
||||
const ctx = createMockContext({
|
||||
path: "/sso/callback/pocket-id",
|
||||
params: { providerId: "pocket-id" },
|
||||
});
|
||||
|
||||
await trustSsoProviderForLinking(ctx);
|
||||
expect(ctx.context.options.account?.accountLinking?.trustedProviders).toEqual(["pocket-id"]);
|
||||
|
||||
await db.update(ssoProvider).set({ autoLinkMatchingEmails: false }).where(eq(ssoProvider.providerId, "pocket-id"));
|
||||
|
||||
await trustSsoProviderForLinking(ctx);
|
||||
expect(ctx.context.options.account?.accountLinking?.trustedProviders).toEqual([]);
|
||||
});
|
||||
|
||||
test("does nothing when account linking is disabled", async () => {
|
||||
await createSsoProviderRecord("pocket-id", true);
|
||||
|
||||
|
|
|
|||
|
|
@ -0,0 +1,48 @@
|
|||
import { describe, expect, test } from "bun:test";
|
||||
import type { AuthMiddlewareContext } from "~/server/lib/auth";
|
||||
import { validateSsoCallbackUrls } from "../validate-sso-callback-urls";
|
||||
|
||||
function createContext(path: string, body: Record<string, unknown>): AuthMiddlewareContext {
|
||||
return {
|
||||
path,
|
||||
body,
|
||||
query: {},
|
||||
headers: new Headers(),
|
||||
request: new Request(`http://localhost:3000${path}`),
|
||||
params: {},
|
||||
method: "POST",
|
||||
context: {} as AuthMiddlewareContext["context"],
|
||||
} as AuthMiddlewareContext;
|
||||
}
|
||||
|
||||
describe("validateSsoCallbackUrls", () => {
|
||||
test("accepts /login", async () => {
|
||||
const ctx = createContext("/sign-in/sso", { callbackURL: "/login" });
|
||||
|
||||
expect(validateSsoCallbackUrls(ctx)).resolves.toBeUndefined();
|
||||
});
|
||||
|
||||
test("rejects https://evil.example", async () => {
|
||||
const ctx = createContext("/sign-in/sso", { callbackURL: "https://evil.example" });
|
||||
|
||||
expect(validateSsoCallbackUrls(ctx)).rejects.toThrow("callbackURL");
|
||||
});
|
||||
|
||||
test("rejects //evil.example", async () => {
|
||||
const ctx = createContext("/sign-in/sso", { callbackURL: "//evil.example" });
|
||||
|
||||
expect(validateSsoCallbackUrls(ctx)).rejects.toThrow("callbackURL");
|
||||
});
|
||||
|
||||
test("rejects /sso/callback/foo", async () => {
|
||||
const ctx = createContext("/sign-in/sso", { callbackURL: "/sso/callback/foo" });
|
||||
|
||||
expect(validateSsoCallbackUrls(ctx)).rejects.toThrow("callbackURL");
|
||||
});
|
||||
|
||||
test("rejects /sso/saml2/foo", async () => {
|
||||
const ctx = createContext("/sign-in/sso", { callbackURL: "/sso/saml2/foo" });
|
||||
|
||||
expect(validateSsoCallbackUrls(ctx)).rejects.toThrow("callbackURL");
|
||||
});
|
||||
});
|
||||
|
|
@ -23,15 +23,21 @@ export async function trustSsoProviderForLinking(ctx: GenericEndpointContext): P
|
|||
return;
|
||||
}
|
||||
|
||||
const provider = await db.query.ssoProvider.findFirst({ where: { providerId, autoLinkMatchingEmails: true } });
|
||||
const provider = await db.query.ssoProvider.findFirst({
|
||||
columns: { organizationId: true },
|
||||
where: { providerId },
|
||||
});
|
||||
if (!provider) {
|
||||
return;
|
||||
}
|
||||
|
||||
const trustedProviders = accountLinking.trustedProviders ?? [];
|
||||
if (trustedProviders.includes(providerId)) {
|
||||
return;
|
||||
}
|
||||
const autoLinkingProviders = await db.query.ssoProvider.findMany({
|
||||
columns: { providerId: true },
|
||||
where: {
|
||||
organizationId: provider.organizationId,
|
||||
autoLinkMatchingEmails: true,
|
||||
},
|
||||
});
|
||||
|
||||
accountLinking.trustedProviders = [...trustedProviders, providerId];
|
||||
accountLinking.trustedProviders = autoLinkingProviders.map((entry) => entry.providerId);
|
||||
}
|
||||
|
|
|
|||
|
|
@ -0,0 +1,36 @@
|
|||
import { APIError } from "better-auth/api";
|
||||
import type { AuthMiddlewareContext } from "~/server/lib/auth";
|
||||
|
||||
function isValidCallbackPath(value: string): boolean {
|
||||
if (!value.startsWith("/") || value.startsWith("//") || value.includes("\\")) {
|
||||
return false;
|
||||
}
|
||||
|
||||
if (value.startsWith("/sso/callback/") || value.startsWith("/sso/saml2/")) {
|
||||
return false;
|
||||
}
|
||||
|
||||
return true;
|
||||
}
|
||||
|
||||
export const validateSsoCallbackUrls = async (ctx: AuthMiddlewareContext) => {
|
||||
if (ctx.path !== "/sign-in/sso") {
|
||||
return;
|
||||
}
|
||||
|
||||
const sources = [ctx.body, ctx.query].filter((s) => s && typeof s === "object");
|
||||
|
||||
for (const source of sources) {
|
||||
const payload = source as Record<string, unknown>;
|
||||
|
||||
for (const field of ["callbackURL", "errorCallbackURL", "newUserCallbackURL"]) {
|
||||
const value = payload[field];
|
||||
|
||||
if (value !== undefined && (typeof value !== "string" || !isValidCallbackPath(value))) {
|
||||
throw new APIError("BAD_REQUEST", {
|
||||
message: `Invalid ${field}. Only relative paths like /login are allowed.`,
|
||||
});
|
||||
}
|
||||
}
|
||||
}
|
||||
};
|
||||
54
app/server/modules/auth/__tests__/auth.sso-security.test.ts
Normal file
54
app/server/modules/auth/__tests__/auth.sso-security.test.ts
Normal file
|
|
@ -0,0 +1,54 @@
|
|||
import { beforeEach, describe, expect, test } from "bun:test";
|
||||
import { createApp } from "~/server/app";
|
||||
import { account, invitation, member, organization, ssoProvider, usersTable } from "~/server/db/schema";
|
||||
import { db } from "~/server/db/db";
|
||||
|
||||
const app = createApp();
|
||||
|
||||
describe("auth SSO sign-in security", () => {
|
||||
beforeEach(async () => {
|
||||
await db.delete(member);
|
||||
await db.delete(account);
|
||||
await db.delete(invitation);
|
||||
await db.delete(ssoProvider);
|
||||
await db.delete(organization);
|
||||
await db.delete(usersTable);
|
||||
});
|
||||
|
||||
test("rejects malicious callback URL", async () => {
|
||||
const response = await app.request("/api/auth/sign-in/sso", {
|
||||
method: "POST",
|
||||
headers: {
|
||||
"Content-Type": "application/json",
|
||||
},
|
||||
body: JSON.stringify({
|
||||
providerId: "missing-provider",
|
||||
callbackURL: "https://evil.example",
|
||||
}),
|
||||
});
|
||||
|
||||
expect(response.status).toBe(400);
|
||||
|
||||
const body = await response.json();
|
||||
expect(body.code).toContain("CALLBACKURL");
|
||||
expect(body.message).toContain("callbackURL");
|
||||
});
|
||||
|
||||
test("allows relative callback URL to continue normal flow", async () => {
|
||||
const response = await app.request("/api/auth/sign-in/sso", {
|
||||
method: "POST",
|
||||
headers: {
|
||||
"Content-Type": "application/json",
|
||||
},
|
||||
body: JSON.stringify({
|
||||
providerId: "missing-provider",
|
||||
callbackURL: "/login",
|
||||
}),
|
||||
});
|
||||
|
||||
expect(response.status).toBe(404);
|
||||
|
||||
const body = await response.json();
|
||||
expect(body.code).toBe("NO_PROVIDER_FOUND_FOR_THE_ISSUER");
|
||||
});
|
||||
});
|
||||
Loading…
Reference in a new issue