Do not ask for storage permissions when it's a fresh installation #4297

grzesiek2010 · 2020-12-11T17:11:14Z

What has been done to verify that this works as intended?

I tested the behavior.

Why is this the best possible solution? Were any other approaches considered?

I decided to call requestStoragePermissions() like before but add one trick which is here:
https://github.com/getodk/collect/pull/4297/files#diff-fe2bc5034d6dfe946ff681f198d0552c83038caed992865f5e7ce030447a276bR98

Another option would be to check if we need storage permission before calling requestStoragePermissions() but that would require adding more if/else statements and the code would be more cluttered.

How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?

Fresh installs should not ask for storage permission that's the only change. In order to confirm that everything works fine it would be good to go through some functionalities where normally we would be asked for permissions.

Do we need any specific form for testing your changes? If so, please attach one.

No.

Does this change require updates to documentation? If so, please file an issue here and include the link below.

No.

Before submitting this PR, please make sure you have:

run ./gradlew checkAll and confirmed all checks still pass OR confirm CircleCI build passes and run ./gradlew connectedDebugAndroidTest locally.
verified that any code or assets from external sources are properly credited in comments and/or in the about file.
verified that any new UI elements use theme colors. UI Components Style guidelines

lognaturel

Would be great to get some testing. Maybe just a small test for areStoragePermissionsGranted that verifies that true is always returned if scoped storage is enabled?

I like what you've done with PermissionUtils. I'm wondering whether it's also a good opportunity to rename it to maybe PermissionsRequester or PermissionsProvider?

grzesiek2010 · 2021-01-08T09:51:44Z

Would be great to get some testing

I thought the same at the end of my day yesterday. I'll try to add something valuable.

grzesiek2010 · 2021-01-11T13:15:22Z

Would be great to get some testing. Maybe just a small test for areStoragePermissionsGranted that verifies that true is always returned if scoped storage is enabled?

I like what you've done with PermissionUtils. I'm wondering whether it's also a good opportunity to rename it to maybe PermissionsRequester or PermissionsProvider?

It's ready, I added some tests and implemented code refactoring.

collect_app/src/test/java/org/odk/collect/android/permissions/PermissionsProviderTest.java

lognaturel

Looks good other than the main test... 🙃 Feel free to mark as needs testing once that's fixed and conflicts are resolved.

…e permission

…lass

lognaturel · 2021-01-21T16:30:51Z

collect_app/src/main/java/org/odk/collect/android/dao/helpers/ContentResolverHelper.java

