[PM-11442] Emergency Cipher Viewing #12054
Conversation
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## main #12054 +/- ##
==========================================
+ Coverage 33.47% 33.49% +0.02%
==========================================
Files 2878 2879 +1
Lines 89941 89982 +41
Branches 17116 17120 +4
==========================================
+ Hits 30106 30142 +36
- Misses 57455 57458 +3
- Partials 2380 2382 +2 ☔ View full report in Codecov by Sentry. |
…t/pm-11442/emergency-access-view-only
…from emergency view
- `ViewPasswordHistoryService` accepts a cipher id or CipherView. When a CipherView is included, the history component no longer has to fetch the cipher.
nick-livefront
dismissed stale reviews from Patrick-Pimentel-Bitwarden and JaredSnider-Bitwarden
via
60e567b
…t/pm-11442/emergency-access-view-only
|
@JaredSnider-Bitwarden @Patrick-Pimentel-Bitwarden @shane-melton After chatting with Kyra on the QA side, we decided to also update the view modal to use the new extension-refresh version. 3ff8a60 This came with a tangental change for the password history component. Previously, the |
nick-livefront
requested review from
Patrick-Pimentel-Bitwarden and
JaredSnider-Bitwarden
|
Added the FIXME comment and associated ticket that we discussed offline: 0cde1e6 |
There was a problem hiding this comment.
❓ This won't be used once the refresh flag is enabled, right?
There was a problem hiding this comment.
Yes, that is accurate!
There was a problem hiding this comment.
🎨 This may be somewhat out of scope. But I'm thinking we should probably just make the component only accept a CipherView which would let use remove all the redundant async init logic (which has likely already been done by the parent component that's opening the history view).
This would also let us simplify the viewPasswordHIstoryService by removing the generic CipherOption and different implementations. I think you've already done most of the leg work in this component so far, and I don't think we even use the CipherId variant after your changes here.
There was a problem hiding this comment.
Good idea, this does simplify the implementation across the board. It also makes the PasswordHistoryComponent more "view" oriented rather than containing logic to fetch the cipher.
|
|
||
| /** | ||
| * The ViewPasswordHistoryService is responsible for displaying the password history for a cipher. | ||
| */ | ||
| export abstract class ViewPasswordHistoryService { | ||
| abstract viewPasswordHistory(cipherId?: CipherId): Promise<void>; | ||
| export abstract class ViewPasswordHistoryService<CipherOption extends CipherId | CipherView> { |
There was a problem hiding this comment.
❓ item-history-v2 component which will always request the CipherView variant. But, that variant will never be registered for the Browser client and should fail.
There was a problem hiding this comment.
The browser uses BrowserViewHistoryService which uses the CipherId. That is because the password history on the extension is a separate page and the cipher id is taken from the query params.
There was a problem hiding this comment.
I'm working through your previous comment and now I just hit the road block that you're mentioning here. I'm not sure how I missed this while testing. 🤔 back to the drawing board!
There was a problem hiding this comment.
Resolved by the fix in this comment
- Remove the option to pass in only an id
…t/pm-11442/emergency-access-view-only
🎟️ Tracking
PM-11442
📔 Objective
The base component
ngOnInitcalculates theviewOnlyproperty based on cipher edit and another property. In the case of emergency access,viewOnlyshould always be true so the user cannot edit or attempt to edit the cipher.📸 Screenshots
emergency-before.mov
emergency-after.mov
🦮 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