Conversation
WalkthroughRaised Go version to 1.25 and removed toolchain directive. Updated dependencies, including baton-sdk to v0.4.1. Added a new LDAP DN parsing test. Removed a redundant blank import in a test. Changed CreateAccount to accept v2.LocalCredentialOptions and added an interface conformance check. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
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}
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 (3)
pkg/connector/user.go (2)
157-164: containsBinaryData wrongly treats non‑ASCII UTF‑8 as binary (i18n bug).
Accented names and non‑Latin scripts will be dropped from profiles/logins.Replace with UTF‑8 validity + control‑char check:
func containsBinaryData(value string) bool { - for _, c := range value { - if c < 32 || c > 126 { - return true - } - } - return false + if !utf8.ValidString(value) { + return true + } + for _, r := range value { + if r < 0x20 && r != '\n' && r != '\r' && r != '\t' { + return true + } + } + return false }Add import (outside this hunk):
import "utf8"Specifically:
import ( ... + "unicode/utf8" )
396-397: Avoid logging PII and raw AddRequest payloads.
Current error logs can leak names, emails, and potentially passwords.Apply:
- l.Error("baton-ldap: create-account failed to extract profile", zap.Error(err), zap.Any("accountInfo", accountInfo)) + l.Error("baton-ldap: create-account failed to extract profile", zap.Error(err)) @@ - l.Error("baton-ldap: create-account failed to create account", zap.Error(err), zap.Any("userParams", user)) + l.Error("baton-ldap: create-account failed to create account", zap.Error(err), zap.String("dn", dn)) @@ - l.Error("baton-ldap: create-account failed to get account", zap.Error(err), zap.Any("accountInfo", accountInfo)) + l.Error("baton-ldap: create-account failed to get account", zap.Error(err), zap.String("dn", dn)) @@ - l.Error("baton-ldap: create-account failed to create resource", zap.Error(err), zap.Any("accountInfo", accountInfo)) + l.Error("baton-ldap: create-account failed to create resource", zap.Error(err), zap.String("dn", dn))Also applies to: 406-408, 413-414, 419-420
go.mod (1)
16-17: Replace golang.org/x/exp/slices with the standard library "slices" and rungo mod tidy -compat=1.25.
- Update import "golang.org/x/exp/slices" → "slices" in: pkg/connector/helpers.go, pkg/connector/user.go, pkg/connector/group.go.
🧹 Nitpick comments (5)
pkg/connector/helpers_test.go (1)
10-22: Make the test table‑driven and add invalid cases.
Improves coverage (escaping, multi‑valued RDNs, invalid DNs) and readability. Also safe to run in parallel.Apply:
func TestParseDN(t *testing.T) { - dn, err := ldap.ParseDN("cn=test,ou=test,dc=example,dc=com") - require.NoError(t, err) - require.Equal(t, "cn=test,ou=test,dc=example,dc=com", dn.String()) - - dn, err = ldap.ParseDN("dc=example,dc=com") - require.NoError(t, err) - require.Equal(t, "dc=example,dc=com", dn.String()) - - dn, err = ldap.ParseDN("ou=example") - require.NoError(t, err) - require.Equal(t, "ou=example", dn.String()) + t.Parallel() + cases := []struct { + name string + in string + ok bool + }{ + {"full", "cn=test,ou=test,dc=example,dc=com", true}, + {"dcOnly", "dc=example,dc=com", true}, + {"ouOnly", "ou=example", true}, + {"escaped", `cn=a\,b,dc=example,dc=com`, true}, + {"invalid", "cn==bad,dc=example,dc=com", false}, + } + for _, tc := range cases { + tc := tc + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + dn, err := ldap.ParseDN(tc.in) + if !tc.ok { + require.Error(t, err) + return + } + require.NoError(t, err) + require.Equal(t, tc.in, dn.String()) + }) + } }pkg/connector/user.go (4)
19-19: Prefer stdlib slices over x/exp.
Reduces deps; Go 1.25 provides slices.Contains/Sort.Apply:
- "golang.org/x/exp/slices" + "slices"
135-136: Deterministic alias order (optional).
Helps stable outputs/tests.Apply:
- return login, aliases.ToSlice() + als := aliases.ToSlice() + slices.Sort(als) + return login, als
101-108: Handle nsAccountLock parse errors.
Currently ignores errors and may misclassify status.Apply:
- } else if nsAccountLockFlag != "" { - locked, _ := strconv.ParseBool(nsAccountLockFlag) + } else if nsAccountLockFlag != "" { + locked, err := strconv.ParseBool(nsAccountLockFlag) + if err != nil { + return userStatus, fmt.Errorf("invalid nsAccountLock value %q: %w", nsAccountLockFlag, err) + }
240-244: Comment contradicts code.
You try AD first (lastLogonTimestamp), then OpenLDAP (authTimestamp).Apply:
- // Try openldap format first, then fall back to Active Directory's format + // Try Active Directory format first, then fall back to OpenLDAP's format
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (105)
.github/workflows/ci.yamlis excluded by none and included by none.github/workflows/release.yamlis excluded by none and included by none.golangci.ymlis 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 nonego.sumis excluded by!**/*.sumand included by nonevendor/github.com/conductorone/baton-sdk/internal/connector/connector.gois excluded by!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pb/c1/connector/v2/annotation_resource_tree.pb.gois excluded by!**/*.pb.go,!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pb/c1/connector/v2/annotation_resource_tree.pb.validate.gois excluded by!vendor/**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/event_feed.pb.gois excluded by!**/*.pb.go,!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pb/c1/connector/v2/event_feed.pb.validate.gois excluded by!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/baton.pb.gois excluded by!**/*.pb.go,!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pb/c1/connectorapi/baton/v1/baton.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/bid/bid.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/config/config.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/connectorrunner/runner.gois excluded by!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pkg/connectorstore/connectorstore.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/dotc1z/c1file_attached.gois excluded by!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pkg/dotc1z/clone_sync.gois excluded by!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pkg/dotc1z/decoder.gois excluded by!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pkg/dotc1z/diff.gois excluded by!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pkg/dotc1z/dotc1z.gois excluded by!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pkg/dotc1z/file.gois excluded by!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pkg/dotc1z/manager/local/local.gois excluded by!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pkg/dotc1z/manager/manager.gois excluded by!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pkg/dotc1z/manager/s3/s3.gois excluded by!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pkg/dotc1z/sql_helpers.gois excluded by!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pkg/dotc1z/sync_runs.gois excluded by!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pkg/field/defaults.gois excluded by!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pkg/field/validation.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/lambda/grpc/transport.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/ratelimit/mem_ratelimiter.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/expand/cycle.gois excluded by!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pkg/sync/expand/graph.gois excluded by!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pkg/sync/expand/scc/bitset.gois excluded by!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pkg/sync/expand/scc/scc.gois excluded by!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pkg/sync/expand/scc/test_source.gois excluded by!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pkg/sync/state.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/synccompactor/attached/attached.gois excluded by!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pkg/synccompactor/compactor.gois excluded by!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pkg/synccompactor/naive/naive.gois excluded by!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pkg/synccompactor/naive/naive_unroll.gois excluded by!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pkg/tasks/c1api/delete_resource.gois excluded by!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pkg/tasks/c1api/full_sync.gois excluded by!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pkg/tasks/c1api/manager.gois excluded by!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pkg/tasks/c1api/service_client.gois excluded by!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pkg/tasks/local/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/types/types.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/github.com/conductorone/baton-sdk/pkg/uhttp/dbcache.gois excluded by!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pkg/uhttp/wrapper.gois excluded by!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pkg/us3/s3.gois excluded by!vendor/**and included by nonevendor/github.com/klauspost/compress/README.mdis excluded by!vendor/**and included by nonevendor/github.com/klauspost/compress/huff0/bitreader.gois excluded by!vendor/**and included by nonevendor/github.com/klauspost/compress/internal/le/le.gois excluded by!vendor/**and included by nonevendor/github.com/klauspost/compress/internal/le/unsafe_disabled.gois excluded by!vendor/**and included by nonevendor/github.com/klauspost/compress/internal/le/unsafe_enabled.gois excluded by!vendor/**and included by nonevendor/github.com/klauspost/compress/s2sx.modis excluded by!vendor/**and included by nonevendor/github.com/klauspost/compress/zstd/README.mdis excluded by!vendor/**and included by nonevendor/github.com/klauspost/compress/zstd/bitreader.gois excluded by!vendor/**and included by nonevendor/github.com/klauspost/compress/zstd/blockdec.gois excluded by!vendor/**and included by nonevendor/github.com/klauspost/compress/zstd/blockenc.gois excluded by!vendor/**and included by nonevendor/github.com/klauspost/compress/zstd/decoder.gois excluded by!vendor/**and included by nonevendor/github.com/klauspost/compress/zstd/enc_base.gois excluded by!vendor/**and included by nonevendor/github.com/klauspost/compress/zstd/matchlen_generic.gois excluded by!vendor/**and included by nonevendor/github.com/klauspost/compress/zstd/seqdec.gois excluded by!vendor/**and included by nonevendor/github.com/klauspost/compress/zstd/seqdec_amd64.sis excluded by!vendor/**and included by nonevendor/github.com/klauspost/compress/zstd/seqdec_generic.gois excluded by!vendor/**and included by nonevendor/github.com/klauspost/compress/zstd/seqenc.gois excluded by!vendor/**and included by nonevendor/github.com/klauspost/compress/zstd/snappy.gois excluded by!vendor/**and included by nonevendor/github.com/klauspost/compress/zstd/zstd.gois excluded by!vendor/**and included by nonevendor/modules.txtis excluded by!vendor/**and included by none
📒 Files selected for processing (4)
go.mod(2 hunks)pkg/connector/connector_test.go(0 hunks)pkg/connector/helpers_test.go(1 hunks)pkg/connector/user.go(2 hunks)
💤 Files with no reviewable changes (1)
- pkg/connector/connector_test.go
⏰ 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 (5)
pkg/connector/helpers_test.go (1)
10-22: Sanity test looks good.
Covers common DN shapes and asserts round‑trip via String().go.mod (2)
6-6: baton-sdk upgrade acknowledged.
Matches CreateAccount signature change in pkg/connector/user.go.
3-3: Ensure CI uses Go 1.25go.mod contains "go 1.25", but I couldn't find any CI config in the repo (.github/workflows missing; no .circleci, .gitlab-ci.yml, azure-pipelines.yml, .go-version, or Dockerfiles). Confirm your CI (or external pipeline) is pinned to Go 1.25 (e.g., actions/setup-go@v4 with go-version: "1.25" or use golang:1.25 images) or add a toolchain directive in your CI to avoid toolchain drift.
pkg/connector/user.go (2)
58-59: Interface conformance check is good.
Early compile‑time breakage for AccountManager.
381-382: Signature updated to LocalCredentialOptions: LGTM.
Matches v0.4.1 API and capability details (NO_PASSWORD).
Ranrg -nP --type go '\bCredentialOptions\b'in the sandbox — no matches found; run the same command locally to confirm there are no lingering references.
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.
Use github workflow for releases. Fix linter. Fix lint errors.
Summary by CodeRabbit
Chores
Tests