GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — fix(coderd/idpsync): Group sync may only add users to groups of the syncing org by eric-hughes-coder · Pull Request #30706 · coder/coder

Conversation

@eric-hughes-coder

@eric-hughes-coder eric-hughes-coder commented Oct 13, 2026

Copy link
Copy Markdown
Contributor

Group sync settings can contain group IDs that belong to another organization or that no longer exist. At login, SyncGroups passed mapped IDs straight through to InsertUserGroupsByID, which has no organization filter, so the settings for one organization could cause a user to be added to a group in a different organization.

SyncGroups now partitions each organization's expected groups with PartitionOutInvalidSettings and only syncs those
inside the organization. Groups in another organization, and IDs that match no group, are dropped and reported with one
warning per kind. Login and the rest of the sync proceed.

Generated by Claude Code under the direction of @eric-hughes-coder.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
eric-hughes-coder and others added 5 commits October 13, 2026 16:28
@eric-hughes-coder @claude
test(coderd/idpsync): stub PartitionOutInvalidSettings with failing t…
d5a34ab 
…ests

Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
@eric-hughes-coder @claude
feat(coderd/idpsync): implement PartitionOutInvalidSettings
874b6ec 
Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
@eric-hughes-coder @claude
test(coderd/idpsync): add failing SyncGroups tests for outside and mi…
9a145f2 
…ssing groups

Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
@eric-hughes-coder @claude
fix(coderd/idpsync): only sync groups inside the syncing organization
c2e1627 
Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
@eric-hughes-coder @claude
test(coderd/idpsync): cover only-missing and unmapped SyncGroups cases
9c0417e 
Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
@linear-code

linear-code Bot commented Oct 13, 2026

Copy link
Copy Markdown

PLAT-750
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@github-actions

github-actions Bot commented Oct 13, 2026 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

All contributors have signed the CLA ✍️ ✅
Posted by the CLA Assistant Lite bot.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@github-actions github-actions Bot assigned eric-hughes-coder Oct 13, 2026
@eric-hughes-coder

eric-hughes-coder commented Oct 13, 2026

Copy link
Copy Markdown
Contributor Author

I have read the CLA Document and I hereby sign the CLA
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
cdrci2 added a commit to coder/cla that referenced this pull request Oct 13, 2026
@cdrci2
@eric-hughes-coder has signed the CLA in coder/coder#30706
577394a
@hwang251
hwang251 requested review from a team and geokat October 13, 2026 19:05
@eric-hughes-coder
Merge branch 'main' into erichughes/plat-750-security-group-sync-must…
fb2a4f5 
…-only-add-users-to-the-syncing
Emyrk
Emyrk reviewed Oct 14, 2026
View reviewed changes

@Emyrk Emyrk left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch 👍

1 nit

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread coderd/idpsync/group.go Outdated
// is a group in some other organization. Missing holds entries whose GroupID
// matches no group at all, whether deleted or never created. Only Inside is
// safe to sync; Outside and Missing come from invalid sync settings.
func PartitionOutInvalidSettings(ctx context.Context, tx database.Store, orgID uuid.UUID, expected []ExpectedGroup) (inside, outside, missing []ExpectedGroup, err error) {

@Emyrk Emyrk Oct 14, 2026

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function highly depends on the authz permissions of the caller. If it's called with a user permissions, the response could mark all groups as missing or outside.

I think we should not export this function so people outside the pkg know not to call it.

Suggested change
func PartitionOutInvalidSettings(ctx context.Context, tx database.Store, orgID uuid.UUID, expected []ExpectedGroup) (inside, outside, missing []ExpectedGroup, err error) {
func partitionOutInvalidSettings(ctx context.Context, tx database.Store, orgID uuid.UUID, expected []ExpectedGroup) (inside, outside, missing []ExpectedGroup, err error) {

For unit tests, just make the file group_internal_test.go and it can share the same pkg

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@eric-hughes-coder eric-hughes-coder Oct 14, 2026

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Taken.

Added an explicit precondition statement to the function in addition.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Emyrk reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@eric-hughes-coder @claude
refactor(coderd/idpsync): unexport partitionOutInvalidSettings and do…
08c1eb1 
…cument its preconditions

Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
@geokat
geokat removed their request for review October 14, 2026 17:36
Emyrk
Emyrk approved these changes Oct 14, 2026
View reviewed changes
@eric-hughes-coder eric-hughes-coder added security Area: security backport cherry-pick labels Oct 28, 2026
@eric-hughes-coder
eric-hughes-coder merged commit 296da93 into main Oct 28, 2026
50 checks passed

Uh oh!

There was an error while loading. Please reload this page.
@eric-hughes-coder
eric-hughes-coder deleted the erichughes/plat-750-security-group-sync-must-only-add-users-to-the-syncing branch October 28, 2026 15:13
@github-actions github-actions Bot locked and limited conversation to collaborators Oct 28, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@Emyrk Emyrk Emyrk approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

@eric-hughes-coder eric-hughes-coder

Labels

backport cherry-pick security Area: security

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

2 participants

@eric-hughes-coder @Emyrk
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.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.