fix: rebuild WIMSE URI when identity_type changes in UpdateIdentity - #243
fix: rebuild WIMSE URI when identity_type changes in UpdateIdentity#243saucam wants to merge 4 commits into
Conversation
The WIMSE URI embeds identity_type as a path segment; when UpdateIdentity changed identity_type the stored URI was not rebuilt, leaving GetIdentityByWIMSEURI broken and old JWT sub claims dangling. Rebuilds the URI in lockstep (matching the pattern in reconcileDiscovered) and cascade-revokes active credentials with reason identity_retyped so old tokens stop resolving. Adds a domain unit test pinning that BuildWIMSEURI embeds identity_type as a distinct path segment, and two integration tests covering the retype round-trip (old URI → 404, new URI → 200) and URI-stable cases. Fixes #239 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
runDeactivationCleanup must run after the DB write succeeds — if repo.Update fails, calling cleanup first irreversibly revokes credentials while the identity row remains unchanged, leaving it permanently broken. Follows the same post-persist ordering used for the deactivated and expired cleanup paths at the bottom of UpdateIdentity. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
|
Auto-fixed by pr-shepherd (iteration 1):
Re-running CI. If you were watching: these are mechanical changes with no behavior impact. |
|
xDS RBAC Authorization Bypass via Metadata & RequestedServerName matchers. High Severity? 🤔 No behavior 🫡 |
…s timeout) make test was running ./... including tests/integration/ with a 120s timeout, but testcontainers startup alone takes ~60s, causing the integration binary to consistently hit the timeout. CI runs unit and integration tests as separate jobs with 300s timeouts. Update make test to match CI: exclude ./tests/... (identical to the go list grep in pr-check.yml) and raise the timeout to 300s. Add make test-all for the combined suite and make test-integration now also uses 300s to accommodate container startup. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
![highflame-oracle[bot]](https://avatars.githubusercontent.com/u/141064602?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔮 Oracle Review
🎯 Start Here
internal/service/identity.go (~30 min) — Security changes in identity.go
📋 PR Summary
What this PR does: Fixes a data consistency bug where WIMSEURI was not rebuilt when identity_type changes during UpdateIdentity, preventing stale URIs and dangling JWT sub claims.
Key changes:
- Captures priorType before mutations to detect type changes
- Rebuilds WIMSEURI when identity_type changes to maintain URI consistency
- Cascade-revokes active credentials after successful DB write
- Fixes cleanup ordering: runDeactivationCleanup now runs after repo.Update to prevent irreversible credential revocation on DB failure
Areas affected: Identity management, WIMSE URI generation, Credential lifecycle, Test infrastructure
Testing notes: Comprehensive test coverage includes unit tests for URI type variation, integration tests for type changes verifying URI rebuild and 404 on old URI, and stability tests for unchanged types
🔍 Code Review
This is a well-executed fix that addresses a critical data consistency bug with thoughtful edge-case handling. The author demonstrates deep understanding of the codebase by matching existing patterns (reconcileDiscovered) and identifying a secondary issue with cleanup ordering that could have caused permanent credential loss.
What's good:
- ✨ Excellent pattern matching with existing reconcileDiscovered logic for credential revocation
- ✨ Thoughtful fix for the cleanup ordering issue that prevents irreversible side effects on DB failure
- ✨ Comprehensive test coverage including unit tests, integration tests, and edge cases
- ✨ Clear documentation of the root cause and the rationale behind the fix approach
Review Stats:
Generated by Oracle - Highflame's AI Code Reviewer
|
|
||
| test: ## Run all tests (unit + integration) | ||
| go test ./... -v -race -count=1 -timeout=120s | ||
| test: ## Run unit tests (matches CI unit-test step; integration requires Docker — use make test-integration) |
There was a problem hiding this comment.
Consider increasing timeout beyond 300s for larger test suites
While 300s is a reasonable increase from 120s, Go test suites with race detection can sometimes exceed this as the codebase grows. Consider 600s to future-proof against flaky timeouts in CI, or make it configurable via an environment variable.
Suggested fix:
| test: ## Run unit tests (matches CI unit-test step; integration requires Docker — use make test-integration) | |
| test: ## Run unit tests (matches CI unit-test step; integration requires Docker — use make test-integration) | |
| go test $$(go list ./... | grep -v '/tests') -v -race -count=1 -timeout=$${TEST_TIMEOUT:-300s} |
🚧 Oracle Review FailedOracle hit an error mid-review and could not post a review on this PR. Comment |
2 participants
Summary
UpdateIdentitychangesidentity_type, the storedWIMSEURIwas not rebuilt. The URI embedsidentity_typeas a path segment (spiffe://{domain}/{account}/{project}/{identity_type}/{external_id}), so after a retype it went stale — breakingGetIdentityByWIMSEURIand leaving old JWTsubclaims dangling.priorTypebefore applying mutations, rebuilds the URI if the type changed, and cascade-revokes active credentials after the DB write succeeds (matching thereconcileDiscoveredpattern which already handled this correctly).runDeactivationCleanupnow runs afterrepo.Update— calling it before would irreversibly revoke credentials if the DB write failed.Test plan
TestBuildWIMSEURI_TypeSegmentVaries— unit test: all 4 identity types produce distinct URIs with the type as a path segmentTestUpdateIdentity_TypeChange_WIMSEURIRebuild— integration: retypeagent→application, verify new URI in response, old URI → 404 from/identities/by-wimse, new URI → 200TestUpdateIdentity_TypeUnchanged_URIStable— integration: name-only update leaveswimse_uriunchangedFixes #239
🤖 Generated with Claude Code