Skip to content

Add support for managed settings using 3rdparty policy #2366

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Oct 25, 2024
Merged

Add support for managed settings using 3rdparty policy #2366

merged 1 commit into from
Oct 25, 2024

Conversation

varjolintu
Copy link
Member

@varjolintu varjolintu commented Oct 20, 2024 •
edited
Loading

Adds support for distributing global settings to the extension using 3rdparty policy. Includes a new managed_storage.json file that describes the settings that can be set. The schema file must be formatted correctly or the extension does not start because of failed validation.

Easiest way to test this was to use web-ext with Firefox that had the policy JSON file added. Just made sure I deleted all storage data from the temporary extension before launching it again. That caused the settings to be an empty Object, and the getter for managed settings will be called.

Example policy.json file:

{
  "policies": {
    "3rdparty": {
      "Extensions": {
        "[email protected]": {
          "settings": {
            "defined-custom-fields": {
              "https://example.com": {
                "fields": [
                  [
                    "/html/body/table/tbody/tr[10]/td[4]/table/tbody/tr/td/table/tbody/tr[2]/td[4]/form/table/tbody/tr[2]/td[2]/input",
                    "INPUT password form_password "
                  ]
                ],
                "password": [
                  "/html/body/table/tbody/tr[10]/td[4]/table/tbody/tr/td/table/tbody/tr[2]/td[4]/form/table/tbody/tr/td[2]/input",
                  "INPUT text form_username "
                ],
                "username": [
                  "/html/body/table/tbody/tr[10]/td[4]/table/tbody/tr/td/table/tbody/tr[2]/td[4]/form/table/tbody/tr[2]/td[2]/input",
                  "INPUT password form_password "
                ]
              }
            },
            "passkeys": true,
            "sitePreferences": [
              {
                "allowIframes": false,
                "ignore": "ignoreNothing",
                "improvedFieldDetection": false,
                "url": "https://example.com",
                "usernameOnly": true
              }
            ]
          }
        }
      }
    }
  }
}

Related links:
https://extensionworkshop.com/documentation/enterprise/enterprise-development/#how-to-add-policy
https://developer.chrome.com/docs/extensions/reference/manifest/storage

Fixes #2365.

@varjolintu varjolintu marked this pull request as draft October 20, 2024 12:11
@varjolintu varjolintu mentioned this pull request Oct 20, 2024
Closed
@mihakrumpestar
Copy link

mihakrumpestar commented Oct 20, 2024 •
edited
Loading

I tried to test it.

Build using npm run build -- --skip-translations which builds the extension, but it won't get installed due to error:

Installer error

Do I need the Transifex API token?

@varjolintu
Copy link
Member Author

I need to check the build script with the new file addition. For testing you do not need to build this. Just load the temporary extension directly from the sources.

@mihakrumpestar
Copy link

I did read the instructions for development now. Loading as temporary add-on also does not seem to work completely properly:

Temporary add-on

The settings are then missing Passkey options, which are also absent when I export config from temporary add-on.
Note that I am using a completely new and clean profile, so I am loading this without any prior or old setting.

@varjolintu
Copy link
Member Author

That warning in the temporary extension page is expected, becase the default manifest.json is using some properties that are Chromium-only. It still works.

@varjolintu
Copy link
Member Author

Do you mean the passkeys option is missing even if you have enabled it?

@mihakrumpestar
Copy link

mihakrumpestar commented Oct 20, 2024 •
edited
Loading

Do you mean the passkeys option is missing even if you have enabled it?

No, the Passkey section in UI is missing altogether.

image

Left is the current one I am using (the official one), right is the dev temporary one (both in different profiles).

@varjolintu
Copy link
Member Author

varjolintu commented Oct 20, 2024 •
edited
Loading

I cannot reproduce that kind of error. I tried this with both temporary profile and a newly created profile with web-ext. Does the JavaScript console say anything?

@mihakrumpestar
Copy link

