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

Replace EXTERNAL_STORAGE permissions on android api level 33+ #5314

Conversation

sabercodic
Copy link
Contributor

@sabercodic sabercodic commented Oct 17, 2023

Add new permission instead of READ/WRITE _EXTERNAL_STORAGE on android api level 33+.

This change is Reviewable

@sabercodic sabercodic added the Android Issues related to Android label Oct 17, 2023
@linear
Copy link

linear bot commented Oct 17, 2023

DROID-367 Remove WRITE_EXTERNAL_STORAGE permission for android 13+

READ/WRITE EXTERNAL_STORAGE is used in two places:

  • "test" module use this permissions to access screenshots folder and sqlite files
  • "app" module on debug mode

@sabercodic sabercodic force-pushed the remove-write_external_storage-permission-for-android-13-droid-367 branch 2 times, most recently from b1a9f17 to a77e10c Compare October 18, 2023 07:19
@sabercodic sabercodic marked this pull request as ready for review October 27, 2023 13:23
@sabercodic sabercodic force-pushed the remove-write_external_storage-permission-for-android-13-droid-367 branch from a77e10c to 0ab9a7d Compare October 27, 2023 13:25
Copy link
Collaborator

@albin-mullvad albin-mullvad left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 4 files reviewed, 1 unresolved discussion (waiting on @sabercodic)


android/test/e2e/src/main/kotlin/net/mullvad/mullvadvpn/test/e2e/EndToEndTest.kt line 29 at r1 (raw file):

    val permissionRule: GrantPermissionRule =
        if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.TIRAMISU) {
            GrantPermissionRule.grant(Manifest.permission.READ_MEDIA_IMAGES)

This doesn't look correct since:

  • We don't use storage for images.
  • Any change like this would required some sort of data migration logic. Or is that not the case?

Code quote:

READ_MEDIA_IMAGES

Copy link
Collaborator

@albin-mullvad albin-mullvad left a comment

Choose a reason for hiding this comment

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

Is it for 13+ or 14+? The Linear issue and PR says different things.

Reviewed 4 of 4 files at r1.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @sabercodic)


-- commits line 2 at r1:
Can you clarify what this fixes in the commit message?

Code quote:

Fix InlinedApi lint issue

android/test/e2e/src/main/kotlin/net/mullvad/mullvadvpn/test/e2e/EndToEndTest.kt line 29 at r1 (raw file):

Previously, albin-mullvad wrote…

This doesn't look correct since:

  • We don't use storage for images.
  • Any change like this would required some sort of data migration logic. Or is that not the case?

Sorry NVM, I confused this with also being used outside of the e2e/debug code. :lgtm:

Copy link
Collaborator

@albin-mullvad albin-mullvad left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @sabercodic)

a discussion (no related file):
Has this been verified to work on both newer and older API levels? Our tests run on API levels back to version 26.



-- commits line 2 at r1:

Previously, albin-mullvad wrote…

Can you clarify what this fixes in the commit message?

Also clarify the PR title 🙏

Copy link
Collaborator

@albin-mullvad albin-mullvad left a comment

Choose a reason for hiding this comment

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

Reviewed all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @sabercodic)

@sabercodic sabercodic changed the title Fix InlinedApi lint issue Replace EXTERNAL_STORAGE permissions on android api level 33+ Nov 13, 2023
@sabercodic sabercodic force-pushed the remove-write_external_storage-permission-for-android-13-droid-367 branch from 0ab9a7d to dc446ba Compare November 13, 2023 10:54
Copy link
Contributor Author

@sabercodic sabercodic left a comment

Choose a reason for hiding this comment

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

13+

Reviewable status: 3 of 4 files reviewed, 2 unresolved discussions (waiting on @albin-mullvad)


-- commits line 2 at r1:

Previously, albin-mullvad wrote…

Also clarify the PR title 🙏

Done.

Copy link
Collaborator

@albin-mullvad albin-mullvad left a comment

Choose a reason for hiding this comment

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

I also updated the PR description since it was outdated

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @sabercodic)

Copy link
Collaborator

@albin-mullvad albin-mullvad left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @sabercodic)

a discussion (no related file):

Previously, albin-mullvad wrote…

Has this been verified to work on both newer and older API levels? Our tests run on API levels back to version 26.

@sabercodic ping! Did you verify that this works as expected both on the range of supported API levels?


Copy link
Collaborator

@albin-mullvad albin-mullvad left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @sabercodic)

a discussion (no related file):

Previously, albin-mullvad wrote…

@sabercodic ping! Did you verify that this works as expected both on the range of supported API levels?

^ both


@sabercodic sabercodic force-pushed the remove-write_external_storage-permission-for-android-13-droid-367 branch 3 times, most recently from 1ad4f0f to 80f9050 Compare December 4, 2023 12:59
Copy link
Collaborator

@albin-mullvad albin-mullvad left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @sabercodic)

a discussion (no related file):

Previously, albin-mullvad wrote…

^ both

@sabercodic ping!


@sabercodic sabercodic force-pushed the remove-write_external_storage-permission-for-android-13-droid-367 branch from 80f9050 to db99a38 Compare December 13, 2023 12:10
Copy link
Contributor Author

@sabercodic sabercodic left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @albin-mullvad)

a discussion (no related file):

Previously, albin-mullvad wrote…

@sabercodic ping!

Yes I verified. Tested on different api levels from 26 to 34


Copy link
Collaborator

@albin-mullvad albin-mullvad left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@albin-mullvad albin-mullvad merged commit c1e2a22 into main Dec 13, 2023
17 checks passed
@albin-mullvad albin-mullvad deleted the remove-write_external_storage-permission-for-android-13-droid-367 branch December 13, 2023 12:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Android Issues related to Android
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants