Local user password change (embedded IdP)#5132
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughReplaces extensive Dex connector lifecycle code in Changes
Sequence Diagram(s)sequenceDiagram
participant Client as HTTP Client
participant Handler as HTTP Handler
participant AcctMgr as DefaultAccountManager
participant EmbeddedMgr as EmbeddedIdPManager
participant Provider as Dex Provider
participant Storage as Storage
Client->>Handler: PUT /users/{userId}/password {old_password,new_password}
Handler->>Handler: validate method & parse body
Handler->>AcctMgr: UpdateUserPassword(accountID,currentUserID,targetUserID,old,new)
AcctMgr->>AcctMgr: validate inputs & ensure embedded IdP enabled
AcctMgr->>EmbeddedMgr: UpdateUserPassword(currentUserID,targetUserID,old,new)
EmbeddedMgr->>EmbeddedMgr: enforce ownership & new != old
EmbeddedMgr->>Provider: UpdateUserPassword(userID,old,new)
Provider->>Storage: GetUserByID(userID)
Storage-->>Provider: user record (hash,email)
Provider->>Provider: verify old password
Provider->>Provider: bcrypt-hash new password
Provider->>Storage: UpdatePasswordByEmail(email,newHash)
Storage-->>Provider: success/error
Provider-->>EmbeddedMgr: result
EmbeddedMgr-->>AcctMgr: result
AcctMgr->>AcctMgr: record UserPasswordChanged activity
AcctMgr-->>Handler: result
Handler-->>Client: 200 OK or error
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
![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 `@shared/management/http/api/openapi.yml`:
- Around line 3222-3257: The OpenAPI spec for the PUT operation at
/api/users/{userId}/password currently leaves requestBody optional; update the
operation's requestBody block (under the PUT for /api/users/{userId}/password)
to include required: true so clients and generators must send the
PasswordChangeRequest payload (schema: PasswordChangeRequest) when calling the
Change user password endpoint.
🧹 Nitpick comments (4)
management/server/http/handlers/users/users_handler_test.go (1)
956-973: Consider using the router for method validation test.This test calls
handler.changePassword(rr, req)directly rather than going through the router. While this works for verifying the handler's method check, it doesn't test the route registration. Other similar tests (e.g.,TestApproveUserEndpoint) userouter.ServeHTTP(rr, req)for consistency.♻️ Suggested refactor for consistency
func TestChangePasswordEndpoint_WrongMethod(t *testing.T) { am := &mock_server.MockAccountManager{} handler := newHandler(am) + router := mux.NewRouter() + router.HandleFunc("/users/{userId}/password", handler.changePassword).Methods("PUT", "OPTIONS") req, err := http.NewRequest("POST", "/users/test-user/password", bytes.NewBufferString(`{}`)) require.NoError(t, err) userAuth := auth.UserAuth{ AccountId: existingAccountID, UserId: existingUserID, } req = nbcontext.SetUserAuthInRequest(req, userAuth) rr := httptest.NewRecorder() - handler.changePassword(rr, req) + router.ServeHTTP(rr, req) - assert.Equal(t, http.StatusMethodNotAllowed, rr.Code) + assert.Equal(t, http.StatusMethodNotAllowed, rr.Code) }management/server/http/handlers/users/users_handler.go (1)
415-419: Use the generatedapi.PasswordChangeRequesttype to eliminate duplication.The
PasswordChangeRequesttype is already defined in the generated OpenAPI types atshared/management/http/api/types.gen.go. Theapipackage is already imported in this file, so replace the local struct definition with the generated one to maintain consistency with other endpoints.♻️ Suggested refactor
-// passwordChangeRequest represents the request body for password change -type passwordChangeRequest struct { - OldPassword string `json:"old_password"` - NewPassword string `json:"new_password"` -}Then update the handler:
- var req passwordChangeRequest + var req api.PasswordChangeRequestmanagement/server/user.go (1)
252-281: Implementation looks good with one consideration on error handling.The method correctly:
- Validates embedded IdP is enabled
- Checks for empty passwords
- Type asserts to get the embedded IdP manager
- Records the password change event
One consideration: Line 275 wraps all errors from
UpdateUserPasswordwithstatus.InvalidArgument. Some underlying errors (e.g., database failures) might be more appropriately classified asstatus.Internal. The current approach preserves the error message but may affect how clients interpret the error type.Consider differentiating error types (optional)
err := embeddedIdp.UpdateUserPassword(ctx, currentUserID, targetUserID, oldPassword, newPassword) if err != nil { - return status.Errorf(status.InvalidArgument, "failed to update password: %v", err) + // Check if it's a user input error vs internal error + if strings.Contains(err.Error(), "current password is incorrect") || + strings.Contains(err.Error(), "users can only change their own password") || + strings.Contains(err.Error(), "new password must be different") { + return status.Errorf(status.InvalidArgument, "failed to update password: %v", err) + } + return status.Errorf(status.Internal, "failed to update password: %v", err) }shared/management/http/api/openapi.yml (1)
47-60: Add password schema hints (format/minLength) to align with existing policy.This schema is missing
format: passwordand a minimum length likeSetupRequest.password(Line 2584). Adding these improves client-side validation/masking and keeps the contract consistent.Suggested update
PasswordChangeRequest: type: object properties: old_password: description: The current password type: string + format: password + minLength: 8 example: "currentPassword123" new_password: description: The new password to set type: string + format: password + minLength: 8 example: "newSecurePassword456" required: - old_password - new_password
…provider.go to the new connector.go file
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@idp/dex/connector.go`:
- Around line 175-198: The OIDC connector config build currently loses the
original provider type because you rely on inference later; modify
buildOIDCConnectorConfig to include and persist the explicit provider type
(e.g., add a "provider" or "type" key set to cfg.Type into the oidcConfig before
calling encodeConnectorConfig) so provider-specific settings (scopes,
claimMapping, getUserInfo) remain tied to the original provider, and update the
parsing/reading path (where inferOIDCProviderType is used, referenced around
lines 247-265) to prefer this persisted "provider"/"type" field when present
rather than falling back to inferOIDCProviderType.
- Around line 227-236: The parsed connector config currently assigns client
secrets into cfg.ClientSecret (in connector.go) which causes
GetConnector/ListConnectors to return sensitive OAuth secrets; change parsing
and return behavior so cfg.ClientSecret is not populated or is redacted by
default (e.g., set cfg.ClientSecret = "" or replace with a fixed placeholder)
unless an explicit, auditable request/flag is provided to reveal secrets; update
the code paths that assemble connector responses (GetConnector/ListConnectors)
to ensure they never expose cfg.ClientSecret.
- Around line 277-287: The current ensureLocalConnector function returns early
if any connector exists; change it to scan the connectors returned by
stor.ListConnectors and only return if a local/password connector is present. In
ensureLocalConnector, iterate over connectors (from stor.ListConnectors) and
check the connector's identifying field (e.g., connector.Type or connector.Kind)
for the local/password value (e.g., "local" or the project's local constant);
only skip creation when such a local connector is found, otherwise proceed to
create the local connector as before.
In `@idp/dex/provider.go`:
- Around line 587-615: UpdateUserPassword currently checks the old password
before calling p.storage.UpdatePassword, which creates a TOCTOU race; change the
logic so UpdatePassword's mutation callback re-validates the stored hash against
oldPassword (using bcrypt.CompareHashAndPassword) and returns a well-known
invalid-password error if it doesn't match, then only wrap errors from
p.storage.UpdatePassword that are not the invalid-password sentinel; update
references: Provider.UpdateUserPassword, p.storage.UpdatePassword,
storage.Password (old.Hash), and GetUserByID to perform the pre-read only for
lookup, moving auth validation into the UpdatePassword atomic update callback
and preserving error semantics.
Comment on lines
+175
to
+198
| // buildOIDCConnectorConfig creates config for OIDC-based connectors | ||
| func buildOIDCConnectorConfig(cfg *ConnectorConfig, redirectURI string) ([]byte, error) { | ||
| oidcConfig := map[string]interface{}{ | ||
| "issuer": cfg.Issuer, | ||
| "clientID": cfg.ClientID, | ||
| "clientSecret": cfg.ClientSecret, | ||
| "redirectURI": redirectURI, | ||
| "scopes": []string{"openid", "profile", "email"}, | ||
| "insecureEnableGroups": true, | ||
| //some providers don't return email verified, so we need to skip it if not present (e.g., Entra, Okta, Duo) | ||
| "insecureSkipEmailVerified": true, | ||
| } | ||
| switch cfg.Type { | ||
| case "zitadel": | ||
| oidcConfig["getUserInfo"] = true | ||
| case "entra": | ||
| oidcConfig["claimMapping"] = map[string]string{"email": "preferred_username"} | ||
| case "okta": | ||
| oidcConfig["scopes"] = []string{"openid", "profile", "email", "groups"} | ||
| case "pocketid": | ||
| oidcConfig["scopes"] = []string{"openid", "profile", "email", "groups"} | ||
| } | ||
| return encodeConnectorConfig(oidcConfig) | ||
| } |
Contributor
There was a problem hiding this comment.
Persist the original provider type; current inference can lose it.
If a connector ID doesn’t include the provider name (e.g., “corp-oidc”), inferOIDCProviderType returns "oidc", so a read→update cycle will drop provider-specific behavior (scopes/claimMapping). Persist the provider type explicitly and prefer it on parse.
🔧 Proposed fix (persist provider type and read it back)
func buildOIDCConnectorConfig(cfg *ConnectorConfig, redirectURI string) ([]byte, error) {
oidcConfig := map[string]interface{}{
"issuer": cfg.Issuer,
"clientID": cfg.ClientID,
"clientSecret": cfg.ClientSecret,
"redirectURI": redirectURI,
"scopes": []string{"openid", "profile", "email"},
"insecureEnableGroups": true,
//some providers don't return email verified, so we need to skip it if not present (e.g., Entra, Okta, Duo)
"insecureSkipEmailVerified": true,
+ "provider": cfg.Type,
}
switch cfg.Type {
case "zitadel":
oidcConfig["getUserInfo"] = true
case "entra":
oidcConfig["claimMapping"] = map[string]string{"email": "preferred_username"}
case "okta":
oidcConfig["scopes"] = []string{"openid", "profile", "email", "groups"}
case "pocketid":
oidcConfig["scopes"] = []string{"openid", "profile", "email", "groups"}
}
return encodeConnectorConfig(oidcConfig)
}
-func inferIdentityProviderType(dexType, connectorID string, _ map[string]interface{}) string {
+func inferIdentityProviderType(dexType, connectorID string, config map[string]interface{}) string {
if dexType != "oidc" {
return dexType
}
+ if v, ok := config["provider"].(string); ok && v != "" {
+ return v
+ }
return inferOIDCProviderType(connectorID)
}Also applies to: 247-265
🤖 Prompt for AI Agents
In `@idp/dex/connector.go` around lines 175 - 198, The OIDC connector config build
currently loses the original provider type because you rely on inference later;
modify buildOIDCConnectorConfig to include and persist the explicit provider
type (e.g., add a "provider" or "type" key set to cfg.Type into the oidcConfig
before calling encodeConnectorConfig) so provider-specific settings (scopes,
claimMapping, getUserInfo) remain tied to the original provider, and update the
parsing/reading path (where inferOIDCProviderType is used, referenced around
lines 247-265) to prefer this persisted "provider"/"type" field when present
rather than falling back to inferOIDCProviderType.
Comment on lines
+227
to
+236
| // Extract common fields | ||
| if v, ok := configMap["clientID"].(string); ok { | ||
| cfg.ClientID = v | ||
| } | ||
| if v, ok := configMap["clientSecret"].(string); ok { | ||
| cfg.ClientSecret = v | ||
| } | ||
| if v, ok := configMap["redirectURI"].(string); ok { | ||
| cfg.RedirectURI = v | ||
| } |
Contributor
There was a problem hiding this comment.
Avoid returning client secrets from parsed connector configs.
Returning clientSecret in GetConnector/ListConnectors risks leaking OAuth credentials to callers and logs. Prefer omitting it unless explicitly requested.
🔒 Proposed fix (redact clientSecret by default)
// Extract common fields
if v, ok := configMap["clientID"].(string); ok {
cfg.ClientID = v
}
- if v, ok := configMap["clientSecret"].(string); ok {
- cfg.ClientSecret = v
- }
if v, ok := configMap["redirectURI"].(string); ok {
cfg.RedirectURI = v
}🤖 Prompt for AI Agents
In `@idp/dex/connector.go` around lines 227 - 236, The parsed connector config
currently assigns client secrets into cfg.ClientSecret (in connector.go) which
causes GetConnector/ListConnectors to return sensitive OAuth secrets; change
parsing and return behavior so cfg.ClientSecret is not populated or is redacted
by default (e.g., set cfg.ClientSecret = "" or replace with a fixed placeholder)
unless an explicit, auditable request/flag is provided to reveal secrets; update
the code paths that assemble connector responses (GetConnector/ListConnectors)
to ensure they never expose cfg.ClientSecret.
Comment on lines
+587
to
615
| // UpdateUserPassword updates the password for a user identified by userID. | ||
| // The userID can be either an encoded Dex ID (base64 protobuf) or a raw UUID. | ||
| // It verifies the current password before updating. | ||
| func (p *Provider) UpdateUserPassword(ctx context.Context, userID string, oldPassword, newPassword string) error { | ||
| // Get the user by ID to find their email | ||
| user, err := p.GetUserByID(ctx, userID) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to build connector: %w", err) | ||
| return fmt.Errorf("failed to get user: %w", err) | ||
| } | ||
|
|
||
| if err := p.storage.CreateConnector(ctx, storageConn); err != nil { | ||
| return nil, fmt.Errorf("failed to create connector: %w", err) | ||
| } | ||
|
|
||
| p.logger.Info("connector created", "id", cfg.ID, "type", cfg.Type) | ||
| return cfg, nil | ||
| } | ||
|
|
||
| // GetConnector retrieves a connector by ID from Dex storage. | ||
| func (p *Provider) GetConnector(ctx context.Context, id string) (*ConnectorConfig, error) { | ||
| conn, err := p.storage.GetConnector(ctx, id) | ||
| if err != nil { | ||
| if err == storage.ErrNotFound { | ||
| return nil, err | ||
| } | ||
| return nil, fmt.Errorf("failed to get connector: %w", err) | ||
| // Verify old password | ||
| if err := bcrypt.CompareHashAndPassword(user.Hash, []byte(oldPassword)); err != nil { | ||
| return fmt.Errorf("current password is incorrect") | ||
| } | ||
|
|
||
| return p.parseStorageConnector(conn) | ||
| } | ||
|
|
||
| // ListConnectors returns all connectors from Dex storage (excluding the local connector). | ||
| func (p *Provider) ListConnectors(ctx context.Context) ([]*ConnectorConfig, error) { | ||
| connectors, err := p.storage.ListConnectors(ctx) | ||
| // Hash the new password | ||
| newHash, err := bcrypt.GenerateFromPassword([]byte(newPassword), bcrypt.DefaultCost) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to list connectors: %w", err) | ||
| } | ||
|
|
||
| result := make([]*ConnectorConfig, 0, len(connectors)) | ||
| for _, conn := range connectors { | ||
| // Skip the local password connector | ||
| if conn.ID == "local" && conn.Type == "local" { | ||
| continue | ||
| } | ||
|
|
||
| cfg, err := p.parseStorageConnector(conn) | ||
| if err != nil { | ||
| p.logger.Warn("failed to parse connector", "id", conn.ID, "error", err) | ||
| continue | ||
| } | ||
| result = append(result, cfg) | ||
| return fmt.Errorf("failed to hash new password: %w", err) | ||
| } | ||
|
|
||
| return result, nil | ||
| } | ||
|
|
||
| // UpdateConnector updates an existing connector in Dex storage. | ||
| func (p *Provider) UpdateConnector(ctx context.Context, cfg *ConnectorConfig) error { | ||
| storageConn, err := p.buildStorageConnector(cfg) | ||
| // Update the password in storage | ||
| err = p.storage.UpdatePassword(ctx, user.Email, func(old storage.Password) (storage.Password, error) { | ||
| old.Hash = newHash | ||
| return old, nil | ||
| }) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to build connector: %w", err) | ||
| } | ||
|
|
||
| if err := p.storage.UpdateConnector(ctx, cfg.ID, func(old storage.Connector) (storage.Connector, error) { | ||
| return storageConn, nil | ||
| }); err != nil { | ||
| return fmt.Errorf("failed to update connector: %w", err) | ||
| } | ||
|
|
||
| p.logger.Info("connector updated", "id", cfg.ID, "type", cfg.Type) | ||
| return nil | ||
| } | ||
|
|
||
| // DeleteConnector removes a connector from Dex storage. | ||
| func (p *Provider) DeleteConnector(ctx context.Context, id string) error { | ||
| // Prevent deletion of the local connector | ||
| if id == "local" { | ||
| return fmt.Errorf("cannot delete the local password connector") | ||
| } | ||
|
|
||
| if err := p.storage.DeleteConnector(ctx, id); err != nil { | ||
| return fmt.Errorf("failed to delete connector: %w", err) | ||
| return fmt.Errorf("failed to update password: %w", err) | ||
| } |
Contributor
There was a problem hiding this comment.
Avoid TOCTOU by validating the stored hash at update time.
There’s a race between checking the old hash and updating it. Another concurrent password change could be overwritten even though the old password is no longer valid. Re-validate inside UpdatePassword and only wrap errors that aren’t invalid-password.
🔧 Proposed fix (validate within UpdatePassword)
func (p *Provider) UpdateUserPassword(ctx context.Context, userID string, oldPassword, newPassword string) error {
// Get the user by ID to find their email
user, err := p.GetUserByID(ctx, userID)
if err != nil {
return fmt.Errorf("failed to get user: %w", err)
}
- // Verify old password
- if err := bcrypt.CompareHashAndPassword(user.Hash, []byte(oldPassword)); err != nil {
- return fmt.Errorf("current password is incorrect")
- }
-
- // Hash the new password
- newHash, err := bcrypt.GenerateFromPassword([]byte(newPassword), bcrypt.DefaultCost)
- if err != nil {
- return fmt.Errorf("failed to hash new password: %w", err)
- }
-
- // Update the password in storage
- err = p.storage.UpdatePassword(ctx, user.Email, func(old storage.Password) (storage.Password, error) {
- old.Hash = newHash
- return old, nil
- })
+ invalidPassErr := errors.New("current password is incorrect")
+ // Update the password in storage
+ err = p.storage.UpdatePassword(ctx, user.Email, func(old storage.Password) (storage.Password, error) {
+ if err := bcrypt.CompareHashAndPassword(old.Hash, []byte(oldPassword)); err != nil {
+ return old, invalidPassErr
+ }
+ newHash, err := bcrypt.GenerateFromPassword([]byte(newPassword), bcrypt.DefaultCost)
+ if err != nil {
+ return old, fmt.Errorf("failed to hash new password: %w", err)
+ }
+ old.Hash = newHash
+ return old, nil
+ })
if err != nil {
- return fmt.Errorf("failed to update password: %w", err)
+ if errors.Is(err, invalidPassErr) {
+ return invalidPassErr
+ }
+ return fmt.Errorf("failed to update password: %w", err)
}
return nil
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // UpdateUserPassword updates the password for a user identified by userID. | |
| // The userID can be either an encoded Dex ID (base64 protobuf) or a raw UUID. | |
| // It verifies the current password before updating. | |
| func (p *Provider) UpdateUserPassword(ctx context.Context, userID string, oldPassword, newPassword string) error { | |
| // Get the user by ID to find their email | |
| user, err := p.GetUserByID(ctx, userID) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to build connector: %w", err) | |
| return fmt.Errorf("failed to get user: %w", err) | |
| } | |
| if err := p.storage.CreateConnector(ctx, storageConn); err != nil { | |
| return nil, fmt.Errorf("failed to create connector: %w", err) | |
| } | |
| p.logger.Info("connector created", "id", cfg.ID, "type", cfg.Type) | |
| return cfg, nil | |
| } | |
| // GetConnector retrieves a connector by ID from Dex storage. | |
| func (p *Provider) GetConnector(ctx context.Context, id string) (*ConnectorConfig, error) { | |
| conn, err := p.storage.GetConnector(ctx, id) | |
| if err != nil { | |
| if err == storage.ErrNotFound { | |
| return nil, err | |
| } | |
| return nil, fmt.Errorf("failed to get connector: %w", err) | |
| // Verify old password | |
| if err := bcrypt.CompareHashAndPassword(user.Hash, []byte(oldPassword)); err != nil { | |
| return fmt.Errorf("current password is incorrect") | |
| } | |
| return p.parseStorageConnector(conn) | |
| } | |
| // ListConnectors returns all connectors from Dex storage (excluding the local connector). | |
| func (p *Provider) ListConnectors(ctx context.Context) ([]*ConnectorConfig, error) { | |
| connectors, err := p.storage.ListConnectors(ctx) | |
| // Hash the new password | |
| newHash, err := bcrypt.GenerateFromPassword([]byte(newPassword), bcrypt.DefaultCost) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to list connectors: %w", err) | |
| } | |
| result := make([]*ConnectorConfig, 0, len(connectors)) | |
| for _, conn := range connectors { | |
| // Skip the local password connector | |
| if conn.ID == "local" && conn.Type == "local" { | |
| continue | |
| } | |
| cfg, err := p.parseStorageConnector(conn) | |
| if err != nil { | |
| p.logger.Warn("failed to parse connector", "id", conn.ID, "error", err) | |
| continue | |
| } | |
| result = append(result, cfg) | |
| return fmt.Errorf("failed to hash new password: %w", err) | |
| } | |
| return result, nil | |
| } | |
| // UpdateConnector updates an existing connector in Dex storage. | |
| func (p *Provider) UpdateConnector(ctx context.Context, cfg *ConnectorConfig) error { | |
| storageConn, err := p.buildStorageConnector(cfg) | |
| // Update the password in storage | |
| err = p.storage.UpdatePassword(ctx, user.Email, func(old storage.Password) (storage.Password, error) { | |
| old.Hash = newHash | |
| return old, nil | |
| }) | |
| if err != nil { | |
| return fmt.Errorf("failed to build connector: %w", err) | |
| } | |
| if err := p.storage.UpdateConnector(ctx, cfg.ID, func(old storage.Connector) (storage.Connector, error) { | |
| return storageConn, nil | |
| }); err != nil { | |
| return fmt.Errorf("failed to update connector: %w", err) | |
| } | |
| p.logger.Info("connector updated", "id", cfg.ID, "type", cfg.Type) | |
| return nil | |
| } | |
| // DeleteConnector removes a connector from Dex storage. | |
| func (p *Provider) DeleteConnector(ctx context.Context, id string) error { | |
| // Prevent deletion of the local connector | |
| if id == "local" { | |
| return fmt.Errorf("cannot delete the local password connector") | |
| } | |
| if err := p.storage.DeleteConnector(ctx, id); err != nil { | |
| return fmt.Errorf("failed to delete connector: %w", err) | |
| return fmt.Errorf("failed to update password: %w", err) | |
| } | |
| // UpdateUserPassword updates the password for a user identified by userID. | |
| // The userID can be either an encoded Dex ID (base64 protobuf) or a raw UUID. | |
| // It verifies the current password before updating. | |
| func (p *Provider) UpdateUserPassword(ctx context.Context, userID string, oldPassword, newPassword string) error { | |
| // Get the user by ID to find their email | |
| user, err := p.GetUserByID(ctx, userID) | |
| if err != nil { | |
| return fmt.Errorf("failed to get user: %w", err) | |
| } | |
| invalidPassErr := errors.New("current password is incorrect") | |
| // Update the password in storage | |
| err = p.storage.UpdatePassword(ctx, user.Email, func(old storage.Password) (storage.Password, error) { | |
| if err := bcrypt.CompareHashAndPassword(old.Hash, []byte(oldPassword)); err != nil { | |
| return old, invalidPassErr | |
| } | |
| newHash, err := bcrypt.GenerateFromPassword([]byte(newPassword), bcrypt.DefaultCost) | |
| if err != nil { | |
| return old, fmt.Errorf("failed to hash new password: %w", err) | |
| } | |
| old.Hash = newHash | |
| return old, nil | |
| }) | |
| if err != nil { | |
| if errors.Is(err, invalidPassErr) { | |
| return invalidPassErr | |
| } | |
| return fmt.Errorf("failed to update password: %w", err) | |
| } | |
| return nil | |
| } |
🤖 Prompt for AI Agents
In `@idp/dex/provider.go` around lines 587 - 615, UpdateUserPassword currently
checks the old password before calling p.storage.UpdatePassword, which creates a
TOCTOU race; change the logic so UpdatePassword's mutation callback re-validates
the stored hash against oldPassword (using bcrypt.CompareHashAndPassword) and
returns a well-known invalid-password error if it doesn't match, then only wrap
errors from p.storage.UpdatePassword that are not the invalid-password sentinel;
update references: Provider.UpdateUserPassword, p.storage.UpdatePassword,
storage.Password (old.Hash), and GetUserByID to perform the pre-read only for
lookup, moving auth validation into the UpdatePassword atomic update callback
and preserving error semantics.
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 91-106: Provider.UpdateConnector currently replaces the stored
connector wholesale, risking loss of existing fields; change the
storage.UpdateConnector callback (in Provider.UpdateConnector) to first parse
the provided old storage.Connector into a ConnectorConfig or equivalent, merge
non-empty/non-zero fields from the incoming cfg into that parsed config
(preserving fields like ClientSecret, Issuer, ClientID when cfg omits them),
then call buildStorageConnector on the merged config and return that
storage.Connector; after this change remove the redundant ClientSecret
preservation logic in EmbeddedIdPManager.UpdateConnector since preservation is
now handled defensively at Provider.UpdateConnector.
♻️ Duplicate comments (2)
idp/dex/connector.go (2)
176-199: Persist the original provider type; current inference can lose it.
If a connector ID doesn’t include the provider name, inference falls back to"oidc"and drops provider‑specific behavior. Persist and prefer the explicit provider type.🔧 Proposed fix (persist provider type and read it back)
func buildOIDCConnectorConfig(cfg *ConnectorConfig, redirectURI string) ([]byte, error) { oidcConfig := map[string]interface{}{ "issuer": cfg.Issuer, "clientID": cfg.ClientID, "clientSecret": cfg.ClientSecret, "redirectURI": redirectURI, "scopes": []string{"openid", "profile", "email"}, "insecureEnableGroups": true, //some providers don't return email verified, so we need to skip it if not present (e.g., Entra, Okta, Duo) "insecureSkipEmailVerified": true, + "provider": cfg.Type, } ... } -func inferIdentityProviderType(dexType, connectorID string, _ map[string]interface{}) string { +func inferIdentityProviderType(dexType, connectorID string, config map[string]interface{}) string { if dexType != "oidc" { return dexType } + if v, ok := config["provider"].(string); ok && v != "" { + return v + } return inferOIDCProviderType(connectorID) }Also applies to: 248-255
228-234: Avoid returning client secrets from parsed connector configs.
Surfacing secrets inGetConnector/ListConnectorsrisks leakage through API responses or logs.🔒 Proposed fix (redact by default)
if v, ok := configMap["clientID"].(string); ok { cfg.ClientID = v } - if v, ok := configMap["clientSecret"].(string); ok { - cfg.ClientSecret = v - }
🧹 Nitpick comments (5)
idp/dex/connector.go (5)
14-30: Confirm the JSON contract for ConnectorConfig.
If this struct is serialized directly, the default JSON keys will beID,Name, etc., which may not match lower‑camel API schemas. Verify usage; add tags if needed.🧩 Optional struct tags
type ConnectorConfig struct { - ID string + ID string `json:"id"` // Name is a human-readable name for the connector - Name string + Name string `json:"name"` // Type is the connector type (oidc, google, microsoft) - Type string + Type string `json:"type"` // Issuer is the OIDC issuer URL (for OIDC-based connectors) - Issuer string + Issuer string `json:"issuer,omitempty"` // ClientID is the OAuth2 client ID - ClientID string + ClientID string `json:"clientID,omitempty"` // ClientSecret is the OAuth2 client secret - ClientSecret string + ClientSecret string `json:"clientSecret,omitempty"` // RedirectURI is the OAuth2 redirect URI - RedirectURI string + RedirectURI string `json:"redirectURI,omitempty"` }
32-51: Validate required fields before creating a connector.
Creating with emptyID/Type(or nilcfg) will likely produce unusable storage entries. Consider fast‑fail validation.✅ Example guard
func (p *Provider) CreateConnector(ctx context.Context, cfg *ConnectorConfig) (*ConnectorConfig, error) { + if cfg == nil || cfg.ID == "" || cfg.Type == "" { + return nil, fmt.Errorf("invalid connector config: missing id/type") + } // Fill in the redirect URI if not provided if cfg.RedirectURI == "" { cfg.RedirectURI = p.GetRedirectURI() }
66-89: Consider surfacing parse errors instead of silently skipping connectors.
Logging and continuing can hide misconfigurations and return partial results without an error signal.
123-174: De‑duplicate redirect URI logic.
GetRedirectURIandresolveRedirectURIimplement the same path construction. Consider delegating to keep them consistent.♻️ Example cleanup
func (p *Provider) GetRedirectURI() string { - if p.config == nil { - return "" - } - issuer := strings.TrimSuffix(p.config.Issuer, "/") - if !strings.HasSuffix(issuer, "/oauth2") { - issuer += "/oauth2" - } - return issuer + "/callback" + return p.resolveRedirectURI("") }
304-330: Consider reconciling connector Type during static updates.
ensureStaticConnectorsupdates onlyNameandConfig. If the static connector’sTypechanges, the old type will persist. Verify if that’s possible; if so, update or recreate.🧩 Possible adjustment
if err := stor.UpdateConnector(ctx, conn.ID, func(old storage.Connector) (storage.Connector, error) { old.Name = storConn.Name + old.Type = storConn.Type old.Config = storConn.Config return old, nil }); err != nil {
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 91-129: UpdateConnector currently merges several fields from the
existing connector but fails to preserve cfg.Type, causing buildStorageConnector
to error and breaking the Issuer-preservation check that compares cfg.Type to
oldCfg.Type; update the merging logic inside Provider.UpdateConnector (the
closure passed to p.storage.UpdateConnector) to set cfg.Type = oldCfg.Type when
cfg.Type is empty/zero, similar to how ClientSecret, RedirectURI, ClientID, and
Name are preserved, so parseStorageConnector/oldCfg.Type is used to keep Type
consistent before calling buildStorageConnector.
♻️ Duplicate comments (2)
idp/dex/connector.go (2)
199-222: Persist provider type for OIDC connectors to avoid losing behavior.Inference from connector ID can collapse provider-specific behavior (scopes/claimMapping) when IDs don’t contain provider names. Persist the provider type in config and prefer it when parsing.
🔧 Proposed fix
func buildOIDCConnectorConfig(cfg *ConnectorConfig, redirectURI string) ([]byte, error) { oidcConfig := map[string]interface{}{ "issuer": cfg.Issuer, "clientID": cfg.ClientID, "clientSecret": cfg.ClientSecret, "redirectURI": redirectURI, "scopes": []string{"openid", "profile", "email"}, "insecureEnableGroups": true, //some providers don't return email verified, so we need to skip it if not present (e.g., Entra, Okta, Duo) "insecureSkipEmailVerified": true, + "provider": cfg.Type, } ... } -func inferIdentityProviderType(dexType, connectorID string, _ map[string]interface{}) string { +func inferIdentityProviderType(dexType, connectorID string, config map[string]interface{}) string { if dexType != "oidc" { return dexType } + if v, ok := config["provider"].(string); ok && v != "" { + return v + } return inferOIDCProviderType(connectorID) }Please confirm Dex tolerates unknown config keys and won’t reject a stored
providerfield:Dex OIDC connector configuration: does it ignore unknown fields in JSON/YAML configs, and can custom keys like "provider" be stored safely?Also applies to: 271-278
233-269: Avoid returning client secrets from Get/List connector APIs.
parseStorageConnectorpopulatesClientSecret, which meansGetConnector/ListConnectorscan expose OAuth secrets. Redact by default and only include secrets for internal flows (e.g., UpdateConnector merge).🔒 Proposed fix
-func (p *Provider) parseStorageConnector(conn storage.Connector) (*ConnectorConfig, error) { +func (p *Provider) parseStorageConnector(conn storage.Connector, includeSecret bool) (*ConnectorConfig, error) { ... - if v, ok := configMap["clientSecret"].(string); ok { - cfg.ClientSecret = v - } + if includeSecret { + if v, ok := configMap["clientSecret"].(string); ok { + cfg.ClientSecret = v + } + } ... }Update call sites so GetConnector/ListConnectors pass
includeSecret=false, while UpdateConnector’s merge path passestrue.
Comment on lines
+91
to
+129
| // UpdateConnector updates an existing connector in Dex storage. | ||
| // It merges incoming updates with existing values to prevent data loss on partial updates. | ||
| func (p *Provider) UpdateConnector(ctx context.Context, cfg *ConnectorConfig) error { | ||
| if err := p.storage.UpdateConnector(ctx, cfg.ID, func(old storage.Connector) (storage.Connector, error) { | ||
| // Parse existing connector to preserve unset fields | ||
| oldCfg, err := p.parseStorageConnector(old) | ||
| if err != nil { | ||
| return storage.Connector{}, fmt.Errorf("failed to parse existing connector: %w", err) | ||
| } | ||
|
|
||
| // Merge: preserve existing values for empty fields in the update | ||
| if cfg.ClientSecret == "" { | ||
| cfg.ClientSecret = oldCfg.ClientSecret | ||
| } | ||
| if cfg.RedirectURI == "" { | ||
| cfg.RedirectURI = oldCfg.RedirectURI | ||
| } | ||
| if cfg.Issuer == "" && cfg.Type == oldCfg.Type { | ||
| cfg.Issuer = oldCfg.Issuer | ||
| } | ||
| if cfg.ClientID == "" { | ||
| cfg.ClientID = oldCfg.ClientID | ||
| } | ||
| if cfg.Name == "" { | ||
| cfg.Name = oldCfg.Name | ||
| } | ||
|
|
||
| storageConn, err := p.buildStorageConnector(cfg) | ||
| if err != nil { | ||
| return storage.Connector{}, fmt.Errorf("failed to build connector: %w", err) | ||
| } | ||
| return storageConn, nil | ||
| }); err != nil { | ||
| return fmt.Errorf("failed to update connector: %w", err) | ||
| } | ||
|
|
||
| p.logger.Info("connector updated", "id", cfg.ID, "type", cfg.Type) | ||
| return nil | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Search for UpdateConnector calls and their usage patterns
rg -n "UpdateConnector\(" --type go -B 3 -A 8Repository: netbirdio/netbird
Length of output: 3635
🏁 Script executed:
# Find the buildStorageConnector implementation to check Type requirement
rg -n "func.*buildStorageConnector" --type go -A 15Repository: netbirdio/netbird
Length of output: 991
🏁 Script executed:
# Search for how ConnectorConfig is constructed in callers
rg -n "ConnectorConfig\{" --type go -B 2 -A 5Repository: netbirdio/netbird
Length of output: 1272
🏁 Script executed:
# Check the connector.go file for full context on parseStorageConnector and Type handling
fd -e go -p "connector.go" | head -5Repository: netbirdio/netbird
Length of output: 81
🏁 Script executed:
# Get complete buildStorageConnector implementation
sed -n '160,190p' idp/dex/connector.goRepository: netbirdio/netbird
Length of output: 1117
🏁 Script executed:
# Check if there's more to the switch statement (default case)
sed -n '160,200p' idp/dex/connector.goRepository: netbirdio/netbird
Length of output: 1432
🏁 Script executed:
# Look at the full UpdateConnector caller to understand the flow
sed -n '200,215p' management/server/identity_provider.goRepository: netbirdio/netbird
Length of output: 651
🏁 Script executed:
# Check identityProviderToConnectorConfig to confirm Type is always set
sed -n '264,280p' management/server/identity_provider.goRepository: netbirdio/netbird
Length of output: 844
🏁 Script executed:
# Check if Issuer preservation actually matters and how it's used
rg -n "cfg.Issuer" idp/dex/connector.go -B 2 -A 2Repository: netbirdio/netbird
Length of output: 608
🏁 Script executed:
# Final comprehensive search for all UpdateConnector calls
rg -n "\.UpdateConnector\(" --type goRepository: netbirdio/netbird
Length of output: 517
🏁 Script executed:
# Also check if ConnectorConfig is ever created without Type
rg -n "ConnectorConfig\s*\{" --type go -A 6Repository: netbirdio/netbird
Length of output: 1038
Preserve connector Type on partial updates to prevent build failure and ensure consistent field merging.
The UpdateConnector method claims to support partial updates via field merging, but doesn't preserve Type. If Type is omitted, buildStorageConnector will fail with "unsupported connector type" (default case at line 177 returns error for unmatched Type). Additionally, the Issuer preservation logic at line 108 depends on Type not changing (cfg.Type == oldCfg.Type), so omitting Type breaks that too. For consistency with other preserved fields (ClientSecret, RedirectURI, ClientID, Name), Type should also be preserved from the existing connector.
🔧 Proposed fix
// Merge: preserve existing values for empty fields in the update
+ if cfg.Type == "" {
+ cfg.Type = oldCfg.Type
+ }
if cfg.ClientSecret == "" {
cfg.ClientSecret = oldCfg.ClientSecret
}🤖 Prompt for AI Agents
In `@idp/dex/connector.go` around lines 91 - 129, UpdateConnector currently merges
several fields from the existing connector but fails to preserve cfg.Type,
causing buildStorageConnector to error and breaking the Issuer-preservation
check that compares cfg.Type to oldCfg.Type; update the merging logic inside
Provider.UpdateConnector (the closure passed to p.storage.UpdateConnector) to
set cfg.Type = oldCfg.Type when cfg.Type is empty/zero, similar to how
ClientSecret, RedirectURI, ClientID, and Name are preserved, so
parseStorageConnector/oldCfg.Type is used to keep Type consistent before calling
buildStorageConnector.
|
bcmmbaga
approved these changes
Jan 20, 2026
jpmchia
pushed a commit
to jpmchia/netbird
that referenced
this pull request
Jan 21, 2026
Merged 26 commits from main including: - Local user password change (embedded IdP) (netbirdio#5132) - [Management/Client] Trigger debug bundle runs from API/Dashboard - [client] Add support to wildcard custom records - Feature/embedded STUN - [management] Add custom dns zones - Various bug fixes and improvements Resolved conflict in idp/dex/provider.go by: - Keeping UpdateUserPassword method from main - Preserving connector management code from idp-logto (in connector.go)
7 tasks
This was referenced Mar 30, 2026
7 tasks
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
2 participants
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
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
Documentation
Tests
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.