feat: SSO configuration and auth method management#2686
Conversation
…red login flows Add end-to-end SSO support including: - SSO provider configuration UI (SAML/OIDC via Okta, Entra, etc.) - Auth method toggles (email-password, Google, per-SSO-provider) - Domain-filtered method picker on login and invitation pages - Auto-provisioning with pending-invitation precedence - Members page extracted from settings with invitation management - No-org page shows pending invitations for unauthenticated members - UI guard preventing disabling all auth methods - Auth lookup API for email-based org/method discovery Made-with: Cursor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
🦋 Changeset detectedLatest commit: 1cc7ccc The changes in this PR will be included in the next version bump. This PR includes changesets to release 10 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Thorough review of the SSO configuration and auth method management PR. This is a large, well-structured feature touching auth core, data access, API routes, frontend UI, and build config. Found one critical runtime bug (null reference in SSO auto-provisioning), one likely broken package export path, and several security/performance concerns that should be addressed before merging.
Critical:
provisionUserinsso()callsauth.api.addMember(...)butauthresolves to the module-levelnullsentinel — will throw at runtime during SSO auto-provisioning.- The
./authpackage export pointsimportto./dist/auth.jsbut tsdown withunbundle: trueoutputs to./dist/auth/auth.js.
High:
- The unauthenticated
/api/auth-lookupendpoint leaks org names/slugs/IDs for any email — email enumeration and org info disclosure risk. parseAllowedAuthMethodsuses an unsafeascast on untrusted JSON from the DB.- No rate limiting on the unauthenticated auth-lookup endpoint.
Medium:
getTrustedOriginsfires OIDC discovery fetches for every SSO provider on every auth request with no caching.- N+1 query pattern in
getAuthLookupForEmail— redundant org fetches. - Duplicate SpiceDB sync in
beforeAddMember+beforeAcceptInvitation. beforeAddMembernow hard-throws on SpiceDB failure (behavior change from soft-fail).
| await auth.api.addMember({ | ||
| body: { | ||
| userId: user.id, | ||
| organizationId: provider.organizationId, | ||
| role: 'member', | ||
| }, | ||
| }); |
There was a problem hiding this comment.
Bug (critical): auth here resolves to the module-level export const auth = null as unknown as AuthInstance (line 431), not the betterAuth instance being constructed. The old code had const auth = betterAuth({...}); return auth; so the closure captured the real instance, but the refactor changed it to return betterAuth({...}) as unknown as AuthInstance; — there is no local auth variable in scope.
This will throw TypeError: Cannot read properties of null (reading 'api') at runtime when SSO auto-provisioning fires.
Fix: restore a local variable assignment:
const auth = betterAuth({ ... });
// ... (provisionUser closure now captures the real instance)
return auth as unknown as AuthInstance;
packages/agents-core/package.json
Outdated
| "types": "./src/auth/auth.ts", | ||
| "import": "./dist/auth.js" |
There was a problem hiding this comment.
Bug: The import path ./dist/auth.js is almost certainly wrong. With unbundle: true in tsdown, entry src/auth/auth.ts preserves directory structure and outputs to dist/auth/auth.js — consistent with every other ./auth/* export in this file (e.g., ./dist/auth/auth-schema.js, ./dist/auth/auth-types.js). This will cause ERR_MODULE_NOT_FOUND for consumers importing @inkeep/agents-core/auth.
Also: types points to raw .ts source (./src/auth/auth.ts) instead of a .d.ts. This works in-monorepo but will break for published npm consumers since src/ is not in the files array. The second tsdown entry has dts: false and the first excludes auth.ts, so no .d.ts is generated at all.
| export function parseAllowedAuthMethods(raw: string | null | undefined): AllowedAuthMethod[] { | ||
| if (!raw) return [{ method: 'email-password' }]; | ||
| try { | ||
| const parsed = JSON.parse(raw) as AllowedAuthMethod[]; | ||
| if (!Array.isArray(parsed)) return [{ method: 'email-password' }]; | ||
| return parsed; | ||
| } catch { | ||
| return [{ method: 'email-password' }]; | ||
| } |
There was a problem hiding this comment.
JSON.parse(raw) as AllowedAuthMethod[] trusts the shape of data stored in the DB column without validation. If the JSON is malformed (e.g., an SSO entry missing enabled or autoProvision), downstream code like shouldAutoProvision reads undefined for those fields and relies on falsy coercion.
You already have Zod schemas in this file. Consider validating with a schema and falling back to the default on failure, rather than casting.
| try { | ||
| const providers = await dbClient | ||
| .select({ issuer: authSchema.ssoProvider.issuer }) | ||
| .from(authSchema.ssoProvider); | ||
|
|
||
| const issuerOrigins = providers | ||
| .map((p) => { | ||
| try { | ||
| return new URL(p.issuer).origin; | ||
| } catch { | ||
| return null; | ||
| } | ||
| }) | ||
| .filter((origin): origin is string => origin !== null); | ||
|
|
||
| const discoveryResults = await Promise.all( | ||
| providers.map((p) => fetchOidcDiscoveryOrigins(p.issuer)) | ||
| ); | ||
|
|
||
| return [...staticOrigins, ...dynamicOrigins, ...issuerOrigins, ...discoveryResults.flat()]; | ||
| } catch { | ||
| return [...staticOrigins, ...dynamicOrigins]; | ||
| } | ||
| } |
There was a problem hiding this comment.
Performance: getTrustedOrigins is called on every auth request (it's the trustedOrigins callback). It queries the DB for all SSO providers and then fires N outbound OIDC discovery HTTP fetches (each with a 5s timeout) on every invocation. With 5 SSO providers, this adds 5 HTTP round-trips to every auth request.
Consider caching the discovery results with a TTL (e.g., 5-10 minutes keyed by issuer), or computing the trusted origins list at startup and refreshing when providers change.
| const orgRow = await db | ||
| .select({ | ||
| id: organization.id, | ||
| name: organization.name, | ||
| slug: organization.slug, | ||
| allowedAuthMethods: organization.allowedAuthMethods, | ||
| preferredAuthMethod: organization.preferredAuthMethod, | ||
| }) | ||
| .from(organization) | ||
| .where(eq(organization.id, orgId)) | ||
| .limit(1); | ||
|
|
||
| const org = orgRow[0]; | ||
| if (!org) continue; | ||
|
|
||
| const allowed = await getAllowedAuthMethods(db)(orgId); |
There was a problem hiding this comment.
N+1 query: For each orgId from the SSO domain lookup, line 376-386 queries organization to get allowedAuthMethods (among other fields), then getAllowedAuthMethods(db)(orgId) at line 391 re-queries the same table for the same orgId just to read allowedAuthMethods. The data is already available in org.allowedAuthMethods — call parseAllowedAuthMethods(org.allowedAuthMethods) directly.
The same pattern repeats at line 424 for membership orgs — m.allowedAuthMethods is already fetched in the join at line 414.
| setError(null); | ||
| setState({ step: 'org-picker', orgs: [] }); | ||
| handleEmailContinue({ preventDefault: () => {} } as React.FormEvent); |
There was a problem hiding this comment.
Two issues with handleBack from method-picker (multiOrg):
-
Fragile fake event:
handleEmailContinueis called with{ preventDefault: () => {} } as React.FormEvent. If it ever accesses other event properties, this will throw. Extract the async lookup logic into a separate function (e.g.,performEmailLookup()) that bothhandleEmailContinueandhandleBackcall. -
Empty org list flash: State is set to
{ step: 'org-picker', orgs: [] }synchronously, then the lookup fires async. During the network round-trip, the user sees the org-picker with zero org buttons. Either stay on method-picker with a loading indicator, or transition to the email step's spinner instead.
| <Button variant="outline" onClick={onClose}> | ||
| Cancel | ||
| </Button> | ||
| <Button variant="destructive" onClick={handleRemove}> | ||
| Remove | ||
| </Button> |
There was a problem hiding this comment.
The "Remove" button has no loading/disabled state. handleRemove is async (deletes provider → fetches org → updates allowedAuthMethods), but the button remains clickable throughout. A user can click it multiple times, triggering duplicate deletes and a race on the allowedAuthMethods update. Add an isRemoving state and disable both buttons during the operation.
| ); | ||
| } | ||
|
|
||
| if (!orgSettings?.preferredAuthMethod) { | ||
| throw new Error( | ||
| `Organization ${data.organizationId} does not have a preferredAuthMethod configured` | ||
| ); | ||
| } | ||
| const resolvedMethod = data.authMethod || orgSettings.preferredAuthMethod || 'email-password'; |
There was a problem hiding this comment.
The fallback chain data.authMethod || orgSettings.preferredAuthMethod || 'email-password' could produce a method that isn't in the org's allowedAuthMethods list. For example, a caller could pass authMethod: 'google' for an org that only allows email-password. Since allowedAuthMethods is already fetched here, consider validating that resolvedMethod is permitted.
| # Exact pin: better-auth ships breaking changes in patch releases (PR #7363). | ||
| better-auth: 1.4.19 | ||
| "@better-auth/sso": 1.4.19 | ||
| better-auth: 1.5.5 |
There was a problem hiding this comment.
The previous version had a comment explaining why better-auth versions are exact-pinned: # Exact pin: better-auth ships breaking changes in patch releases (PR #7363). The comment was removed but the concern still applies. Consider keeping it so future contributors don't add ^ prefixes.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Review Summary
(12) Total Issues | Risk: High
🔴❗ Critical (2) ❗🔴
🔴 1) auth.ts:225 SSO auto-provisioning will crash due to null reference
Issue: The provisionUser callback references auth.api.addMember(), but auth is the module-level export defined on line 431 as null as unknown as AuthInstance. When an SSO user triggers auto-provisioning, this will throw TypeError: Cannot read properties of null (reading 'api').
Why: The callback executes after createAuth() returns, so auth cannot reference the instance being created. This completely breaks SSO auto-provisioning for all users — any organization with autoProvision: true will fail when new SSO users attempt to sign in.
Fix: Use the database client directly instead of the Better Auth API:
import { addUserToOrganization } from '../data-access/runtime/organizations';
// In provisionUser callback:
await addUserToOrganization(config.dbClient)({
userId: user.id,
organizationId: provider.organizationId,
role: 'member',
});Refs:
🔴 2) package.json:81-84 Build output path mismatch will cause module resolution failures
Issue: The ./auth export's import field points to ./dist/auth.js, but with unbundle: true in tsdown.config.ts, the actual output is ./dist/auth/auth.js (directory structure is preserved).
Why: Consumers importing from @inkeep/agents-core/auth will get "module not found" errors at runtime. This is a ship-blocking build configuration issue.
Fix: Update the import path to match the actual build output:
"./auth": {
"types": "./src/auth/auth.ts",
"import": "./dist/auth/auth.js"
}Refs:
Inline Comments:
- 🔴 Critical:
auth.ts:225SSO auto-provisioning null reference - 🔴 Critical:
package.json:81-84Build output path mismatch
🟠⚠️ Major (6) 🟠⚠️
🟠 1) authLookup.ts:21-43 Auth lookup endpoint enables organization enumeration
Issue: The new unauthenticated /manage/api/auth-lookup endpoint returns organization names, slugs, IDs, and auth methods for any email address. This enables reconnaissance attacks to enumerate organizations, confirm user existence, and fingerprint SSO providers.
Why: While necessary for the email-first login UX, this creates an information disclosure surface without rate limiting or anti-enumeration measures.
Fix: Add rate limiting middleware and consider returning minimal data (just organizationId and methods) for SSO-domain-matched orgs where the user is not already a member.
Refs:
🟠 2) auth-types.ts:94-103 JSON parsing uses unsafe type assertion without validation
Issue: parseAllowedAuthMethods casts parsed JSON directly to AllowedAuthMethod[] without runtime validation. Invalid JSON structures will pass through and cause downstream failures.
Why: Database corruption or manual edits could inject invalid data that the type system trusts but runtime code doesn't handle, causing cryptic errors in auth flows.
Fix: Add Zod validation using a discriminated union schema that enforces required fields per method type.
Refs:
🟠 3) auth-config-utils.ts:207-209 registerSSOProvider silently swallows database errors
Issue: The catch block logs errors but doesn't re-throw or return failure status. Callers cannot distinguish success from failure.
Why: Admins configuring SSO would see success UI while the provider was never saved, leading to users unable to authenticate with no clear diagnosis path.
Fix: Return { success: boolean; error?: string } to let callers handle failures appropriately.
🟠 4) login/page.tsx:123-143 Silent fallback to password step on auth lookup errors
Issue: Both the non-OK response handler and catch block silently fall back to password login without logging or user notification.
Why: SSO-only users will see a password form they cannot use when the auth-lookup API fails. No error trace makes debugging difficult.
Fix: Log errors and show user-facing messages for 5xx errors instead of silent fallback.
🟠 5) scope Missing test coverage for critical auth functions
Issue: The PR deletes auth.test.ts without adding tests for the new auth logic. Functions without test coverage include:
shouldAutoProvision(security-critical authorization decision)getFilteredAuthMethodsForEmail(domain filtering logic)getAuthLookupForEmail(login flow entry point)parseAllowedAuthMethods(JSON parsing)
Why: These functions implement security-critical authorization and authentication logic. Regressions could cause unauthorized access, broken login flows, or role assignment bugs.
Fix: Add unit tests covering the key code paths, especially the invitation precedence check in shouldAutoProvision.
Refs:
🟠 6) sso-configuration.tsx:151-152 Remove button lacks loading state
Issue: The destructive "Remove" button has no disabled state or spinner during async operation, allowing double-clicks.
Why: Double-clicking delete could cause race conditions or confusing error states.
Fix: Add isRemoving state and disable button during the async operation.
Inline Comments:
- 🟠 Major:
authLookup.ts:21-43Organization enumeration - 🟠 Major:
auth-types.ts:94-103Unsafe JSON parsing - 🟠 Major:
auth-config-utils.ts:207-209Silent error swallowing - 🟠 Major:
login/page.tsx:123-143Silent fallback on errors - 🟠 Major:
sso-configuration.tsx:151-152Missing loading state
🟡 Minor (4) 🟡
🟡 1) invite-member-dialog.tsx:191 Debug console.log left in production code
Issue: console.log('invitationResults', invitationResults) should be removed.
🟡 2) accept-invitation/page.tsx:347-351 Empty state message could be more helpful
Issue: The "no sign-in methods available" message doesn't explain domain-based filtering or show the user's domain.
Fix: Include the domain in the message so users understand why SSO isn't available for their email.
🟡 3) members-table.tsx:282 Empty TableHead lacks accessible content
Issue: The actions column header has no screen reader text.
Fix: Add <span className="sr-only">Actions</span>.
🟡 4) accept-invitation/page.tsx:539 Inconsistent aria-hidden on decorative icon
Issue: This AlertCircleIcon lacks aria-hidden unlike the same icon at line 369.
Fix: Add aria-hidden for consistency.
Inline Comments:
- 🟡 Minor:
invite-member-dialog.tsx:191Debug console.log - 🟡 Minor:
accept-invitation/page.tsx:347-351Unhelpful empty state - 🟡 Minor:
members-table.tsx:282Missing sr-only text - 🟡 Minor:
accept-invitation/page.tsx:539Missing aria-hidden
💭 Consider (4) 💭
💭 1) changeset Version bump may need adjustment
Issue: The changeset specifies minor bumps, but this PR removes public exports (createAuth0Provider, createOIDCProvider from agents-api). Removing exported functions is typically a breaking change requiring a major bump.
Fix: Either restore the exports with deprecation warnings, or bump to major version.
💭 2) auth-types.ts:10-17 MethodOption schema allows illegal states
Issue: The Zod schema permits { method: 'sso' } without providerId, contradicting runtime expectations. Consider using a discriminated union.
💭 3) auth-types.ts:74-81 SSOProviderConfig permits neither OIDC nor SAML config
Issue: Both oidcConfig and samlConfig are optional, allowing semantically invalid providers.
💭 4) organizations.ts:375-395 N+1 query pattern in getAuthLookupForEmail
Issue: Loop makes individual database queries for each organization. Consider batching with inArray().
🧹 While You're Here (1) 🧹
🧹 1) scope SSO documentation is missing
Issue: This PR introduces significant SSO functionality but no customer-facing documentation was added. Self-hosted users won't know how to configure SSO providers.
Fix: Add documentation covering: (1) SSO setup guide for OIDC providers, (2) Auth method configuration, (3) Domain filtering behavior.
🚫 REQUEST CHANGES
Summary: This PR introduces valuable SSO configuration and auth method management features, but has two critical blocking issues that must be fixed before merge:
-
SSO auto-provisioning is broken — The
auth.api.addMember()call references a null module export, causing runtime crashes for all SSO users attempting to join organizations. -
Build output path is wrong — The package.json export points to a non-existent file path, breaking all consumers of
@inkeep/agents-core/auth.
Once these are fixed, the remaining Major issues around information disclosure, error handling, and test coverage should be addressed to ensure production reliability.
Discarded (8)
| Location | Issue | Reason Discarded |
|---|---|---|
auth-schema.ts:92 |
Missing FK constraint on ssoProvider.organizationId | Valid observation but pre-existing schema design, not introduced by this PR |
organizations.ts:331-356 |
allowedMethodsToMethodOptions lacks tests | Low severity — pure function with straightforward logic |
device/page.tsx:308 |
Suspense fallback lacks accessible loading | Pattern exists elsewhere in codebase, not introduced by this PR |
link/page.tsx:204 |
AlertCircleIcon lacks aria-hidden | Same pattern as other files, pre-existing |
auth-method-configuration.tsx:171,215 |
Icons lack aria-hidden | Lucide icons may set this by default, needs verification |
login/page.tsx:192 |
Loading spinner lacks accessible text | Valid but minor, pattern exists elsewhere |
login/page.tsx:443-445 |
MethodIcon lacks aria-hidden | Lucide icons may set this by default |
sso-configuration.tsx:188 |
useMemo vs useState for providerId | Working correctly, nitpick |
Reviewers (13)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-standards |
1 | 1 | 0 | 0 | 1 | 0 | 0 |
pr-review-security-iam |
6 | 1 | 2 | 0 | 1 | 0 | 2 |
pr-review-product |
6 | 1 | 0 | 1 | 1 | 0 | 3 |
pr-review-consistency |
9 | 0 | 0 | 0 | 0 | 0 | 9 |
pr-review-breaking-changes |
5 | 0 | 1 | 0 | 0 | 0 | 4 |
pr-review-frontend |
15 | 1 | 0 | 0 | 3 | 0 | 11 |
pr-review-tests |
6 | 1 | 0 | 0 | 0 | 0 | 5 |
pr-review-types |
4 | 1 | 2 | 0 | 1 | 0 | 0 |
pr-review-errors |
15 | 2 | 0 | 0 | 2 | 0 | 11 |
pr-review-architecture |
5 | 0 | 1 | 0 | 0 | 0 | 4 |
pr-review-devops |
7 | 1 | 1 | 0 | 1 | 0 | 4 |
pr-review-docs |
1 | 0 | 0 | 1 | 0 | 0 | 0 |
pr-review-precision |
7 | 1 | 0 | 0 | 1 | 0 | 5 |
| Total | 87 | 10 | 7 | 2 | 11 | 0 | 58 |
| return; | ||
| } | ||
|
|
||
| await auth.api.addMember({ |
There was a problem hiding this comment.
🔴 CRITICAL: SSO auto-provisioning will crash due to null reference
Issue: The provisionUser callback references auth.api.addMember(), but auth is the module-level export defined on line 431 as null as unknown as AuthInstance. When an SSO user triggers auto-provisioning, this will throw TypeError: Cannot read properties of null (reading 'api').
Why: The callback executes after createAuth() returns, so auth cannot reference the instance being created. This breaks the entire SSO auto-provisioning flow.
Fix: Insert the member directly via the database client instead of using the Better Auth API:
| await auth.api.addMember({ | |
| await addUserToOrganization(config.dbClient)({ | |
| userId: user.id, | |
| organizationId: provider.organizationId, | |
| role: 'member', | |
| }); |
Note: You'll need to import addUserToOrganization from '../data-access/runtime/organizations'.
Refs:
| "./auth": { | ||
| "types": "./dist/auth/auth.d.ts", | ||
| "import": "./dist/auth/auth.js" | ||
| "types": "./src/auth/auth.ts", | ||
| "import": "./dist/auth.js" | ||
| }, |
There was a problem hiding this comment.
🔴 CRITICAL: Build output path mismatch will cause module resolution failures
Issue: The import field points to ./dist/auth.js, but with unbundle: true in tsdown.config.ts, the actual output path is ./dist/auth/auth.js (directory structure is preserved).
Why: Consumers importing from @inkeep/agents-core/auth will get "module not found" errors at runtime because the file doesn't exist at the specified path.
Fix:
| "./auth": { | |
| "types": "./dist/auth/auth.d.ts", | |
| "import": "./dist/auth/auth.js" | |
| "types": "./src/auth/auth.ts", | |
| "import": "./dist/auth.js" | |
| }, | |
| "./auth": { | |
| "types": "./src/auth/auth.ts", | |
| "import": "./dist/auth/auth.js" | |
| }, |
Refs:
| export function parseAllowedAuthMethods(raw: string | null | undefined): AllowedAuthMethod[] { | ||
| if (!raw) return [{ method: 'email-password' }]; | ||
| try { | ||
| const parsed = JSON.parse(raw) as AllowedAuthMethod[]; | ||
| if (!Array.isArray(parsed)) return [{ method: 'email-password' }]; | ||
| return parsed; | ||
| } catch { | ||
| return [{ method: 'email-password' }]; | ||
| } | ||
| } |
There was a problem hiding this comment.
🟠 MAJOR: JSON parsing uses unsafe type assertion without validation
Issue: The parseAllowedAuthMethods function casts parsed JSON directly to AllowedAuthMethod[] without runtime validation. Invalid JSON like [{ "method": "sso" }] (missing required SSO fields) will pass through and cause downstream failures.
Why: Database corruption, manual edits, or future schema changes could inject invalid data that the type system trusts but runtime code doesn't handle.
Fix: Add Zod validation:
const allowedAuthMethodSchema = z.discriminatedUnion('method', [
z.object({ method: z.literal('email-password') }),
z.object({ method: z.literal('google') }),
z.object({
method: z.literal('sso'),
providerId: z.string(),
displayName: z.string(),
autoProvision: z.boolean(),
enabled: z.boolean(),
}),
]);
export function parseAllowedAuthMethods(raw: string | null | undefined): AllowedAuthMethod[] {
if (!raw) return [{ method: 'email-password' }];
try {
const parsed = JSON.parse(raw);
const result = z.array(allowedAuthMethodSchema).safeParse(parsed);
if (!result.success) return [{ method: 'email-password' }];
return result.data;
} catch {
return [{ method: 'email-password' }];
}
}Refs:
| if (!response.ok) { | ||
| setState({ step: 'password' }); | ||
| setIsLookingUp(false); | ||
| return; | ||
| } | ||
|
|
||
| const [formData, setFormData] = useState({ | ||
| email: '', | ||
| password: '', | ||
| }); | ||
| const result = authLookupResponseSchema.parse(await response.json()); | ||
| const orgs = result.organizations; | ||
|
|
||
| const handleSubmit = async (e: React.FormEvent) => { | ||
| if (orgs.length === 0) { | ||
| setState({ step: 'password' }); | ||
| } else if (orgs.length === 1) { | ||
| setState({ step: 'method-picker', org: orgs[0], multiOrg: false }); | ||
| } else { | ||
| setState({ step: 'org-picker', orgs }); | ||
| } | ||
| } catch { | ||
| setState({ step: 'password' }); | ||
| } finally { | ||
| setIsLookingUp(false); | ||
| } |
There was a problem hiding this comment.
🟠 MAJOR: Silent fallback to password step on auth lookup errors
Issue: Both the non-OK response handler (line 124) and catch block (line 139) silently fall back to the password step without logging or user notification. Users won't know why they can't find their SSO options.
Why: If the auth-lookup API is misconfigured, rate-limited, or down, SSO-only users will see a password form they cannot use. Debugging this is difficult since there's no trace of what went wrong.
Fix: Log the error and distinguish server errors from "no orgs found":
if (!response.ok) {
console.error('[login] Auth lookup failed:', response.status);
if (response.status >= 500) {
setError('Unable to look up sign-in options. Please try again.');
setIsLookingUp(false);
return;
}
// 4xx likely means no org found - fallback to password is reasonable
setState({ step: 'password' });
setIsLookingUp(false);
return;
}And in the catch block:
} catch (err) {
console.error('[login] Auth lookup error:', err);
setState({ step: 'password' });
}Refs:
| <Button variant="destructive" onClick={handleRemove}> | ||
| Remove |
There was a problem hiding this comment.
🟠 MAJOR: Remove button lacks loading state during async operation
Issue: The destructive "Remove" button has no disabled state or spinner during the async handleRemove operation. Users can click multiple times, potentially causing duplicate deletion attempts.
Why: Per web-design-guidelines, submit buttons should show a spinner during requests. Double-clicking a delete button could cause race conditions or confusing error states.
Fix: Add loading state:
const [isRemoving, setIsRemoving] = useState(false);
const handleRemove = async () => {
if (!provider) return;
setIsRemoving(true);
try {
// ... existing logic
} finally {
setIsRemoving(false);
}
};
// In the button:
<Button variant="destructive" onClick={handleRemove} disabled={isRemoving}>
{isRemoving ? (
<><Loader2 className="mr-2 h-4 w-4 animate-spin" />Removing...</>
) : (
'Remove'
)}
</Button>Refs:
agents-manage-ui/src/components/members/invite-member-dialog.tsx
Outdated
Show resolved
Hide resolved
agents-manage-ui/src/app/accept-invitation/[invitationId]/page.tsx
Outdated
Show resolved
Hide resolved
Ito Test Report ❌15 test cases ran. 14 passed, 1 failed. 🔍 Verification focused on authentication routing, invitation handling, and SSO lifecycle behavior. Most covered flows behaved correctly. One production-code defect is confirmed in SSO provider removal: removing a provider can persist an empty ✅ Passed (14)
❌ Failed (1)
Remove SSO provider cleans allowedAuthMethods and removes login option – Failed
📋 View Recording |
There was a problem hiding this comment.
PR Review Summary
(1) Total Issues | Risk: High
Delta Assessment
This is a delta-scoped re-review following the previous review (commit 946968a6c9a2) and Ito automated test results.
✅ Critical Issues FIXED in the delta commit (7ad8708):
-
SSO auto-provisioning null reference — Now correctly uses
instance.api.addMember()instead of referencing the module-levelauthplaceholder. The fix at line 225 captures the localinstancevariable properly. -
Build output path mismatch — The
./authexport now correctly points to./dist/auth/auth.js(with.d.tstypes), matching the unbundled tsdown output structure. -
Type improvements —
sessionAuth.tsnow has proper middleware typing, andapp.tsderivesAuthInstancefromReturnType<typeof createAuth>instead of importing an internal type. -
Debug code removed —
console.log('invitationResults', ...)has been removed frominvite-member-dialog.tsx.
🔴❗ Critical (1) ❗🔴
🔴 1) sso-configuration.tsx:104 SSO provider removal bypasses last-method safety guard (Ito LOGIC-4)
Issue: The RemoveSSODialog.handleRemove() function deletes SSO providers without checking if this is the last enabled auth method. The safety guard in auth-method-configuration.tsx (lines 98-100, 114-117, 131-133) only protects the toggle switches, not the "Remove" action.
Why: If an organization has only one auth method enabled (an SSO provider), removing it will leave allowedAuthMethods empty or with all methods disabled. Users will see "No sign-in methods available" on the login page and be locked out of the organization. This was confirmed by Ito automated testing (LOGIC-4 test case failed).
Fix: Add a pre-removal check that validates at least one other enabled auth method exists before proceeding with deletion. See inline comment for implementation.
Refs:
Inline Comments:
- 🔴 Critical:
sso-configuration.tsx:104SSO provider removal bypasses last-method safety guard
🕐 Pending Recommendations (10)
Issues from the prior review that remain applicable and should still be addressed:
- 🟠
authLookup.ts:43Auth lookup endpoint enables organization enumeration (rate limiting recommended) - 🟠
auth-types.ts:103JSON parsing uses unsafe type assertion without validation - 🟠
auth-config-utils.ts:207-209registerSSOProvider silently swallows database errors - 🟠
login/page.tsx:123-143Silent fallback to password step on auth lookup errors - 🟠
scopeMissing test coverage for critical auth functions - 🟡
accept-invitation/page.tsx:347-351Empty state message could be more helpful - 🟡
members-table.tsx:282Empty TableHead lacks accessible content - 🟡
accept-invitation/page.tsx:539Inconsistent aria-hidden on decorative icon - 💭
changesetVersion bump may need adjustment (major for removed exports) - 💭
organizations.ts:375-395N+1 query pattern in getAuthLookupForEmail
🚫 REQUEST CHANGES
Summary: The delta commit successfully fixes both critical issues from the previous review (SSO auto-provisioning null reference and build output path mismatch). However, one new critical issue remains: the Ito LOGIC-4 bug where SSO provider removal can bypass the last-method safety guard and leave users locked out. This must be fixed before merge.
Additionally, the pending recommendations from the prior review (rate limiting, error handling, test coverage) should be addressed to ensure production reliability.
Discarded (0)
No findings were discarded in this delta review.
Reviewers (1)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
orchestrator (manual delta review) |
1 | 1 | 0 | 0 | 1 | 10 | 0 |
| Total | 1 | 1 | 0 | 0 | 1 | 10 | 0 |
Note: This was a small delta (6 files), so manual orchestrator review was sufficient without dispatching subagents.
| }: RemoveSSODialogProps) { | ||
| const authClient = useAuthClient(); | ||
|
|
||
| const handleRemove = async () => { |
There was a problem hiding this comment.
🔴 CRITICAL: SSO provider removal can leave zero enabled auth methods (Ito LOGIC-4)
Issue: The handleRemove function deletes the SSO provider and filters it from allowedAuthMethods, but does NOT check if this is the last enabled auth method. This bypasses the "at least one sign-in method" guard that exists in auth-method-configuration.tsx (lines 98-100, 114-117, 131-133).
Why: If an organization has only one auth method enabled (an SSO provider) and an admin removes it, users will be locked out with "No sign-in methods available" on the login page. This was confirmed by Ito automated testing (LOGIC-4).
Fix: Add a check before removing:
| const handleRemove = async () => { | |
| const handleRemove = async () => { | |
| if (!provider) return; | |
| // Check if this is the last enabled auth method | |
| const orgResult = await authClient.organization.getFullOrganization({ | |
| query: { organizationId }, | |
| }); | |
| const currentRaw = orgResult.data?.allowedAuthMethods; | |
| if (currentRaw) { | |
| try { | |
| const current = JSON.parse(currentRaw) as { method: string; providerId?: string; enabled?: boolean }[]; | |
| const enabledMethods = current.filter((m) => { | |
| if (m.method === 'sso' && m.providerId === provider.providerId) return false; | |
| if (m.method === 'sso') return m.enabled !== false; | |
| return true; // email-password and google are always "enabled" when present | |
| }); | |
| if (enabledMethods.length === 0) { | |
| toast.error('Cannot remove the last sign-in method. Enable another method first.'); | |
| return; | |
| } | |
| } catch { | |
| // continue with removal if parsing fails | |
| } | |
| } | |
| try { | |
| await authClient.sso.deleteProvider({ providerId: provider.providerId }); |
Refs:
There was a problem hiding this comment.
PR Review Summary
(0) Total Issues | Risk: Low
Delta Assessment
This is a delta-scoped re-review of commit 0dd7d14b7 ("update cypress login") following the previous review at commit 7ad8708ddfef.
✅ Critical Issue FIXED (Ito LOGIC-4):
The SSO provider removal flow now correctly enforces the last-method safety guard:
const hasRemainingMethod =
updated.some((m) => m.method === 'email-password') ||
updated.some((m) => m.method === 'google') ||
updated.some((m) => m.method === 'sso' && m.enabled);
if (!hasRemainingMethod) {
toast.error('At least one sign-in method must remain enabled...');
return;
}The fix correctly:
- Fetches current
allowedAuthMethodsbefore attempting deletion - Filters out the provider being removed
- Validates at least one enabled method would remain (accounting for the different data shapes: presence-based for email-password/google,
enabledflag for SSO) - Shows a user-friendly error and aborts if no methods would remain
- Only proceeds with deletion after validation passes
Other delta changes:
- ✅ Cypress login tests updated to support the new multi-step login flow (email → method picker → password)
- ✅ Removed unused
getAuthMethodLabelandgetAuthMethodIconfunctions fromauth-method-configuration.tsx - ✅ Cleaned up unnecessary dependencies (
openid-client,@better-auth/drizzle-adapter,@better-auth/infra) from package.json files
🕐 Pending Recommendations (10)
Issues from prior reviews that remain applicable (not addressed in this delta):
- 🟠
authLookup.ts:43Auth lookup endpoint enables organization enumeration (rate limiting recommended) - 🟠
auth-types.ts:103JSON parsing uses unsafe type assertion without validation - 🟠
auth-config-utils.ts:207-209registerSSOProvider silently swallows database errors - 🟠
login/page.tsx:123-143Silent fallback to password step on auth lookup errors - 🟠
scopeMissing test coverage for critical auth functions - 🟡
accept-invitation/page.tsx:347-351Empty state message could be more helpful - 🟡
members-table.tsx:282Empty TableHead lacks accessible content - 🟡
accept-invitation/page.tsx:539Inconsistent aria-hidden on decorative icon - 💭
changesetVersion bump may need adjustment (major for removed exports) - 💭
organizations.ts:375-395N+1 query pattern in getAuthLookupForEmail
💡 APPROVE WITH SUGGESTIONS
Summary: The critical LOGIC-4 bug (SSO provider removal bypassing last-method safety guard) has been properly fixed. The Ito automated test failure should now pass. The fix is well-implemented, checking for remaining enabled methods before allowing deletion.
The pending recommendations from prior reviews (rate limiting, error handling, test coverage) remain applicable and should be addressed before or shortly after merge to ensure production reliability, but they are not blocking for this delta.
Reviewers (1)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
orchestrator (delta review) |
0 | 0 | 0 | 0 | 0 | 10 | 0 |
| Total | 0 | 0 | 0 | 0 | 0 | 10 | 0 |
Note: Small delta (7 files, 1 commit) — manual orchestrator review was sufficient without dispatching subagents.
Ito Test Report ❌20 test cases ran. 19 passed, 1 failed. ✅ Most authentication, invitation, members, and security checks behaved as expected under code-first review. ❌ One login navigation defect is still reproducible and aligns with a concrete state-transition bug in the login flow implementation. ✅ Passed (19)
❌ Failed (1)
Login email-first flow resolves to single org method picker – Failed
📋 View Recording |
There was a problem hiding this comment.
PR Review Summary
(0) Total Issues | Risk: Low
Delta Assessment
This is a delta-scoped re-review of commit 1bcfc1687 ("feedback") since the last review at 0dd7d14b7.
✅ Prior Review Feedback Successfully Addressed:
| Prior Issue | Resolution in Delta |
|---|---|
| Auth lookup rate limiting | ✅ Added (20 req/min per IP with cleanup) |
| Org slug info disclosure | ✅ organizationSlug removed from response |
Unsafe JSON parsing (as cast) |
✅ Now validates with Zod discriminated union |
N+1 query in getAuthLookupForEmail |
✅ Fixed — uses already-fetched allowedAuthMethods |
| Login back-nav (Ito ROUTE-1) | ✅ Fixed — preserves fromOrg state in password step |
| Members table accessibility | ✅ Added sr-only text to actions column |
| 429 error code support | ✅ Added to ErrorCode enum and mappings |
💭 Consider (3) 💭
💭 1) authLookup.ts:12 Document in-memory rate limiter limitation
The rate limiter is per-instance (process-local Map). In multi-instance deployments, requests distributed across instances bypass the limit. This is acceptable as defense-in-depth but should be documented. Consider adding a comment noting this limitation and relying on infrastructure-level rate limiting (CDN/WAF) for production.
💭 2) authLookup.ts:26 IP extraction trusts proxy headers
The getClientIp function trusts x-forwarded-for unconditionally. This is standard practice when behind a properly configured proxy (Vercel, Cloudflare), but could allow rate limit bypass if exposed directly. No action needed for typical deployments.
💭 3) authLookup.ts:55 Missing Retry-After header on 429
The rate limit response doesn't include Retry-After. Low priority since the login UI handles this gracefully.
Inline Comments:
- 🟠 Major:
authLookup.ts:12In-memory rate limiter per-instance limitation - 💭 Consider:
authLookup.ts:26IP extraction trusts proxy headers - 💭 Consider:
authLookup.ts:55Missing Retry-After header
🕐 Pending Recommendations (6)
Issues from prior reviews that remain applicable (not addressed in this delta but noted for tracking):
- 🟠
auth-config-utils.ts:207-209registerSSOProvider silently swallows database errors - 🟠
login/page.tsx:123-143Silent fallback to password step on auth lookup errors - 🟠
scopeMissing test coverage for critical auth functions - 🟡
accept-invitation/page.tsx:347-351Empty state message could be more helpful - 💭
changesetVersion bump may need adjustment (major for removed exports) - 🧹
scopeSSO documentation is missing
✅ APPROVE
Summary: The delta successfully addresses all the critical and major feedback from the prior review:
- Rate limiting — Added with reasonable defaults (20 req/min). The in-memory limitation is documented in inline comments and acceptable for defense-in-depth.
- Info disclosure —
organizationSlugremoved from the public response. - Type safety — Zod validation replaces unsafe
ascast inparseAllowedAuthMethods. - Query optimization — N+1 pattern fixed.
- UX bug fix — Login back-navigation now correctly returns to method-picker.
The remaining pending recommendations (error handling, test coverage, docs) are tracked but not blocking for this delta. The rate limiter observations are "Consider" items for hardening rather than functional issues.
Well done addressing the feedback! 🎉
(Note: Unable to submit formal approval due to GitHub App permissions. This review recommends approval.)
Discarded (2)
| Location | Issue | Reason Discarded |
|---|---|---|
authLookup.ts:14-24 |
setInterval without cleanup may delay graceful shutdown | Very minor — interval is lightweight (60s), callback is fast, and Node.js handles this on process exit |
authLookup.ts:86-89 |
Logging orgCount reveals successful lookups | The API response already reveals this; logging it doesn't create additional disclosure |
Reviewers (4)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-standards |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
pr-review-security-iam |
3 | 0 | 2 | 0 | 1 | 0 | 0 |
pr-review-sre |
3 | 0 | 1 | 0 | 0 | 0 | 2 |
pr-review-types |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
| Total | 6 | 0 | 3 | 0 | 1 | 0 | 2 |
Note: Small delta (8 files, 1 commit) focused on addressing prior review feedback. Standards and Types reviewers found no issues — the changes improved code quality.
|
|
||
| const RATE_LIMIT_WINDOW_MS = 60_000; | ||
| const RATE_LIMIT_MAX_REQUESTS = 20; | ||
| const ipRequestTimestamps = new Map<string, number[]>(); |
There was a problem hiding this comment.
🟠 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:
- Document the limitation in the code comments
- Infrastructure-level rate limiting (CDN/WAF) for production
- Future: Redis-based rate limiting if enumeration becomes a real threat
For now, adding a comment acknowledging this limitation would be helpful:
// NOTE: In-memory rate limiter provides per-instance protection only.
// For multi-instance deployments, rely on infrastructure-level rate limiting.Refs:
| } | ||
| }, RATE_LIMIT_WINDOW_MS); | ||
|
|
||
| function getClientIp(c: { req: { header: (name: string) => string | undefined } }): string { |
There was a problem hiding this comment.
💭 Consider: IP extraction trusts proxy headers unconditionally
Issue: The x-forwarded-for header is trusted without validation. An attacker could spoof this header to bypass rate limiting:
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 x-forwarded-for headers. However, if the API is exposed directly or behind a misconfigured proxy, the rate limit can be bypassed.
Fix: This is deployment-dependent. Options:
- Ensure the load balancer/reverse proxy strips incoming
x-forwarded-forheaders (infrastructure fix) - Use Hono's
c.req.ipwith trusted proxy configuration - Add an env flag to control whether proxy headers are trusted
No action required if your deployment is behind a properly configured proxy (Vercel, Cloudflare, etc.).
|
|
||
| if (recent.length >= RATE_LIMIT_MAX_REQUESTS) { | ||
| logger.warn({ clientIp, count: recent.length }, 'auth-lookup rate limit exceeded'); | ||
| throw createApiError({ |
There was a problem hiding this comment.
💭 Consider: Adding Retry-After header to 429 response
Issue: The 429 response doesn't include a Retry-After header to indicate when the client should retry.
Why: The codebase's retryWithBackoff utility in packages/agents-core/src/utils/retry.ts respects Retry-After headers. Without it, clients must guess retry timing.
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.
There was a problem hiding this comment.
PR Review Summary
(0) Total Issues | Risk: Low
Delta Assessment
This is a delta-scoped re-review of commit d5ade894d ("fix test") since the last approved review at 7904496.
Delta contents: 2 files, 9 lines changed — minor code quality improvements and a bugfix.
| File | Change | Assessment |
|---|---|---|
sso-configuration.tsx |
Replaced inline JSON parsing with parseAllowedAuthMethods() in RegisterSSOForm |
✅ Code quality — uses established Zod-validated parsing utility instead of inline try/catch |
init.ts |
Added isServiceAccount: true when creating admin user |
✅ Bugfix — properly sets serviceAccountUserId on the organization record |
✅ Delta Changes Verified
1. sso-configuration.tsx:242 — Good refactor
// Before (inline JSON parsing with silent catch):
let currentMethods: AllowedAuthMethod[] = [];
try {
const raw = orgResult.data?.allowedAuthMethods;
if (raw) currentMethods = JSON.parse(raw);
} catch {
// default to empty
}
// After (uses validated utility):
const currentMethods = parseAllowedAuthMethods(orgResult.data?.allowedAuthMethods);This is consistent with the prior review feedback to use Zod validation. The parseAllowedAuthMethods function (added in commit 1bcfc1687) validates the JSON structure and provides safe fallback behavior.
2. init.ts:142 — Correct fix
The isServiceAccount: true flag ensures the admin user created during pnpm db:auth:init is properly registered as the organization's service account. This sets serviceAccountUserId on the organization record, which other parts of the codebase may depend on.
📊 Ito Test Status
The most recent Ito test report (2026-03-16T16:44:37Z) showed 3 failures:
- LOGIC-3: SSO lifecycle regression causing admin lockout
- EDGE-4: No-org page not showing pending invitations
- ADV-2: Script-like email accepted by auth-lookup
These failures are in different code paths than what this delta modifies:
- LOGIC-3 involves the SSO provider edit flow, not the registration flow touched here
- EDGE-4 involves the no-organization page authorization context
- ADV-2 involves email validation in
authLookup.ts(not modified)
These remain as known issues from prior Ito runs and should be tracked separately from this delta review.
🕐 Pending Recommendations (5)
Issues from prior reviews that remain applicable but are not blocking:
- 🟠
auth-config-utils.ts:207-209registerSSOProvider silently swallows database errors - 🟠
login/page.tsx:123-143Silent fallback to password step on auth lookup errors - 🟡
accept-invitation/page.tsx:347-351Empty state message could be more helpful - 💭
changesetVersion bump may need adjustment (major for removed exports) - 🧹
scopeSSO documentation is missing
✅ APPROVE
Summary: The delta is a minimal code quality improvement commit. The sso-configuration.tsx change properly uses the Zod-validated parseAllowedAuthMethods() utility instead of inline JSON parsing, and the init.ts change correctly sets the service account flag on the admin user.
The Ito test failures from the prior test run are in different code paths and are not regressions introduced by this delta. The previous approval at 7904496 stands with these minor improvements.
This PR is ready to merge once CI passes! 🎉
(Note: Unable to submit formal approval due to GitHub App permissions. This review recommends approval.)
Reviewers (1)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
orchestrator (delta review) |
0 | 0 | 0 | 0 | 0 | 5 | 0 |
| Total | 0 | 0 | 0 | 0 | 0 | 5 | 0 |
Note: Minimal delta (2 files, 1 commit, 9 lines) — direct orchestrator review without subagent dispatch.
Ito Test Report ✅20 test cases ran. 20 passed. The login, settings, SSO lifecycle, members, invitation security, returnUrl hardening, and mobile coverage scenarios completed successfully under code-first review, with no confirmed production-code defects meeting the failure bar in the included results. ✅ Passed (20)
📋 View Recording |
|
TL;DR — Adds per-organization SSO provider management (register/edit/remove OIDC and SAML providers from the UI), an email-first login flow that resolves auth methods by domain, and controlled auto-provisioning that defers to pending invitations. Members management is extracted from the settings page into its own Key changes
Summary ｜ 72 files ｜ 7 commits ｜ base:
|
| Source | Origins |
|---|---|
| Static | localhost:3000, localhost:3002, MANAGE_UI_URL, API_URL, TRUSTED_ORIGIN |
| DB | All sso_provider.issuer origins |
| OIDC discovery | authorization_endpoint, token_endpoint, userinfo_endpoint, jwks_uri, revocation_endpoint, introspection_endpoint origins |
| Request body (register only) | Issuer from POST body + its discovery endpoints |
auth-config-utils.ts · auth.ts · auth.ts (DAL)
Members page extraction and SpiceDB sync improvements
Before: Member management (table, invitations, role assignment, project access) lived inside the settings page; SpiceDB sync ran only in
afterAcceptInvitationand silently swallowed errors.
After: Members have a dedicated/[tenantId]/membersroute with layout, loading skeleton, and page. SpiceDB sync now runs inbeforeAddMember(covering auto-provisioned SSO users) andbeforeAcceptInvitation, and throws on failure instead of logging silently.
members/page.tsx · members/layout.tsx · auth.ts
|
TL;DR — Adds per-organization SSO provider management (register/edit/remove OIDC and SAML providers from the UI), an email-first login flow that resolves auth methods by domain, and controlled auto-provisioning that defers to pending invitations. Members management is extracted from the settings page into its own Key changes
Summary ｜ 72 files ｜ 8 commits ｜ base:
|
| Source | Origins |
|---|---|
| Static | localhost:3000, localhost:3002, MANAGE_UI_URL, API_URL, TRUSTED_ORIGIN |
| DB | All sso_provider.issuer origins |
| OIDC discovery | authorization_endpoint, token_endpoint, userinfo_endpoint, jwks_uri, revocation_endpoint, introspection_endpoint origins |
| Request body (register only) | Issuer from POST body + its discovery endpoints |
auth-config-utils.ts · auth.ts · auth.ts (DAL)
Members page extraction and SpiceDB sync improvements
Before: Member management (table, invitations, role assignment, project access) lived inside the settings page; SpiceDB sync ran only in
afterAcceptInvitationand silently swallowed errors.
After: Members have a dedicated/[tenantId]/membersroute with layout, loading skeleton, and page. SpiceDB sync now runs inbeforeAddMember(covering auto-provisioned SSO users) andbeforeAcceptInvitation, and throws on failure instead of logging silently.
members/page.tsx · members/layout.tsx · auth.ts
|
Claude encountered an error —— View job PR Review Progress
Phase 1 Complete: This is a delta review scoped to changes since the last automated review at commit Key Observations:
Analyzing delta to verify SSO files are unchanged... |
Ito Test Report ❌15 test cases ran. 14 passed, 1 failed. ✅ Most authentication, routing, invitation validation, and settings scenarios executed successfully in this run. 🔍 Verification found one likely product bug in the no-organization invitation entry path where pending invitations can be suppressed in the UI despite valid invitation data. ✅ Passed (14)
❌ Failed (1)
No-organization page surfaces pending invitations and deep links – Failed
📋 View Recording |
imanilie
left a comment
imanilie
left a comment
There was a problem hiding this comment.
Hallo guten Abend bin ich ganz neu in diese abgekommen und sie haben mir Nachrichten gelassen. Ich musste mich melden bei Ihnen und ich hab mich gemeldet
2 participants
Summary
allowedAuthMethodsmanagementshouldAutoProvisionrespects the org toggle and skips auto-provision when a pending invitation exists (preserving explicit roles like Admin)/membersroute with invitation management, role assignment, and project access dialogsPackages changed
agents-coregetFilteredAuthMethodsForEmail,shouldAutoProvisionwith invitation check, new runtime migrationagents-apiagents-manage-uiKey files
packages/agents-core/src/auth/auth.ts— Better Auth config withorganizationProvisioning: { disabled: true }and customprovisionUserpackages/agents-core/src/auth/auth-config-utils.ts—shouldAutoProvision,parseAllowedAuthMethods, auth method utilitiespackages/agents-core/src/data-access/runtime/organizations.ts—getFilteredAuthMethodsForEmail,getAllowedAuthMethods, SSO provider queriesagents-api/src/domains/manage/routes/authLookup.ts— new unauthenticated/manage/api/auth-lookupendpointagents-manage-ui/src/app/login/page.tsx— multi-step login (email → org picker → method picker)agents-manage-ui/src/components/settings/sso-configuration.tsx— SSO provider CRUD UIagents-manage-ui/src/components/settings/auth-method-configuration.tsx— auth method toggles with last-method guardTest plan
SSO Login & Invite Test Cases.mdmanually testedMade with Cursor