Skip to content

feat: multi-org UX improvements#31

Open
c1-dev-bot[bot] wants to merge 1 commit into
mainfrom
fix/multi-org-ux-improvements
Open

feat: multi-org UX improvements#31
c1-dev-bot[bot] wants to merge 1 commit into
mainfrom
fix/multi-org-ux-improvements

Conversation

@c1-dev-bot
Copy link
Copy Markdown

@c1-dev-bot c1-dev-bot Bot commented Mar 19, 2026

Summary

Addresses three multi-org UX issues reported by a user running a self-hosted Retool instance with multiple Organization Spaces:

  1. User DisplayName now includes org name — Users synced from different orgs are now distinguishable in the C1 UI. DisplayName is formatted as "FirstName LastName (OrgName)" instead of just "FirstName LastName". The organization_name is also added as a user profile attribute.

  2. Org-match validation in Grant/Revoke — Before adding/removing a user from a group, the connector now validates that the user's organization matches the group's organization. If they don't match, a descriptive error is returned instead of silently proceeding with a no-op grant.

  3. --organization-id config flag — A new optional flag restricts the connector to syncing a single Retool org. This enables running one connector instance per org for clean isolation in C1's UI. Downstream resource queries (users, groups, pages, resources) are already scoped by org, so the filter only needs to be applied at the ListOrganizations level.

Changes

  • pkg/client/organization.go — Added GetOrganization() method; updated ListOrganizations() to accept optional org ID filter with WHERE "id"=$N clause
  • pkg/client/user.go — Added GetUser() method for org-match validation
  • pkg/connector/users.go — Fetch org name and append to user DisplayName; add organization_name profile attribute
  • pkg/connector/groups.go — Added validateOrgMatch() helper; called from both Grant() and Revoke()
  • pkg/connector/organizations.go — Pass organizationID filter through to ListOrganizations
  • pkg/connector/connector.go — Accept and parse organizationID string config; store as *int64
  • cmd/baton-retool/config.go — Added OrganizationID string field
  • cmd/baton-retool/main.go — Read and pass organization-id config to connector

Test Plan

  • Verify build succeeds with go build ./cmd/baton-retool/
  • Test multi-org sync: confirm user DisplayNames include org names
  • Test --organization-id flag: confirm only the specified org is synced
  • Test Grant with mismatched user/group orgs: confirm descriptive error is returned
  • Test Grant with matching user/group orgs: confirm grant succeeds as before
  • Test Revoke with mismatched user/group orgs: confirm descriptive error is returned

Fixes: CXH-1212


Automated PR Notice

This PR was automatically created by c1-dev-bot as a potential implementation.

This code requires:

  • Human review of the implementation approach
  • Manual testing to verify correctness
  • Approval from the appropriate team before merging

…g filtering

- Append organization name to user DisplayName (e.g., "Alice Smith (Org A)")
  to disambiguate users across multiple Retool orgs in the C1 UI
- Add org-match validation in Grant/Revoke to return a descriptive error when
  a user's org doesn't match the target group's org, preventing silent failures
- Add --organization-id config flag to restrict sync to a single Retool org,
  enabling one connector instance per org for clean isolation
- Add GetOrganization and GetUser client methods to support the above changes
@c1-dev-bot c1-dev-bot Bot requested a review from a team March 19, 2026 18:03
@linear
Copy link
Copy Markdown

linear Bot commented Mar 19, 2026

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Mar 19, 2026

Claude finished @c1-dev-bot[bot]'s task in 1m 36s —— View job


PR Review: Multi-Org UX Improvements

  • Gather context and read changed files
  • Review code changes for bugs, security, and best practices
  • Post review feedback

Overall this is a well-structured PR. The three features are logically separated and the implementation follows existing patterns in the codebase. I found a couple of issues worth addressing:


Issue 1: GetOrganization called per page, not per List call (Performance — Low)

In pkg/connector/users.go:46-49, GetOrganization() is called on every invocation of List(). Since List() is called once per page of results, the same org is fetched from the database on every page. This is a minor inefficiency — the org name won't change between pages.

This is acceptable for now since it's just a DB query and the overhead is small, but worth noting.


Issue 2: validateOrgMatch makes two DB queries per Grant/Revoke (Performance — Medium)

pkg/connector/groups.go:235-255validateOrgMatch fetches both the user and the group from the database every time it's called. In Grant(), the group member is then fetched again immediately after at line 165. This means every Grant operation makes 3 DB round-trips before doing any work.

Consider whether the group is already available from the entitlement context, or whether the user's org could be validated against the entitlement's parent resource ID (which already carries the org) instead of re-fetching both entities.

Fix this →


Issue 3: SQL injection is not possible, but the dynamic query building pattern is fragile (Style — Low)

pkg/client/organization.go:47-49 — The WHERE clause is built using fmt.Sprintf with positional parameter indices. This is safe because the value is passed as a parameterized argument ($N), but the pattern of mixing fmt.Sprintf for clause construction makes the code harder to review for safety. The existing code already used this pattern for LIMIT/OFFSET, and this PR extends it consistently, so no action needed — just flagging for awareness.


Issue 4: Missing ExternalId on user resources (Pre-existing — Important)

This isn't introduced by this PR, but per the CLAUDE.md guidelines, WithExternalID() should be set during sync so that Grant/Revoke can retrieve the native system ID. Currently users.go doesn't set ExternalId, and groups.go Grant/Revoke uses parseObjectID(principial.Id.Resource) to get the user ID. This works because formatObjectID embeds the native ID, but it's worth noting this diverges from the recommended pattern.


Issue 5: groupOrgID != 0 guard may mask real issues (Logic — Low)

pkg/connector/groups.go:247 — When groupOrgID is 0 (i.e., the group's OrganizationID is nil), the validation is silently skipped. If a group with nil org ID is unexpected in a multi-org deployment, this could mask a data integrity issue. Consider logging a warning when groupOrgID == 0 so operators can detect misconfigured groups.

Fix this →


Issue 6: DisplayName format assumes non-empty names (Edge Case — Low)

pkg/connector/users.go:84 — If both FirstName and LastName are nil, the display name becomes " (OrgName)" (with leading spaces). Consider trimming or handling the empty-name case.

Fix this →


Positive notes

  • The parameterized query building in ListOrganizations correctly uses positional args ($N) — no SQL injection risk.
  • The org-match validation is a good defensive check that prevents confusing cross-org grant failures.
  • The --organization-id flag is cleanly threaded through the stack with proper *int64 nil semantics.
  • Error messages follow the baton-retool: prefix convention per CLAUDE.md.

Note: I was unable to run go build or go vet due to missing tool permissions. Recommend verifying the build passes in CI.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

0 participants