Fix: Add tenant ID validation in UserPrincipalManager#49631
Open
rujche wants to merge 7 commits into
Open
Conversation
…bypass
- Validate the 'tid' (tenant ID) claim in JWT tokens against the configured tenant
- Only accept tokens from the configured tenant if tenant ID is specified
- Maintains backward compatibility by skipping validation when tenant is not configured
- Fixes the authentication bypass vulnerability in UserPrincipalManager
Changes:
1. UserPrincipalManager.java:
- Added tenant ID claim validation in getValidator() method
- Extract configured tenant ID from AadAuthenticationProperties
- Reject tokens where tid doesn't match configured tenant
- Added debug logging for successful tenant validation
- Added org.springframework.util.StringUtils import
2. UserPrincipalManagerTests.java:
- Added 4 new unit tests for tenant ID validation:
* tenantIdValidationSucceedsWhenMatchingConfiguredTenant()
* tenantIdValidationFailsWhenMismatchedTenant()
* tenantIdValidationSkippedWhenNoTenantConfigured()
* tenantIdValidationSkippedWhenTenantPropertyIsEmpty()
- Added reflection helper methods for testing private methods
- All 9 tests pass successfully (5 original + 4 new)
rujche
requested review from
a team,
Netyyyy,
moarychan and
saragluna
as code owners
Add entry for the cross-tenant authentication bypass fix in AAD authentication filters to the 7.4.0-beta.1 release notes.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds JWT tid (tenant ID) claim validation to UserPrincipalManager to prevent cross-tenant token acceptance when a single tenant is intended, along with unit tests covering matching/mismatching/skip scenarios.
Changes:
- Added
tidvs configured tenant validation insideUserPrincipalManager#getValidator()’s claims verifier. - Added new unit tests covering tenant match/mismatch and validation skip behavior.
- Introduced reflection-based helpers in tests to reach private implementation details.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| sdk/spring/spring-cloud-azure-autoconfigure/src/main/java/com/azure/spring/cloud/autoconfigure/implementation/aad/filter/UserPrincipalManager.java | Adds tid claim validation against configured tenant. |
| sdk/spring/spring-cloud-azure-autoconfigure/src/test/java/com/azure/spring/cloud/autoconfigure/implementation/aad/filter/UserPrincipalManagerTests.java | Adds tests + reflection helpers to validate the new tenant-checking behavior. |
- Skip tenant ID validation for multi-tenant values (common, organizations, consumers) - Normalize tenant IDs to lowercase for case-insensitive comparison - Fix test helper method to properly accept and use JWSAlgorithm parameter - Update tests to reflect actual Spring behavior (default 'common' for unconfigured tenant) - Use 'organizations' instead of empty string for multi-tenant test scenario These changes ensure: 1. Multi-tenant applications (using common/organizations/consumers endpoints) still work 2. Tenant ID comparison is case-insensitive and normalized 3. Tests accurately reflect real-world Spring configuration defaults 4. Code is cleaner and follows best practices
- Use Locale.ROOT for security-sensitive tenant ID normalization (prevents locale-dependent behavior) - Change direct String cast to Object cast with toString() to safely handle non-String tid claims - Rename test method from tenantIdValidationSkippedWhenTenantPropertyIsEmpty() to tenantIdValidationSkippedWhenOrganizationsConfigured() for clarity - Maintains consistency with AadResourceServerConfiguration#getTrimmedTenantId patterns - Improves robustness against malicious tokens with unexpected claim types
…h rule - Reformatted normalizedTokenTid initialization to split the ternary operator across multiple lines - Ensures line 236 stays under 120 characters as enforced by Checkstyle - All 9 tests pass, BUILD SUCCESS
…n of final fields - Added UserPrincipalManager(JWKSource, AadAuthenticationProperties) package-private constructor - Updated tenant ID validation tests to use the new constructor instead of reflection - Removed setAadAuthenticationProperties() helper method that modified private final fields - Tests now avoid brittle reflection and are cleaner - All 9 tests pass, BUILD SUCCESS
…orts - Moved com.azure.spring.cloud... imports before com.nimbusds... imports - Aligned with module import-order conventions (consistent with other test classes)
rujche
changed the title
UserPrincipalManager
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.
Description
Changes:
UserPrincipalManager.java:
UserPrincipalManagerTests.java:
All SDK Contribution checklist:
General Guidelines and Best Practices
Testing Guidelines