Add support for setting passwords - #77
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds credential-rotation configuration and encrypted-password support; centralizes password generation into a new generatePassword helper; replaces Changes
Sequence Diagram(s)sequenceDiagram
participant US as userSyncer
participant GP as generatePassword
participant PQ as prepareQueryInputs
participant DB as DB/Provisioner
rect rgba(230,248,255,0.6)
Note over US,GP: CreateAccount / Rotate -> password generation
US->>GP: generatePassword(ctx, LocalCredentialOptions)
GP-->>US: password or error
end
rect rgba(240,250,230,0.6)
Note over US,PQ: Prepare inputs & collect plaintext
US->>PQ: prepareQueryInputs(ctx, provisioningCfg, accountInfo, LocalCredentialOptions)
PQ-->>US: queryInputs + plaintextDataList
end
rect rgba(255,240,230,0.6)
Note over US,DB: Execute provisioning (transactional)
US->>DB: execute provisioning queries (txn)
DB-->>US: success / failure
US-->>Caller: plaintextDataList, annotations, error
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used🧬 Code graph analysis (2)pkg/bsql/provisioning.go (3)
pkg/bsql/user_syncer.go (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (3)
Comment |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
pkg/bsql/config.go (1)
375-380: JSON/YAML tag mismatch on AccountValidationConfig.Queryjson tag should be "query", not "queries". Current tag will break JSON unmarshalling.
Apply this diff:
type AccountValidationConfig struct { // Vars provides variables that can be used within account validation SQL queries. Vars map[string]string `yaml:"vars,omitempty" json:"vars,omitempty"` // Queries is a list of SQL statements to execute for account validation. - Query string `yaml:"query" json:"queries"` + Query string `yaml:"query" json:"query"` }pkg/bsql/provisioning.go (1)
33-37: Dynamic entitlements: missing ID match may return wrong provisioning configYou return the first dynamic entitlement with Provisioning enabled without checking the entitlementID. This can pick the wrong config.
Apply this diff:
- for _, e := range s.config.Entitlements.Map { - if e.Provisioning != nil { + for _, e := range s.config.Entitlements.Map { + if e.Id != entitlementID { + continue + } + if e.Provisioning != nil { l.Info("provisioning is enabled for entitlement", zap.String("entitlement_id", entitlementID)) return e.Provisioning, true } }pkg/bsql/user_syncer.go (1)
61-94: Capability details omit PlaintextPasswordSupport and preferred sets only include NoPassword and RandomPassword. Add PlaintextPassword to reflect the new flow and config (after renaming in config.go).
Apply this diff:
if accountProvisioning.Credentials.RandomPassword != nil { supportedCredentials = append(supportedCredentials, v2.CapabilityDetailCredentialOption_CAPABILITY_DETAIL_CREDENTIAL_OPTION_RANDOM_PASSWORD) if accountProvisioning.Credentials.RandomPassword.Preferred { preferredCredentialOption = append(preferredCredentialOption, v2.CapabilityDetailCredentialOption_CAPABILITY_DETAIL_CREDENTIAL_OPTION_RANDOM_PASSWORD) } } + // Plaintext password (user-supplied) + if accountProvisioning.Credentials.PlaintextPassword != nil { + supportedCredentials = append(supportedCredentials, v2.CapabilityDetailCredentialOption_CAPABILITY_DETAIL_CREDENTIAL_OPTION_PLAINTEXT_PASSWORD) + if accountProvisioning.Credentials.PlaintextPassword.Preferred { + preferredCredentialOption = append(preferredCredentialOption, v2.CapabilityDetailCredentialOption_CAPABILITY_DETAIL_CREDENTIAL_OPTION_PLAINTEXT_PASSWORD) + } + }
🧹 Nitpick comments (2)
pkg/bsql/user_syncer_test.go (1)
129-135: Consider adding a NoPassword caseAdd a test where credentialOptions is NoPassword to assert no "password" key is injected and no plaintext data is produced.
Would you like me to draft this test?
pkg/bsql/provisioning.go (1)
221-224: Don’t drop int fields set to 0Zero is a valid value; currently it’s excluded.
Apply this diff:
- case "int": - if numValue := value.GetNumberValue(); numValue != 0 { - parsedValue = int(numValue) - } + case "int": + parsedValue = int(value.GetNumberValue())
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
pkg/bsql/config.go(2 hunks)pkg/bsql/helpers.go(0 hunks)pkg/bsql/helpers_test.go(7 hunks)pkg/bsql/provisioning.go(4 hunks)pkg/bsql/user_syncer.go(4 hunks)pkg/bsql/user_syncer_test.go(6 hunks)
💤 Files with no reviewable changes (1)
- pkg/bsql/helpers.go
🧰 Additional context used
🧬 Code graph analysis (3)
pkg/bsql/provisioning.go (1)
pkg/bsql/config.go (1)
AccountProvisioning(324-333)
pkg/bsql/user_syncer_test.go (1)
pkg/bsql/config.go (1)
AccountProvisioningField(336-342)
pkg/bsql/user_syncer.go (1)
pkg/bsql/resource_types.go (1)
ErrNoAccountProvisioningDefined(13-13)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Cursor Bugbot
- GitHub Check: test
- GitHub Check: go-test (ubuntu-latest)
🔇 Additional comments (4)
pkg/bsql/user_syncer_test.go (1)
12-12: Good: context propagation into testsUsing t.Context() is consistent with the new ctx-aware helpers.
pkg/bsql/helpers_test.go (1)
180-188: NoPassword case covered: looks goodThe test ensures generatePassword returns empty with no error for NoPassword.
pkg/bsql/provisioning.go (2)
272-292: generatePassword: behavior is fineSwitching on LocalCredentialOptions types and delegating to crypto.GeneratePassword is correct.
8-11: Update baton-sdk dependency to v0.4.5
Pingithub.com/conductorone/baton-sdkto v0.4.5 in go.mod so thatLocalCredentialOptionsandcrypto.GeneratePasswordare available.
| NoPassword *NoPasswordConfig `yaml:"no_password,omitempty" json:"no_password,omitempty"` | ||
| RandomPassword *RandomPasswordConfig `yaml:"random_password,omitempty" json:"random_password,omitempty"` | ||
| EncryptedPassword *EncryptedPasswordConfig `yaml:"encrypted_password,omitempty" json:"encrypted_password,omitempty"` | ||
| } |
There was a problem hiding this comment.
Align credential config with SDK: use PlaintextPassword instead of EncryptedPassword
LocalCredentialOptions supports PlaintextPassword; this config introduces EncryptedPassword which is unused elsewhere and mismatched. Rename to PlaintextPassword to avoid confusion and wire-up with capability details.
Apply this diff:
type AccountCredentials struct {
NoPassword *NoPasswordConfig `yaml:"no_password,omitempty" json:"no_password,omitempty"`
RandomPassword *RandomPasswordConfig `yaml:"random_password,omitempty" json:"random_password,omitempty"`
- EncryptedPassword *EncryptedPasswordConfig `yaml:"encrypted_password,omitempty" json:"encrypted_password,omitempty"`
+ PlaintextPassword *PlaintextPasswordConfig `yaml:"plaintext_password,omitempty" json:"plaintext_password,omitempty"`
}📝 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.
| NoPassword *NoPasswordConfig `yaml:"no_password,omitempty" json:"no_password,omitempty"` | |
| RandomPassword *RandomPasswordConfig `yaml:"random_password,omitempty" json:"random_password,omitempty"` | |
| EncryptedPassword *EncryptedPasswordConfig `yaml:"encrypted_password,omitempty" json:"encrypted_password,omitempty"` | |
| } | |
| type AccountCredentials struct { | |
| NoPassword *NoPasswordConfig `yaml:"no_password,omitempty" json:"no_password,omitempty"` | |
| RandomPassword *RandomPasswordConfig `yaml:"random_password,omitempty" json:"random_password,omitempty"` | |
| PlaintextPassword *PlaintextPasswordConfig `yaml:"plaintext_password,omitempty" json:"plaintext_password,omitempty"` | |
| } |
🤖 Prompt for AI Agents
In pkg/bsql/config.go around lines 346 to 349, the credential struct field is
named EncryptedPassword and uses EncryptedPasswordConfig which mismatches the
SDK that expects PlaintextPassword; rename the field to PlaintextPassword,
change its type to *PlaintextPasswordConfig, and update the yaml/json tags from
"encrypted_password" to "plaintext_password"; then search and update all
references/usages (constructors, unmarshalling, tests, and capability wiring) to
use PlaintextPassword and PlaintextPasswordConfig so the config aligns with the
SDK.
| // EncryptedPasswordConfig defines configuration for encrypted password generation. | ||
| type EncryptedPasswordConfig struct { | ||
| BaseCredentialConfig `yaml:",inline"` | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Rename config type to match Plaintext option
Update the config type to reflect PlaintextPassword and consistent docs.
Apply this diff:
-// EncryptedPasswordConfig defines configuration for encrypted password generation.
-type EncryptedPasswordConfig struct {
+// PlaintextPasswordConfig defines configuration for user-provided plaintext passwords.
+type PlaintextPasswordConfig struct {
BaseCredentialConfig `yaml:",inline"`
}📝 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.
| // EncryptedPasswordConfig defines configuration for encrypted password generation. | |
| type EncryptedPasswordConfig struct { | |
| BaseCredentialConfig `yaml:",inline"` | |
| } | |
| // PlaintextPasswordConfig defines configuration for user-provided plaintext passwords. | |
| type PlaintextPasswordConfig struct { | |
| BaseCredentialConfig `yaml:",inline"` | |
| } |
🤖 Prompt for AI Agents
In pkg/bsql/config.go around lines 369 to 373, the config type is currently
named EncryptedPasswordConfig but should reflect the Plaintext option; rename
the type and its doc comment to PlaintextPasswordConfig, keep the embedded
BaseCredentialConfig with the same yaml inline tag, and update any
references/usages, tests, and documentation to use PlaintextPasswordConfig to
maintain consistency.
| func (s *userSyncer) RotateCapabilityDetails(ctx context.Context) (*v2.CredentialDetailsCredentialRotation, annotations.Annotations, error) { | ||
| l := ctxzap.Extract(ctx) | ||
| resourceTypeID, accountProvisioning, err := s.fullConfig.ExtractAccountProvisioning() | ||
| if err != nil { | ||
| if errors.Is(err, ErrNoAccountProvisioningDefined) { | ||
| return nil, nil, nil | ||
| } | ||
|
|
||
| return nil, nil, err | ||
| } | ||
|
|
||
| l.Debug("account provisioning is enabled", zap.String("resource_type_id", resourceTypeID)) | ||
|
|
||
| if accountProvisioning == nil { | ||
| return nil, nil, errors.New("no account provisioning defined") | ||
| } | ||
|
|
||
| if accountProvisioning.Credentials == nil { | ||
| return nil, nil, errors.New("no credential options defined") | ||
| } | ||
|
|
||
| var supportedCredentials []v2.CapabilityDetailCredentialOption | ||
| var preferredCredentialOption []v2.CapabilityDetailCredentialOption | ||
|
|
||
| if accountProvisioning.Credentials.NoPassword != nil { | ||
| supportedCredentials = append(supportedCredentials, v2.CapabilityDetailCredentialOption_CAPABILITY_DETAIL_CREDENTIAL_OPTION_NO_PASSWORD) | ||
| if accountProvisioning.Credentials.NoPassword.Preferred { | ||
| preferredCredentialOption = append(preferredCredentialOption, v2.CapabilityDetailCredentialOption_CAPABILITY_DETAIL_CREDENTIAL_OPTION_NO_PASSWORD) | ||
| } | ||
| } | ||
|
|
||
| if accountProvisioning.Credentials.RandomPassword != nil { | ||
| supportedCredentials = append(supportedCredentials, v2.CapabilityDetailCredentialOption_CAPABILITY_DETAIL_CREDENTIAL_OPTION_RANDOM_PASSWORD) | ||
| if accountProvisioning.Credentials.RandomPassword.Preferred { | ||
| preferredCredentialOption = append(preferredCredentialOption, v2.CapabilityDetailCredentialOption_CAPABILITY_DETAIL_CREDENTIAL_OPTION_RANDOM_PASSWORD) | ||
| } | ||
| } | ||
|
|
||
| if len(supportedCredentials) == 0 { | ||
| return nil, nil, nil | ||
| } | ||
|
|
||
| if len(preferredCredentialOption) > 1 { | ||
| return nil, nil, errors.New("multiple preferred credential options are not supported") | ||
| } | ||
|
|
||
| if len(preferredCredentialOption) == 0 { | ||
| preferredCredentialOption = []v2.CapabilityDetailCredentialOption{supportedCredentials[0]} | ||
| } | ||
|
|
||
| return &v2.CredentialDetailsCredentialRotation{ | ||
| SupportedCredentialOptions: supportedCredentials, | ||
| PreferredCredentialOption: preferredCredentialOption[0], | ||
| }, nil, nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Rotation capability details also omit PlaintextPassword
Mirror CreateAccount capability details for rotation.
Apply this diff:
if accountProvisioning.Credentials.RandomPassword != nil {
supportedCredentials = append(supportedCredentials, v2.CapabilityDetailCredentialOption_CAPABILITY_DETAIL_CREDENTIAL_OPTION_RANDOM_PASSWORD)
if accountProvisioning.Credentials.RandomPassword.Preferred {
preferredCredentialOption = append(preferredCredentialOption, v2.CapabilityDetailCredentialOption_CAPABILITY_DETAIL_CREDENTIAL_OPTION_RANDOM_PASSWORD)
}
}
+ if accountProvisioning.Credentials.PlaintextPassword != nil {
+ supportedCredentials = append(supportedCredentials, v2.CapabilityDetailCredentialOption_CAPABILITY_DETAIL_CREDENTIAL_OPTION_PLAINTEXT_PASSWORD)
+ if accountProvisioning.Credentials.PlaintextPassword.Preferred {
+ preferredCredentialOption = append(preferredCredentialOption, v2.CapabilityDetailCredentialOption_CAPABILITY_DETAIL_CREDENTIAL_OPTION_PLAINTEXT_PASSWORD)
+ }
+ }🤖 Prompt for AI Agents
In pkg/bsql/user_syncer.go around lines 185 to 239, Rotation capability details
currently only include NoPassword and RandomPassword and therefore omit
PlaintextPassword; update the function to mirror CreateAccount capability
behavior by adding handling for
accountProvisioning.Credentials.PlaintextPassword: append the corresponding
v2.CapabilityDetailCredentialOption_CAPABILITY_DETAIL_CREDENTIAL_OPTION_PLAINTEXT_PASSWORD
to supportedCredentials when non-nil, and if PlaintextPassword.Preferred is true
append it to preferredCredentialOption; ensure preferred selection logic (max
one preferred, default to first supported) remains unchanged and return the
preferredCredentialOption[0] as before.
…is no longer used.
d212f87 to
1ada9b3
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
…ion test to CI tests.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (8)
.github/workflows/ci.yamlis excluded by none and included by none.gon-amd64.jsonis excluded by none and included by none.gon-arm64.jsonis excluded by none and included by none.goreleaser.docker.yamlis excluded by none and included by none.goreleaser.yamlis excluded by none and included by noneDockerfile.testis excluded by none and included by nonedocker-compose-postgres-test.ymlis excluded by none and included by noneexamples/postgres-test.ymlis excluded by none and included by none
📒 Files selected for processing (4)
pkg/bsql/config.go(5 hunks)pkg/bsql/provisioning.go(5 hunks)pkg/bsql/resource_types.go(1 hunks)pkg/bsql/user_syncer.go(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
pkg/bsql/resource_types.go (1)
pkg/connector/connector.go (1)
New(83-90)
pkg/bsql/user_syncer.go (2)
pkg/bsql/config.go (1)
AccountCredentials(348-352)pkg/bsql/resource_types.go (1)
ErrNoCredentialRotationDefined(14-14)
pkg/bsql/config.go (1)
pkg/bsql/resource_types.go (1)
ErrNoCredentialRotationDefined(14-14)
pkg/bsql/provisioning.go (3)
pkg/bsql/config.go (2)
AccountProvisioning(327-336)CredentialRotation(396-403)pkg/bsql/sql_syncer.go (1)
SQLSyncer(20-27)pkg/bsql/resource_types.go (1)
ErrNoCredentialRotationDefined(14-14)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (4)
pkg/bsql/config.go (1)
349-376: Rename EncryptedPassword option to PlaintextPassword
LocalCredentialOptionsonly exposesNoPassword,RandomPassword, andPlaintextPasswordbranches. IntroducingEncryptedPasswordhere means we surface a credential option the SDK neither understands nor can marshal, and the downstream code already fails to generate or advertise such an option. Please rename this field/type (and the associated YAML/JSON tags) back toPlaintextPasswordso the config stays aligned with the SDK schema.Apply this diff (and update call sites accordingly):
type AccountCredentials struct { NoPassword *NoPasswordConfig `yaml:"no_password,omitempty" json:"no_password,omitempty"` RandomPassword *RandomPasswordConfig `yaml:"random_password,omitempty" json:"random_password,omitempty"` - EncryptedPassword *EncryptedPasswordConfig `yaml:"encrypted_password,omitempty" json:"encrypted_password,omitempty"` + PlaintextPassword *PlaintextPasswordConfig `yaml:"plaintext_password,omitempty" json:"plaintext_password,omitempty"` } -// EncryptedPasswordConfig defines configuration for encrypted password generation. -type EncryptedPasswordConfig struct { +// PlaintextPasswordConfig defines configuration for user-provided plaintext passwords. +type PlaintextPasswordConfig struct { BaseCredentialConfig `yaml:",inline"` }pkg/bsql/user_syncer.go (2)
40-78: Advertise Plaintext credential support instead of EncryptedSame issue as the config: the SDK’s enum exposes
CAPABILITY_DETAIL_CREDENTIAL_OPTION_PLAINTEXT_PASSWORD, not an encrypted variant. The current branch will fail to compile/advertise the wrong capability. Please switch this block back to Plaintext so capability details line up with the actual options we can issue.- if credentials.EncryptedPassword != nil { - supportedCredentials = append(supportedCredentials, v2.CapabilityDetailCredentialOption_CAPABILITY_DETAIL_CREDENTIAL_OPTION_ENCRYPTED_PASSWORD) - if credentials.EncryptedPassword.Preferred { - preferredCredentialOption = append(preferredCredentialOption, v2.CapabilityDetailCredentialOption_CAPABILITY_DETAIL_CREDENTIAL_OPTION_ENCRYPTED_PASSWORD) - } - } + if credentials.PlaintextPassword != nil { + supportedCredentials = append(supportedCredentials, v2.CapabilityDetailCredentialOption_CAPABILITY_DETAIL_CREDENTIAL_OPTION_PLAINTEXT_PASSWORD) + if credentials.PlaintextPassword.Preferred { + preferredCredentialOption = append(preferredCredentialOption, v2.CapabilityDetailCredentialOption_CAPABILITY_DETAIL_CREDENTIAL_OPTION_PLAINTEXT_PASSWORD) + } + }
169-193: Rotation queries need resource and credential contextWe currently execute the update queries with only
passwordin the parameter map, leavingresourceIdcompletely unused and never attaching thecredentialsnamespace. That prevents the SQL from targeting the specific user and from referencingcredentials.password, and on top of that we enqueue an empty password whenLocalCredentialOptionsisNoPassword. Please populate the resource identifiers, only add password values when non-empty, and wire the credentials namespace before running the queries.- queryInputs := make(map[string]any) - credentials := make(map[string]any) + queryInputs := make(map[string]any) + credentials := make(map[string]any) + if resourceId == nil || resourceId.GetResource() == "" { + return nil, nil, errors.New("resource id is required for rotation") + } + queryInputs["resource"] = resourceId.GetResource() + if rt := resourceId.GetResourceType(); rt != "" { + queryInputs["resource_type"] = rt + } var plaintextDataList []*v2.PlaintextData password, err := generatePassword(ctx, credentialOptions) if err != nil { return nil, nil, err } - queryInputs["password"] = password - credentials["password"] = password - // Create plaintext data for return - passwordData := &v2.PlaintextData{ - Name: "password", - Bytes: []byte(password), - } - plaintextDataList = append(plaintextDataList, passwordData) + if password != "" { + queryInputs["password"] = password + credentials["password"] = password + passwordData := &v2.PlaintextData{ + Name: "password", + Bytes: []byte(password), + } + plaintextDataList = append(plaintextDataList, passwordData) + } + if len(credentials) > 0 { + queryInputs["credentials"] = credentials + }pkg/bsql/provisioning.go (1)
241-254: Skip injecting blank password for NoPassword option
generatePasswordreturns""forLocalCredentialOptions_NoPassword, yet we still setqueryInputs["password"]and return empty plaintext data. That regresses the prior behavior where the password fields were omitted entirely. Guard onpassword != ""before touching the maps or plaintext list.- queryInputs["password"] = password - credentials["password"] = password - // Create plaintext data for return - passwordData := &v2.PlaintextData{ - Name: "password", - Bytes: []byte(password), - } - plaintextDataList = append(plaintextDataList, passwordData) + if password != "" { + queryInputs["password"] = password + credentials["password"] = password + passwordData := &v2.PlaintextData{ + Name: "password", + Bytes: []byte(password), + } + plaintextDataList = append(plaintextDataList, passwordData) + }
| resourceTypeID, rotationConfig, err := s.extractAndValidateCredentialRotation() | ||
| if err != nil { | ||
| return nil, nil, err | ||
| } | ||
|
|
||
| logger.Debug("rotating credential", zap.String("resource_type_id", resourceTypeID)) | ||
|
|
||
| queryInputs := make(map[string]any) | ||
| credentials := make(map[string]any) | ||
| var plaintextDataList []*v2.PlaintextData | ||
| password, err := generatePassword(ctx, credentialOptions) | ||
| if err != nil { | ||
| return nil, nil, err | ||
| } | ||
| queryInputs["password"] = password |
There was a problem hiding this comment.
Avoid nil dereference when rotation config is absent
extractAndValidateCredentialRotation returns ( "", nil, nil ) when no rotation block is defined. Line [168] immediately dereferences rotationConfig, which will panic in that case. Short-circuit by returning ErrNoCredentialRotationDefined (or similar) before accessing the struct.
resourceTypeID, rotationConfig, err := s.extractAndValidateCredentialRotation()
if err != nil {
return nil, nil, err
}
+ if rotationConfig == nil {
+ return nil, nil, ErrNoCredentialRotationDefined
+ }Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In pkg/bsql/user_syncer.go around lines 162 to 176,
extractAndValidateCredentialRotation can return ("", nil, nil) when no rotation
block is defined, but the code immediately dereferences rotationConfig causing a
possible panic; fix by checking if rotationConfig == nil right after the call
and return a sentinel error such as ErrNoCredentialRotationDefined (or create it
if missing) before any access to rotationConfig, so the function short-circuits
safely instead of dereferencing a nil pointer.
faa668e to
341586b
Compare
ggreer
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/bsql/helpers_test.go(8 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cursor Bugbot
…word into inputs, and don't return encrypted password data.
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
pkg/bsql/helpers_test.go(8 hunks)pkg/bsql/provisioning.go(5 hunks)pkg/bsql/user_syncer.go(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
pkg/bsql/user_syncer.go (2)
pkg/bsql/config.go (1)
AccountCredentials(348-352)pkg/bsql/resource_types.go (1)
ErrNoCredentialRotationDefined(14-14)
pkg/bsql/provisioning.go (3)
pkg/bsql/config.go (2)
AccountProvisioning(327-336)CredentialRotation(396-401)pkg/bsql/sql_syncer.go (1)
SQLSyncer(20-27)pkg/bsql/resource_types.go (1)
ErrNoCredentialRotationDefined(14-14)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Cursor Bugbot
- GitHub Check: go-test (ubuntu-latest)
🔇 Additional comments (2)
pkg/bsql/user_syncer.go (1)
163-177: Guard against missing rotation config
extractAndValidateCredentialRotationcan return( "", nil, nil )when the config is absent, and we dereference it immediately. That panics. Short-circuit with the sentinel error before touching the struct.resourceTypeID, rotationConfig, err := s.extractAndValidateCredentialRotation() if err != nil { return nil, nil, err } + if rotationConfig == nil { + return nil, nil, ErrNoCredentialRotationDefined + }pkg/bsql/helpers_test.go (1)
190-200: Set the expected plaintext passwordThe new assertion never fires because
expectedValuestays empty for the plaintext case, so we still don’t verify pass-through behavior. Populate it so the comparison actually runs.{ name: "valid plaintext password", credentialOptions: &v2.LocalCredentialOptions{ Options: &v2.LocalCredentialOptions_PlaintextPassword_{ PlaintextPassword: &v2.LocalCredentialOptions_PlaintextPassword{ PlaintextPassword: "password", }, }, }, expectError: false, expectNonEmpty: true, + expectedValue: "password", },
| case *v2.LocalCredentialOptions_RandomPassword_, *v2.LocalCredentialOptions_PlaintextPassword_: | ||
| password, err = crypto.GeneratePassword(ctx, credentialOptions) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to generate password: %w", err) | ||
| } | ||
|
|
||
| default: | ||
| return nil, fmt.Errorf("unsupported credential options: %v", credentialOptions) | ||
| } |
There was a problem hiding this comment.
Handle encrypted password options
generatePassword now rejects LocalCredentialOptions_EncryptedPassword_, even though the capability advertises support for it. That breaks encrypted-password provisioning/rotation. Include the encrypted case so we still delegate to crypto.GeneratePassword.
- case *v2.LocalCredentialOptions_RandomPassword_, *v2.LocalCredentialOptions_PlaintextPassword_:
+ case *v2.LocalCredentialOptions_RandomPassword_,
+ *v2.LocalCredentialOptions_PlaintextPassword_,
+ *v2.LocalCredentialOptions_EncryptedPassword_:🤖 Prompt for AI Agents
In pkg/bsql/provisioning.go around lines 284 to 292, the switch over
LocalCredentialOptions omits the EncryptedPassword case so encrypted password
types are rejected; add the *v2.LocalCredentialOptions_EncryptedPassword_ type
to the case list (or add a separate case that delegates to
crypto.GeneratePassword) so encrypted-password options are passed through to
crypto.GeneratePassword and handled like RandomPassword/PlaintextPassword,
leaving the default branch unchanged.
| queryInputs["password"] = password | ||
| credentials["password"] = password | ||
| // Create plaintext data for return | ||
| passwordData := &v2.PlaintextData{ | ||
| Name: "password", | ||
| Bytes: []byte(*password), | ||
| } | ||
| plaintextDataList = append(plaintextDataList, passwordData) | ||
| } | ||
|
|
||
| // Execute account creation queries | ||
| useTransaction := !rotationConfig.Update.NoTransaction | ||
| if err := s.runProvisioningQueries(ctx, rotationConfig.Update.Queries, queryInputs, useTransaction); err != nil { | ||
| return nil, nil, err | ||
| } | ||
|
|
||
| return plaintextDataList, nil, nil | ||
| } | ||
|
|
There was a problem hiding this comment.
Return strings (and expose credentials) during rotation
Same pointer issue as provisioning: storing *string in queryInputs breaks Exec. Additionally, we build a credentials map but never expose it, so CEL expressions like credentials.password fail. Mirror the create flow by storing the string and namespacing the credentials.
var plaintextDataList []*v2.PlaintextData
password, err := generatePassword(ctx, credentialOptions)
if err != nil {
return nil, nil, err
}
if password != nil {
- queryInputs["password"] = password
- credentials["password"] = password
+ pw := *password
+ queryInputs["password"] = pw
+ credentials["password"] = pw
// Create plaintext data for return
passwordData := &v2.PlaintextData{
Name: "password",
- Bytes: []byte(*password),
+ Bytes: []byte(pw),
}
plaintextDataList = append(plaintextDataList, passwordData)
}
+ if len(credentials) > 0 {
+ queryInputs["credentials"] = credentials
+ }
2 participants
Description
Upgrade baton-sdk. Start to hook up CI tests again.
Add support for setting passwords.
Bug fix
New feature
Useful links:
Summary by CodeRabbit
Note
Upgrade to baton-sdk v0.4.x and add encrypted/plaintext password support across account create/rotate, plus session cache and sync enhancements.
CredentialOptionsflows withLocalCredentialOptions; support random, plaintext, and encrypted passwords (client-secret decryption) for account create/rotate.ActiveSyncIdthrough requests; introduce sync types (full,partial,resources_only) and option to skip entitlements/grants..c1zdecoder/options tweaks.client-secretin context.Written by Cursor Bugbot for commit c187ca3. This will update automatically on new commits. Configure here.