Skip to content

enhance: move local auth owner creation to dedicated route /admin/setup - #7943

Merged
ivyjeong13 merged 5 commits into
obot-platform:mainfrom
ivyjeong13:enhance/move-create-local-auth-owner-to-dedicated-page
Sep 17, 2026
Merged

ivyjeong13 merged 5 commits into
obot-platform:mainfrom
ivyjeong13:enhance/move-create-local-auth-owner-to-dedicated-page

Conversation

@ivyjeong13

@ivyjeong13 ivyjeong13 commented Sep 16, 2026

Copy link
Copy Markdown
Collaborator

Addresses #7942

This addresses so that, on bootstrap login, local auth owner setup is its own separate route/experience rather than being dialogs overlayed over the Auth Providers page.

Also includes following asks: Hide Dashboard from standard users and redirect them to /vmcps instead of /dashboard

@ivyjeong13
ivyjeong13 force-pushed the enhance/move-create-local-auth-owner-to-dedicated-page branch 2 times, most recently from a96aa5d to 073828b Compare September 16, 2026 22:00
@ivyjeong13
ivyjeong13 marked this pull request as ready for review September 16, 2026 22:37
Copilot AI balanced review requested due to automatic review settings September 16, 2026 22:37

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Claude Code Review

This pull request is from a fork — automated review is disabled. A repository maintainer can comment @claude review to run a one-time review.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Route-loop and onboarding regressions can block setup, while configuration failure can leave the owner form unusable.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Adds a dedicated /admin/setup experience for bootstrap users configuring local authentication and the initial owner.

Changes:

  • Adds the setup route and integration tests.
  • Extracts reusable splash, owner prompt, and initial-user components.
  • Updates redirects, responsive styling, and shared splash-state handling.
File summaries
File Description
ui/user/src/routes/login/local/+page.svelte Improves mobile sizing.
ui/user/src/routes/identity-access/AuthProvidersView.svelte Uses the shared owner prompt.
ui/user/src/routes/admin/setup/page.svelte.spec.ts Tests the setup flow.
ui/user/src/routes/admin/setup/+page.ts Loads and guards setup data.
ui/user/src/routes/admin/setup/+page.svelte Implements dedicated setup UI.
ui/user/src/routes/admin/+page.ts Redirects bootstrap users to setup.
ui/user/src/routes/admin/+page.svelte Reuses splash-state constant.
ui/user/src/lib/services/admin/operations.ts Adds request options to EULA loading.
ui/user/src/lib/constants.ts Defines the splash storage key.
ui/user/src/lib/components/navbar/Profile.svelte Reuses splash-state constant.
ui/user/src/lib/components/admin/SetupSplashDialog.svelte Routes bootstrap onboarding to setup.
ui/user/src/lib/components/admin/SetupSplashContent.svelte Extracts shared welcome content.
ui/user/src/lib/components/admin/OwnerSetupPrompt.svelte Extracts owner sign-in prompt.
ui/user/src/lib/components/admin/LocalAuthInitialUserForm.svelte Extracts initial-owner form.
ui/user/src/lib/components/admin/LocalAuthConfigure.svelte Uses the extracted owner form.
Review details
  • Files reviewed: 15/15 changed files
  • Comments generated: 4
  • Review effort level: Balanced

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread ui/user/src/lib/components/admin/SetupSplashDialog.svelte Outdated
Comment thread ui/user/src/lib/components/admin/LocalAuthInitialUserForm.svelte
Comment thread ui/user/src/routes/admin/setup/+page.svelte
Comment thread ui/user/src/lib/components/admin/OwnerSetupPrompt.svelte
Copilot AI review requested due to automatic review settings September 16, 2026 22:42

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Unresolved critical configuration and redirect-loop issues, plus setup-state recovery issues, block approval.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (2)

ui/user/src/lib/components/admin/LocalAuthInitialUserForm.svelte:98

  • When ensureConfigured() returns false, this early return bypasses the create-user catch and the only saving = false assignment. A configuration error therefore leaves the form permanently disabled, so the bootstrap user cannot retry after fixing the provider configuration; reset saving before returning (or wrap the whole submission in a finally).
			if (!(await ensureConfigured())) {
				return;
			}

