-
Notifications
You must be signed in to change notification settings - Fork 112
feat: SSO configuration and auth method management #2686
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
946968a
7ad8708
0dd7d14
1bcfc16
5237929
2d50400
0800710
7904496
d5ade89
7b70907
aec9f75
1cc7ccc
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| --- | ||
| "@inkeep/agents-core": minor | ||
| "@inkeep/agents-api": minor | ||
| "@inkeep/agents-manage-ui": minor | ||
| --- | ||
|
|
||
| Add SSO configuration, auth method management, and domain-filtered login and invitation flows |
This file was deleted.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,105 @@ | ||
| import { createApiError, getAuthLookupForEmail } from '@inkeep/agents-core'; | ||
| import { Hono } from 'hono'; | ||
| import { HTTPException } from 'hono/http-exception'; | ||
| import runDbClient from '../../../data/db/runDbClient'; | ||
| import { getLogger } from '../../../logger'; | ||
| import type { ManageAppVariables } from '../../../types/app'; | ||
|
|
||
| const logger = getLogger('auth-lookup'); | ||
|
|
||
| const RATE_LIMIT_WINDOW_MS = 60_000; | ||
| const RATE_LIMIT_MAX_REQUESTS = 20; | ||
| const ipRequestTimestamps = new Map<string, number[]>(); | ||
|
|
||
| setInterval(() => { | ||
| const cutoff = Date.now() - RATE_LIMIT_WINDOW_MS; | ||
| for (const [ip, timestamps] of ipRequestTimestamps) { | ||
| const recent = timestamps.filter((t) => t > cutoff); | ||
| if (recent.length === 0) { | ||
| ipRequestTimestamps.delete(ip); | ||
| } else { | ||
| ipRequestTimestamps.set(ip, recent); | ||
| } | ||
| } | ||
| }, RATE_LIMIT_WINDOW_MS); | ||
|
|
||
| function getClientIp(c: { req: { header: (name: string) => string | undefined } }): string { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💭 Consider: IP extraction trusts proxy headers unconditionally Issue: The curl -H 'x-forwarded-for: fake-ip-1' /api/auth-lookup?email=test@example.com
curl -H 'x-forwarded-for: fake-ip-2' /api/auth-lookup?email=test@example.comWhy: This is a common pattern and typically safe when deployed behind a trusted proxy that overwrites incoming Fix: This is deployment-dependent. Options:
No action required if your deployment is behind a properly configured proxy (Vercel, Cloudflare, etc.). |
||
| return ( | ||
| c.req.header('x-forwarded-for')?.split(',')[0]?.trim() || c.req.header('x-real-ip') || 'unknown' | ||
| ); | ||
| } | ||
|
|
||
| const authLookupRoutes = new Hono<{ Variables: ManageAppVariables }>(); | ||
|
|
||
| /** | ||
| * GET /api/auth-lookup?email=user@example.com | ||
| * | ||
| * Unauthenticated endpoint for the email-first login flow. | ||
| * Returns org-aware auth methods: | ||
| * 1. Checks SSO providers by email domain -> resolves org -> returns org's allowed methods (SSO filtered to domain-matched providers) | ||
| * 2. Checks existing user account -> resolves org membership -> returns org's allowed methods (SSO filtered to domain-matched providers) | ||
| * | ||
| * Returns empty organizations array if no match is found. | ||
| * | ||
| * Rate-limited per IP to mitigate email/org enumeration. | ||
| * organizationSlug is intentionally omitted from the response to minimize info disclosure. | ||
| */ | ||
| authLookupRoutes.get('/', async (c) => { | ||
| const clientIp = getClientIp(c); | ||
| const now = Date.now(); | ||
| const timestamps = ipRequestTimestamps.get(clientIp) || []; | ||
| const recent = timestamps.filter((t) => t > now - RATE_LIMIT_WINDOW_MS); | ||
|
|
||
| if (recent.length >= RATE_LIMIT_MAX_REQUESTS) { | ||
| logger.warn({ clientIp, count: recent.length }, 'auth-lookup rate limit exceeded'); | ||
| throw createApiError({ | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💭 Consider: Adding Issue: The 429 response doesn't include a Why: The codebase's Fix: Optional improvement: // Calculate time until oldest request expires from window
const oldestTimestamp = recent[0];
const retryAfterSeconds = Math.ceil((RATE_LIMIT_WINDOW_MS - (now - oldestTimestamp)) / 1000);
// Could add to response headers if createApiError supported it
// For now, the message "try again later" is sufficientLow priority since this endpoint is called by the login UI which handles the error gracefully. |
||
| code: 'too_many_requests', | ||
| message: 'Too many requests. Please try again later.', | ||
| }); | ||
| } | ||
|
|
||
| recent.push(now); | ||
| ipRequestTimestamps.set(clientIp, recent); | ||
|
|
||
| const email = c.req.query('email'); | ||
|
|
||
| if (!email) { | ||
| throw createApiError({ | ||
| code: 'bad_request', | ||
| message: 'Email parameter is required', | ||
| }); | ||
| } | ||
|
|
||
| const emailRegex = /^[^\s@]+@[^\s@]+\.[^\s@]+$/; | ||
| if (!emailRegex.test(email)) { | ||
| throw createApiError({ | ||
| code: 'bad_request', | ||
| message: 'Invalid email format', | ||
| }); | ||
| } | ||
|
|
||
| try { | ||
| const organizations = await getAuthLookupForEmail(runDbClient)(email); | ||
|
|
||
| const sanitized = organizations.map(({ organizationSlug: _slug, ...rest }) => rest); | ||
|
|
||
| logger.info( | ||
| { clientIp, emailDomain: email.split('@')[1], orgCount: organizations.length }, | ||
| 'auth-lookup completed' | ||
| ); | ||
|
|
||
| return c.json({ organizations: sanitized }); | ||
| } catch (error) { | ||
omar-inkeep marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| if (error instanceof HTTPException) { | ||
| throw error; | ||
| } | ||
|
|
||
| logger.error({ clientIp, error }, 'auth-lookup failed'); | ||
| throw createApiError({ | ||
| code: 'internal_server_error', | ||
| message: 'Failed to look up authentication method', | ||
| }); | ||
| } | ||
| }); | ||
|
|
||
| export default authLookupRoutes; | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟠 MAJOR: In-memory rate limiter is per-instance only
Issue: The rate limiter uses a module-level
Map<string, number[]>which is process-local. In a multi-instance deployment behind a load balancer, each instance maintains its own independent rate counter. An attacker can bypass the limit by distributing requests across instances.Why: At 20 req/min per instance with N instances, an attacker could effectively enumerate N × 20 emails per minute. The codebase explicitly acknowledges multi-instance deployments as a supported configuration (see
getInProcessFetch()pattern).Fix: This is acceptable as defense-in-depth for single-instance or light-traffic scenarios, but consider:
For now, adding a comment acknowledging this limitation would be helpful:
Refs: