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

fix permissions for ~ android 11 #2732

Merged
merged 11 commits into from
Oct 12, 2023
Merged

fix permissions for ~ android 11 #2732

merged 11 commits into from
Oct 12, 2023

Conversation

r10s
Copy link
Member

@r10s r10s commented Oct 9, 2023 •
edited
Loading

fixes #2726

  • fix import backup from welcome activity on android 11
  • check other file access points on android 11
  • check other androids (i assume, android10 is same as android11, so the change should be at sdk29 already, but that needs to be double checked and tested)
    • EDIT: tried at least import-after-fresh-install, export-image, camera-roll on:
      • android5 (sdk21): ok
      • android9 (sdk28): ok
      • android10 (sdk29): ok
      • android11 (sdk30): ok
      • android12 (sdk31): cannot test, emulator issues
      • android12 (sdk32): ok
      • android13 (sdk33): ok
    • EDIT: tried backup-after-fresh-install-without-any-rights-granted:
      • android5 (sdk21): ok
      • android9 (sdk28): ok
      • android10 (sdk29): ok
      • android11 (sdk30): ok
      • android12 (sdk32): ok
      • android13 (sdk33): ok

other file access points are:

  • export media from chat
  • export media from gallery
  • export keys
  • export log
  • export backup
  • import key
  • import backup on welcome screen

@r10s r10s added the bug label Oct 9, 2023
@r10s r10s changed the title fix permissions for sdk29 - sdk 32 fix permissions for ~ android 11 Oct 9, 2023
@github-actions
Copy link

github-actions bot commented Oct 9, 2023

To test the changes in this pull request, install this apk:
📦 app-preview.apk

@r10s r10s force-pushed the fix-android-11-file-access branch 2 times, most recently from d4a03fe to a213fe8 Compare October 9, 2023 22:25
@github-actions
Copy link

github-actions bot commented Oct 9, 2023

To test the changes in this pull request, install this apk:
📦 app-preview.apk

@adbenitez
Copy link
Member

about Android 10:
https://stackoverflow.com/questions/64221188/write-external-storage-when-targeting-android-10
https://developer.android.com/about/versions/10/privacy/changes#scoped-storage

@r10s r10s added the blocker label Oct 12, 2023
@r10s r10s requested a review from adbenitez October 12, 2023 10:48
@r10s r10s marked this pull request as ready for review October 12, 2023 10:48
@r10s
Copy link
Member Author

r10s commented Oct 12, 2023

seems as if sdk29 is fine already. so, this pr really fixes only android11 on-point, from the logic, the older androids should be unchanged

@r10s
Copy link
Member Author

r10s commented Oct 12, 2023

@adbenitez for logic review, i think, that paths of other android-versions are not changed, seems calming

@r10s r10s marked this pull request as draft October 12, 2023 11:15
@r10s r10s force-pushed the fix-android-11-file-access branch from a213fe8 to 18b0135 Compare October 12, 2023 11:18
@github-actions
Copy link

To test the changes in this pull request, install this apk:
📦 app-preview.apk

@adbenitez
Copy link
Member

back to draft: android12 seems special ... ooh no, was on a wrong branch :) have to check the things again ...

oh :( talk about android fragmentation

@r10s r10s marked this pull request as ready for review October 12, 2023 12:10
@r10s
Copy link
Member Author

r10s commented Oct 12, 2023

so, finally, this is ready for review; i double checked the android versions mentioned above ...

adbenitez
adbenitez reviewed Oct 12, 2023
View reviewed changes
src/org/thoughtcrime/securesms/permissions/Permissions.java
@@ -100,6 +102,20 @@ public PermissionsBuilder ifNecessary(boolean condition) {
return this;
}

public PermissionsBuilder alwaysGrantOnSdk30() {
Copy link
Member

@adbenitez adbenitez Oct 12, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I would just have a generic function alwaysGrantOnSdk(int version) and then call Permissions.alwaysGrantOnSdk(30) etc. but is fine as it is as well

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah, let's leave this as is for now. it looks more explicit that 33/30 is no typo in a parameter :)

@github-actions
Copy link

To test the changes in this pull request, install this apk:
📦 app-preview.apk

adbenitez
adbenitez approved these changes Oct 12, 2023
View reviewed changes
Copy link
Member

@adbenitez adbenitez left a comment

Choose a reason for hiding this comment

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

great! thanks for fixing and enduring having to try in all that emulators! overall the code looks good, I can't test it myself other than in sdk 33

@r10s
Copy link
Member Author

r10s commented Oct 12, 2023

quite some stuff :)

Screenshot 2023-10-12 at 15 49 41

@r10s r10s merged commit 8c93155 into main Oct 12, 2023
2 checks passed
@r10s r10s deleted the fix-android-11-file-access branch October 12, 2023 14:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adbenitez adbenitez adbenitez approved these changes

Assignees
No one assigned
Labels
blocker bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Cannot grant permission to access storage (cannot restore backups) on Android11 (maybe others)
2 participants
@r10s @adbenitez