Repository navigation
fix: specify and validate group sync settings - #31718
Draft
eric-hughes-coder wants to merge 18 commits into
Draft
eric-hughes-coder wants to merge 18 commits into
eric-hughes-coder wants to merge 18 commits into
Conversation
Define well-formed (W1-W8) and valid (V1-V5) group sync settings, the IdP input, the sync algorithm as data stages with postconditions, the sources of misbehavior, and the relaxed mode that rewrites legacy settings into strict form without changing behavior. Refs CORE-643. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Add the table of test vectors for the syntax validator of group sync settings, which checks the well-formedness conditions W1 through W8 in coderd/idpsync/SYNC_SETTINGS.md. The vectors come before the validator and the tests that use them. Refs CORE-643. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Add the table of test vectors for the full validator of group sync settings, which checks the validity conditions V1 through V5 in SYNC_SETTINGS.md against a snapshot of the deployment. The vectors come before the validator and the tests that use them. Refs CORE-643. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Add the test vectors for the policy between the validators and the rewriter of group sync settings: which finding codes the rewriter can rewrite, the relaxed policy's action for every code on every route, and how Decide combines per-finding actions into one outcome. The vectors come before the policy and the tests that use them. Refs CORE-643. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Add the test vectors for the rewriter of group sync settings, in three groups: fixed points returned unchanged, input brought into canonical strict form, and input that is not rewritten. Each vector gives the findings, the expected outcome, the expected settings, and one warning per finding. The vectors come before the rewriter and the tests that use them. Refs CORE-643. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Add GroupSyncSettings.WellFormedFindings, which reports every violation of the well-formedness conditions W1 through W8 that is visible after decoding, as findings with a stable code and a JSON Pointer, ordered by path and code. Add the tests that run it over the syntax test vectors, which now use the exported finding codes. Refs CORE-643. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Add idpsync.ValidityFindings, which reports violations of the validity conditions V1 through V5 against a snapshot of the deployment that the caller reads and passes in, so the validator itself is pure. It returns an error, and no findings, when the snapshot does not cover every UUID in the settings. The five V finding codes are idpsync constants of type codersdk.GroupSyncFindingCode. In codersdk, add GroupSyncSettings.CanonicalMappingUUIDs, the canonical list of UUIDs in a mapping, export CompareGroupSyncFindings as the one definition of finding order, and export the JSON Pointer escaping helpers. Add the tests that run these over the validity test vectors, which now use exported codes and a snapshot value. The policy and rewriter vectors use the exported codes too. Refs CORE-643. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Add the rewrite policy that sits between the validators and the rewriter of group sync settings. RelaxedGroupSyncAction picks an action for each finding by code and route, keeping current behavior except that V2, V3, and V4 violations are rejected on the request routes. GroupSyncRewritable reports which codes the rewrite rules R1 through R7 cover. RewritePlan applies the policy to a list of findings and gives one instruction for the settings, failing closed when an action function asks for a rewrite no rule covers. Add the tests that run these over the policy test vectors, which now use the exported routes, actions, and instructions. The rewriter's outcome constants move to its own vector file. Refs CORE-643. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
"Strict form" and "strictly valid" meant exactly "valid", since validity already includes well-formedness, and the qualifier suggested a weaker kind of validity that does not exist. Use "valid" throughout the spec, the policy's comments, and the test vectors, and rename the subsection on strict form to "Valid and legacy settings". "Strict mode" stays, as the name of a mode. The behavior described is unchanged. Refs CORE-643. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Add idpsync.RewriteResult, which carries out a rewrite plan without making decisions of its own. Valid settings and kept settings are returned unchanged. Rewritten settings have each finding's rewrite rule applied, R1 through R7, and are returned in canonical form. Every finding gets a warning giving the element as received and as accepted. Add codersdk.GroupSyncWarning, the warning as it goes over the wire. Valid settings are now saved and returned as received rather than in canonical form: settings are rewritten only when they need a rewrite rule. The spec and the accept instruction say so. Add the tests that run the rewriter over its test vectors, including a check that every rewritten result is valid. Refs CORE-643. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…write plan A rewrite plan held its actions in a slice parallel to the findings, so RewriteResult took the findings separately and had to check that the two matched one for one. Hold each finding with its action instead, as GroupSyncPlannedAction, so a plan cannot disagree with its findings. RewriteResult now takes only the settings and the plan. Refs CORE-643. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The rewriter's rules R6 and R7 remove the same UUIDs that partitionOutInvalidSettings drops at sync time. Say that the overlap is intentional and why: the partition guards sync against decay in validity, and must stay until Coder keeps group sync settings valid through every operation that affects them; the rewriter is the start of dealing with legacy settings. The comment at the partition's call site cites the section of SYNC_SETTINGS.md that describes the decay. Refs CORE-643. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Add GroupSnapshotReader, the function type that supplies the full validator's snapshot of the deployment, and DatabaseGroupSnapshotReader, which reads one from a store: the target organization's status, and the status of each requested UUID relative to it. Test code can supply a reader with no database. The reader is separate from partitionOutInvalidSettings, which is unchanged. Add database-backed tests over symbolic vectors. List the absent organization in the spec's settings that are not rewritten, beside the deleted one. Refs CORE-643. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The group sync settings endpoints now check settings against the organization's groups and apply the relaxed rewrite policy. Requests that name a group outside the organization or a group that does not exist are rejected with a 400 that names each element by path and code. Rewritable findings are repaired and reported as warnings, alongside the settings as received. GET and the config and mapping-diff endpoints repair a stored row that has gone stale before using it, and audit the repair as Coder's action. All four endpoints run in a transaction, so validation and the write see the same state, and a full replace audits the row it actually replaced. The SDK gains WithWarnings methods that return the warnings; the old methods wrap them and are deprecated. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The five validity codes appear in API warnings and rejections alongside the syntax codes, but were declared in coderd/idpsync, where the API type generators do not look. They now live in codersdk/idpsync_validity.go, and GroupSyncFindingCode moves to idpsync_warning.go, since it spans both kinds of finding. The checking stays in coderd/idpsync. CompareGroupSyncFindings no longer reads a non-numeric index token as index 0; such a path sorts as if it had no index. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Regenerate the swagger spec, API reference, and TypeScript types for the group sync settings response, warning, and all twelve finding codes. A warning's received and accepted members hold a JSON value of varying kind. TypeScript now types them unknown, through the generator's table of raw JSON fields, which generalizes the agent hook mutation. Swagger types them as object, following the audit log's additional_fields. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Add a section on the checks Coder applies when group sync settings are
saved: which conditions are rejected, which are corrected with a warning,
which are kept with a warning, and how settings that stop being valid are
corrected when viewed or partly changed.
Correct the default settings example, which shows the mapping as `{}`.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Member
|
The original notion is here: https://app.notion.com/p/coderhq/Group-Role-sync-503ad942a11245dc967b8e8a317ab328 |
This branch has not been deployed
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
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.
This PR is much larger than I originally anticipated it would be. The narrow problem of CORE-643 is that group sync settings can map IdP groups to group UUIDs that belong to another organization. But they can also map to UUIDs that name no group. And when you dig into it, there are lots of other kinds of misbehavior. This PR gets at the root cause of all such symptoms and sets the stage for a permanent fix for all of them.
This PR introduces no breaking changes; the changes required for CORE-643 are relatively narrow. There are some other behavior changes, but they do not break existing functionality. This PR defines a relaxed mode for group sync; it largely matches the current behavior. It also sets out a path for strict mode, which can eventually forestall the whole class of problems that CORE-643 represents.
I also want to make something of a bold claim. Even as large as this PR is, it's something of a minimal solution to get at the root cause behind a whole class of symptoms. The one thing that caught me by surprise was the need to do rewriting in this PR. It turns out it's necessary to avoid breaking the UI with what would have been surprising and counterintuitive behaviors.
Setting the stage for future work
This PR can only set the stage because of legacy data. There are invalid group sync settings in user deployments. It is neither feasible nor even possible to automatically update this data during a version upgrade. The rewriter can be used to upgrade settings semi-automatically, meaning it can mechanically rewrite some settings to validity, but not all of them. Full removal of invalid settings might require some amount of manual intervention. That will require an engagement with customer administrators that is beyond the scope of engineering itself.
There are two classes of future work required.
The size of the solution for all this should not be underestimated. To give a sense about that, here are a couple of estimates about the current PR:
So around half of this PR is necessary for correct operation, regardless of how the code evolved. The other half is the first payment toward technical debt. It's not the last payment.
Structure of this PR
While there's a lot of code in this PR, it's conceptually relatively straightforward.
The design here is meant as a model for the rest of the code base. That's most directly the case for organization and role sync. It also, however, applies more broadly to configuration data above a rather low complexity threshold.
SYNC_SETTINGS.md
If there's a single root cause, it's that there was no proper specification of the group sync settings grammar, nor of the group sync algorithm. There are now proper specifications for both. This document was written before writing any code; everything else derives from it. (There were only minor changes discovered while writing code.) If you want to understand what's going on with this PR, you really want to start with this document.
There are three core definitions in this document.
There's an extensive list of ways that group sync can misbehave. Some of them are self-inflicted; others cannot be avoided because the IdP is external to the deployment.
Lastly, there's a full description of rewriting to deal semi-automatically with legacy data.
The validator
The two parts of the validator: verification that the settings are well-formed and validation that the settings are coherent with the current state of the deployment. Verification follows the W criteria in the settings specification; validation follows the V criteria. Each kind of failure has its own identifier; these are analogous to compiler error messages. These are returned in API responses, so that in a later version they might be passed through to the user, informing them about why the system thinks a settings text is invalid.
The rewriter
The rewriter starts with the two stages of the validator. It adds two more stages. The first is a policy that makes decisions about whether to rewrite or not based both on the route by which the settings came in and on the results from the validator. The second is the rewriter proper, which makes no decisions itself but simply executes a rewrite plan that came from the policy.
Integration with endpoints
Endpoints that configure group sync settings use the validator and rewriter to ensure that they are dealing with valid settings. If settings from a request or from the database are invalid, they are rewritten or rejected, as appropriate. The rewriter does not change visible behavior of the group sync algorithm.
There's one mostly invisible change here. Viewing settings that are invalid but can be safely rewritten causes them to be updated in the database.
How this fixes CORE-643
CORE-643 is about invalid mappings to groups in a different organization. This PR rejects such a mapping when it is saved, and removes it from stored settings the next time they are viewed or partly changed. The sync-time filter added by CORE-750 (
partitionOutInvalidSettings) is unchanged; it stays as a guard until Coder keeps group sync settings valid through every operation.What administrators see
The user-facing behavior is described in the new section "How Coder checks group sync settings" in
docs/admin/users/idp-sync.md. It lists the conditions Coder rejects (outside_group,missing_group), the ones it corrects with a warning, and the ones it keeps with a warning. It explains how settings that stop being valid, for example after a group is deleted, are corrected when viewed or partly changed, with each correction recorded in the audit log.Relevant test vectors
In order to verify that this PR does indeed correct the defects in CORE-643, these are the test vectors that read on group membership: each turns on whether a UUID names a group in the target organization. The full test plan is outside the scope of this description.
Snapshot reader: classifying each UUID
active/outside(coderd/idpsync/group_snapshot_test.go:86): a group in another organization is outside.active/does_not_exist(coderd/idpsync/group_snapshot_test.go:95): a UUID that names no group does not exist.active/other_everyone_outside(coderd/idpsync/group_snapshot_test.go:112): another organization's Everyone group is outside.active/mixed(coderd/idpsync/group_snapshot_test.go:124): inside, outside, and missing UUIDs in one request are each classified correctly.absent/outside_and_missing(coderd/idpsync/group_snapshot_test.go:185): with no organization at the target ID, a group elsewhere is outside and a UUID that names nothing does not exist.Full validator: detecting the UUIDs
v4/outside_group(coderd/idpsync/group_validity_test.go:304): a group in another organization is reported (V4).v4/other_organizations_everyone_group(coderd/idpsync/group_validity_test.go:317): another organization's Everyone group is reported as outside, not as Everyone.v3/missing_group(coderd/idpsync/group_validity_test.go:253): a UUID that names no group is reported (V3).v3/missing_group_after_valid(coderd/idpsync/group_validity_test.go:266): the report points at the offending element, not the array.v3/missing_group_under_two_keys(coderd/idpsync/group_validity_test.go:285): each occurrence is reported.Policy: rejecting on save, repairing on read
relaxedActionVectors,missing_grouponfull_replace,merge, andfetch(coderd/idpsync/group_policy_test.go:118-120): rejected on both request routes; removed from a stored row on fetch (R6), so stale entries never block a save.relaxedActionVectors,outside_grouponfull_replace,merge, andfetch(coderd/idpsync/group_policy_test.go:124-126): rejected on both request routes, which covers the mapping-diff endpoint'sAddlist; removed from a stored row on fetch (R7).single/reject(coderd/idpsync/group_policy_test.go:256): one outside group rejects the whole request.route/missing_group_on_merge(coderd/idpsync/group_policy_test.go:324): a missing group in a merged request is rejected.route/missing_group_on_fetch(coderd/idpsync/group_policy_test.go:333): the same finding in a stored row is repaired, not rejected.Rewriter: removing the UUIDs from stored settings
rewritable/r6_missing_group(coderd/idpsync/group_rewrite_test.go:439): a UUID that names no group is removed; in-org groups are kept.rewritable/r7_outside_group(coderd/idpsync/group_rewrite_test.go:467): a group in another organization is removed, leaving[].Handlers: the endpoints end to end
TestPatchGroupSyncSettings/RejectsOutsideGroup(enterprise/coderd/idpsync_test.go:246): a full replace naming a group in another organization gets a 400 naming/mapping/<key>/<index>andoutside_group, and the settings are not saved.TestPatchGroupSyncSettings/RejectsMissingGroup(enterprise/coderd/idpsync_test.go:262): a full replace naming a UUID that names no group gets a 400 naming the element andmissing_group.TestPatchGroupSyncMapping/RejectsOutsideGroupInAdd(enterprise/coderd/idpsync_test.go:546): the mapping-diff endpoint rejects an outside group in itsAddlist.TestPatchGroupSyncMapping/RejectsMissingGroupInAdd(enterprise/coderd/idpsync_test.go:557): the same for a missing group.TestPatchGroupSyncMapping/StaleStoredEntryDoesNotBlock(enterprise/coderd/idpsync_test.go:588): a stored row with a missing group is repaired before the merge, so an unrelated valid change succeeds.TestGetGroupSyncSettings/RewritesStoredRow(enterprise/coderd/idpsync_test.go:92): GET removes outside and missing groups from a stored row, warns about each, and replaces the row, so a second GET has nothing to report.This PR was produced with the assistance of Claude Code under the direction of @eric-hughes-coder.
Closes CORE-643