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

Make Brave-specific options configurable through Windows Group Policy templates #25710

Merged
merged 9 commits into from
Oct 11, 2024

Conversation

bsclifton
Copy link
Member

@bsclifton bsclifton commented Sep 24, 2024

Updates Brave's group policies to match the style that Chromium does.

Notable changes:

  • Group policies have been removed from src/brave/chromium_src/components/policy/tools/generate_policy_source.py and broken out to individual files under src/brave/components/policy/resources/templates/policy_definitions/BraveSoftware/. This matches the style upstream in Chromium. For example, see https://source.chromium.org/chromium/chromium/src/+/main:components/policy/resources/templates/policy_definitions/Accessibility/AccessibilityShortcutsEnabled.yaml
  • Group policies are now in a "group" called BraveSoftware.
  • New hook added to DEPS file. This copies the Brave specific group policies found in src/brave/components/policy/resources/templates/policy_definitions/ into the Chromium specific directory at ``src/components/policy/resources/templates/policy_definitions/`.
  • src/brave/chromium_src/components/policy/resources/policy_templates.py was added to intercept the loading of the policies.yaml file upstream and add our own group policies.

This new approach generates a policy_templates.zip and brave_policy_templates.zip which has all of the Brave specific group policy values present.

  • The windows/examples/brave.reg has examples for using all Brave specific registry keys
  • The windows/admx/brave.admx group policy file has the Brave specific entries in it

image

image

This approach also does generate macOS (example plist file) and Linux (example JSON file) resources with the Brave specific group policy values- but we are not using those anywhere. More info about those can be found here:
https://source.chromium.org/chromium/chromium/src/+/main:components/policy/BUILD.gn

Fixes brave/brave-browser#26502

Submitter Checklist:

  • I confirm that no security/privacy review is needed and no other type of reviews are needed, or that I have requested them
  • There is a ticket for my issue
  • Used Github auto-closing keywords in the PR description above
  • Wrote a good PR/commit description
  • Squashed any review feedback or "fixup" commits before merge, so that history is a record of what happened in the repo, not your PR
  • Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
  • Checked the PR locally:
    • npm run test -- brave_browser_tests, npm run test -- brave_unit_tests wiki
    • npm run presubmit wiki, npm run gn_check, npm run tslint
  • Ran git rebase master (if needed)

Reviewer Checklist:

  • A security review is not needed, or a link to one is included in the PR description
  • New files have MPL-2.0 license header
  • Adequate test coverage exists to prevent regressions
  • Major classes, functions and non-trivial code blocks are well-commented
  • Changes in component dependencies are properly reflected in gn
  • Code follows the style guide
  • Test plan is specified in PR before merging

After-merge Checklist:

Test Plan:

Generating the assets

Building yourself

  1. Be on Windows
  2. npm run build
  3. npm run build -- --target=brave/components/policy:pack_policy_templates

Trying out the policy

  1. Unzip brave_policy_templates.zip (in the out directory) to somewhere
  2. Open gpedit.msc and right click Administrative Templates and pick Add/Remove Templates
  3. Navigate into the directory you extracted brave_policy_templates.zip and add either the ADM or ADMX
  4. Look for the group policies - you should see all of the Chromium ones and then the Brave specific ones are going to be under the Brave Software settings
    image

@bsclifton bsclifton self-assigned this Sep 24, 2024
@bsclifton bsclifton requested review from a team as code owners September 24, 2024 21:37
@github-actions github-actions bot added CI/run-audit-deps Check for known npm/cargo vulnerabilities (audit_deps) puLL-Merge labels Sep 24, 2024
Copy link
Contributor

The security team is monitoring all repositories for certain keywords. This PR includes the word(s) "policy" and so security team members have been added as reviewers to take a look.

No need to request a full security review at this stage, the security team will take a look shortly and either clear the label or request more information/changes.

Notifications have already been sent, but if this is blocking your merge feel free to reach out directly to the security team on Slack so that we can expedite this check.

@bsclifton bsclifton force-pushed the bsc-brave-specific-templates branch from 95b648f to a380566 Compare September 25, 2024 00:04
@mihaiplesa mihaiplesa marked this pull request as draft September 26, 2024 13:02
@mihaiplesa
Copy link
Collaborator

@bsclifton what would be needed for changing the cloud.google.com URLs to ours?

@bsclifton
Copy link
Member Author

@mihaiplesa I'll check that out next

@bsclifton bsclifton force-pushed the bsc-brave-specific-templates branch from a32557c to d6a715c Compare October 1, 2024 20:19
@bsclifton bsclifton changed the title WIP - Make Brave-specific options configurable through Windows Group Policy templates Make Brave-specific options configurable through Windows Group Policy templates Oct 1, 2024
@bsclifton bsclifton marked this pull request as ready for review October 1, 2024 22:17
@bsclifton
Copy link
Member Author

@mihaiplesa I found the following places of interest:

  1. https://source.chromium.org/chromium/chromium/src/+/main:components/policy/tools/template_writers/writer_configuration.py;l=31;drc=1f2feb50362f0eda403283200fc2687f1150a753

This is one place to make the change. We'd want to have a similar URL / path before we changed it. Something like https://enterprise.brave.com/policies/. We might be able to upload the HTML that gets output in policy_templates.zip. Would be good to check that out

  1. https://source.chromium.org/chromium/chromium/src/+/main:components/policy/resources/webui/policy_base.ts;l=174-187;drc=0cf150a7e8038b9271462fd00de5f25287cc4ccf

This seems to be the webUI - basically if you navigate to brave://policy

I think we'd need to make the change in both places. Could we do that as a follow up?

@bsclifton bsclifton force-pushed the bsc-brave-specific-templates branch 2 times, most recently from 3c05d44 to af74527 Compare October 1, 2024 23:01
Copy link
Member

@kdenhartog kdenhartog left a comment

Choose a reason for hiding this comment

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

approving for js-deps-reviewers. No concerns with the changes to package.json.

Some other non-blocking questions that I think are worth considering/following up on:

  1. Do we want to add a policy for webTorrent and playlists (could be used to download internal videos)?
  2. Would it make sense to follow up with a group policy that configures controls over default lists/shields configurations?
  3. Do we have a support doc already written about how to configure and use this? I could see it being a useful link to supply to people who complain about having lots of features they don't want on (tell them how to configure a group policy for themselves by linking to the docs for this page)

@bsclifton
Copy link
Member Author

bsclifton commented Oct 2, 2024

@kdenhartog answers to questions 😄

  1. We could definitely add those types of group policies (web torrent, playlist). This PR is converting the ones we already have into a format that Chromium is using for their policies. Changing the way we format existing ones puts the right stuff into the generated assets (ex: Brave specific items now show - where they didn't before)
  2. We do have an existing group policy that can force shields up or down on a list of domains. We don't have a policy for filter lists - that is an interesting one. We could create an issue for this
  3. Here's what we have live at the moment: https://support.brave.com/hc/en-us/articles/360039248271-Group-Policy
    Feedback welcome! Basically, this captures the Brave specific ones we support now. It also has a policy_templates.zip download.. but unfortunately, that only has the Chromium policies and not the Brave ones. We do show how to do an example on Windows, macOS, and Linux on the page. Once this PR is merged, that policy_templates.zip will start including the Brave specific ones 😄 We can also update the help article text to mention this. I need to update anyways as SyncUrl was a recent one added which isn't captured yet

@mihaiplesa
Copy link
Collaborator

@mihaiplesa I found the following places of interest:

  1. https://source.chromium.org/chromium/chromium/src/+/main:components/policy/tools/template_writers/writer_configuration.py;l=31;drc=1f2feb50362f0eda403283200fc2687f1150a753

This is one place to make the change. We'd want to have a similar URL / path before we changed it. Something like https://enterprise.brave.com/policies/. We might be able to upload the HTML that gets output in policy_templates.zip. Would be good to check that out

  1. https://source.chromium.org/chromium/chromium/src/+/main:components/policy/resources/webui/policy_base.ts;l=174-187;drc=0cf150a7e8038b9271462fd00de5f25287cc4ccf

This seems to be the webUI - basically if you navigate to brave://policy

I think we'd need to make the change in both places. Could we do that as a follow up?

@bsclifton I was thinking we can have something else before https://enterprise.brave.com/policies/ gets rolled out. Basically just replace https://chromeenterprise.google/policies/ with https://support.brave.com/hc/en-us/articles/360039248271-Group-Policy/ for now. And the rest as a follow-up.

DEPS Outdated Show resolved Hide resolved
build/util/update_group_policy.py Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
Copy link
Member

@goodov goodov left a comment

Choose a reason for hiding this comment

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

lgtm with few things to improve

Copy link
Contributor

[puLL-Merge] - brave/brave-core@25710

Description

This PR introduces significant changes to the policy management system in Brave, including the addition of new Brave-specific policies and modifications to the policy template generation process. The changes aim to provide more granular control over Brave-specific features through group policies.

Changes

Changes

  1. chromium_src/components/policy/resources/policy_templates.py:

    • Removed a large block of code that previously added Brave policies manually.
    • Added a single line to set the CHROMIUM_POLICY_KEY for Brave.
  2. chromium_src/components/policy/tools/generate_policy_source.py:

    • Removed the AddBravePolicies function and related code.
    • Simplified the file to only include the CHROMIUM_POLICY_KEY constant.
  3. components/policy/BUILD.gn:

    • Added dependencies for Brave-specific policy template generation.
  4. components/policy/pack_policy_templates.py:

    • Updated script to prepare Brave-specific version of policy_templates.zip.
    • Added comments explaining the purpose and usage of the script.
  5. Added new policy definition files under components/policy/resources/templates/policy_definitions/BraveSoftware/:

    • .group.details.yaml: Defines the Brave Software settings group.
    • Various policy YAML files (e.g., BraveAIChatEnabled.yaml, BraveRewardsDisabled.yaml, etc.): Define specific Brave policies.
  6. Added components/policy/resources/templates/policy_definitions/README.md:

    • Provides documentation on how to add new policies in Brave.
  7. Added patches:

    • patches/components-policy-resources-policy_templates.py.patch: Adds Brave-specific override to the policy template generation.
    • patches/components-policy-resources-webui-policy_base.ts.patch: Updates the policy documentation URL to point to Brave's support page.

Possible Issues

  1. The removal of the manual policy addition in policy_templates.py and generate_policy_source.py might lead to unintended consequences if any part of the system still relies on the old method of policy definition.

  2. The changes to the build process and policy template generation might require updates to documentation or build scripts that are not included in this PR.

Security Hotspots

No significant security issues are apparent in this change. However, careful review of the new policy definitions is recommended to ensure they don't inadvertently expose sensitive functionality or information.

@bsclifton
Copy link
Member Author

bsclifton commented Oct 11, 2024

Made final changes - thanks @goodov, @mherrmann, and @diracdeltas and everyone who helped review 😄👍

Added test plan to top of brave/brave-browser#26502

@bsclifton bsclifton merged commit ee5c512 into master Oct 11, 2024
17 checks passed
@bsclifton bsclifton deleted the bsc-brave-specific-templates branch October 11, 2024 19:22
@github-actions github-actions bot added this to the 1.73.x - Nightly milestone Oct 11, 2024
@brave-builds
Copy link
Collaborator

Released in v1.73.8

@bsclifton bsclifton modified the milestones: 1.73.x - Nightly, 1.74.x - Nightly Nov 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI/run-audit-deps Check for known npm/cargo vulnerabilities (audit_deps) puLL-Merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make Brave-specific options configurable through Windows Group Policy templates
10 participants