PM-13632: Add support for configuring multiple allowed origins #6317
Conversation
abergs
requested review from
withinfocus
and removed request for
withinfocus
abergs
force-pushed
the
anders/configurable-fido2-origins
branch
from
011e779 to
e15cda9
Compare
abergs
force-pushed
the
anders/configurable-fido2-origins
branch
from
e15cda9 to
baa3d0a
Compare
|
New Issues (3)Checkmarx found the following issues in this Pull Request
Fixed Issues (3)Great job! The following issues were fixed in this Pull Request
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6317 +/- ##
==========================================
+ Coverage 50.00% 50.15% +0.14%
==========================================
Files 1831 1840 +9
Lines 81457 81670 +213
Branches 7217 7232 +15
==========================================
+ Hits 40735 40960 +225
+ Misses 39157 39137 -20
- Partials 1565 1573 +8 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
abergs
requested review from
withinfocus,
justindbaur,
a team and
JaredSnider-Bitwarden
|
|
||
| public class Fido2Settings | ||
| { | ||
| public HashSet<string> Origins { get; set; } |
There was a problem hiding this comment.
ℹ️ I am assuming the configuration readers in .NET can work with this type but an array or list of strings is more typical.
|
We don't need a separata QA test for this, right? I'm good to merge as is? No behaviour should change unless we start adding configuration. |
Since it is not feature flagged, I would recommend a QA smoke test per our standard practices. |
| } | ||
| else | ||
| { | ||
| options.Origins = new HashSet<string> { globalSettings.BaseServiceUri.Vault }; |
There was a problem hiding this comment.
Add hardcoded extension ids and such
There was a problem hiding this comment.
Would you prefer to have those hard-coded at compile-time or inserted via environment variables at runtime? I assume that by hardcoding you mean to put them into Constants.cs so that we don't have to ask self-hosted customers to configure their extensions as well? We do have a precedent for hard-coding those on clients but no need up to this point on server.
Is there any need to provide an avenue for override/extension by third-party extension developers to add to the list for a self-hosted scenario? If these extension IDs were compile-time constants, and we have the Vault URL, then we could eliminate the need to expose the ability to configure the Origins via environment variables (other than the Vault URI).
There was a problem hiding this comment.
I am trying to eliminate the need for customers to have to configure this on their end. Client IDs/origins don't really change, and if they need to break away from them for some reason (custom client, QA environment, etc), they could override it with their own ENV variable global setting that is exposed.
There was a problem hiding this comment.
I've added chromium constants in 26e8cf4.
I also included the development extension so that "Login with passkey" would work for local dev / testing out of the box too.
I verified that I was able to use the feature successfully without any custom config.
There was a problem hiding this comment.
@abergs I was thinking that we would provide defaults/overrides at the extension ID level, rather than wholesale "override all values or use the defaults" as done here.
Perhaps the globalSettings could be extended with an ExtensionIdentifierSettings class, like we do for web push, with default values set to the production extension IDs. That way if you want to override the Chrome extension to use the Developer version, you could make just that change in your appSettings.Development.json and not have to also override all the other values. It also means that we could avoid having to list the developer extension along with the others.
Part of this ties in with @JaredSnider-Bitwarden's question here, as I'm not 100% sure what the use-case is for the override and so not absolutely sure how the override API could be designed to fit the anticipated case.
There was a problem hiding this comment.
Also, we believe that the hard-coding of the development extension is not valid. From what QA has told us, when you side-load an extension you get a different, random ID for each time you do so. So hard-coding that value is just capturing what that random GUID is for you and will not work for others.
abergs
requested review from
withinfocus,
trmartin4 and
JaredSnider-Bitwarden
and removed request for
justindbaur
|
|
||
| if (globalSettings.Fido2?.Origins?.Any() == true) | ||
| { | ||
| options.Origins = new HashSet<string>(globalSettings.Fido2.Origins); |
There was a problem hiding this comment.
❓ : What real world scenario do we expect any values to be set in globalSettings.Fido2.Origins? With the recent change to hardcode extension ids down below... I'm not seeing when this would be used?
There was a problem hiding this comment.
@JaredSnider-Bitwarden Any browser extension that is not already hard coded ("beta", new browser etc) or custom domain / extension-id in a selfhost/enterprise deployment.
As well as the ability to remove the support for some extension, for whatever reason that may be.
Co-authored-by: Matt Bishop <[email protected]>
|
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-13632
📔 Objective
This PR contains enablement work for the browser extension work, tracked by https://bitwarden.atlassian.net/browse/PM-13632.
It enables us to configure multiple allowed origins, so that we can list our domain (https://bitwarden.com etc) as well as our browser extension (chrome-extension://aabbbccc).
It loads additional Origins from the configuraiton to allow us to update it dynamically or use different values in different environments.
📸 Screenshots
⏰ Reminders before review
🦮 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