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

[container-gen] Custom Android permissions configuration for native components #606

Merged
merged 3 commits into from
Feb 21, 2018

Conversation

comigor
Copy link
Contributor

@comigor comigor commented Feb 19, 2018

Some third-party native components should have custom Android permissions (eg. android.permission.CAMERA) on the container's AndroidManifest.xml, so it should be configurable.

Also, it's good practice to implement react-native's PermissionAwareActivity so on newer versions of Android, the user will be prompted to accept it on runtime.

@belemaire belemaire requested a review from deepueg February 20, 2018 17:53
return PackageManager.PERMISSION_GRANTED;
}

@Override
Copy link
Contributor

Choose a reason for hiding this comment

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

please add @RequiresApi(api = Build.VERSION_CODES.M) to avoid lint warning


@Override
public int checkPermission(String permission, int pid, int uid) {
return PackageManager.PERMISSION_GRANTED;
Copy link
Contributor

Choose a reason for hiding this comment

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

This method is supposed to return PERMISSION_GRANTED or PERMISSION_DENIED based on if the permission is allowed for a particular pid/uid. Overriding with permission granted may have an inverse effect for the feature ?.

@Override
public void requestPermissions(String[] permissions, int requestCode, PermissionListener listener) {
mPermissionListener = listener;
ActivityCompat.requestPermissions(this, permissions, requestCode);
Copy link
Contributor

Choose a reason for hiding this comment

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

Please call mReactActivityDelegate. requestPermissions() as the delegate now extends the ReactActivityDelegate.


@Override
public void onRequestPermissionsResult(int requestCode, @NonNull String[] permissions, @NonNull int[] grantResults) {
mPermissionListener.onRequestPermissionsResult(requestCode, permissions, grantResults);
Copy link
Contributor

Choose a reason for hiding this comment

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

Please call mReactActivityDelegate. onRequestPermissionsResult() as the delegate now extends the ReactActivityDelegate.

@@ -111,4 +117,25 @@ public boolean onKeyUp(int keyCode, KeyEvent event) {
public void onBackKey() {
finish();
}

@Override
public int checkPermission(String permission, int pid, int uid) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be okay to remove this method as it's returning default int value (there is no logic involved)

}

@Override
public int checkSelfPermission(String permission) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be okay to remove this method as it's returning default int value (there is no logic involved)

@krunalsshah
Copy link
Contributor

@Igor1201 thanks for turning the PR, we were planning to implement the PermissionAwareActivity for this release , Glad you picked it up 👍

@comigor
Copy link
Contributor Author

comigor commented Feb 21, 2018

@krunalsshah Nice to know we're in sync! :)
@deepueg I've made some changes to the code. Could you review it again?

@deepueg
Copy link
Contributor

deepueg commented Feb 21, 2018

Looks good

@deepueg deepueg merged commit f7d05ba into electrode-io:master Feb 21, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants