Fix Heimdall principal claims pattern for client IDs (LFXV2-922) #32
Conversation
Change the client ID pattern from 'clients@{client_id}' to '{client_id}@Clients'
to match the Auth0 convention used in the platform.
- Renamed MachineUserPrefix to MachineUserSuffix constant for accuracy
- Updated machine user detection logic to use HasSuffix instead of HasPrefix
- Fixed test cases to use the correct suffix pattern
- Updated README documentation to reflect suffix identification
Related: LFXV2-922
🤖 Generated with [GitHub Copilot](https://github.com/features/copilot) (via Zed)
Signed-off-by: Eric Searcy <[email protected]>
WalkthroughThis change switches machine user detection from prefix-based identification ( Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (4)
🧰 Additional context used📓 Path-based instructions (3)**/*.go📄 CodeRabbit inference engine (CLAUDE.md)
Files:
**/*_test.go📄 CodeRabbit inference engine (CLAUDE.md)
Files:
internal/infrastructure/auth/**/*.go📄 CodeRabbit inference engine (CLAUDE.md)
Files:
🧬 Code graph analysis (2)internal/infrastructure/auth/auth_repository_test.go (1)
internal/infrastructure/auth/auth_repository.go (1)
⏰ 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)
🔇 Additional comments (5)
Comment |
There was a problem hiding this comment.
Pull request overview
This PR corrects the Heimdall principal claims pattern for machine users from a prefix-based approach (clients@{client_id}) to a suffix-based approach ({client_id}@clients) to align with Auth0's standard convention.
- Renamed constant from
MachineUserPrefixtoMachineUserSuffixwith updated value"@clients" - Updated detection logic throughout the codebase to use
HasSuffixinstead ofHasPrefix - Modified tests to reflect the corrected pattern
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
pkg/constants/auth.go |
Renamed constant and changed value from "clients@" to "@clients" |
internal/infrastructure/auth/auth_repository.go |
Updated machine user detection to use suffix-based pattern matching and updated log field names |
internal/infrastructure/auth/auth_repository_test.go |
Updated test case to use correct suffix pattern with the renamed constant |
README.md |
Updated documentation to reflect suffix-based machine user identification |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if strings.HasSuffix(principal, constants.MachineUserSuffix) { | ||
| isMachineUser = true | ||
| r.logger.Info("Machine user detected in authorization header", | ||
| "auth_id", authID, | ||
| "principal", r.safePrincipalLog(principal), | ||
| "machine_user_prefix", constants.MachineUserPrefix) | ||
| "machine_user_suffix", constants.MachineUserSuffix) |
There was a problem hiding this comment.
With the change to suffix-based machine user detection, machine user principals now follow the pattern {client_id}@clients. However, the safePrincipalLog function treats any string containing @ as an email address and redacts it to {prefix}@***. This means machine user principals will be logged as {client_id}@*** instead of showing the full identifier.
Consider updating the safePrincipalLog function to check if a principal is a machine user (using isMachineUser()) before applying email redaction logic, since machine user identifiers are not personal information and should be logged in full for debugging purposes.
| t.Run("isMachineUser", func(t *testing.T) { | ||
| // Test machine user | ||
| result := repo.isMachineUser(constants.MachineUserPrefix + "test-machine") | ||
| result := repo.isMachineUser("test-machine" + constants.MachineUserSuffix) |
There was a problem hiding this comment.
The test updates correctly verify the machine user detection logic, but there's missing test coverage for how safePrincipalLog handles machine user principals with the new @clients suffix pattern.
Since machine users now end with @clients, they will be treated as email addresses by safePrincipalLog and redacted. Consider adding a test case in the safePrincipalLog test suite to verify the expected behavior when logging machine user principals (e.g., whether they should be redacted or logged in full).
1 participant
Description
This PR fixes the Heimdall principal claims pattern for client IDs to match the Auth0 convention.
Changes
MachineUserPrefixtoMachineUserSuffixand changed value from"clients@"to"@clients"strings.HasSuffixinstead ofstrings.HasPrefix"test-machine@clients")Background
While working on the mock data loader and v1 meetings, it was discovered that the PoC implementation had reversed the
{client_id}@clientspattern used by Auth0. This change corrects the pattern to align with Auth0's expected format across the indexer service.Files Modified
pkg/constants/auth.go- Updated constant name and valueinternal/infrastructure/auth/auth_repository.go- Updated detection logicinternal/infrastructure/auth/auth_repository_test.go- Updated test casesREADME.md- Updated documentationTesting
All existing tests continue to pass with the updated logic.
Related Issues