-
Notifications
You must be signed in to change notification settings - Fork 35
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
Request camera permission on the WebViewAuthorizationFragment #2258
Conversation
common/src/main/AndroidManifest.xml
Outdated
@@ -3,6 +3,8 @@ | |||
|
|||
<uses-permission android:name="android.permission.INTERNET" /> | |||
<uses-permission android:name="android.permission.ACCESS_NETWORK_STATE" /> | |||
<!-- Camera permission is required for QR + PIN authorization method --> | |||
<uses-permission android:name="android.permission.CAMERA" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is QR + PIN feature exclusive to broker, or is this also applicable to oneauth + MSAL?
I'm asking because this would be displayed on all consuming app's permission that they now require access to camera. (for those who are not aware of this QR+Pin feature... it could sound scary)
If it's broker exclusive, let's put the camera code in broker and wire the code here (should be easier to talk to broker apps about this).
If not, this is (kind of) a breaking change. we can discuss this in the standup whether calling out this new permission in the changelog is enough, or do we expect any pushback from customers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is exclusive to broker so I would need to move this permission to the broker my b.
I can communicate this to them over email.
Also, the camera permission will not be requested at the first time the app is used like other permissions.
The camera permission is a Runtime permission, therefore the permission must be requested at runtime once the permission is required. and because right now there is no eSTS UX that requires the camera I don't expect this permission request to show up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
both teams are informed about this change.
...va/com/microsoft/identity/common/internal/providers/oauth2/WebViewAuthorizationFragment.java
Outdated
Show resolved
Hide resolved
...va/com/microsoft/identity/common/internal/providers/oauth2/WebViewAuthorizationFragment.java
Show resolved
Hide resolved
...va/com/microsoft/identity/common/internal/providers/oauth2/WebViewAuthorizationFragment.java
Show resolved
Hide resolved
|
||
/** | ||
* Call this method to grant the permission to access the camera resource. | ||
* The granted permission is only valid for the current WebView. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does current
mean here? On next launch of WebView will the user need to grant permission again?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the user decided to be asked every time, every time the user launches the WebView will be prompted for camera permission, if the user chooses the option 1, the WebView automatically will grant this permission. see L252 to 260
* @return true if the given permission request is for camera, false otherwise. | ||
*/ | ||
private boolean iSPermissionRequestForCamera(final PermissionRequest request) { | ||
if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.LOLLIPOP) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add some comments about OS versions checks?
It seems if the OS is not from among that range then we are assuming permission request isn't for camera by hard code returning false
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a comment with more context about this version check.
* Note: This method is only available on API level 21 or higher.
* Devices running on lower API levels will not be able to grant or deny camera permission requests.
* getResources() method is only available on API level 21 or higher.
and a warning message
Logger.warn(TAG, "PermissionRequest.getResources() method is not available on API:"
+ Build.VERSION.SDK_INT + ". We cannot determine if the request is for camera.");
Why?
When a request is made to ESTS with Preferred authorization method "QR code + PIN", ESTS UX will return a page were the user needs to scan a QR code
What is in this PR?
Added handler to the WebView for when a camera request is detected.
related PR: https://github.com/AzureAD/ad-accounts-for-android/pull/2673