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-18583] Separate Desktop and CLI ClientTypes #5441

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

trmartin4
Copy link
Member

@trmartin4 trmartin4 commented Feb 24, 2025

🎟️ Tracking

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

📔 Objective

We currently group the desktop and CLI clients together in the DesktopTypes.

This causes downstream issues for feature flag targeting, as the ClientType is used here to assign to the LaunchDarkly context for feature flag targeting.

This means that it is impossible to target a different CLI version than desktop version, without specifying individual device types in LaunchDarkly (bypassing the convenience of the ClientType).

The reason for this request related to the linked issue is that for New Device Verification, we will have different versions for the CLI and the desktop in the upcoming release (2025.2.1 vs. 2025.2.0), and in setting up the feature flags for this release I noticed the grouping.

⏰ 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

@trmartin4 trmartin4 changed the title Separate desktop and CLI for ClientType checks Separate Desktop and CLI ClientTypes Feb 24, 2025
@trmartin4 trmartin4 changed the title Separate Desktop and CLI ClientTypes [PM-18583] Separate Desktop and CLI ClientTypes Feb 24, 2025
@trmartin4 trmartin4 requested a review from mzieniukbw February 24, 2025 21:52
@trmartin4 trmartin4 marked this pull request as ready for review February 24, 2025 21:52
@trmartin4
Copy link
Member Author

@mzieniukbw was there a specific intent behind grouping the desktop and CLI together? I want to make sure that I don't break any other downstream contracts that might be relying on them being the same ClientType.

Copy link
Contributor

Logo
Checkmarx One – Scan Summary & Detailsca21cc5c-9e21-4010-972d-d7007d79c02b

Great job, no security vulnerabilities found in this Pull Request

Copy link

codecov bot commented Feb 24, 2025

Codecov Report

Attention: Patch coverage is 80.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 44.51%. Comparing base (0f10ca5) to head (a867414).

Files with missing lines Patch % Lines
src/Core/Utilities/DeviceTypes.cs 80.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #5441   +/-   ##
=======================================
  Coverage   44.51%   44.51%           
=======================================
  Files        1513     1513           
  Lines       70379    70383    +4     
  Branches     6349     6350    +1     
=======================================
+ Hits        31327    31330    +3     
  Misses      37704    37704           
- Partials     1348     1349    +1     

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

Copy link
Contributor

@withinfocus withinfocus left a comment

Choose a reason for hiding this comment

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

Is this safe to do? I am not sure if anything else ... somewhere ... considers the CLI a desktop client type. I understand your intention, but if my question is difficult to answer you could just use device type more explicitly for LD targeting.

@trmartin4
Copy link
Member Author

Is this safe to do? I am not sure if anything else ... somewhere ... considers the CLI a desktop client type. I understand your intention, but if my question is difficult to answer you could just use device type more explicitly for LD targeting.

I am not sure where all the ClientTypes are used. I posited the same question to @mzieniukbw as well.

On the other hand, this is currently a bug, since the CLI does not accept login approval requests. It's a hidden bug, because the CLI doesn't even register to receive the requests at all, but it shows that we have assumed that Desktop != CLI as consumers of the ClientType.

Are you suggesting that we would need to evaluate the current context targeting, as well as all other uses of the ClientType, before making this change?

@withinfocus
Copy link
Contributor

Are you suggesting that we would need to evaluate the current context targeting, as well as all other uses of the ClientType, before making this change?

Yes, just to be safe that the CLI wouldn't be expected to fall into other targets. This seems unlikely, but I just don't know.

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