Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[PM-17776] New Device - SSO Check #13177

Merged
merged 5 commits into from
Feb 7, 2025
Merged

Conversation

nick-livefront
Copy link
Collaborator

@nick-livefront nick-livefront commented Jan 31, 2025

🎟️ Tracking

PM-17776

📔 Objective

Refactor SSO policy conditional to check for SSO users that have ssoBound true on any of their organizations within their profile.

Edit: the previous ssoBound check did not capture admin and owner users who logged in with their master password. I added a check to see if the user is an admin or owner of the ssoBound organization and logged in with their master password. Those users should see the notice.

If they log in with SSO the will not. *This will cause different experiences for those users depending on how they log in.

The previous logic checked the policies for a user which requires a sync to be completed to know the policies. A sync is always not complete when you're an enterprise TDE user and thus the policies have not been populated resulting in an SSO user seeing the new device warning. @shane-melton has a well written summarization of other options within the ticket.

🦮 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

Copy link
Contributor

github-actions bot commented Jan 31, 2025

Logo
Checkmarx One – Scan Summary & Details78cd3b5d-c5fb-4d55-9723-2f419438a34b

Great job, no security vulnerabilities found in this Pull Request

Copy link

codecov bot commented Jan 31, 2025

Codecov Report

Attention: Patch coverage is 92.59259% with 2 lines in your changes missing coverage. Please review.

Project coverage is 35.07%. Comparing base (b2de591) to head (584e0e2).
Report is 16 commits behind head on main.

Files with missing lines Patch % Lines
...ngular/src/vault/services/vault-profile.service.ts 87.50% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #13177      +/-   ##
==========================================
+ Coverage   35.05%   35.07%   +0.02%     
==========================================
  Files        3036     3036              
  Lines       92723    92744      +21     
  Branches    16890    16897       +7     
==========================================
+ Hits        32502    32528      +26     
+ Misses      57757    57751       -6     
- Partials     2464     2465       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

gbubemismith
gbubemismith previously approved these changes Jan 31, 2025
Copy link
Member

@gbubemismith gbubemismith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

shane-melton
shane-melton previously approved these changes Feb 3, 2025
@nick-livefront nick-livefront requested review from gbubemismith and shane-melton and removed request for gbubemismith February 6, 2025 14:53
gbubemismith
gbubemismith previously approved these changes Feb 6, 2025
@nick-livefront nick-livefront merged commit dd55086 into main Feb 7, 2025
94 of 95 checks passed
@nick-livefront nick-livefront deleted the vault/pm-17776/new-device-sso branch February 7, 2025 15:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants