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

uses-permissions in AndroidManifest.xml are not exported to App #5411

Closed
steeve opened this issue Jun 15, 2018 · 3 comments
Closed

uses-permissions in AndroidManifest.xml are not exported to App #5411

steeve opened this issue Jun 15, 2018 · 3 comments
Assignees
Labels
P2 We'll consider working on this in future. (Assignee optional) team-Android Issues for Android team type: feature request

Comments

@steeve
Copy link
Contributor

steeve commented Jun 15, 2018

Description of the problem / feature request:

As per the documentation, uses-permissions attributes are never exported to the main app. What is the rationale behind this decision?
Is there any way to have an android_library export its permissions to an app that includes it ?

Thanks!

What operating system are you running Bazel on?

macOS

What's the output of bazel info release?

release 0.14.1-homebrew

Have you found anything relevant by searching the web?

They are manually removed here:

private static final String[] PERMISSION_TAGS =
new String[] {"uses-permission", "uses-permission-sdk-23"};
private static final StdLogger stdLogger = new StdLogger(StdLogger.Level.WARNING);
private static final Logger logger = Logger.getLogger(ManifestMergerAction.class.getName());
private static Options options;
private static Path removePermissions(Path manifest, Path outputDir)
throws IOException, ParserConfigurationException, TransformerConfigurationException,
TransformerException, TransformerFactoryConfigurationError, SAXException {
DocumentBuilder docBuilder = DocumentBuilderFactory.newInstance().newDocumentBuilder();
Document doc = docBuilder.parse(manifest.toFile());
for (String tag : PERMISSION_TAGS) {
NodeList permissions = doc.getElementsByTagName(tag);
if (permissions != null) {
for (int i = permissions.getLength() - 1; i >= 0; i--) {
Node permission = permissions.item(i);
permission.getParentNode().removeChild(permission);
}
}
}
// Write resulting manifest to the output directory, maintaining full path to prevent collisions
Path output = outputDir.resolve(manifest.toString().replaceFirst("^/", ""));
Files.createDirectories(output.getParent());
TransformerFactory.newInstance()
.newTransformer()
.transform(new DOMSource(doc), new StreamResult(output.toFile()));
return output;
}

@jin
Copy link
Member

jin commented Jun 15, 2018

I have no idea - @asteinb @ahumesky?

@ahumesky
Copy link
Contributor

It's mostly because changing the permissions of your app prevents auto-updating on the play store.

If you have a code base where there are many apps that are independently owned by different teams, and you have many libraries that are similarly independently owned by other teams, and if you inherit permissions from your dependencies, then it means that other teams can change the permissions of your app without your permission or noticing. Another way to put it is that app owners should own the permissions their app asks for. If a new permission gets by and ends up in a release, then your app can't be auto-updated.

One possible response is that this could instead be taken care of in a test where the test asserts the set of permissions that the app has. But this results in basically the same situation: a flat list somewhere of the permissions the app is allowed, just in a test rather than the top-level manifest.

That said, it should be possible to add an attribute to android_binary that will enable permissions inheritance. It should also be possible to write a skylark aspect that traverses the dependencies of an android_binary that will at least give you all the permissions from the dependencies.

@ahumesky ahumesky added type: feature request P2 We'll consider working on this in future. (Assignee optional) and removed under investigation labels Jun 15, 2018
@jin jin added team-Android Issues for Android team and removed category: rules > android labels Aug 11, 2018
@steeve
Copy link
Contributor Author

steeve commented Jan 10, 2019

I'm closing this as the rationale makes sense.

@steeve steeve closed this as completed Jan 10, 2019
bazel-io pushed a commit that referenced this issue Apr 5, 2022
Adding support for conditionally merging `uses-permissions`.

#10628
#5411

Closes #13445.

RELNOTES: Enable merging permissions during Android manifest merging with the --merge_android_manifest_permissions flag.
PiperOrigin-RevId: 439613035
ted-xie pushed a commit to ted-xie/bazel that referenced this issue Jul 6, 2022
Adding support for conditionally merging `uses-permissions`.

bazelbuild#10628
bazelbuild#5411

Closes bazelbuild#13445.

RELNOTES: Enable merging permissions during Android manifest merging with the --merge_android_manifest_permissions flag.
PiperOrigin-RevId: 439613035
ted-xie pushed a commit to ted-xie/bazel that referenced this issue Jul 6, 2022
Adding support for conditionally merging `uses-permissions`.

bazelbuild#10628
bazelbuild#5411

Closes bazelbuild#13445.

RELNOTES: Enable merging permissions during Android manifest merging with the --merge_android_manifest_permissions flag.
PiperOrigin-RevId: 439613035
ted-xie pushed a commit to ted-xie/bazel that referenced this issue Jul 6, 2022
Adding support for conditionally merging `uses-permissions`.

bazelbuild#10628
bazelbuild#5411

Closes bazelbuild#13445.

RELNOTES: Enable merging permissions during Android manifest merging with the --merge_android_manifest_permissions flag.
PiperOrigin-RevId: 439613035
ckolli5 pushed a commit that referenced this issue Jul 6, 2022
* Support uses-permission merging in manifest merger

Adding support for conditionally merging `uses-permissions`.

#10628
#5411

Closes #13445.

RELNOTES: Enable merging permissions during Android manifest merging with the --merge_android_manifest_permissions flag.
PiperOrigin-RevId: 439613035

* Make ManifestMergerAction worker compatible

Calling `System#exit` kills the worker during the build. Passing the exception up to the worker should be enough for it to end up in the worker or local execution output.

Closes #14427.

PiperOrigin-RevId: 447808701

* Fix ManifestMergerAction test case on windows

`manifest.toString().replaceFirst("^/", "")` silently fails on windows machines causing `removePermissions` to write to the original test file. This pull request creates a new temp file that `removePermissions` can write the modified manifest to.

Pulling this change out of another PR so that it's easier to merge. Original PR here https://github.com/bazelbuild/bazel/pull/13445/files#r631575251

Closes #13760.

PiperOrigin-RevId: 438643774

Co-authored-by: Ben Lee <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P2 We'll consider working on this in future. (Assignee optional) team-Android Issues for Android team type: feature request
Projects
None yet
Development

No branches or pull requests

3 participants