[PM-16251] Remove ActiveUserState from Policy Service by BTreston · Pull Request #13231 · bitwarden/clients

BTreston · 2025-02-03T19:30:10Z

🎟️ Tracking

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

📔 Objective

This PR adds in the vNext implementations of the Policy Service with ActiveUserState references removes. Promise based async functions are refactored to return Observables, and the service is refactored to remove get$ and getAll$. Functions are refactored to expect a UserId as a required param.

This code is not yet wired up, this is only the initial implementation. This will be wired up in followup PRs

📸 Screenshots

⏰ 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

github-actions · 2025-02-03T19:38:54Z

Checkmarx One – Scan Summary & Details – 6bee6686-cada-47d8-9d3d-3aff256f2036

Great job, no security vulnerabilities found in this Pull Request

codecov · 2025-02-03T21:32:55Z

Codecov Report

Attention: Patch coverage is 58.16327% with 41 lines in your changes missing coverage. Please review.

Project coverage is 35.32%. Comparing base (59ee262) to head (75d3628).
Report is 83 commits behind head on main.

Files with missing lines	Patch %	Lines
...le/services/policy/default-vnext-policy.service.ts	59.13%	24 Missing and 14 partials ⚠️
...onsole/abstractions/policy/vnext-policy.service.ts	0.00%	2 Missing ⚠️
...dmin-console/services/policy/vnext-policy-state.ts	66.66%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #13231      +/-   ##
==========================================
+ Coverage   35.29%   35.32%   +0.02%     
==========================================
  Files        3126     3129       +3     
  Lines       92597    92695      +98     
  Branches    16805    16835      +30     
==========================================
+ Hits        32684    32741      +57     
- Misses      57473    57500      +27     
- Partials     2440     2454      +14

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

…en/clients into ac/PM-16251-vNext-Policy-Service

JimmyVo16 · 2025-02-05T18:19:05Z

libs/common/src/admin-console/services/policy/default-vnext-policy.service.ts

+      const organization = orgDict[policy.organizationId];
+
+      // This shouldn't happen, i.e. the user should only have policies for orgs they are a member of
+      // But if it does, err on the side of enforcing the policy


non-blocking question: is there a way for us to get visibility into this in the rare event it does happen?

I checked Datadog, and it doesn't look like we use RUM. I'm not sure if we're using another monitoring tool for the client side.

@JimmyVo16 : We don't have any observability for front-end errors. (There was an innovation sprint proposal related to this but it was deferred if you want to look it up for context - user privacy is a big issue with logging/analytics.)

libs/common/src/admin-console/abstractions/policy/vnext-policy.service.abstraction.ts

libs/common/src/admin-console/services/policy/default-vnext-policy.service.ts

eliykat · 2025-02-06T00:51:07Z

libs/common/src/admin-console/services/policy/default-vnext-policy.service.ts

+      const organization = orgDict[policy.organizationId];
+
+      // This shouldn't happen, i.e. the user should only have policies for orgs they are a member of
+      // But if it does, err on the side of enforcing the policy


@JimmyVo16 : We don't have any observability for front-end errors. (There was an innovation sprint proposal related to this but it was deferred if you want to look it up for context - user privacy is a big issue with logging/analytics.)

libs/common/src/admin-console/abstractions/policy/vnext-policy.service.abstraction.ts

libs/common/src/admin-console/services/policy/default-vnext-policy.service.ts

libs/common/src/admin-console/services/policy/default-vnext-policy.service.spec.ts

eliykat · 2025-02-06T01:05:19Z

libs/common/src/admin-console/services/policy/default-vnext-policy.service.spec.ts

+  describe("policies$", () => {
+    it("returns all policies", async () => {


This tests contradicts the next tests: it can't return all policies and not return policies that are disabled or not enforced. As per previous comment this behavior is likely to change, but just consider how the test can be as specific as possible to its intended behavior.

libs/common/src/admin-console/services/policy/default-vnext-policy.service.spec.ts

libs/common/src/admin-console/services/policy/default-vnext-policy.service.ts

libs/common/src/admin-console/abstractions/policy/vnext-policy.service.abstraction.ts

libs/common/src/admin-console/services/policy/default-vnext-policy.service.spec.ts

sonarqubecloud · 2025-02-17T02:40:39Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
13.2% Duplication on New Code

See analysis details on SonarQube Cloud

initial impl

30d3674

BTreston requested a review from a team as a code owner February 3, 2025 19:30

BTreston requested a review from JimmyVo16 February 3, 2025 19:30

BTreston marked this pull request as draft February 3, 2025 19:30

BTreston added 2 commits February 3, 2025 14:31

Merge branch 'main' into ac/PM-16251-vNext-Policy-Service

71241bc

rename file to fix linter error

fd35548

Rename vNext-policy-state.ts to vnext-policy-state.ts

c8d1dc0

Merge branch 'main' into ac/PM-16251-vNext-Policy-Service

b17c765

BTreston marked this pull request as ready for review February 4, 2025 15:30

BTreston requested a review from eliykat February 4, 2025 15:30

BTreston changed the title ~~initial impl~~ [PM-16251] Remove ActiveUserState from Policy Service Feb 4, 2025

BTreston added 2 commits February 4, 2025 11:26

fix masterPasswordPolicyOptions$

e29018b

Merge branch 'ac/PM-16251-vNext-Policy-Service' of github.com:bitward…

4632fe0

…en/clients into ac/PM-16251-vNext-Policy-Service

JimmyVo16 reviewed Feb 5, 2025

View reviewed changes

JimmyVo16 previously approved these changes Feb 5, 2025

View reviewed changes

eliykat requested changes Feb 6, 2025

View reviewed changes

fix ts-strict errors, refactor policies$ and tests

58747c6

BTreston dismissed JimmyVo16’s stale review via 58747c6 February 6, 2025 17:28

BTreston requested review from JimmyVo16 and eliykat February 6, 2025 19:52

cleanup

edf1228

eliykat requested changes Feb 6, 2025

View reviewed changes

cleanup

34bd1a4

BTreston requested a review from eliykat February 6, 2025 22:21

eliykat approved these changes Feb 6, 2025

View reviewed changes

JimmyVo16 approved these changes Feb 10, 2025

View reviewed changes

Merge branch 'main' into ac/PM-16251-vNext-Policy-Service

75d3628

BTreston merged commit 9a66aea into main Feb 24, 2025
93 of 94 checks passed

BTreston deleted the ac/PM-16251-vNext-Policy-Service branch February 24, 2025 15:16

		describe("policies$", () => {
		it("returns all policies", async () => {

Conversation

BTreston commented Feb 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🎟️ Tracking

📔 Objective

📸 Screenshots

⏰ Reminders before review

🦮 Reviewer guidelines

Uh oh!

github-actions bot commented Feb 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Great job, no security vulnerabilities found in this Pull Request

Uh oh!

codecov bot commented Feb 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

JimmyVo16 Feb 5, 2025

Choose a reason for hiding this comment

Uh oh!

eliykat Feb 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

eliykat Feb 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

eliykat Feb 6, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sonarqubecloud bot commented Feb 17, 2025

Quality Gate passed

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

BTreston commented Feb 3, 2025 •

edited

Loading

github-actions bot commented Feb 3, 2025 •

edited

Loading

codecov bot commented Feb 3, 2025 •

edited

Loading

eliykat Feb 6, 2025 •

edited

Loading

eliykat Feb 6, 2025 •

edited

Loading