Disable local users for a smooth single-idp mode#5226
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughAdds runtime control to disable/enable local (email/password) auth: Provider gains HasNonLocalConnectors/DisableLocalAuth/EnableLocalAuth (IsLocalAuthEnabled removed); EmbeddedIdPConfig gains LocalAuthDisabled; settings, API, handlers, and user flows are updated to propagate and enforce this flag. Changes
Sequence Diagram(s)sequenceDiagram
actor Operator
participant HTTP as "HTTP Start"
participant Settings as "Settings.Manager"
participant IdP as "EmbeddedIdPManager"
participant Provider as "Provider (dex)"
Operator->>HTTP: Start with LocalAuthDisabled=true
HTTP->>Settings: NewManager(..., idpConfig{LocalAuthDisabled:true})
HTTP->>IdP: NewEmbeddedIdPManager(config)
IdP->>Provider: HasNonLocalConnectors(ctx)?
Provider-->>IdP: (true / false)
alt no non-local connectors
IdP-->>HTTP: error — cannot disable local auth without other connectors
else non-local connectors exist
IdP->>Provider: DisableLocalAuth(ctx)
Provider->>Provider: list connectors, validate, delete local connector
Provider-->>IdP: success
IdP-->>HTTP: started with local auth disabled
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
braginini
requested review from
mlsmaycon,
pascal-fischer and
shuuri-labs
and removed request for
pascal-fischer
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@idp/dex/connector.go`:
- Around line 349-385: The GetConnector call in IsLocalAuthEnabled and
DisableLocalAuth must verify the returned connector's Type to avoid an ID
collision with non-local providers: after calling p.storage.GetConnector(ctx,
"local") inspect the returned connector (e.g., conn.Type or connector.Type) and
ensure it equals the local connector type (use the project’s canonical local
type string/const, e.g., "local" or ConnectorTypeLocal); in IsLocalAuthEnabled
return false if the connector exists but is not of local type, and in
DisableLocalAuth refuse to delete and return a clear error if the found
connector is not of local type (so you don’t remove a non-local
provider)—alternatively enforce the same check at CreateConnector to prevent
creating non-local connectors with ID "local".
🧹 Nitpick comments (1)
idp/dex/connector.go (1)
330-345: Consider downgrading per-connector logs to Debug.
HasNonLocalConnectorslogs each connector at Info level, which can be noisy and may leak connector names into standard logs. Consider Debug level or logging only counts.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@management/server/idp/embedded.go`:
- Around line 593-604: The runtime enable/disable functions DisableLocalAuth and
EnableLocalAuth on EmbeddedIdPManager call through to m.provider but do not
update the stored flag m.config.LocalAuthDisabled, causing IsLocalAuthDisabled
to report stale state; after a successful call to
m.provider.DisableLocalAuth(ctx) set m.config.LocalAuthDisabled = true, and
after a successful call to m.provider.EnableLocalAuth(ctx) set
m.config.LocalAuthDisabled = false (ensure you only update the config when the
provider call returns nil error and preserve existing error return behavior).
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@management/server/http/handlers/instance/instance_handler.go`:
- Line 49: The log message uses a debug marker "->>>>>>:"; update the call to
log.WithContext(r.Context()).Infof that references setupRequired to remove the
debug markers and follow project style (e.g., change the format string to
"instance setup status: %v") or remove the log statement entirely if redundant;
locate the usage of log.WithContext(r.Context()).Infof and replace the message
accordingly.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@management/server/idp/embedded.go`:
- Line 202: The current debug log in embedded.go uses
log.WithContext(ctx).Debugf("initializing embedded Dex IDP with config: %+v",
config) which will print sensitive fields like Owner.Hash; replace this with a
safe log that omits or redacts secrets—e.g., build and log a sanitized view of
the config (only safe fields or a copy with Owner.Hash and other secret fields
set to "<redacted>") or explicitly list non-sensitive fields to include; update
the call site using the log.WithContext(ctx).Debugf invocation and the config
variable so no raw credential material is ever formatted into logs.
In `@management/server/instance/manager.go`:
- Around line 106-123: In DefaultManager.loadSetupRequired, detect when local
auth is disabled and avoid marking setupRequired true when there are no store
accounts and no local users; specifically, after calling
m.store.GetAccountsCounter and m.embeddedIdpManager.GetAllAccounts,
short‑circuit and set m.setupRequired = false if the manager/config flag for
LocalAuthDisabled is enabled (e.g., m.LocalAuthDisabled or
m.options.LocalAuthDisabled), so CreateOwnerUser (which relies on local user
creation) won't be required; keep locking via m.setupMu around writes to
m.setupRequired and ensure the method still returns errors from the store or
embeddedIdpManager calls.
🧹 Nitpick comments (1)
management/server/idp/embedded.go (1)
312-324: Consider de-duplicating the debug messages.Both logs use the same wording; the second could be clarified to avoid confusion.
♻️ Suggested wording tweak
- log.WithContext(ctx).Debugf("retrieved %d users from embedded IdP", len(indexedUsers[UnsetAccountID])) + log.WithContext(ctx).Debugf("indexed %d users under UnsetAccountID in embedded IdP", len(indexedUsers[UnsetAccountID]))
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Describe your changes
Add LocalAuthDisabled option to embedded IdP configuration
This adds the ability to disable local (email/password) authentication when using the embedded Dex identity provider. When disabled, users can only authenticate via external
identity providers (Google, OIDC, etc.).
This simplifies user login when there is only one external IdP configured. The login page will redirect directly to the IdP login page.
Key changes:
Issue ticket number and link
Stack
Checklist
Documentation
Select exactly one:
Docs PR URL (required if "docs added" is checked)
Paste the PR link from https://github.com/netbirdio/docs here:
https://github.com/netbirdio/docs/pull/__
Summary by CodeRabbit
New Features
Behavior / Safety
Tests
✏️ Tip: You can customize this high-level summary in your review settings.