feat: local auth bootstrapping - #7671
Conversation
|
@claude review |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
This automated review found real issues on this local-auth bootstrapping PR — see the inline comments (an open-redirect on the change-password route, an ownership-check ordering issue in EnsureInitialOwner, and missing colocated specs for the new routes/component) — so a human review is still needed. Beyond those, I also checked the bootstrap-token disabling logic in pkg/services/config.go (OBOT_BOOTSTRAP_TOKEN is ignored whenever the initial-owner env vars are set) and confirmed it's an intentional mutual-exclusivity design between local-auth bootstrapping and legacy bootstrap-token auth, not a bug.
Extended reasoning...
Beyond the three confirmed inline findings, I examined pkg/services/config.go around line 1069-1117 where bootstrapOptions = append(bootstrapOptions, bootstrap.Disabled()) unconditionally disables OBOT_BOOTSTRAP_TOKEN auth whenever the initial local-auth owner env vars (LocalAuthInitialOwnerEmail/LocalAuthInitialOwnerSetupToken) are configured, regardless of whether the owner has already been provisioned. This looked at first glance like it could permanently lock out bootstrap-token access, but reading the surrounding code confirms it is a deliberate design choice: local-auth owner provisioning and the legacy bootstrap-token mechanism are treated as mutually exclusive auth-bootstrapping paths, with a slog.Warn emitted to inform the operator when the token is being ignored. This is consistent with the ADR's described intent and is not a defect. The other candidate I looked at (missing spec for routes/activate/+page.svelte:61) duplicates one of the three findings already queued as inline comments, so it isn't new information. Given the security-sensitive nature of this change (auth bootstrapping, password-change flows, redirect handling) and the confirmed open-redirect and ordering bugs already flagged inline, this PR clearly warrants human review regardless of this additional check.
There was a problem hiding this comment.
Pull request overview
Adds secure Local-auth bootstrapping for provisioned Obot environments using expiring setup links and restricted password-setup sessions.
Changes:
- Provisions an initial owner and disables bootstrap authentication.
- Adds activation, forced password-change flows, and backend restrictions.
- Updates admin UI, tests, Helm configuration, documentation, and ADR.
Reviewed changes
Copilot reviewed 44 out of 45 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
ui/user/src/routes/change-password/page.svelte.spec.ts |
Tests password-change UI. |
ui/user/src/routes/change-password/+page.ts |
Guards password-change route. |
ui/user/src/routes/change-password/+page.svelte |
Implements password setup form. |
ui/user/src/routes/activate/page.svelte.spec.ts |
Tests account activation UI. |
ui/user/src/routes/activate/+page.svelte |
Exchanges setup-link tokens. |
ui/user/src/routes/+layout.ts |
Redirects restricted users. |
ui/user/src/lib/services/user/types.ts |
Exposes password-change state. |
ui/user/src/lib/services/user/operations.ts |
Adds activation/password APIs. |
ui/user/src/lib/services/admin/types.ts |
Extends Local user metadata. |
ui/user/src/lib/services/admin/operations.ts |
Sends password-change options. |
ui/user/src/lib/constants.ts |
Allows anonymous activation route. |
ui/user/src/lib/components/admin/LocalAuthConfigure.svelte.spec.ts |
Tests admin password controls. |
ui/user/src/lib/components/admin/LocalAuthConfigure.svelte |
Adds forced-change management. |
pkg/storage/openapi/generated/openapi_generated.go |
Updates generated user schema. |
pkg/services/config.go |
Configures initial-owner provisioning. |
pkg/proxy/proxy.go |
Propagates restricted-session state. |
pkg/localauth/users.go |
Implements provisioning and password changes. |
pkg/localauth/users_test.go |
Tests initial-owner lifecycle. |
pkg/localauth/provider.go |
Adds activation sessions and redirects. |
pkg/localauth/password_test.go |
Removes superseded redirect tests. |
pkg/gateway/types/localauth.go |
Adds setup state fields. |
pkg/gateway/server/user.go |
Returns password-change requirement. |
pkg/gateway/client/localauth.go |
Persists activation and completion state. |
pkg/gateway/client/localauth_test.go |
Tests setup-session behavior. |
pkg/gateway/client/identity_user_limit_test.go |
Adapts Local user test API. |
pkg/bootstrap/bootstrap.go |
Supports disabling bootstrap login. |
pkg/bootstrap/bootstrap_test.go |
Tests disabled bootstrap state. |
pkg/auth/redirect.go |
Centralizes safe redirects. |
pkg/auth/redirect_test.go |
Tests redirect sanitization. |
pkg/auth/auth.go |
Extends serialized auth state. |
pkg/api/server/server.go |
Enforces restricted-session access. |
pkg/api/server/password_change_test.go |
Tests allowed restricted requests. |
pkg/api/router/router.go |
Registers activation endpoints. |
pkg/api/handlers/localauth.go |
Handles activation and password completion. |
pkg/api/authz/authz.go |
Authorizes setup-flow requests. |
docs/docs/installation/enabling-authentication.md |
Documents initial-owner setup. |
docs/docs/installation/docker-deployment.md |
Adds Docker setup guidance. |
docs/docs/configuration/server-configuration.md |
Documents new environment variables. |
docs/docs/configuration/auth-providers.md |
Documents forced password changes. |
chart/values.yaml |
Adds Helm setup values. |
chart/templates/secret.yaml |
Validates and stores setup secrets. |
chart/templates/NOTES.txt |
Prints activation guidance. |
chart/templates/internal-configmap.yaml |
Prevents secret ConfigMap exposure. |
apiclient/types/user.go |
Extends API user representation. |
adr/2026-08-14-initial-local-owner-setup.md |
Records the architectural decision. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
6126c1e to
835ba87
Compare
|
@claude review |
There was a problem hiding this comment.
Findings marked 🟡 are optional suggestions and need no follow-up push.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🔴
pkg/gateway/server/oauth.go— The new auth-provider Verify flow is broken end-to-end: redirect() unconditionally calls CreateAPIKeyFromSetupTokenRequest, which still rejects any TokenRequest.Purpose other than TokenRequestPurposeSetup (pkg/gateway/client/apikey.go:240), but Verify() creates its token request with the new TokenRequestPurposeAuthProviderVerify. Every admin who stages a replacement provider and completes the verification login gets an error instead of being promoted back to /admin/auth-providers, so staged providers can never be verified/activated via this feature. Fix: update CreateAPIKeyFromSetupTokenRequest (or add a verify-specific completion path) to accept TokenRequestPurposeAuthProviderVerify, matching the purposes already accepted by CreateTokenRequestState/VerifyTokenRequestState.Extended reasoning...
Admin stages a replacement provider (Stage), calls Verify() which creates a TokenRequest{Purpose: TokenRequestPurposeAuthProviderVerify} and returns a redirectURL to /api/oauth/start/{id}/... . oauth.go's oauth() (line ~39) now accepts this purpose via the widened CreateTokenRequestState (token.go oauthRoundTripPurposes) and starts the OAuth dance. After a successful login with the staged provider, redirect() (oauth.go) calls VerifyTokenRequestState (also widened, succeeds) then unconditionally calls apiContext.GatewayClient.CreateAPIKeyFromSetupTokenRequest(ctx, userID, tr) at line 88. That function (pkg/gateway/client/apikey.go:240) still does
if tr.Purpose != types.TokenRequestPurposeSetup { return error }, so it fails for the verify-purpose token, s.errorToken() records the error and returns HTTP 500 instead of redirecting to tr.CompletionRedirectURL (/admin/auth-providers?verified=...). Activate() can then never be reached because the caller never actually completes a successful sign-in through the staged provider via this path.Verification: normal severity — a new feature this PR adds is broken end-to-end. Verify() (pkg/api/handlers/authprovider.go:306-316) creates a TokenRequest with Purpose=TokenRequestPurposeAuthProviderVerify and redirects to /oauth/start/{id}/{ns}/{name}. The oauth start/redirect handlers (pkg/gateway/server/router.go:65-66 -> oauth.go) now accept this purpose because CreateTokenRequestState (token.go:181) and…
|
@claude review |
538a7c5 to
424e65b
Compare
424e65b to
eefd706
Compare
7405ae6 to
cd4cba1
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Verification state is not bound to staged configuration, and partial provider switches are not retry-safe.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (2)
pkg/controller/handlers/providerconfigurationchange/providerconfigurationchange.go:258
- The switch is not retry-safe after this deletion. If any later step fails (deconfiguring the outgoing provider, updating either status, or advancing a daemon revision), reconciliation retries with the incoming credential already promoted but no replacement credential left. Depending on whether the outgoing credential was removed,
GetConfiguredAuthProviderthen either still returns the outgoing provider and the retry terminates with “no staged configuration”, or returns the incoming provider and fails the expected-outgoing conflict check. This can strand two live credentials or stale provider status instead of completing recovery. Keep enough durable phase/state to recognize and finish a partially applied switch (or make the credential transition transactional) before deleting the replacement record.
// Cleared here rather than in the caller, so there is no window where a provider is both
// configured and still staged against itself.
if _, err := h.gatewayClient.DeleteCredential(ctx, system.ReplacementAuthProviderCredentialContext, authProvider.Name); err != nil {
return fmt.Errorf("clear staged configuration for auth provider %q: %w", authProvider.Name, err)
}
if err := h.deconfigureAuthProvider(ctx, client, outgoingProvider); err != nil {
pkg/api/handlers/authprovider.go:275
- Clearing only the completed verification cache after staging does not reliably invalidate proof for the previous settings. The new settings are already committed if this delete fails, and an earlier in-flight verification callback can complete after the delete and repopulate the cache;
Activatechecks only the provider name, so either case lets proof obtained against old settings activate the new credential. Bind cached proof to a staged credential generation and compare it during activation, or atomically revoke/consume all earlier verification requests as part of re-staging.
// These settings are not the ones any earlier verification ran against, so its result no longer
// describes what activation would promote.
if err := req.GatewayClient.ClearTempUserCache(req.Context()); err != nil {
return fmt.Errorf("failed to clear the verification for the previous settings: %w", err)
- Files reviewed: 68/69 changed files
- Comments generated: 1
- Review effort level: Balanced
| For provisioned trial environments, you can replace the bootstrap token with a secure initial-owner setup link. See [Provision an initial local owner with a secure setup link](./enabling-authentication.md#provision-an-initial-local-owner-with-a-secure-setup-link). | ||
|
|
There was a problem hiding this comment.
Should this (and the other docs changes) be documented? This seems like more of a feature for us than for self-hosted users.
| // Drop any daemon left from an earlier staging on this replica so the next verification uses | ||
| // these settings. Other replicas drop theirs from the revision the change advanced. | ||
| ap.dispatcher.StopAuthProvider(authProvider.Namespace, authProvider.Name) |
There was a problem hiding this comment.
nit: while it doesn't do any harm to stop the provider daemon here, the replica that handled this request should also stop its daemon based on the ProviderSync that will get created by the controller.
| // The ODP requires a verification to be tied to the Owner who started the switch, so holding the | ||
| // ID is not on its own enough to open the replacement provider's login. |
There was a problem hiding this comment.
nit: remove direct reference to ODP
There was a problem hiding this comment.
🟡 Changes recommended
Critical and moderate provider-verification and switch-safety issues remain unresolved.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (5)
pkg/api/handlers/authprovider.go:275
- The verification is invalidated only after the replacement settings are committed, and it is keyed only by provider name. A crash/clear failure leaves the previous credential's proof cached; an in-flight callback can also repopulate that cache after this clear. In either case,
activatecan promote settings that were never verified. Bind the cached verification/token request to a staged-credential revision and require that revision during activation (with invalidation committed alongside re-staging).
// These settings are not the ones any earlier verification ran against, so its result no longer
// describes what activation would promote.
if err := req.GatewayClient.ClearTempUserCache(req.Context()); err != nil {
return fmt.Errorf("failed to clear the verification for the previous settings: %w", err)
pkg/controller/handlers/providerconfigurationchange/providerconfigurationchange.go:258
- The switch is not retry-safe after the replacement is promoted. If clearing the staged credential succeeds and any later deconfiguration, status update, or daemon-revision operation fails, the next reconcile cannot reload the staged source (or sees the incoming provider as configured) and records a terminal error, leaving a partially applied switch. Keep enough durable progress state to resume/rollback each step, or perform the credential transition transactionally before deleting the staged source.
// Cleared here rather than in the caller, so there is no window where a provider is both
// configured and still staged against itself.
if _, err := h.gatewayClient.DeleteCredential(ctx, system.ReplacementAuthProviderCredentialContext, authProvider.Name); err != nil {
return fmt.Errorf("clear staged configuration for auth provider %q: %w", authProvider.Name, err)
}
if err := h.deconfigureAuthProvider(ctx, client, outgoingProvider); err != nil {
ui/user/src/routes/activate/+page.svelte:59
- Activation failures replace the loading state asynchronously, but this notification has no live-region semantics, so assistive technology may not announce that the link was rejected. Mark it as an alert.
ui/user/src/routes/admin/auth-providers/+page.svelte:529 - Provider-switch errors appear only after an asynchronous action and are not announced to screen readers. Add alert semantics to this notification so the failure is surfaced without requiring the user to discover it visually.
ui/user/src/routes/change-password/+page.svelte:75 - This validation/server error is inserted dynamically but is not exposed as a live alert, so screen-reader users may receive no feedback after submitting the form. Mark the notification as
role="alert".
- Files reviewed: 68/69 changed files
- Comments generated: 3
- Review effort level: Balanced
998cd89 to
d357dbe
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved correctness, security, recovery, UI, accessibility, and documentation issues must be addressed.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (7)
docs/docs/installation/enabling-authentication.md:57
- The newly documented mode disables bootstrap login, but Step 2 still tells every operator to use the bootstrap token and never explains how to construct the activation URL. Anyone following this guide with these settings has no usable login instruction. Add the fragment-based activation link and branch Step 2 between initial-owner and bootstrap setup.
| `OBOT_SERVER_LOCAL_AUTH_INITIAL_OWNER_EMAIL` | No | Initial local-auth owner's email. Must be set with the setup token. |
| `OBOT_SERVER_LOCAL_AUTH_INITIAL_OWNER_SETUP_TOKEN` | No | At least 32 characters of high-entropy, randomly generated secret material used to activate the initial owner. Store as a secret; `openssl rand -hex 32` is the recommended generator. |
| `OBOT_SERVER_LOCAL_AUTH_INITIAL_OWNER_SETUP_TOKEN_EXPIRATION_HOURS` | No | Setup-link validity in hours. Defaults to `168`. |
pkg/api/handlers/authprovider.go:275
- Invalidating the old verification only after the staging change completes leaves a race: once the controller publishes the new replacement credential, a concurrent Activate request can still consume the old temp-user cache and switch to settings that were never verified. Bind verification to a credential revision and validate that revision inside the serialized switch, or invalidate it atomically before the new credential becomes visible.
// These settings are not the ones any earlier verification ran against, so its result no longer
// describes what activation would promote.
if err := req.GatewayClient.ClearTempUserCache(req.Context()); err != nil {
return fmt.Errorf("failed to clear the verification for the previous settings: %w", err)
pkg/controller/handlers/providerconfigurationchange/providerconfigurationchange.go:258
- The replacement's staged credential is deleted before
deconfigureAuthProviderand the later status/daemon updates. If any of those operations fails, the retry finds the incoming credential already promoted but no staged credential, records a terminal error, and can leave both providers configured or cleanup half-complete. The switch reconciliation needs an idempotent state machine (or a credential transaction) that can resume every partial state and only retires staging after the outgoing credential is removed.
// configured and still staged against itself.
if _, err := h.gatewayClient.DeleteCredential(ctx, system.ReplacementAuthProviderCredentialContext, authProvider.Name); err != nil {
return fmt.Errorf("clear staged configuration for auth provider %q: %w", authProvider.Name, err)
}
if err := h.deconfigureAuthProvider(ctx, client, outgoingProvider); err != nil {
pkg/gateway/client/token.go:146
- This predicate leaves a verification usable for its full 15-minute expiry even after
VerifyTokenRequestStateconsumes its OAuth state, and it does not bind the request to a provider or staged-settings revision. A browser retaining the cookie can therefore keep reaching the staged login, and an in-flight verification can be reused after another provider/settings set is staged. Persist the target provider and staged revision on the token request, validate them on both OAuth legs, and mark the verification consumed after the callback.
pkg/localauth/users.go:138 - This lookup makes “initial owner” provisioning per-email rather than one-time. After owner A completes setup, changing the deployment email/token to B creates another claimable owner; changing it while A is pending also leaves A’s token and setup sessions valid. That allows deployment settings to mint additional owners instead of only provisioning the first one. Persist a singleton provisioning/completion marker and revoke any superseded pending claim before creating a different email.
ui/user/src/lib/components/admin/McpServerEntryForm.svelte:738 - If this refresh fails after an entry update, the rejection is discarded and
showUpdateExistingDeploymentsConfirmis never set, even when deployments still need updating. The success toast still appears, so users are told the save succeeded without being warned that deployed instances retain the old configuration. Surface the failure or defer the success state until this check completes.
ui/user/src/lib/components/admin/ProviderConfigure.svelte:494 - A custom footer bypasses the component's existing read-only guard. On the auth-provider page, read-only users can therefore see and invoke Discard, Continue, Sign in, and Switch actions; the backend rejects them, but the dialog no longer behaves as a read-only view.
- Files reviewed: 67/68 changed files
- Comments generated: 3
- Review effort level: Balanced
Obot serves logins from one configured auth provider, and "configured" means holding a complete credential in that provider's own context. That made both getting the first owner and changing providers hazardous: the former required a bootstrap token and manual provider setup, and the latter had a window with nothing configured and no session left to fix it. A provisioned deployment can now name an initial Local owner from an email and a high-entropy setup token. Obot configures Local, creates the account with an unusable password, and stores only the token's hash. The activation link carries the token in its fragment, so it never reaches a request line or an access log. Until a password is set, the session is restricted to the password page and the handful of endpoints it needs. Completion is atomic and single-winner. Bootstrap is disabled while this is configured. A replacement provider's settings are staged into a separate credential context, so it is not configured, does not serve logins, and survives a restart without changing who does. A one-time login through it, authorized only for the owner who started the switch and pinned to their browser, proves it works and produces the identity that will hold Owner. Activation promotes the staged credential before deconfiguring the outgoing provider, in one reconcile, so a partial failure still leaves someone able to sign in. Deconfiguring the provider currently serving logins is refused. Signed-off-by: Nick Hale <4175918+njhale@users.noreply.github.com>
Signed-off-by: Nick Hale <4175918+njhale@users.noreply.github.com>
d357dbe to
dc723bb
Compare
There was a problem hiding this comment.
🟡 Changes recommended
A critical provider-verification flaw and multiple moderate security and reliability issues remain unresolved.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (4)
pkg/api/handlers/authprovider.go:275
- The new staged settings are committed before the old verification cache is cleared. If this delete fails or the process stops here, the endpoint returns an error but the new settings remain staged; because activation matches proof only by provider name, the previous verification now authorizes activating settings that were never tested. Invalidate/version the proof atomically with replacing the staged credential (and account for in-flight verification callbacks).
// These settings are not the ones any earlier verification ran against, so its result no longer
// describes what activation would promote.
if err := req.GatewayClient.ClearTempUserCache(req.Context()); err != nil {
return fmt.Errorf("failed to clear the verification for the previous settings: %w", err)
pkg/api/handlers/localauth.go:174
- This cookie-authenticated password mutation has no CSRF/origin check.
Context.Readunmarshals JSON regardless of Content-Type, so a same-site sibling origin can submit a craftedtext/plainform with the victim's Lax cookie, choose an attacker-known password, and complete the owner setup. Require a same-origin request or enforce an equivalent CSRF boundary before accepting the body.
var body localAuthUserRequest
if err := req.Read(&body); err != nil {
return types.NewErrBadRequest("invalid request body: %v", err)
}
pkg/controller/handlers/providerconfigurationchange/providerconfigurationchange.go:258
- The replacement credential is deleted before the outgoing provider is deconfigured and before status/daemon updates complete. If any later operation fails, reconciliation retries without the staged credential (or sees the incoming provider as configured), converts the retry into a terminal error, and leaves the switch partially applied. Keep the operation's retry inputs until all steps finish and make the switched reconciliation explicitly idempotent for partially promoted/deconfigured states.
// Cleared here rather than in the caller, so there is no window where a provider is both
// configured and still staged against itself.
if _, err := h.gatewayClient.DeleteCredential(ctx, system.ReplacementAuthProviderCredentialContext, authProvider.Name); err != nil {
return fmt.Errorf("clear staged configuration for auth provider %q: %w", authProvider.Name, err)
}
if err := h.deconfigureAuthProvider(ctx, client, outgoingProvider); err != nil {
ui/user/src/routes/admin/auth-providers/+page.svelte:557
- This warning says everyone is signed out, but the verification session from the replacement provider is deliberately preserved so the user finishes the switch signed in. Describe the outgoing-provider sessions specifically to avoid misleading the owner about the post-switch state.
- Files reviewed: 67/68 changed files
- Comments generated: 3
- Review effort level: Balanced
Signed-off-by: Nick Hale <4175918+njhale@users.noreply.github.com>
Signed-off-by: Nick Hale <4175918+njhale@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
Critical and moderate authentication-switching and owner-promotion issues remain unresolved.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (3)
Previously missed (1) — in code that hasn't changed since the last review.
pkg/api/handlers/setup/confirm_owner.go:68
PromoteToOwnernow creates aUserRoleChangeitself, butConfirmOwnerstill creates another one immediately below. A newly promoted bootstrap owner therefore emits two role-change resources and triggers duplicate propagation. Remove the old creation block from this handler.
pkg/api/handlers/authprovider.go:275
- Clearing only the cached result after staging does not invalidate an already-issued verification token. An OAuth callback for the previous settings can race this call (or arrive after the same provider is re-staged), repopulate
TempSetupUser, and then satisfyActivatefor credentials it never verified. Bind the token request/cache entry to the staged credential version and reject stale callbacks, or invalidate outstanding verification requests as part of the serialized staging change.
// These settings are not the ones any earlier verification ran against, so its result no longer
// describes what activation would promote.
if err := req.GatewayClient.ClearTempUserCache(req.Context()); err != nil {
return fmt.Errorf("failed to clear the verification for the previous settings: %w", err)
ui/user/src/routes/admin/auth-providers/+page.svelte:559
- This warning is not universally true. Verified providers are linked to an existing Obot user by verified email (
pkg/gateway/client/identity.go:239-243), so a Google-to-GitHub switch using the same verified address reuses the user and their work. Make the copy conditional on provider identity semantics or explain that transfer depends on verified-email matching.
- Files reviewed: 67/68 changed files
- Comments generated: 2
- Review effort level: Balanced
|
btw I reviewed backend only |
| </div> | ||
| <p class="text-muted-content text-xs font-light"> | ||
| Not the right account? | ||
| <button class="text-link underline" onclick={handleVerifyStagedProvider}> |
There was a problem hiding this comment.
disabled={switching} here too!
| // Auth provider mutations disable or replace the way everyone signs in, so they belong to owners | ||
| // and administrators only. An auditor is a read-only role and must not reach them, even though it | ||
| // is granted the reveal route that sits under the same path prefix. | ||
| func TestAuthProviderMutationsAreNotAvailableToAuditors(t *testing.T) { |
There was a problem hiding this comment.
The name of this test function is not indicative of what it is testing.
| // Reveal is a POST but is a read. The rest of the auth-provider POSTs replace or | ||
| // disable how everyone signs in, so an auditor must not inherit them from a prefix. |
There was a problem hiding this comment.
This comment requires some context. Specifically, I don't know what it is calling out about a prefix.
| if err := ap.license.RequireEntitlements(req.Context(), authProvider.Spec.RequiredEntitlements); err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
You can use providers.AuthProviderStatus for this and the missing configuration stuff.
| if !req.UserIsOwner() { | ||
| return types.NewErrHTTP(http.StatusForbidden, "only an owner can verify a replacement auth provider") | ||
| } |
There was a problem hiding this comment.
This should be done in the authorization layer and not here.
|
|
||
| // Activate promotes the staged provider and deconfigures the outgoing one. It requires a recorded | ||
| // verification for the staged provider, which only a successful Verify produces. | ||
| func (ap *AuthProviderHandler) Activate(req api.Context) error { |
There was a problem hiding this comment.
This this also only be available to an owner?
| // Enforced after audit logging is installed and refreshed provider cookies are replayed, so | ||
| // rejected probes stay auditable and a cookie refresh is not lost to a blocked operation. | ||
| if utils.FirstSet(user.GetExtra()["password_change_required"]...) == "true" && !passwordChangeRequestAllowed(req) { | ||
| if strings.HasPrefix(req.URL.Path, "/api/") { |
There was a problem hiding this comment.
If the intention here is to send a forbidden error for all API requests and redirect for UI requests, then this can be
| if strings.HasPrefix(req.URL.Path, "/api/") { | |
| if req.Pattern != "/" { |
| func passwordChangeRequestAllowed(req *http.Request) bool { | ||
| if isStaticAssetPath(req.URL.Path) || req.URL.Path == "/change-password" || strings.HasPrefix(req.URL.Path, "/change-password/") || req.URL.Path == "/oauth2/sign_out" { | ||
| return true | ||
| } | ||
| return (req.Method == http.MethodGet && slices.Contains([]string{ | ||
| "/api/me", | ||
| "/api/version", | ||
| "/api/license", | ||
| "/api/app-preferences", | ||
| }, req.URL.Path)) || | ||
| (req.Method == http.MethodPost && req.URL.Path == "/api/local-auth/change-password") | ||
| } | ||
|
|
There was a problem hiding this comment.
Can this be in the authorization layer. We should have access to the user and the extra field there.
The only thing that gives me pause here is the redirect.
| tests := []struct { | ||
| rd, want string | ||
| }{ | ||
| {"", "/"}, |
| return func(o *options) { o.disabled = true } | ||
| } | ||
|
|
||
| func New(ctx context.Context, serverURL string, c *client.Client, authProviderGetter configuredAuthProviderGetter, authEnabled, forceEnableBootstrap bool, opts ...Option) (*Bootstrap, error) { |
There was a problem hiding this comment.
It seems weird to have authEnabled and forceEnableBootstrap be sent as bools, but disabled to be set using a functional option pattern.
|
|
||
| credEnv := map[string]string{} | ||
| cred, err := d.gatewayClient.RevealCredential(ctx, []string{authProvider.Name, system.GenericAuthProviderCredentialContext}, authProvider.Name) | ||
| cred, err := d.gatewayClient.RevealCredential(ctx, []string{authProvider.Name, system.GenericAuthProviderCredentialContext, system.ReplacementAuthProviderCredentialContext}, authProvider.Name) |
There was a problem hiding this comment.
Maybe this is why you did it this way in the other place where I commented you can use providers.AuthProviderStatus. Does it make sense to include this credential context there, too?
Provision the first local auth owner from deployment settings and let that owner
switch to a replacement auth provider without a bootstrap token.
An operator sets an owner email and a high-entropy setup token. Obot configures
local auth, creates the owner with an unusable password, and stores only a hash
of the setup token. The owner activates through a fragment-delivered link, and
their session can do nothing but read its own profile, set a password, or sign
out until setup is complete. A completed account is never rearmed or reset from
environment settings.
Switching away from local auth stages the replacement's settings while local
auth keeps serving logins, verifies them through a one-time login bound to the
verification that issued it, and then activates as a single provider
configuration change so a partial failure cannot lock everyone out.