Allow customizing the featureStates

[PKizzle]

Instead of statically disabling autofill-v2 and enabling fido2-vault-credentials let the user decide which features to en-/disable.

By default the above mentioned standard values are used but can be changed by specifying a comma separated list of any of the currently available in the bitwarden clients: https://github.com/bitwarden/clients/blob/00fd45a678956c76dde6caa76e8ba266c204471c/libs/common/src/enums/feature-flag.enum.ts

To disable a feature just prepend an exclamation mark (!). The default value is: !autofill-v2,fido2-vault-credentials

[BlackDex]

I'm not very fond of adding this.
The main reason is that people might enable something which either needs server changes or is not fully supported yet on all the clients if not updated.

One item which we do not (yet) support is flexible collections for example (and i do not even know what it does too).
And there are several others which are pointless when using Vaultwarden.

So, this will be a no-go from me.

[PKizzle]

I understand your concerns regarding possible issues with unsupported server-side features. However, I would like to suggest that those features could simply be removed from the list of allowed features.

When it comes to old client versions that do not support all features enabled in the server, they simply ignore those features. I have already tested it with the autofill-overlay feature flag, which is only available in the latest client release, and older clients simply ignore it.

In the end, it is up to the user to make sure their configuration is stable and secure, as there are countless other ways to screw up your configuration. As the default values are exactly the same as before, a warning could be added to the documentation making sure that users are aware of the implications that en- or disabling of feature flags has.

I respectfully disagree with your decision to reject this feature, as I believe it would provide more flexibility and customization for the users. I hope you will reconsider your position and give this feature a chance.

PS: As already mentioned above I have enabled the autofill-overlay feature which for me is one of the biggest improvements to Bitwarden and has been demanded by the community since 2018. It is currently hidden behind this feature flag so users wanting to test it require this PR to enable it.

[BlackDex]

I meant to say as-is actually.

But we currently only support:

• autofill-v2
• fido2-vault-credentials

I think a few others might also work. So if you can prune the list to the bare minimum support items, that would be cool.

[PKizzle]

I guess the following feature flags are purely client related if I am not mistaken?

• autofill-overlay (requires autofill-v2)
• autofill-v2
• browser-fileless-import
• display-kdf-iteration-warning
• passwordless-login (requires trusted-device-encryption)

And these might require additional changes in Vaultwarden:

• bulk-collection-access
• flexible-collections
• flexible-collections-v-1
• item-share
• trusted-device-encryption

Do you agree with this list?

[BlackDex]

I think browser-fileless-import is used to import from LastPass via a client without using uploaded files. I suppose they use the same import endpoints so that shouldn't be an issue.

[PKizzle]

Then the list of available featureStates in Vaultwarden would look like:

• autofill-overlay (requires autofill-v2)
• autofill-v2
• browser-fileless-import
• display-kdf-iteration-warning
• fido2-vault-credentials

By the way is something like trusted-device-encryption (Bitwarden doc) and passwordless-login (WebAuthn client login) planned for Vaultwarden as well?

[BlackDex]

@PKizzle, the trusted-device-encryption is linked to SSO. There is a PR open for SSO (Which i still need to look better at) so if, it should be part of that, or can be worked upon after it has been merged.

The passwordless-login probably needs some new endpoints or login validation.
While we already support Webauthn as 2FA/MFA, using it as a passwordless type probably needs some adjustments.

Keep in mind that the Bitwarden Self Hosted currently only supports

• trusted-device-encryption (Because it has SSO support)
• fido2-vault-credentials

[PKizzle]

Yes, exactly. This is how Vaultwarden currently looks when enabling passwordless-login (the spinner never stops as the API call fails):

[stefan0xC]

Last time I checked the display-kdf-iteration-warning feature flag is not used anymore (since web-v2023.7.0, cf. bitwarden/clients@4820c88) so it should not make a difference for newer web-vault versions.

[BlackDex]

Looks almost ok to me.
There are a few items i left a comment about.

I'm also missing the new feature in the .env.template with comments so that needs to be added.

And, there might be an issue with using ! as an exclusion option.
In bash this will cause issues when used between " double quotes.

We can either add a good note regarding this in the config/template/wiki or think of a different character or other way to negate it? Maybe a ^ could be an option? While i do like the ! though, it is kinda universal.

[stefan0xC]

I think if we let users decide which features should be enabled/disabled, we should use a conservative approach (i.d. the features should be disabled by default).

The problem with making it configurable by users at all is that we can't provide the defaults anymore for all users because whether or not we enable or disable them by default, once a user has the default saved in the config.json they'd need to update the feature flag list accordingly, even if we decided that it is not supported anymore. (I mean removing them from the list of supported/known flags would be a way to explicitly disable it yes, but the reverse would also be the case so some users would still have to explicitly configure all new flags to enable them and the comment that all feature flags are disabled by default would be misleading).

So if this is implemented I'd rather tell users that they'll have to deliberately enable a (known) feature flag to help us test a new feature instead of telling users that they'll have to disable features which we have set as a default and have noticed that they were not ready yet.

