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-13452] - add password health raw data component #11519

Merged
merged 9 commits into from
Oct 16, 2024
Merged

Conversation

jaasen-livefront
Copy link
Collaborator

🎟️ Tracking

https://bitwarden.atlassian.net/browse/PM-13452

📔 Objective

This PR adds the health report raw data component to the access intelligence page. This is part 1 of 3 of a progressively more complete version of the password health report.

📸 Screenshots

Screenshot 2024-10-11 at 4 49 13 PM

⏰ Reminders before review

  • Contributor guidelines followed
  • All formatters and local linters executed and passed
  • Written new unit and / or integration tests where applicable
  • Protected functional changes with optionality (feature flags)
  • Used internationalization (i18n) for all UI strings
  • CI builds passed
  • Communicated to DevOps any deployment requirements
  • Updated any necessary documentation (Confluence, contributing docs) or informed the documentation team

🦮 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

@jaasen-livefront jaasen-livefront requested a review from a team as a code owner October 11, 2024 23:50
Copy link
Contributor

github-actions bot commented Oct 12, 2024

Logo
Checkmarx One – Scan Summary & Detailsfb55b335-be48-4a42-87f8-c29fb373c47a

New Issues

Severity Issue Source File / Package Checkmarx Insight
MEDIUM Client_Privacy_Violation /apps/web/src/app/tools/access-intelligence/password-health.component.html: 50 Attack Vector
MEDIUM Client_Privacy_Violation /apps/web/src/app/tools/access-intelligence/password-health.component.html: 45 Attack Vector
MEDIUM Client_Privacy_Violation /apps/web/src/app/tools/access-intelligence/password-health.component.html: 40 Attack Vector
MEDIUM Client_Privacy_Violation /apps/web/src/app/tools/access-intelligence/password-health.component.html: 45 Attack Vector

Comment on lines 211 to 223
const result = this.passwordStrengthService.getPasswordStrength(
login.password,
null,
userInput.length > 0 ? userInput : null,
);
this.passwordStrengthCache.set(cacheKey, result.score);
}
const score = this.passwordStrengthCache.get(cacheKey);

if (score != null && score <= 2) {
this.passwordStrengthMap.set(id, this.scoreKey(score));
this.weakPasswordCiphers.push(ciph);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Please have a look at the most recent version of the weak-password-report and how it handles adding the score to the ResultRow.
It simplifies the storage and removes the need for passwordStrengthCache and passwordStrengthMap.

Similar work has also been done on the exposed-password-report

Comment on lines 127 to 137
const promise = this.auditService.passwordLeaked(login.password).then((exposedCount) => {
if (exposedCount > 0) {
this.exposedPasswordCiphers.push(ciph);
this.exposedPasswordMap.set(id, exposedCount);
if (!this.reportCipherIds.includes(ciph.id)) {
this.reportCipherIds.push(ciph.id);
this.reportCiphers.push(ciph);
}
}
});
promises.push(promise);
Copy link
Contributor

Choose a reason for hiding this comment

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

Please have a look at the most recent version of the exposed-password-report. It removes the need for the exposedPasswordMap and the lookup from the markup.

Copy link

codecov bot commented Oct 14, 2024

Codecov Report

Attention: Patch coverage is 75.28090% with 22 lines in your changes missing coverage. Please review.

Project coverage is 33.20%. Comparing base (fa41f29) to head (2ecf235).
Report is 24 commits behind head on main.

✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
...s/access-intelligence/password-health.component.ts 76.13% 10 Missing and 11 partials ⚠️
...cess-intelligence/access-intelligence.component.ts 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #11519      +/-   ##
==========================================
+ Coverage   33.16%   33.20%   +0.03%     
==========================================
  Files        2779     2780       +1     
  Lines       86225    86398     +173     
  Branches    16420    16465      +45     
==========================================
+ Hits        28597    28688      +91     
- Misses      55362    55425      +63     
- Partials     2266     2285      +19     

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

@djsmith85 djsmith85 removed request for a team and dani-garcia October 15, 2024 09:28
TableModule,
],
})
export class PasswordHealthComponent extends CipherReportComponent implements OnInit {
Copy link
Contributor

Choose a reason for hiding this comment

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

This component does not need to extend CipherReportComponent. This will clean-up the dependencies in the ctor.

  • Within ngOnInit we'll need to get the organizationId from the route
  • The dataSource from CipherReportComponent will need to be moved into this component
  • As there is no organization filter toggle above the table, as there is only one organization in the current context, the call to this.filterCiphersByOrg(this.reportCiphers);can be replaced with this.dataSource.data = this.reportCiphers
  • getAllCipherswill need to be replaced with a call to return this.cipherService.getAllFromApiForOrganization(this.organization.id);

Copy link
Contributor

@djsmith85 djsmith85 left a comment

Choose a reason for hiding this comment

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

The changes under reports/pages don't seem to be relevant to the password-health.component

Copy link
Contributor

@djsmith85 djsmith85 left a comment

Choose a reason for hiding this comment

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

Just one more thing, then I'll leave it up to @ttalty to decide if he wants to refine this further or merge this so we can continue with adding the member access data with #11494

Comment on lines 70 to 72
this.organizationService.organizations$.pipe(takeUntilDestroyed()).subscribe((orgs) => {
this.organization = orgs[0];
});
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ As mentioned in my previous comment, please retrieve the organizationId from the route and not just pick the first Organization.

djsmith85
djsmith85 previously approved these changes Oct 15, 2024
@jaasen-livefront jaasen-livefront merged commit d70d2cb into main Oct 16, 2024
35 of 36 checks passed
@jaasen-livefront jaasen-livefront deleted the PM-13452 branch October 16, 2024 16:21
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.

2 participants