-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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-11404] Account Management: Prevent a verified user from purging their vault #4853
[PM-11404] Account Management: Prevent a verified user from purging their vault #4853
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4853 +/- ##
=======================================
Coverage 41.75% 41.75%
=======================================
Files 1363 1363
Lines 63914 63915 +1
Branches 5857 5853 -4
=======================================
+ Hits 26685 26686 +1
- Misses 36024 36026 +2
+ Partials 1205 1203 -2 ☔ View full report in Codecov by Sentry. |
New Issues
Fixed Issues
|
…rifiedUserEmailDomainAsync and refactor to return a list. Remove ManagedByOrganizationId from ProfileResponseMode. Add ManagesActiveUser to ProfileOrganizationResponseModel
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Auth changes LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a couple of comments based on our recent discussions, but I'll still defer to @jrmccannon for the full review.
@@ -31,6 +31,8 @@ | |||
using File = System.IO.File; | |||
using JsonSerializer = System.Text.Json.JsonSerializer; | |||
|
|||
#nullable enable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please don't enable nullable unless you've reviewed the code to make sure its null handling is correct. In this case, you've enabled it on this entire class, but if you look at it in the Github diff, there are null warnings everywhere because the existing code doesn't handle nullable types properly.
You should enable it just for your new methods by putting #nullable enable
immediately before the block of new code, and then #nullable disable
immediately after. Then double check the Github diff to find and fix any null warnings.
Same for any other files here.
/// The organization must be enabled and able to have verified domains. | ||
/// This property is null if the Account Deprovisioning feature flag is disabled. | ||
/// </remarks> | ||
public bool? ManagesActiveUser { get; set; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think "active" is confusing here. This class doesn't have any concept of whether the user it relates to is "active" in the client-side state or not.
This class (and Organization.cs
on the client-side) already relates to a specific user - it has a UserId
and an OrganizationUserId
. So there is no need to qualify who the user is - it's not necessarily the "active" user, it's the user that matches the UserId
in this class.
I suggest taking the "active" wording out of this. Maybe UserIsManagedByOrganization
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is fair, I'll rename it to UserIsManagedByOrganization
, thanks for suggesting it.
I named it "active" because when we return this object its always in relation to the active user. You're right that it doesn't have to be though.
6d1859f
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My feedback has been resolved, thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, I don't see any problems with this and it is consistent with the rest of the code base. I did have one ⛏️, but its a non-blocking change.
…sManagedByAnyOrganizationAsync to not return nullable objects. Update ProfileOrganizationResponseModel.UserIsManagedByOrganization to not be nullable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Vault changes look good
cfec28e
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-11404
📔 Objective
If the Account Deprovisioning feature flag is enabled then prevent organization managed users from purging their vault.
Clients PR: bitwarden/clients#11411
⏰ 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