ui/user/src/routes/admin/setup/+page.svelte:86

  • startOwnerTempLogin() cancels the previous temporary login before requesting a new URL, but this state is not cleared until the request succeeds. After closing the Manage Local Users dialog—or if re-initiation fails—the page can still render the canceled/failed old link, so clicking Sign in uses an invalid token. Clear setupTempLoginUrl before starting the cancellation/re-initiation flow.
	async function prepareSignIn() {
		if (!localProvider) return;
		try {
			const result = await startOwnerTempLogin(localProvider);
			explicitOwners = result.explicitOwners;
			setupTempLoginUrl = result.redirectUrl;
  • Files reviewed: 15/15 changed files
  • Comments generated: 3
  • Review effort level: Lite (auto)

Note

Copilot is running an experiment and ran this review at Lite.

Comment thread ui/user/src/lib/components/admin/LocalAuthInitialUserForm.svelte
Comment thread ui/user/src/routes/admin/setup/+page.svelte
Comment thread ui/user/src/routes/admin/setup/+page.ts
Copilot AI review requested due to automatic review settings September 16, 2026 22:52
@ivyjeong13
ivyjeong13 force-pushed the enhance/move-create-local-auth-owner-to-dedicated-page branch from b786d8f to 320ef57 Compare September 16, 2026 22:54

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Critical setup redirect issues and moderate local-auth state/configuration issues remain unresolved.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (4)

Previously missed (1) — in code that hasn't changed since the last review.

ui/user/src/routes/admin/setup/+page.svelte:89

  • After startOwnerTempLogin fails, step stays 'signin' and setupTempLoginUrl is never reset or retried. On the initial visit this leaves OwnerSetupPrompt showing its indefinite spinner; after managing users, cancelTempLogin() may already have invalidated the old URL, which remains displayed. The bootstrap user is stuck until a full reload. Clear the URL and render a retryable error/action (or return to the owner step) when preparation fails.

ui/user/src/lib/components/admin/LocalAuthInitialUserForm.svelte:40

  • This component is also mounted when the route finds a configured local provider with zero users (for example, after an interrupted setup or deleting the last account), but this unconditionally submits OBOT_AUTH_PROVIDER_EMAIL_DOMAINS=*. That overwrites an existing allowed-domain policy and broadens which local accounts can be created. Only auto-configure when provider is not already configured; otherwise preserve the existing configuration.
	onMount(() => {
		configurePromise = autoConfigure();

ui/user/src/routes/admin/setup/+page.svelte:192

  • This modal is used after local auth is already configured, but its configuration values are hard-coded to *. If the user opens the owner-account manager, goes back from Users to Configuration, and saves, LocalAuthConfigure sends this wildcard to configureAuthProvider and replaces the existing allowed-domain policy. Load/reveal the current provider values before opening this modal, or do not expose the configuration step, instead of supplying a wildcard.
	values={{ OBOT_AUTH_PROVIDER_EMAIL_DOMAINS: '*' }}

ui/user/src/routes/admin/setup/+page.svelte:83

  • After the sign-in prompt has a URL, handleLocalAuthClose calls prepareSignIn again when the owner account is changed. This function leaves the old one-time URL in setupTempLoginUrl while cancellation and initiation are in flight (and if initiation fails), so the prompt can show the new email while its link points to the canceled/old token. Clear the URL before starting a new preparation so the link is not clickable until it belongs to the current account.
		if (!localProvider) return;
		try {
  • Files reviewed: 15/15 changed files
  • Comments generated: 2
  • Review effort level: Lite (auto)

Note

Copilot is running an experiment and ran this review at Lite.

Comment thread ui/user/src/routes/admin/+page.ts
Comment thread ui/user/src/routes/admin/setup/+page.ts Outdated
Copilot AI review requested due to automatic review settings September 16, 2026 23:01

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Five unresolved moderate review findings remain.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (4)

ui/user/src/lib/components/admin/LocalAuthInitialUserForm.svelte:41

  • When /admin/setup is revisited after local auth has already been configured but before its first user exists, provider.configured is true, yet mounting this form still calls autoConfigure() with OBOT_AUTH_PROVIDER_EMAIL_DOMAINS: '*'. That silently replaces any existing domain allow-list and broadens who can create local accounts. Only auto-configure an unconfigured provider, or preserve the existing configuration here.
	onMount(() => {
		configurePromise = autoConfigure();
	});

ui/user/src/routes/admin/+page.ts:16

  • This now sends every bootstrap session to /admin/setup, including a force-enabled bootstrap session after an owner already exists. In that state Bootstrap.Enabled is true but SetupEnabled is false (see pkg/bootstrap/bootstrap_test.go:124-129), so the setup endpoints used by the new page return 404 and the page is left on its sign-in spinner. Preserve the normal admin/auth-provider redirect or gate this redirect on setupEnabled.
	hasVMCPs?: boolean,

ui/user/src/routes/admin/setup/+page.svelte:145

  • This route is only rendered after +page.ts confirms the current profile is the bootstrap user, but unauthorized hides the Profile menu. Unlike the previous Auth Providers flow, the bootstrap user therefore has no UI action to log out or abandon setup, and /admin just redirects back here. Render the authenticated navbar (or add an explicit bootstrap logout action) so setup can be exited.
		<Navbar class="dark:bg-gray-990 sticky top-0 left-0 z-30 w-full" unauthorized />

ui/user/src/routes/admin/setup/+page.svelte:192

  • This management dialog is also opened for an already configured local provider, but it is initialized with a hard-coded *. Saving the Configuration step without changing anything can therefore overwrite a previously restricted email-domain allow-list. Load/reveal the provider's current values before opening this dialog instead of supplying this default.
	values={{ OBOT_AUTH_PROVIDER_EMAIL_DOMAINS: '*' }}
  • Files reviewed: 18/18 changed files
  • Comments generated: 1
  • Review effort level: Lite (auto)

Note

Copilot is running an experiment and ran this review at Lite.

Comment thread ui/user/src/routes/admin/setup/+page.svelte
Copilot AI review requested due to automatic review settings September 16, 2026 23:10
@ivyjeong13
ivyjeong13 force-pushed the enhance/move-create-local-auth-owner-to-dedicated-page branch from 2c233d8 to 8b5dab4 Compare September 16, 2026 23:19

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Unresolved bootstrap routing, dashboard navigation, setup logout, and local-provider domain-preservation issues remain.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (3)

ui/user/src/lib/components/admin/LocalAuthInitialUserForm.svelte:48

  • This form is also selected when the local provider is already configured but has no users. In that case the unconditional auto-configuration posts OBOT_AUTH_PROVIDER_EMAIL_DOMAINS=*, replacing any narrower domain restriction whenever setup is revisited. Skip configuration for an already configured provider (or preserve its existing settings) so entering owner setup cannot broaden who may authenticate.
	async function autoConfigure(): Promise<boolean> {
		configuring = true;
		configError = undefined;
		try {
			const err = await onConfigure({ [DOMAINS_KEY]: '*' });
			if (err) {

ui/user/src/routes/admin/setup/+page.svelte:145

  • This route is rendered for an authenticated bootstrap user, but unauthorized suppresses the Profile menu in Navbar, including the existing bootstrap logout action. The setup flow has no other logout control, so a user who needs to abandon or switch the setup session is trapped on this route; render the authorized navbar here.
		<Navbar class="dark:bg-gray-990 sticky top-0 left-0 z-30 w-full" unauthorized />

ui/user/src/routes/admin/setup/+page.svelte:192

  • The regular local-user management dialog is given a hard-coded * value instead of the provider's current configuration. If an existing local provider has restricted email domains and the user returns to the config step, continuing from this dialog overwrites that restriction with *; load/reveal the current provider credentials before opening the dialog and pass those values here.
	values={{ OBOT_AUTH_PROVIDER_EMAIL_DOMAINS: '*' }}
  • Files reviewed: 19/19 changed files
  • Comments generated: 2
  • Review effort level: Lite (auto)

Note

Copilot is running an experiment and ran this review at Lite.

Comment thread ui/user/src/lib/components/Layout.svelte Outdated
Comment thread ui/user/src/routes/admin/+page.ts
Copilot AI review requested due to automatic review settings September 16, 2026 23:22

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Unresolved issues affect Dashboard role gating, setup redirects, auth-domain preservation, setup completion handling, and sign-out access.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (9)

ui/user/src/lib/components/Layout.svelte:228

  • hasAdminAccess() is true for auditors as well as admins, so a basic user with the AUDITOR role still gets the Dashboard route. The updated Layout.svelte.spec.ts case for [Group.USER, Group.AUDITOR] expects no /dashboard link; use a dashboard-specific privilege check instead of the admin-access predicate.
		...(isAtLeastPoweruser || profile.current.hasAdminAccess?.()
			? [

ui/user/src/lib/components/admin/LocalAuthInitialUserForm.svelte:47

  • This form is now rendered by /admin/setup when the local provider is already configured but has no users, yet mounting it always saves OBOT_AUTH_PROVIDER_EMAIL_DOMAINS=*. That silently broadens an existing local-auth domain restriction before the owner enters an email and leaves the provider accepting any domain; only auto-configure an unconfigured provider.
			const err = await onConfigure({ [DOMAINS_KEY]: '*' });

ui/user/src/lib/components/admin/LocalAuthInitialUserForm.svelte:115

  • If user creation succeeds but the follow-up listLocalAuthUsers() call fails, this sets initialUserError and then still invokes onCreated from finally. Both callers close or advance the flow on that callback, so the read failure is hidden and setup proceeds with unverified state; only complete after a successful refresh, or make the created response authoritative and handle the refresh failure without closing.
			} catch (err) {
				initialUserError = errorMessage(err, 'Failed to list local auth users.');
				userCount = 1;
			} finally {
				onCreated?.(userCount, email);

ui/user/src/routes/+page.ts:27

  • Because hasAdminAccess() is also true for auditors, an auditor entering the root route is redirected to /dashboard, where the new role policy says they should be sent to /vmcps. Keep this redirect aligned with the dashboard guard and sidebar by checking the dashboard-specific role instead.
		const isAtLeastPoweruser =
			profile?.groups.includes(Group.POWERUSER) || profile?.hasAdminAccess?.();
		throw redirect(302, isAtLeastPoweruser ? '/dashboard' : '/vmcps');

ui/user/src/routes/admin/+page.ts:54

  • bootstrapStatus is only populated in the getProfile error branch. After bootstrap login, getProfile succeeds, so this redirect receives undefined for isSetupEnabled and sends the bootstrap user to /dashboard or /vmcps instead of the new /admin/setup flow. Fetch the bootstrap status for an authenticated bootstrap profile before choosing this redirect.
			getAdminRedirectPath(profile, vmcps.length > 0, bootstrapStatus?.setupEnabled)
		);

ui/user/src/routes/admin/+page.ts:29

  • hasAdminAccess() includes auditors, so an auditor entering /admin is redirected to /dashboard even though the updated layout test explicitly hides Dashboard for auditors. This helper should use the dashboard-specific privilege check rather than admin-page access.
	const isAtLeastPoweruser =
		profile?.groups.includes(Group.POWERUSER) || profile?.hasAdminAccess?.();
	return isAtLeastPoweruser ? '/dashboard' : '/vmcps';

ui/user/src/routes/admin/setup/+page.svelte:193

  • The full local-auth management dialog is given a hard-coded wildcard value here. LocalAuthConfigure.resetDialogState() uses values.OBOT_AUTH_PROVIDER_EMAIL_DOMAINS as the editable configuration, so opening Configuration and saving can replace an existing restricted domain list with *. Load the provider's current credentials before opening this dialog instead of supplying a wildcard for an already configured provider.
	values={{ OBOT_AUTH_PROVIDER_EMAIL_DOMAINS: '*' }}
	onConfigure={handleLocalAuthConfigure}

ui/user/src/routes/admin/setup/+page.svelte:145

  • This route is only reachable for an authenticated bootstrap user, but unauthorized hides the Profile menu and there is no other sign-out/leave control on the page. That prevents the bootstrap user from using the existing bootstrapLogout flow to leave setup and return later (the page even documents that revisit case). Render the profile control or add an explicit sign-out action.
		<Navbar class="dark:bg-gray-990 sticky top-0 left-0 z-30 w-full" unauthorized />

ui/user/src/routes/dashboard/+page.ts:10

  • This guard uses hasAdminAccess(), which includes auditors, so an auditor can load the Dashboard directly instead of being redirected to /vmcps. That contradicts the new navigation expectation for [Group.USER, Group.AUDITOR]; use the same dashboard-specific privilege check as the sidebar.
	const isAtLeastPoweruser =
		profile?.groups.includes(Group.POWERUSER) || profile?.hasAdminAccess?.();
  • Files reviewed: 19/19 changed files
  • Comments generated: 1
  • Review effort level: Lite (auto)

Note

Copilot is running an experiment and ran this review at Lite.

Comment thread ui/user/src/lib/components/Layout.svelte
Copilot AI review requested due to automatic review settings September 16, 2026 23:31

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Unresolved critical and moderate review findings remain.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (4)

ui/user/src/lib/components/admin/LocalAuthInitialUserForm.svelte:116

  • This callback is invoked from finally even when listLocalAuthUsers() fails. The setup-page callback immediately replaces this form with the sign-in step, so the list error is hidden and setup proceeds using the synthetic user instead of allowing the user to retry or see the failure. Invoke onCreated only after the user-list request succeeds (or explicitly handle the failed refresh).
			} finally {
				onCreated?.(userCount, email);
				saving = false;

ui/user/src/routes/admin/+page.ts:54

  • bootstrapStatus is only populated in the getProfile error branch above, so a normal bootstrap session reaches this call with isSetupEnabled undefined. Because bootstrap profiles also have admin/owner access, getAdminRedirectPath then skips /admin/setup and sends the user to /vmcps?new=true or /dashboard, making the new setup route unreachable after bootstrap login. Fetch the bootstrap status for authenticated bootstrap profiles before choosing this redirect.
		throw redirect(
			307,
			getAdminRedirectPath(profile, vmcps.length > 0, bootstrapStatus?.setupEnabled)
		);

ui/user/src/routes/admin/setup/+page.svelte:70

  • This catches every cancellation failure, but initiateTempLogin immediately rejects with 409 whenever a temporary user is still cached. If cancellation fails for any reason other than the known 404/no-cache case, the route can remain on the loading state with no usable sign-in link; only the 404 should be ignored and other errors should be propagated.
		} catch {
			// Best-effort. 404 means nothing to cancel; other failures should not block setup.
		}

ui/user/src/routes/admin/setup/+page.svelte:128

  • Clear setupTempLoginUrl before this awaited user-list request. Once the dialog closes, the old link remains rendered during this await; prepareSignIn() will later cancel that token, so a click in the interim follows an invalidated setup link.
		localUsers = await AdminService.listLocalAuthUsers();
  • Files reviewed: 19/19 changed files
  • Comments generated: 1
  • Review effort level: Lite (auto)

Note

Copilot is running an experiment and ran this review at Lite.

Comment thread ui/user/src/lib/components/admin/LocalAuthInitialUserForm.svelte
Copilot AI review requested due to automatic review settings September 16, 2026 23:47

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Unresolved bootstrap-fetch behavior and an unused import that causes lint failure require fixes.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (1)

ui/user/src/lib/components/admin/LocalAuthConfigure.svelte:936

  • LocalAuthConfigure still passes required={!!showInitialAuthProvider} for the legacy/bootstrap provider setup path, but this removes the required-title branch. That dialog will now show Set Up Local in its title bar while the extracted form says Set Up Owner Account; retain the owner-specific title when required is true.
		Set Up {provider?.name}
  • Files reviewed: 20/20 changed files
  • Comments generated: 2
  • Review effort level: Lite (auto)

Note

Copilot is running an experiment and ran this review at Lite.

Comment thread ui/user/src/routes/admin/+page.ts
Comment thread ui/user/src/routes/admin/+page.ts
Copilot AI review requested due to automatic review settings September 16, 2026 23:58
@ivyjeong13
ivyjeong13 force-pushed the enhance/move-create-local-auth-owner-to-dedicated-page branch from 2c39d45 to 40711e7 Compare September 16, 2026 23:59

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Critical lint and multiple moderate routing, role-access, and onboarding issues remain unresolved.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (9)

ui/user/src/lib/components/Layout.svelte:227

  • Role.Groups() emits GroupPowerUserPlus separately from GroupPowerUser (see apiclient/types/user.go:137-141), so this new navigation condition hides Dashboard from Power User Plus-only profiles even though that role is above a standard user. Include Group.POWERUSER_PLUS in the condition.
		...(isAtLeastPoweruser || profile.current.hasAdminAccess?.()

ui/user/src/lib/components/navbar/Profile.svelte:324

  • Role.Groups() emits GroupPowerUserPlus separately from GroupPowerUser (see apiclient/types/user.go:137-141), so a Power User Plus-only profile gets an /vmcps link here instead of the Dashboard. Include Group.POWERUSER_PLUS in this privilege check.
						profile.current.groups.includes(Group.POWERUSER) || profile.current.hasAdminAccess?.()

ui/user/src/routes/+page.ts:29

  • Role.Groups() emits GroupPowerUserPlus separately from GroupPowerUser (see apiclient/types/user.go:137-141), so a Power User Plus-only profile is treated as a standard user here and the post-login landing page becomes /vmcps instead of /dashboard. Include Group.POWERUSER_PLUS in this privilege check.
		const isAtLeastPoweruser =
			profile?.groups.includes(Group.POWERUSER) || profile?.hasAdminAccess?.();
		throw redirect(302, isAtLeastPoweruser ? '/dashboard' : '/vmcps');

ui/user/src/routes/admin/+page.ts:21

  • Role.Groups() emits GroupPowerUserPlus separately from GroupPowerUser (see apiclient/types/user.go:137-141), so a Power User Plus-only profile is treated as a standard user and is sent to /vmcps rather than the Dashboard after visiting /admin. Include Group.POWERUSER_PLUS in this privilege check.
	const isAtLeastPoweruser =
		profile?.groups.includes(Group.POWERUSER) || profile?.hasAdminAccess?.();

ui/user/src/routes/admin/setup/+page.svelte:101

  • After this splash is continued, SetupSplashContent marks SEEN_SPLASH_DIALOG_KEY; when the new owner signs in, the bootstrap session is gone and SetupSplashDialog will not run its existing model-provider handoff. If no model provider is configured, this flow now goes straight to owner sign-in and then /vmcps//dashboard, leaving the required /models?view=model-providers onboarding unreachable. Preserve that continuation across the owner handoff or route to it before marking setup complete.
		if (localUsers.length > 0) {
			step = 'signin';
			await prepareSignIn();
			return;
		}

ui/user/src/routes/admin/setup/+page.svelte:89

  • If listExplicitRoleEmails or initiateTempLogin fails, this catch only appends a notification and leaves setupTempLoginUrl empty. OwnerSetupPrompt then renders only its loading spinner while the page remains on the sign-in step, with no retry or recovery path without reloading; expose an error/retry state or return to a retryable step.
		} catch (err) {
			errors.append(err);
		}

ui/user/src/routes/admin/setup/+page.ts:16

  • This new loader gates a privileged route and chooses among setup, Auth Providers, and Dashboard redirects, but the added spec only renders the component and never exercises these branches. Add loader tests for non-bootstrap/setup-disabled sessions, a non-local configured provider, and local setup so authorization and redirect regressions are caught.
	if (!profile.isBootstrapUser?.() || !bootstrapStatus?.setupEnabled) {
		throw redirect(307, '/dashboard');
	}

ui/user/src/routes/dashboard/+page.ts:10

  • Role.Groups() emits GroupPowerUserPlus separately from GroupPowerUser (see apiclient/types/user.go:137-141), so a Power User Plus-only profile is treated as a standard user here and is redirected to /vmcps instead of being allowed to use Dashboard. Include Group.POWERUSER_PLUS in this privilege check.
	const isAtLeastPoweruser =
		profile?.groups.includes(Group.POWERUSER) || profile?.hasAdminAccess?.();

ui/user/src/routes/identity-access/AuthProvidersView.svelte:713

  • This only moves the markup; prepareOwnerSetup above still runs for bootstrap users on this page and opens setupSignInDialog for a configured local provider. Visiting /identity-access?view=auth-providers after local setup therefore still shows the owner-setup modal, contrary to making local owner setup a standalone /admin/setup experience. Skip the local branch of that effect or redirect bootstrap setup sessions to the new route.
	<OwnerSetupPrompt
		{isLocalSetup}
		localUserEmail={setupLocalUserEmail}
		{explicitOwners}
		provider={configuringAuthProvider}
		tempLoginUrl={setupTempLoginUrl}
		onManageLocalUsers={isLocalSetup ? handleManageLocalUsers : undefined}
		showTitle={false}
	/>
  • Files reviewed: 20/20 changed files
  • Comments generated: 2
  • Review effort level: Lite (auto)

Note

Copilot is running an experiment and ran this review at Lite.

Comment thread ui/user/src/routes/+page.ts Outdated
Comment thread ui/user/src/routes/+page.ts Outdated
Copilot AI review requested due to automatic review settings September 17, 2026 00:08
@ivyjeong13
ivyjeong13 force-pushed the enhance/move-create-local-auth-owner-to-dedicated-page branch from 276882b to 051c812 Compare September 17, 2026 00:09
@ivyjeong13
ivyjeong13 force-pushed the enhance/move-create-local-auth-owner-to-dedicated-page branch from 051c812 to 94f8b29 Compare September 17, 2026 00:11

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

Four unresolved moderate findings affect bootstrap-status handling and local setup routing.

Review details

Suppressed comments (4)

ui/user/src/routes/+page.ts:19

  • This uses the bootstrap enabled flag, but the backend intentionally reports that flag as true when OBOT_SERVER_FORCE_ENABLE_BOOTSTRAP is set even after setup is complete, while setupEnabled remains false. In that recovery mode a bootstrap login is sent to /admin/setup, which immediately redirects to /dashboard and also bypasses any rd target; this handoff should be gated by the setup-specific flag instead.
		if (profile?.isBootstrapUser?.() && bootstrapStatus?.setupEnabled) {
			throw redirect(302, '/admin/setup');

ui/user/src/routes/+page.ts:15

  • This now makes every logged-in user wait for the bootstrap-status endpoint before honoring a normal redirect. That endpoint performs configured-provider and owner lookups and can return an error, so an unrelated bootstrap-status/database failure would make the root route fail for ordinary users even though they never use bootstrap setup. Only fetch this status for unauthenticated sessions or a logged-in bootstrap user.
	const bootstrapStatus = await UserService.getBootstrapStatus();

ui/user/src/routes/admin/+page.ts:35

  • The bootstrap status is only needed here for an unauthenticated bootstrap-login page or for deciding where a bootstrap session goes, but this now fetches it for ordinary logged-in users who visit /admin and should simply receive the existing unauthorized view. Because the status calculation performs provider/owner lookups and can fail, an unrelated backend failure can turn that unauthorized page into a route error; avoid this request for non-bootstrap profiles.
	const bootstrapStatus = await UserService.getBootstrapStatus();

ui/user/src/routes/identity-access/AuthProvidersView.svelte:712

  • This component is still rendered inside the ResponsiveDialog on the Auth Providers page. prepareOwnerSetup opens that dialog for bootstrap sessions whose active provider is local, so a bootstrap user who visits /identity-access still gets the old local owner setup overlay instead of the new /admin/setup experience. Redirect local setup to /admin/setup or suppress this dialog for local providers.
	<OwnerSetupPrompt
		{isLocalSetup}
		localUserEmail={setupLocalUserEmail}
		{explicitOwners}
		provider={configuringAuthProvider}
		tempLoginUrl={setupTempLoginUrl}
		onManageLocalUsers={isLocalSetup ? handleManageLocalUsers : undefined}
		showTitle={false}
  • Files reviewed: 20/20 changed files
  • Comments generated: 0 new
  • Review effort level: Lite (auto)

Note

Copilot is running an experiment and ran this review at Lite.

Copilot AI review requested due to automatic review settings September 17, 2026 00:16
@ivyjeong13
ivyjeong13 merged commit 7066ada into obot-platform:main Sep 17, 2026
5 checks passed

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

Unresolved moderate issues remain in setup failure recovery, bootstrap status lookups, and required-flow title handling.

Review details

Suppressed comments (6)

ui/user/src/lib/components/admin/LocalAuthConfigure.svelte:936

  • The required onboarding dialog still uses this component (for example, AuthProvidersView passes required={!!showInitialAuthProvider}), but this unconditional label changes that flow from “Set Up Owner Account” to “Set Up Local” and contradicts the existing required-flow assertions. Preserve the required-specific title here.
		Set Up {provider?.name}

ui/user/src/lib/components/admin/OwnerSetupPrompt.svelte:84

  • On the dedicated setup route this component is rendered while prepareSignIn() is still pending. If listExplicitRoleEmails() or initiateTempLogin() fails, +page.svelte catches the error but leaves tempLoginUrl empty, so this branch shows a spinner forever with no retry or actionable control. Add an error/retry state for the sign-in preparation failure so bootstrap setup can recover without a full reload.
		{#if tempLoginUrl}
			<a class="btn btn-secondary w-full" href={tempLoginUrl} rel="external">
				{#if provider?.icon}
					<img
						class="bg-base-100 h-6 w-6 rounded-full p-1 dark:bg-gray-600"
						src={provider.icon}
						alt={provider.name}
					/>
				{/if}
				<span class="text-center text-sm font-light">
					{#if isLocalSetup && localUserEmail}
						Sign in as {localUserEmail}
					{:else}
						Continue with {provider?.name}
					{/if}
				</span>
			</a>
		{:else}
			<div class="w-full justify-center items-center flex">
				<Loading class="size-4" />
			</div>

ui/user/src/routes/+page.ts:15

  • This now fetches bootstrap status for every logged-in user before the normal home redirect. A transient failure in the bootstrap/auth-provider lookup can therefore make / fail for standard users, and every such load also pays for a bootstrap database query even though only anonymous or bootstrap sessions need this decision. Fetch it conditionally for those sessions (or otherwise tolerate the failure).
	const bootstrapStatus = await UserService.getBootstrapStatus();

ui/user/src/routes/admin/+page.ts:35

  • This unconditional status request also makes /admin depend on the bootstrap subsystem for every authenticated administrator. The redirect only needs setupEnabled for a bootstrap profile; a failure here now prevents ordinary admins from reaching the admin page, unlike the previous flow. Make the request conditional on profile?.isBootstrapUser?.() (the page component already fetches status for the unauthenticated login view).
	const bootstrapStatus = await UserService.getBootstrapStatus();

ui/user/src/routes/admin/setup/+page.svelte:89

  • If listExplicitRoleEmails or initiateTempLogin fails, this catch only appends a global error and leaves setupTempLoginUrl empty. OwnerSetupPrompt then stays on its loading spinner with no retry or actionable error, so bootstrap setup cannot continue without reloading the page; expose the failed state and provide a retry/error action.
		} catch (err) {
			errors.append(err);
		}

ui/user/src/routes/admin/setup/+page.svelte:83

  • After the user opens the manage-users dialog, setupTempLoginUrl can still contain the previous link. prepareSignIn cancels that link before requesting a replacement, but on any subsequent failure the catch only records a notification, so the page keeps rendering a link that was just canceled. Clear the URL before starting the request so a failed refresh cannot expose a stale sign-in action.
	async function prepareSignIn() {
		if (!localProvider) return;
		try {
  • Files reviewed: 20/20 changed files
  • Comments generated: 0 new
  • Review effort level: Lite (auto)

Note

Copilot is running an experiment and ran this review at Lite.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants