Upgrade baton-sdk. #103
Upgrade baton-sdk. #103
Conversation
WalkthroughUpdated dependency to baton-sdk v0.4.2. Refactored user account creation to use centralized crypto password generation, changed credential options type to LocalCredentialOptions, and now returns plaintext password data. Added interface conformance assertion for userBuilder. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller
participant userBuilder
participant Crypto as crypto.GeneratePassword
participant SDK as Baton SDK / Directory
Caller->>userBuilder: CreateAccount(accountInfo, LocalCredentialOptions)
userBuilder->>Crypto: GeneratePassword(ctx, options)
Crypto-->>userBuilder: password
userBuilder->>SDK: Provision account (accountInfo, credential)
SDK-->>userBuilder: CreateAccountResponse
userBuilder-->>Caller: (response, [plaintext password], annotations)
note over userBuilder,Crypto: Password generation centralized via crypto utility
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pkg/connector/users.go (2)
150-160: Treat NotFound distinctly from other GetUser errors before creating an account.Current logic may attempt creation on transient lookup errors (network, 500), risking duplicates or confusing failures.
- Only proceed to creation when the error is definitively “not found”.
- For other errors, return early.
If the client exposes helpers (e.g.,client.IsNotFound(err)), use them; otherwise map Jira’s HTTP 404 explicitly from the client layer.Also applies to: 171-178
192-201: Fix: require non‑nil RandomPassword options and zero sensitive passwordbaton-sdk v0.4.2 crypto.GeneratePassword requires a non‑nil *v2.LocalCredentialOptions with RandomPassword set — passing nil will return an error. Ensure you supply minimal RandomPassword defaults and reduce lifetime of plaintext in memory.
Locations: pkg/connector/users.go:192-201 (also 203-217, 219-229)
- Guard/populate before call: if credentialOptions == nil || credentialOptions.RandomPassword == nil { set a minimal RandomPassword (e.g. Length = 20) }.
- After appending plaintextData, overwrite/clear the plaintext byte slice and set password = "" (best-effort to reduce sensitive-data lifetime).
🧹 Nitpick comments (4)
pkg/connector/users.go (4)
11-11: Avoid shadowing the standard library “crypto” package; alias the baton import.Using the bare name
cryptofor an SDK package can confuse future maintainers if stdcrypto/*is later imported.@@ - "github.com/conductorone/baton-sdk/pkg/crypto" + c1crypto "github.com/conductorone/baton-sdk/pkg/crypto" @@ - password, err := crypto.GeneratePassword(ctx, credentialOptions) + password, err := c1crypto.GeneratePassword(ctx, credentialOptions)Also applies to: 192-196
182-190: Trim display name to avoid leading/trailing whitespace.When only one of first/last name is present, the current concat can produce a leading space.
@@ - displayName := firstName - if lastName != "" { - displayName += " " + lastName - } + displayName := strings.TrimSpace(firstName + " " + lastName) @@ // If display name is empty, use email as display nameAnd add:
@@ import ( "context" "fmt" + "strings"
47-52: Duplicate email trait addition; collapse to a single conditional.
WithEmailis added unconditionally and then again when non‑empty. Keep one conditional addition to avoid duplicates.@@ userTraitOpts = []sdkResource.UserTraitOption{ sdkResource.WithUserProfile(profile), sdkResource.WithStatus(userStatus), sdkResource.WithUserLogin(u.EmailAddress), - sdkResource.WithEmail(u.EmailAddress, true), } @@ - if u.EmailAddress != "" { - userTraitOpts = append(userTraitOpts, sdkResource.WithEmail(u.EmailAddress, true)) - } + if u.EmailAddress != "" { + userTraitOpts = append(userTraitOpts, sdkResource.WithEmail(u.EmailAddress, true)) + }Also applies to: 62-64
231-247: Optional: gate default‑group add on successful creation to avoid noisy retries.If the account pre‑exists, adding to the default group each time can spam logs on idempotent retries. Consider adding only on create, or handle AlreadyExists distinctly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (36)
.github/workflows/ci.yamlis excluded by none and included by nonego.sumis excluded by!**/*.sumand included by nonescripts/get-baton.shis excluded by none and included by nonevendor/github.com/conductorone/baton-sdk/pb/c1/connector/v2/annotation_sync_id.pb.gois excluded by!**/*.pb.go,!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pb/c1/connector/v2/annotation_sync_id.pb.validate.gois excluded by!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pb/c1/connector/v2/connector.pb.gois excluded by!**/*.pb.go,!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pb/c1/connector/v2/resource.pb.gois excluded by!**/*.pb.go,!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pb/c1/connector/v2/resource.pb.validate.gois excluded by!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pb/c1/connectorapi/baton/v1/session.pb.gois excluded by!**/*.pb.go,!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pb/c1/connectorapi/baton/v1/session.pb.validate.gois excluded by!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pb/c1/connectorapi/baton/v1/session_grpc.pb.gois excluded by!**/*.pb.go,!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pkg/annotations/annotations.gois excluded by!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pkg/cli/cli.gois excluded by!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pkg/cli/commands.gois excluded by!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pkg/cli/lambda_server__added.gois excluded by!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pkg/connectorbuilder/connectorbuilder.gois excluded by!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pkg/crypto/client_secret.gois excluded by!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pkg/crypto/crypto.gois excluded by!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pkg/crypto/password.gois excluded by!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pkg/crypto/providers/jwk/jwk.gois excluded by!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pkg/crypto/providers/registry.gois excluded by!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pkg/dotc1z/c1file.gois excluded by!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pkg/lambda/grpc/config/config.gois excluded by!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pkg/provisioner/provisioner.gois excluded by!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pkg/sdk/version.gois excluded by!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pkg/session/README.mdis excluded by!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pkg/session/grpc_session.gois excluded by!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pkg/session/json.gois excluded by!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pkg/session/memory.gois excluded by!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pkg/session/session.gois excluded by!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pkg/sync/client_wrapper.gois excluded by!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pkg/sync/syncer.gois excluded by!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pkg/types/session_cache.gois excluded by!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pkg/ugrpc/c1_credential_provider.gois excluded by!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pkg/ugrpc/interceptors.gois excluded by!vendor/**and included by nonevendor/modules.txtis excluded by!vendor/**and included by none
📒 Files selected for processing (2)
go.mod(1 hunks)pkg/connector/users.go(4 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). (2)
- GitHub Check: Cursor Bugbot
- GitHub Check: test
🔇 Additional comments (3)
pkg/connector/users.go (2)
23-24: LGTM: compile‑time interface conformance assertion.Good safeguard to track breaking changes from baton-sdk upgrades.
126-131: Signature shift to LocalCredentialOptions and plaintext return — verified. Repo search found only the updated CreateAccount signature in pkg/connector/users.go; no remaining references to *v2.CredentialOptions or callers expecting a two‑value return were found.go.mod (1)
6-6: Tidy failed — resolve missing module and re-run checks.go mod tidy aborted with: "no matching versions for query 'latest'" for github.com/conductorone/baton-jira-datacenter/pb/c1/connector/v2, so tidy didn’t complete and I could not verify the baton-sdk v0.4.2 API changes.
- Fix the missing module (add correct version, add a go.mod replace, or ensure the module is available) and run:
- go mod tidy
- rg -n --type go '\bLocalCredentialOptions\b|\bPlaintextData\b|\bGeneratePassword\b'
- go list -m all | rg 'github.com/conductorone/baton-sdk'
- Confirm CreateAccount API changes (LocalCredentialOptions, PlaintextData) are adopted and update code/tests if needed.
Use crypto.GeneratePassword() instead of one-off functions.
Summary by CodeRabbit
New Features
Chores