Regarding the question of whether or not we should also give users a way to disable features I think this would depend on the clients. Currently if a feature flag is missing the clients should assume that any given feature flag defaults to false (however that might not be the case given that the getFeatureFlag() function technically allows for providing a different defaultValue in the client).

Last point I'd prefer if users only had to list features they want enabled to keep it as simple as possible. (And only implement a way to disable a feature explicitly, when it is really mandatory, which I personally would consider a bug in the clients repository...)

[PKizzle]

@stefan0xC Instead of allowing to disable a feature all known feature flags could be set to false by default to ensure that any defaultValue other than false does not negatively affect Vaultwarden users. This however requires that two feature lists are used. One list that is equal to bitwarden/clients@00fd45a/libs/common/src/enums/feature-flag.enum.ts and another one which includes the ones supported by Vaultwarden. Otherwise users would be able to enable unsupported features.

@BlackDex It seems like Bitwarden is now enabling autofill-v2 by default: https://api.bitwarden.com/config

[stefan0xC]

@PKizzle feature flags as I understand them are not enabled by default. So we should add only those feature flags that might be safe to enable to the KNOWN_FLAGS (and not add something that is known that it is not supported in the backend like trusted-device-encryptions - i.e. the PR that implements this feature would have to also add the appropriate feature flag and once the feature is stable, we could also add this to a default if it makes sense or wait until the feature flag is not checked anymore). So we would not have to add all feature flags that are currently available in Bitwarden but still we'd have to decide which we set to true (like fido2-vault-credentials) and where we would let the users decide (and I'd argue that we should only provide those as default that are a) known to work and b) where Bitwarden would make the decision for self hosted or the demand is high enough / the feature has been stable / whatever the case maybe). - Users could then still decide to disable our defaults if they remove them from the feature list and there's no need for us to have a way to disable a feature (unless Bitwarden messes up there intended feature flag system - which would be a bug in my opinion as this would violate their architecture decision)

[PKizzle]

@stefan0xC It is very unfortunate but exactly this situation has already occurred. As mentioned earlier autofill-v2 was enabled by default in several client releases whilst being disabled again using Bitwarden's featureStates. For more information please have a look at the discussion in #4052

[stefan0xC]

@stefan0xC This is sadly not always the case. As mentioned earlier autofill-v2 was enabled by default in several client releases whilst disabled again using Bitwarden's featureStates. Fore more information please have a look at the discussion in #4052

No it was enabled by us in #3990 not by the clients. We could have also removed the feature flag to disable the feature.

[PKizzle]

@stefan0xC I see. I misunderstood some of the comments in the Bitwarden repo regarding the issue hinting at the client not resetting a feature flag to false once it was enabled. However, it seems like the self-hosted server version had the autofill-v2 feature flag enabled and not the client by default.

[dani-garcia]

Some minor comments, I agree about having a whitelist of features to enable, to avoid someone accidentally breaking their vault if the clients think that the server supports a feature that it doesn't.

How would we handle a change in the defaults for users that have already configured their flags? For example, by default we now set fido2-vault-credentials. If I decide on my instance that I don't care about passkeys but want the new autofill, I might set this setting to autofill-v2.

Now, if we release support for a new feature, we won't have any way to get this user updated, beyond announcing on the README or the changelog that this new feature is available. I think we need a way to forcefully enable some feature flags, regardless of user config. Of course we'd need to be conservative about it to make sure we don't break anything, but we can't afford to let users stay on old feature flags forever.

[PKizzle]

@dani-garcia A possible way to inform users about new features is to display a notice in the UI alongside the recommended default configuration.

Since these features are experimental, users who want to try them should be aware of the potential risks (such as bugs or removal of the feature in the future). It might also be helpful to show a general warning if the user's configuration differs from the default values in terms of the feature flags.

Could you explain what a possible use-case of forcefully enabling certain experimental features would be?

[dani-garcia]

My concern is some future feature flag informing the clients to use some new API endpoint instead of the existing one, eventually we might want to deprecate and then remove the old API endpoints, which can't be done if some older clients haven't enabled that flag.

Honestly this is mostly a future proofing issue and might turn out to be not a big deal, so we can cross that bridge when we get to it.

Yeah showing a warning with the defaults and maybe all the available values in the UI might be nice, but that can be done separately from this change, I think. We can also reserve a section at the release notes to mention any added/removed/updated flags too.

[BlackDex]

As a note, they released a bug fix release which disables the overlay by default.

bitwarden/clients@web-v2023.12.0...browser-v2023.12.1

[PKizzle]

The default value is now off, but the visibility in the client's settings is still controlled by the feature flag, which remains unchanged. I have already enabled the feature and adjusted the settings in multiple clients, so I don’t think this change affects me. Thank you for the information anyway!

[cl1ent]

thank you for this PR! do you have any ETA when this will be part of the next (minor) release? once #4263 is merged maybe?

[bibicadotnet]

thank you for this PR! do you have any ETA when this will be part of the next (minor) release? once #4263 is merged maybe?

If you like, can use image:vaultwarden/server:testing
Add environment - EXPERIMENTAL_CLIENT_FEATURE_FLAGS=autofill-overlay,autofill-v2,browser-fileless-import,fido2-vault-credentials