I am sorry. I was trying some things and forgot I changed to master branch.

Furthermore, I tested again (on the correct branch this time) and the settings for Passkeys are indeed there, and the 3rdparty extension settings also get applied successfully. Tested on NixOS.

TLDR: It is working.

@varjolintu varjolintu force-pushed the feature/managed_schema branch from 37e77a5 to 1093a16 Compare October 20, 2024 19:20
@varjolintu
Copy link
Member Author

Did some fixes to the defined-custom-fields. Those should also work now.

@varjolintu varjolintu marked this pull request as ready for review October 20, 2024 19:22
@varjolintu varjolintu added this to the 1.9.4 milestone Oct 20, 2024
@varjolintu varjolintu force-pushed the feature/managed_schema branch from 1093a16 to 4549219 Compare October 24, 2024 19:21
@droidmonkey
Copy link
Member

Only comment I have: settings that are string values, we should enumerate the possible options either directly or in documentation or in wiki.

@varjolintu
Copy link
Member Author

Added a new wiki page for this: https://github.com/keepassxreboot/keepassxc-browser/wiki/Managed-schema

@varjolintu varjolintu merged commit 47dc2ad into develop Oct 25, 2024
@varjolintu varjolintu deleted the feature/managed_schema branch October 25, 2024 03:47
@varjolintu varjolintu mentioned this pull request Oct 28, 2024
Closed
@JuneStepp
Copy link

@varjolintu Do you mind explaining why you decided to have this only apply when the extension is first installed? It seems like an odd use case to only manage the initial configuration with no updates applying afterwards.

@varjolintu
Copy link
Member Author

varjolintu commented Jan 25, 2025 •
edited
Loading

@JuneStepp Is that something users really want? That would override any user-specific settings for sites etc. Maybe we could add a notification that asks the user if there are new settings available, and update them manually.

@JuneStepp
Copy link

JuneStepp commented Jan 25, 2025 •
edited
Loading

@varjolintu

Is that something users really want? That would override any user-specific settings for sites etc. Maybe we could add a notification that asks the user if there are new settings available, and update them manually.

I don't think a notification prompt makes sense, since the point of managed settings is that something is managed externally. It's up to the external party to determine how things get applied.

I can only vouch for my own needs, but here's an attempt at breaking down some of the potential options for how to load settings at launch:

  1. Fully overwrite with managed settings, if available.
    • What this PR would do without if (Object.keys(item.settings).length === 0).
    • Great for my use case of declaratively configuring and deploying browsers across devices for personal use.
    • For organizations, can interfere with the user if the organization doesn't fully manage the DB(s).
  2. Load the managed settings when there are no local settings.
    • The current behavior.
    • No updating of settings so only useful as a customized default for new installations.
    • I don't really see the need for this.
  3. Merge managed and user settings with user ones taking precedence.
    • The current behavior, but the defaults are kept up to date.
    • Great if the main thing an organization needs are to add custom login fields and site preferences.
  4. Merge managed and user settings with managed ones taking precedence.
    • Lets the organization actually manage the settings they want to without interfering with the user beyond that.
  5. Merge managed and user settings with managed ones taking precedence and being made read-only.
    • Enforced version of above.

3 and/or 5 is what people probably expect when using policies; since this PR is already merged and released, I think keeping 2 but adding an option for 1 makes the most immediate sense. It would be a trivial change and fix my use case. Then, you could wait for someone who needs something more complicated to say so.
I'd be happy to do a PR for that if you want it.

@varjolintu varjolintu mentioned this pull request Feb 8, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@droidmonkey droidmonkey droidmonkey approved these changes

Assignees
No one assigned
Labels
new feature
Projects
None yet
Milestone
1.9.4
Development

Successfully merging this pull request may close these issues.

Add support for setting the settings using 3rdparty policy
4 participants
@varjolintu @mihakrumpestar @droidmonkey @JuneStepp