+    public static String getFileExtensionFromUri(Uri fileUri) {
+        String mimeType = getContentResolver().getType(fileUri);
+
+        String extension = fileUri.getScheme() != null && fileUri.getScheme().equals(ContentResolver.SCHEME_CONTENT)


Wouldn't we always be dealing with content URIs here? In what cases would we have file URIs?

Or was that the selfie video case again?

This was the fix added for a crash some time ago f98b98d so I wanted to keep it just in case seeing how other cases might be tricky.

Got it. Would be great to follow up with #2834 because this method has gone through lots of big changes during this PR that probably would have been avoided with tests.

grzesiek2010 · 2021-01-21T17:13:06Z

@lognaturel Generally I think it's ready for testing unless you see something important.

lognaturel

This has turned into an adventure! Good to see so much code going away.

Lots of little things to check around the media selection questions like are temporary files cleaned up and all the cases around MIME types and file extensions I listed earlier.

grzesiek2010 · 2021-01-25T13:29:21Z

I added some improvements to handle SecurityException which is thrown when read permission is no granted to the returned uri. I was even able to reproduce it on an emulator Api 25 where I can't record video because apparently the default app doesn't grant read permission to returned uri. Please keep an eye on it during testing.

lognaturel · 2021-01-25T22:45:51Z

I was even able to reproduce it on an emulator Api 25

Nice. But does this mean that recording video with that default app would never work? I wonder if this is going to be a broader issue -- are media apps for older OS versions not going to work because they expect the blanket read permissions to be given?

grzesiek2010 · 2021-01-26T10:51:20Z

Nice. But does this mean that recording video with that default app would never work? I wonder if this is going to be a broader issue -- are media apps for older OS versions not going to work because they expect the blanket read permissions to be given?

I tested emulators with apis 22-30 and noticed that problem on api 23 and 25. In those cases recording is not possible with default camera apps. Granting storage permission fixes the issue but I thought it was a bug in those camera apps. In the doc where recording videos is described https://developer.android.com/training/camera/videobasics there is nothing about storage permission and we use exactly that way.

Apparently they love to confuse people.

I also read https://developer.android.com/training/data-storage/shared/media and https://developer.android.com/training/data-storage/shared/documents-files and my understanding is that we shouldn't need that permissions.

mmarciniak90 · 2021-01-26T13:18:48Z

Tested with success

Verified on Android 7.0, 8.1, 10.0

Verified cases:

Image widgets
- choose image from Image directory
- choose image from Downloads
- choose image from Google Drive
- take new photo
- Camera permissions
Audio widget
- choose audio file from Audio directory
- choose audio file from Downloads
- choose audio file from Google Drive
- record new audio
- Microphone permissions
Video widget
- choose video file from Video directory
- choose video file from Downloads
- choose video file from Google Drive
- record new video
- Camera and Microphone permissions
File widget
- choose any file for file widget from Images/Videos/Audio
- choose any file for file widget from Downloads
- choose any file for file widget from Google Drive
- verified file types: amr, ogg, m4a, mp3, 3gp, gif, jpeg, jpg, png, pdf, doc, docx, txt, xls, xlsx, csv, xml, rtf, zip, rar files
Location permissions
Contact permissions
Telephone permissions
enabled storage permission manually
fresh installation
import settings using a qr code file
import settings from .settings / json file
custom splash screen images - file saved in /storage/emulated/0/Android/data/org.odk.collect.android/files
upgrade when storage permission is enabled
upgrade when storage permission is disabled
- if files are migrated, storage permissions are not required
- if files are not migrated, storage permissions are required
jpg with the correct MIME type but no extension (internal storage and GD) - verified on images widgets and file widget
text file with extension .fake and the correct MIME type (internal storage and GD) - verified on file widget

kkrawczyk123 · 2021-01-26T13:22:29Z

Verified also on Androids 5.1, 7.1 and 9.0.

@getodk-bot unlabel "needs testing"
@getodk-bot label "behavior verified"

grzesiek2010 · 2021-01-26T13:25:13Z

Regarding that problems with permissions: If it's not our fault bug external apps that do not grant uri permission we may need to consider adding something like: 17431bb

grzesiek2010 force-pushed the COLLECT-4288 branch 2 times, most recently from 4289b3a to fc3b134 Compare January 7, 2021 13:52

grzesiek2010 marked this pull request as ready for review January 7, 2021 14:10

lognaturel reviewed Jan 7, 2021

View reviewed changes

grzesiek2010 added this to the v1.30 milestone Jan 11, 2021

grzesiek2010 force-pushed the COLLECT-4288 branch from fc3b134 to cfa08fe Compare January 11, 2021 13:12

grzesiek2010 requested a review from lognaturel January 11, 2021 13:15

lognaturel reviewed Jan 13, 2021

View reviewed changes

collect_app/src/test/java/org/odk/collect/android/permissions/PermissionsProviderTest.java Outdated Show resolved Hide resolved

lognaturel requested changes Jan 13, 2021

View reviewed changes

grzesiek2010 added 19 commits January 14, 2021 11:19

Implemented AppStateProvider

a59690c

Enable scoped storage for fresh installs

3466788

Don't ask for storage permission if scoped storage is used

f154afd

Use injected permissionUtils

f519e67

Pass storageStateProvider to PermissionUtils

c34f2fa

Use storageStateProvider object to determine if should ask for storag…

ab274f1

…e permission

areStoragePermissionsGranted() shoul not be static

0bafab9

Fixed areStoragePermissionsGranted()

0040fea

Removed redundant code from providers

9cbb76a

Fixed tests

662430b

Call Dexter.withContext instead of deprecated withActivity

b72331c

Removed redundant finishAllActivities() method from PermissionUtils

65705f9

Got rid of static methods in PermissionUtils class

bbd812e

Implemented PermissionsChecker

d00b793

Do not pass dialogTheme to PermissionUtils constructor

0c5b50d

Moved permissionUtils class to permissions package

265c183

Renamed PermissionUtils -> PermissionsProvider

e7d9782

Removed duplicated AlwaysGrantStoragePermissionsPermissionsProvider c…

2ca5372

…lass

Added tests

d1aa0d3

grzesiek2010 added 2 commits January 21, 2021 12:11

Improved getFileExtensionFromUri() methdo

8894046

Fixed pmd

ecbf1d5

grzesiek2010 force-pushed the COLLECT-4288 branch from f702b0b to ecbf1d5 Compare January 21, 2021 11:49

grzesiek2010 added 2 commits January 21, 2021 13:30

Fixed ArbitraryFileWidgetTest

08e3c9e

Fixed VideoWidgetTest

c6f335c

lognaturel reviewed Jan 21, 2021

View reviewed changes

Fixed AudioWidget

d23c73d

grzesiek2010 force-pushed the COLLECT-4288 branch from 714a264 to d23c73d Compare January 21, 2021 16:33

lognaturel approved these changes Jan 21, 2021

View reviewed changes

lognaturel added the needs testing label Jan 21, 2021

grzesiek2010 added 5 commits January 25, 2021 13:53

Check Uri permission before accesing it

bdfce87

Check Uri permission before accesing it in ODKView

57909eb

Improved tests for intent group

cb1a1a6

Moved the new message about not granted file permission to strings

7f9a90a

Added tests to PermissionsProviderTest

8ace0bf

grzesiek2010 force-pushed the COLLECT-4288 branch from a580e74 to 67b416c Compare January 26, 2021 12:22

Improved isReadUriPermissionGranted() method

bb23041

grzesiek2010 force-pushed the COLLECT-4288 branch from 67b416c to bb23041 Compare January 26, 2021 12:50

getodk-bot added behavior verified and removed needs testing labels Jan 26, 2021

grzesiek2010 merged commit 6f0c92f into getodk:master Jan 26, 2021

grzesiek2010 mentioned this pull request Jan 26, 2021

Handle uris with not granted read permission while collecting media files #4360

Closed

grzesiek2010 mentioned this pull request Feb 11, 2021

Improved handling results from text widgets #4406

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do not ask for storage permissions when it's a fresh installation #4297

Do not ask for storage permissions when it's a fresh installation #4297

grzesiek2010 commented Dec 11, 2020 •

edited

Loading

lognaturel left a comment

grzesiek2010 commented Jan 8, 2021

grzesiek2010 commented Jan 11, 2021

lognaturel left a comment •

edited

Loading

lognaturel Jan 21, 2021

lognaturel Jan 21, 2021

grzesiek2010 Jan 21, 2021

lognaturel Jan 21, 2021

grzesiek2010 commented Jan 21, 2021

lognaturel left a comment •

edited

Loading

grzesiek2010 commented Jan 25, 2021

lognaturel commented Jan 25, 2021

grzesiek2010 commented Jan 26, 2021 •

edited

Loading

mmarciniak90 commented Jan 26, 2021

kkrawczyk123 commented Jan 26, 2021

grzesiek2010 commented Jan 26, 2021

Do not ask for storage permissions when it's a fresh installation #4297

Do not ask for storage permissions when it's a fresh installation #4297

Conversation

grzesiek2010 commented Dec 11, 2020 • edited Loading

What has been done to verify that this works as intended?

Why is this the best possible solution? Were any other approaches considered?

How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?

Do we need any specific form for testing your changes? If so, please attach one.

Does this change require updates to documentation? If so, please file an issue here and include the link below.

Before submitting this PR, please make sure you have:

lognaturel left a comment

Choose a reason for hiding this comment

grzesiek2010 commented Jan 8, 2021

grzesiek2010 commented Jan 11, 2021

lognaturel left a comment • edited Loading

Choose a reason for hiding this comment

lognaturel Jan 21, 2021

Choose a reason for hiding this comment

lognaturel Jan 21, 2021

Choose a reason for hiding this comment

grzesiek2010 Jan 21, 2021

Choose a reason for hiding this comment

lognaturel Jan 21, 2021

Choose a reason for hiding this comment

grzesiek2010 commented Jan 21, 2021

lognaturel left a comment • edited Loading

Choose a reason for hiding this comment

grzesiek2010 commented Jan 25, 2021

lognaturel commented Jan 25, 2021

grzesiek2010 commented Jan 26, 2021 • edited Loading

mmarciniak90 commented Jan 26, 2021

kkrawczyk123 commented Jan 26, 2021

grzesiek2010 commented Jan 26, 2021

grzesiek2010 commented Dec 11, 2020 •

edited

Loading

lognaturel left a comment •

edited

Loading

lognaturel left a comment •

edited

Loading

grzesiek2010 commented Jan 26, 2021 •

edited

Loading