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

[camerax] Revert "Explicitly remove READ_EXTERNAL_STORAGE permission" #7826

Merged
merged 5 commits into from
Oct 9, 2024

Conversation

camsim99
Copy link
Contributor

@camsim99 camsim99 commented Oct 8, 2024

Reverts logic to remove READ_EXTERNAL_STORAGE permission from merged Android manifest (between plugins and Flutter app) by default. Turns out that this logic conflicts with Flutter plugins/apps that actually require the READ_EXTERNAL_STORAGE permission.

Specifically, reverts #4716 and adds documentation so that users know how to manually remove READ_EXTERNAL_STORAGE from the merged manifest if they wish.

Fixes flutter/flutter#156198 and adds documentation to address flutter/flutter#131116.

Pre-launch Checklist

@camsim99 camsim99 requested a review from a team October 8, 2024 21:27
<uses-permission android:name="android.permission.READ_EXTERNAL_STORAGE"
tools:node="remove" />
```

Copy link
Contributor

Choose a reason for hiding this comment

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

Please link to the documentation for camerax and the reasons for why this permission is included.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is linked above this part:

In order to save captured images and videos to files on Android 10 and below, CameraX
requires specifying the `WRITE_EXTERNAL_STORAGE` permission (see [the CameraX documentation][10]).
This is already done in the plugin, so no further action is required on your end. To understand
the implications of specificying this permission, see [the `WRITE_EXTERNAL_STORAGE` documentation][11].

Is that sufficient?

Copy link
Contributor

Choose a reason for hiding this comment

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

"To understand the implications of specificying(sp?) this permission, see [the WRITE_EXTERNAL_STORAGE documentation][11]."

Is technically correct but not as useful. Developers like the original reporter of flutter/flutter#131116 are being asked by their legal team to fill out a justification for a permission. One they didnt add do not actually call.

I think it should be changed to. "If you want to understand the privacy impact of WRITE_EXTERNAL_STORAGE see see [the WRITE_EXTERNAL_STORAGE documentation][11]. We have seen apps also have READ_EXTERNAL_STORAGE permission automatically added this appears to be implied from WRITE_EXTERNAL_STORAGE."

@camsim99 camsim99 added the autosubmit Merge PR when tree becomes green via auto submit App label Oct 9, 2024
@auto-submit auto-submit bot merged commit 345fa98 into flutter:main Oct 9, 2024
76 checks passed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 10, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 10, 2024
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Oct 10, 2024
flutter/packages@9d00fb1...f1a3da2

2024-10-09 matanlurey@users.noreply.github.com Remove additional (harmless but annoying) native stack traces. (flutter/packages#7837)
2024-10-09 engine-flutter-autoroll@skia.org Roll Flutter from 0917e9d to 2d45fb3 (50 revisions) (flutter/packages#7836)
2024-10-09 109111084+yaakovschectman@users.noreply.github.com [camera_android] Begin conversion to Pigeon (flutter/packages#7755)
2024-10-09 109111084+yaakovschectman@users.noreply.github.com [google_maps_flutter_android] Add `PlatformCap` pigeon type. (flutter/packages#7639)
2024-10-09 43054281+camsim99@users.noreply.github.com [camerax] Revert "Explicitly remove READ_EXTERNAL_STORAGE permission" (flutter/packages#7826)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-packages-flutter-autoroll
Please CC flutter-ecosystem@google.com on the revert to ensure that a human
is aware of the problem.

To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autosubmit Merge PR when tree becomes green via auto submit App p: camera platform-android
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[camerax]: Read External Storage defined in another plugin gets rejected from mergedManifest
2 participants