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

Auth/PM-17877 Add device on all API requests #13205

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

trmartin4
Copy link
Member

@trmartin4 trmartin4 commented Feb 1, 2025

🎟️ Tracking

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

📔 Objective

In order to provide LaunchDarkly context for targeting based on device identifier, we would like to include Device-Identifier on all API requests. This is accomplished by adding the header at the ApiService level. As the AppIdService will generate the appId if it does not yet exist, there is not a check for existence before adding the header.

Since the header is now being added on all requests, this PR also removes 2 alterHeaders implementations that were used for updateTrust() and postDeviceTrustLoss(), since the header will already be there.

📸 Screenshots

On POST of cipher update, as an example

image

On update-trust when rotating a user's encryption key with trusted devices

image

⏰ 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

Copy link
Contributor

github-actions bot commented Feb 1, 2025

Logo
Checkmarx One – Scan Summary & Detailsf63b6759-d9a2-489e-bfa1-0fa0190ba54e

Great job, no security vulnerabilities found in this Pull Request

@trmartin4 trmartin4 changed the title Added device on all requests and removed explicit headers Auth/PM-17877 Add device on all requests and remove explicit headers Feb 1, 2025
Copy link

codecov bot commented Feb 1, 2025

Codecov Report

Attention: Patch coverage is 50.00000% with 3 lines in your changes missing coverage. Please review.

Project coverage is 35.48%. Comparing base (1d71212) to head (f4a338f).

Files with missing lines Patch % Lines
apps/browser/src/background/main.background.ts 0.00% 1 Missing ⚠️
...th/services/device-trust.service.implementation.ts 50.00% 1 Missing ⚠️
...uth/services/devices-api.service.implementation.ts 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #13205      +/-   ##
==========================================
- Coverage   35.48%   35.48%   -0.01%     
==========================================
  Files        3007     3007              
  Lines       90899    90894       -5     
  Branches    16908    16908              
==========================================
- Hits        32260    32258       -2     
+ Misses      56137    56134       -3     
  Partials     2502     2502              

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

@trmartin4
Copy link
Member Author

trmartin4 commented Feb 1, 2025

❓ A few questions for recommendations:

  1. This leaves the DeviceService and DeviceApiService in a bit of an unclear place. Some methods expect a deviceIdentifier, but some don't and use the current device identifier that's automatically added in ApiService. I'm open to cleaning up the interface and no longer exposing the deviceIdentifier on any of the methods - instead determining it internally from appIdService.getAppId(). I wasn't sure if this would add too much to this PR though. The only DeviceIdentifier that a client knows is its own, so it feels like we could really think of all methods in the context of the "current device" when dealing with a device identifier.
  2. The ApiService itself doesn't appear to be tested. Any recommendations of where to add tests for this?

@trmartin4 trmartin4 changed the title Auth/PM-17877 Add device on all requests and remove explicit headers Auth/PM-17877 Add device on all API requests Feb 1, 2025
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.

1 participant