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.

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

Update foregroundServiceType to systemExempted for VPN #25244

Merged
merged 1 commit into from
Sep 4, 2024
Merged

Update foregroundServiceType to systemExempted for VPN #25244

merged 1 commit into from
Sep 4, 2024

Conversation

deeppandya
Copy link
Contributor

@deeppandya deeppandya commented Aug 21, 2024 •
edited
Loading

Resolves brave/brave-browser#40571

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:

  1. We need to make sure that VPN tunnel works without any issues
  2. After turning the vpn on, we should make sure the connection stays connected for more than 6 hours.

@deeppandya deeppandya added CI/skip-macos-x64 Do not run CI builds for macOS x64 CI/skip-ios Do not run CI builds for iOS CI/skip-windows-x64 Do not run CI builds for Windows x64 CI/skip-macos-arm64 Do not run CI builds for macOS arm64 labels Aug 21, 2024
@deeppandya deeppandya added this to the 1.71.x - Nightly milestone Aug 21, 2024
@deeppandya deeppandya self-assigned this Aug 21, 2024
deeppandya
deeppandya commented Aug 21, 2024
View reviewed changes
android/java/AndroidManifest.xml
@@ -181,7 +181,7 @@

<service
android:name="org.chromium.chrome.browser.vpn.wireguard.WireguardService"
android:foregroundServiceType="dataSync"
android:foregroundServiceType="systemExempted"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://developer.android.com/about/versions/14/changes/fgs-types-required#system-exempted

deeppandya
deeppandya commented Aug 21, 2024
View reviewed changes
android/java/AndroidManifest_user_permissions.xml
@@ -0,0 +1,2 @@
<uses-permission android:name="android.permission.FOREGROUND_SERVICE_SYSTEM_EXEMPTED" />
<uses-permission android:name="android.permission.USE_EXACT_ALARM"/>
Copy link
Contributor Author

@deeppandya deeppandya Aug 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

In the description, it says :

Reserved for system applications and specific system integrations, to continue to use foreground services.
To use this type, an app must meet **at least one of** the following criteria:

Which includes VPN apps. but
i had to add above permission because of
Error: foregroundServiceType:systemExempted requires permission:[android.permission.FOREGROUND_SERVICE_SYSTEM_EXEMPTED] AND any permission in list:[android.permission.SCHEDULE_EXACT_ALARM, android.permission.USE_EXACT_ALARM] [ForegroundServicePermission] <service android:name="org.chromium.chrome.browser.vpn.wireguard.WireguardService" android:foregroundServiceType="systemExempted" android:exported="false">

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can use https://developer.android.com/about/versions/14/changes/fgs-types-required#special-use. In that case, we need to explain and it may delay the release as reviewers would be manually checking the details.

Copy link
Member

Choose a reason for hiding this comment

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

that's really strange though that is requires some ALARM permission.

@SergeyZhukovsky
Copy link
Member

@deeppandya we need to open a security review as there are new permissions added.

@deeppandya
Copy link
Contributor Author

Yes creating now.

@SergeyZhukovsky
Copy link
Member

Otherwise it looks good to me and it seems that the new service type is the one that Google suggests to use for VPN apps.

SergeyZhukovsky
SergeyZhukovsky approved these changes Aug 21, 2024
View reviewed changes
Copy link
Member

@SergeyZhukovsky SergeyZhukovsky left a comment

Choose a reason for hiding this comment

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

++

@deeppandya
Copy link
Contributor Author

I have also verified the same service type with protonvpn and wireguard.

@SergeyZhukovsky
Copy link
Member

I have also verified the same service type with protonvpn and wireguard.

@deeppandya do they have any ALARM permission as well?

@deeppandya
Copy link
Contributor Author

I looked for the same in their repo but I couldn't find anything related to alarm. But for us, it was giving an error as I mentioned in previous comment. We can use specialUse in this case but need to provide more details.

@deeppandya
Copy link
Contributor Author

I have also verified the same service type with protonvpn and wireguard.

@deeppandya do they have any ALARM permission as well?

it might be because they are still using api 34 which is android 14. here : https://github.com/ProtonVPN/android-app/blob/fc9e7f500fe56bacfb2bf5247611fc6f5c082f69/app/build.gradle#L141C24-L141C30

@deeppandya deeppandya merged commit 745f2e5 into master Sep 4, 2024
23 checks passed
@deeppandya deeppandya deleted the vpn_foregroundservicetype_update_android branch September 4, 2024 20:00
brave-builds added a commit that referenced this pull request Sep 4, 2024
@brave-builds
Uplift of #25244 (squashed) to beta
63f3b60
brave-builds added a commit that referenced this pull request Sep 4, 2024
@brave-builds
Uplift of #25244 (squashed) to release
b14e1b9
This was referenced Sep 4, 2024
Closed
Closed
@deeppandya deeppandya mentioned this pull request Sep 5, 2024
Merged
@deeppandya deeppandya removed this from the 1.71.x - Nightly milestone Sep 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SergeyZhukovsky SergeyZhukovsky SergeyZhukovsky approved these changes

Assignees

@deeppandya deeppandya

Labels
CI/skip-ios Do not run CI builds for iOS CI/skip-macos-arm64 Do not run CI builds for macOS arm64 CI/skip-macos-x64 Do not run CI builds for macOS x64 CI/skip-windows-x64 Do not run CI builds for Windows x64
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Android] Update foregroundServiceType for VPN
2 participants
@deeppandya @SergeyZhukovsky