bitwarden / web

The website vault (vault.bitwarden.com).

https://vault.bitwarden.com

Other

2.58k stars 405 forks source link

[PS-616] Feature/require auth for enroll password reset #1698

Closed MGibson1 closed 2 years ago

MGibson1 commented 2 years ago

Type of change

[x] Bug fix
[ ] New feature development
[ ] Tech debt (refactoring, code cleanup, dependency upgrades, etc)
[ ] Build/deploy pipeline (DevOps)
[ ] Other

Objective

Require user secret validation for password reset enrollment.

Code changes

enroll-master-password-reset.component: modal component to hold our mpr warning and user verification form.
Organization-user.module/oss.module: New module for org user actions
organization-options: move mpr enrollment from this class to a separate component.
messages.json: Now that it's a modal with interaction, Are you sure was redundant.

Screenshots

Before you submit

[x] I have checked for linting errors (npm run lint) (required)
[ ] This change requires a documentation update (notify the documentation team)
[ ] This change has particular deployment requirements (notify the DevOps team)

Draft Reason

[ ] Requires bitwarden/jslib#810

kspearrin commented 2 years ago

@MGibson1 Someone else closer to the new module/architecture work for web vault should probably look at this one rather than me.

eliykat commented 2 years ago

Forgot to mention - I'm not sure that creating an OrganizationUsersModule is the right way to organize it, but I'm not sure where else it would fit given that it's just linked to from the vault page. So it's fine for now, but you might be interested in the discussion here: https://github.com/bitwarden/web/pull/1661

MGibson1 commented 2 years ago

I've seen the discussion there and internally, but it seems that discussion has stagnated. We have comments discouraging adding to our existing bucket, so I'm kind of left with little recourse if my component doesn't fit well anywhere 🤷

addisonbeck commented 2 years ago

I think it's fine to merge this as is. Modules can be picked up and put down with relative ease, what's important is the isolation from other components, which you have achieved here already. But in general the goal with modules is for them to line up with one of these types. It could be argued that OrganizationUser is a business domain, but I would say Bitwarden's current business domains are Secrets Management and Secure File/Text Sharing and that OrganizationUser is just a data model that applies to one of those domains.

With that in mind, I would just call this EnrollMasterPasswordResetModule (this and all modals are widget modules imo, though they also fit the description of the domain module in that they are features) and drop the affiliation to the data model OrganizationUser. That's similar feedback to what is going on with the in-flight OrganizationsModule, but the changes aren't dependent on each other.