[PM-1632] Redirect on SsoRequired - return SsoOrganizationIdentifier
#6597
Conversation
doc: update sso methods and docs
…entifier for SSO required Policy
…entifierQuery to hydrate the new CustomResponse SsoOrganizationIdentifier.
|
Great job! No new security vulnerabilities introduced in this pull request |
…b.com/bitwarden/server into auth/pm-1632/redirect-on-sso-required
ike-kottlowski
changed the title
SsoRequired - return SsoOrganizationIdentifier
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6597 +/- ##
==========================================
- Coverage 56.95% 53.31% -3.64%
==========================================
Files 1913 1909 -4
Lines 85276 85072 -204
Branches 7635 7651 +16
==========================================
- Hits 48568 45360 -3208
- Misses 34884 37958 +3074
+ Partials 1824 1754 -70 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Claude finished @ike-kottlowski's task —— View job Code Review CompleteTask List:
SummaryThis PR implements functionality to return the SSO organization identifier when SSO authentication is required, enabling automatic client-side redirection to the correct IdP. The implementation is well-structured with proper separation of concerns, comprehensive test coverage (99.18%), and appropriate feature flag protection. Previous Review Status: Patrick's non-blocking comments have been addressed. The author chose to structure the legacy code removal intentionally for easier cleanup during feature flag removal, which is a reasonable approach. FindingsFinding 1: Property naming inconsistency - 💭 In Context// Line 47 - Still declared as property
protected IFeatureService _featureService { get; }
// But used as private field throughout
_featureService.IsEnabled(FeatureFlagKeys.RecoveryCodeSupportForSsoRequiredUsers)Recommendation: Either keep it as Finding 2: Organization identifier exposure requires security validation 💭 The new functionality exposes organization identifiers in authentication error responses ( AnalysisThe identifier is only returned when:
This seems reasonable, but confirm that organization identifiers are not considered sensitive. The logic correctly returns
Finding 3: Duplicate SSO validation logic during feature flag transition ♻️ Both ContextThe dual validation paths exist in:
Both paths have nested feature flags for Recommendation: Ensure PM-28281 cleanup is prioritized after feature flag stabilization to remove this duplication. Finding 4: Missing null check for organization in query
Codevar organization = await _organizationRepository.GetByIdAsync(confirmedOrgUser.OrganizationId);
if (organization == null)
{
return null;
}This is likely fine if Finding 5: Incomplete inline documentation 🎨 Suggestion// Current
/// This Key is used when a user is in a single organization...
// Suggested
/// This key is used when a user is in a single organization...Minor style nitpick for consistency. Good Practices Observed
Security Assessment✅ Zero-knowledge principles: Not compromised - only organization identifiers (non-sensitive) are exposed, not vault data Action Items
|
rr-bw
requested review from
Patrick-Pimentel-Bitwarden
and removed request for
rr-bw
Patrick-Pimentel-Bitwarden
left a comment
Patrick-Pimentel-Bitwarden
left a comment
There was a problem hiding this comment.
👍 Non blocking comments! Thinks look really good. Thank you for all the tests!
| validatorContext.SsoRequired = await RequireSsoLoginAsync(user, request.GrantType); | ||
| if (validatorContext.SsoRequired) | ||
| // TODO: Clean up Feature Flag: Remove this if block: PM-28281 | ||
| if (!_featureService.IsEnabled(FeatureFlagKeys.RedirectOnSsoRequired)) |
There was a problem hiding this comment.
⛏️ I feel like we normally check the positive first so that the new work sits above the old, small nit pick.
There was a problem hiding this comment.
We do but I wanted to make sure the legacy was removed with the logic. This way we can remove the whole without changing logic.
It felt like less mental load over all to be able to just remove the code block rather than figure out what all needed to come out when the feature flag is removed.
| validatorContext.SsoRequired = await RequireSsoLoginAsync(validatorContext.User, request.GrantType); | ||
| if (!validatorContext.SsoRequired) | ||
| // TODO: Clean up Feature Flag: Remove this if block: PM-28281 | ||
| if (!_featureService.IsEnabled(FeatureFlagKeys.RedirectOnSsoRequired)) |
There was a problem hiding this comment.
⛏️ I feel like we normally check the positive first so that the new work sits above the old, small nit pick.
| // context.GrantResult.Error = requestContext.ValidationErrorResult.Error; | ||
| // context.GrantResult.IsError = requestContext.ValidationErrorResult.IsError; | ||
| // context.GrantResult.ErrorDescription = requestContext.ValidationErrorResult.ErrorDescription; | ||
| // context.GrantResult.CustomResponse = requestContext.CustomResponse; | ||
| context.GrantResult.IsError = true; | ||
| } |
There was a problem hiding this comment.
Does this need to be removed?
Removed commented-out code for clarity.
🎟️ Tracking
PM-1632
📔 Objective
We need to return the Organization Identifier so that we can consume it to shuttle the user to their IdP automagically. This will speed up the SSO login flow by reducing the number of clicks it takes the user to authenticate with their Organizations IdP.
If a user is an Owner or an Admin they cannot be shuttled to the IdP since they have different policies controlling them.
If a user is a member of more than one organization then they cannot be shuttled to their IdP since there is no way to know which IdP they wish to authenticate with. There will be no functional change for these users.
📸 Screenshots
SSO Required failure with ssoOrganizationIdentifier in response body
firefox_mwtNomqZV5.mp4
A user tries to authenticate with a password, but it fails because the SSO Authentication Required policy applies. Dev tools show the ssoOrganizationIdentifier is present in the response.
SSO not Required User is allowed to login (Regression smoke test)
firefox_JuewqLLFW2.mp4
A user authenticates with password flow and is allowed to login to their vault.
⏰ Reminders before review
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changes