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

fix Android 13 write permissions #581

Closed
wants to merge 2 commits into from
Closed

fix Android 13 write permissions #581

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9c999f4
fix Android 13 write permissions
EYALIN Jul 11, 2023
29f95b8
Indent file
EYALIN Aug 18, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 12 additions & 5 deletions src/android/FileUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -593,14 +593,17 @@ private void getReadPermission(String rawArgs, int action, CallbackContext callb
if (android.os.Build.VERSION.SDK_INT >= Build.VERSION_CODES.TIRAMISU) {
PermissionHelper.requestPermissions(this, requestCode,
new String[]{Manifest.permission.READ_MEDIA_IMAGES, Manifest.permission.READ_MEDIA_VIDEO, Manifest.permission.READ_MEDIA_AUDIO});
} else {
} else {
PermissionHelper.requestPermission(this, requestCode, Manifest.permission.READ_EXTERNAL_STORAGE);
}
}
}

private void getWritePermission(String rawArgs, int action, CallbackContext callbackContext) {
int requestCode = pendingRequests.createRequest(rawArgs, action, callbackContext);
PermissionHelper.requestPermission(this, requestCode, Manifest.permission.WRITE_EXTERNAL_STORAGE);
int requestCode = pendingRequests.createRequest(rawArgs, action, callbackContext);
if (android.os.Build.VERSION.SDK_INT >= Build.VERSION_CODES.TIRAMISU) {
} else {
PermissionHelper.requestPermission(this, requestCode, Manifest.permission.WRITE_EXTERNAL_STORAGE);
}
Comment on lines +603 to +606
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this can be rewritten as to invert the condition so we don't have an empty code block.

Suggested change
if (android.os.Build.VERSION.SDK_INT >= Build.VERSION_CODES.TIRAMISU) {
} else {
PermissionHelper.requestPermission(this, requestCode, Manifest.permission.WRITE_EXTERNAL_STORAGE);
}
if (android.os.Build.VERSION.SDK_INT < Build.VERSION_CODES.TIRAMISU) {
PermissionHelper.requestPermission(this, requestCode, Manifest.permission.WRITE_EXTERNAL_STORAGE);
}

Copy link
Member

@erisu erisu Oct 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

getWritePermission is already wrapped by a condition that requires needPermission to be true, before it is called.

needPermission should then hit this condition:

else if(permissionType == WRITE && hasWritePermission()) {
            return false;
        }

Which should return false once the hasWritePermission method is fixed.

I think the only changes needed in this PR should be:

    private boolean hasWritePermission() {
        return android.os.Build.VERSION.SDK_INT >= Build.VERSION_CODES.TIRAMISU
            ? true
            : PermissionHelper.hasPermission(this, Manifest.permission.WRITE_EXTERNAL_STORAGE);
    }

Copy link
Member

Choose a reason for hiding this comment

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

@EYALIN Can you review the above messages and make the updates?

I believe you do not need to modify the getWritePermission method, as I previously mentioned the reasons why.
Only hasWritePermission needs to be updated. The code could be simplified with what I provided in my previous message.

Also, can you rebase your branch against with Cordova's main branch? There are conflicts.

And include Norman's suggestion about including a comment explaining about the true return.

// Starting with API 33, requesting WRITE_EXTERNAL_STORAGE is an auto permission rejection

}

private boolean hasReadPermission() {
Expand All @@ -614,7 +617,11 @@ private boolean hasReadPermission() {
}

private boolean hasWritePermission() {
return PermissionHelper.hasPermission(this, Manifest.permission.WRITE_EXTERNAL_STORAGE);
if (android.os.Build.VERSION.SDK_INT >= Build.VERSION_CODES.TIRAMISU) {
return true;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return true;
// Starting with API 33, requesting WRITE_EXTERNAL_STORAGE is an auto permission rejection
// Apps now have read-write access to it's own (created) files, so we can assume `true`, however not all files are readable.
// See https://github.com/apache/cordova-plugin-file#androids-external-storage-quirks for more details.
return true;

Copy link
Contributor

Choose a reason for hiding this comment

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

Just to explain why we are returning true for future contributors/code maintainers

} else {
return PermissionHelper.hasPermission(this, Manifest.permission.WRITE_EXTERNAL_STORAGE);
}
}

private boolean needPermission(String nativeURL, int permissionType) throws JSONException {
Expand Down