Make widget web view request system permissions for camera and microphone (PSF-1061) #6149

Johennes · 2022-05-25T10:51:48Z

Type of change

Feature
Bugfix
Technical
Other :

Content

Previously the widget web view prompted to grant the widget permissions but it didn't actually request those permissions from the system. So if the web view requested, e.g. the camera permission but the app hadn't previously been granted that permission, the web view wouldn't get camera access even when the widget permission request had been confirmed.

With this change, the app will also request camera and microphone permissions from the system when needed.

Motivation and context

The concrete use case that led to this change is running Element Call in a widget web view after a fresh app install (where no system permissions have been granted before and so the widget would never actually get camera or microphone access).

Screenshots / GIFs

Before this change, only the widget permission dialog was displayed:

After this change, once the widget permission dialog is confirmed, the actual system permission dialogs appear (unless permission has already been granted before):

Tests

Perform a clean app install or reset the app's camera & microphone permissions
Join #ecwidgettest:matrix.org
Tap through into Element Call in the widget web view
Confirm the widget permission dialog
Notice the new system permission dialogs

Tested devices

Physical
Emulator
OS version(s): 12

Checklist

Changes has been tested on an Android device or Android emulator with API 21
UI change has been tested on both light and dark themes
Accessibility has been taken into account. See https://github.com/vector-im/element-android/blob/develop/CONTRIBUTING.md#accessibility
Pull request is based on the develop branch
Pull request includes a new file under ./changelog.d. See https://github.com/vector-im/element-android/blob/develop/CONTRIBUTING.md#changelog
Pull request includes screenshots or videos if containing UI changes
Pull request includes a sign off
You've made a self review of your PR
If you have modified the screen flow, or added new screens to the application, you have updated the test UiAllScreensSanityTest.allScreensTest()

…hone Previously the widget web view prompted to grant the widget permissions but it didn't actually request those permissions from the system. So if the web view requested, e.g. the camera permission but the app hadn't previously been granted that permission, the web view wouldn't get camera access even when the widget permission request had been confirmed. With this commit, the app will also request camera and microphone permissions from the system when needed. Signed-off-by: Johannes Marbach <[email protected]>

github-actions · 2022-05-25T11:38:45Z

Unit Test Results

146 files +  4 146 suites +4 2m 9s ⏱️ -21s
235 tests +  6 235 ✔️ +  6 0 💤 ±0 0 ❌ ±0
784 runs +24 784 ✔️ +24 0 💤 ±0 0 ❌ ±0

Results for commit 4ebb26d. ± Comparison against base commit 483b1ab.

♻️ This comment has been updated with latest results.

ouchadam · 2022-05-25T13:11:52Z

vector/src/main/java/im/vector/app/features/widgets/webview/WebviewPermissionUtils.kt


 object WebviewPermissionUtils {

+    private var permissionRequest: PermissionRequest? = null


I'm a little weary of including mutable static state in the object instance, would prefer for this class to become instantiable (and then injected where needed) or have the activity/fragment to hold on to the state instead

what do you think?

I had also thought of this, yeah. I had previously decided against putting the state into the fragment because that way state and logic are split between the fragment and WebviewPermissionUtils which I found harder to reason about. I made the utils into a normal class now and used a private instance variable on the fragment. If this needs to be injectable, I might need some help with that. 🙈

ouchadam · 2022-05-25T13:19:23Z

vector/src/main/java/im/vector/app/features/widgets/webview/WebviewPermissionUtils.kt

                }
                .setNegativeButton(R.string.room_widget_resource_decline_permission) { _, _ ->
                    request.deny()
                }
                .show()
    }

+    fun onPermissionResult(result: Map<String, Boolean>) {
+        permissionRequest?.let { request ->


is there a case where we would call onPermissionResult without having a valid permissionRequest available?

I would lean towards throwing instead of silently swallowing the missing promptForPermissions (if the ordering is implicitly required)

No, that shouldn't happen, I believe. The order is required as you say. I've added an NPE instead of the let. Let me know if that's what you had in mind.

sounds good to me, thanks! 💯

…is null

ouchadam · 2022-05-25T14:03:12Z

vector/src/main/java/im/vector/app/features/widgets/webview/WebviewPermissionUtils.kt

+                return@filter result[androidPermission]
+                        ?: return@filter true // Android permission already granted before
+            }
+            if (grantedPermissions.isNotEmpty()) {


tiny nit - semantically it could be cleaner to avoid mapping within the filter, then the filter could be replaced with an any, extensions can also be created to help avoid needing inline comments

val permissions = selectedWebPermissions .onlySupportedAndroidPermissions() .mapWithResult(result) when { permissions.any { (_, isGranted) -> isGranted } -> { request.grant(permissions.map { (name, _ ) -> name }.toTypedArray()) } else -> deny() } reset() private fun List<String>.onlySupportedAndroidPermissions() = filter { webPermissionToAndroidPermission(it) != null } private fun List<String>.mapWithResult(result: Map<String, Boolean>) = mapNotNull { permission -> result[permission]?.let{ isGranted -> permission to isGranted } }

Hm, I think in this snippet we'd put the Android permission strings into request.grant(...), right? We have to put the web permission strings into that one. That's essentially the reason why I mapped inside the filter.

I agree that the filter clause isn't very easy to parse though. ☹️

ah I see! 🤦 yeah there's a few steps happening here but the scope small and it gets the job done (which is why this is an opinionated non blocking nit from me 😄 )

EDIT - updated the snippet to filter by android permissions instead of mapping to them

This kept nagging me so I added a unit test for this part to be able to safely change the implementation. I found that your updated snippet still doesn't work, sadly.

First, onlySupportedAndroidPermissions is actually not needed / desired because there are web permissions such as PermissionRequest.RESOURCE_PROTECTED_MEDIA_ID for which (I think) no Android system permission exists. In those cases we still want to grant those permissions to the web view (based on the fact that the user confirmed them in the widget permission dialog) so we can't filter them out.

Secondly, the chained call of mapWithResult uses the web permission string as key in the Android permission result map which will always result in null.

I've tried to resolve this but in the end it comes down to having the current complicated filter/map logic inside mapWithResult which seemed more or less equivalent to what I have now. 😕

At least we have the tests now though so this endeavor hasn't been in vein. 😀

ouchadam · 2022-05-25T14:10:09Z

vector/src/main/java/im/vector/app/features/widgets/WidgetFragment.kt

@@ -72,6 +72,7 @@ class WidgetFragment @Inject constructor() :

    private val fragmentArgs: WidgetArgs by args()
    private val viewModel: WidgetViewModel by activityViewModel()
+    private val permissionUtils = WebviewPermissionUtils()


if we want to inject this via dagger (not really needed whilst the WebviewPermissionUtils has no dependencies of its own) we can include the instance via the injectable constructor of the fragment

WidgetFragment @Inject constructor()

becomes...

WidgetFragment @Inject constructor( private val permissionUtils: WebviewPermissionUtils )

and the WebviewPermissionUtils itself will need to be marked as injectable

class WebviewPermissionUtils @Inject constructor()

Fragments in Element are injectable because of the fragment module

Ah nice! Thanks a lot for explaining. Made the change to be a good citizen and help remember for next time. 🙂

future us will appreciate the injectable boilerplate being done 😄

mnaturel

I have added small comments. I need to test it.

mnaturel · 2022-05-30T12:30:48Z

changelog.d/6149.bugfix

@@ -0,0 +1 @@
+Make widget web view request system permissions for camera and microphone (PSF-1061)


Do we want the PSF number into the changelog entry?

Happy to remove it but having it in the changelog / release notes would allow us to directly cross reference with Jira. I think it's essentially also not better or worse than including it in commits and pull requests – which we already do today.

This will be visible from the release page, so would be better to have a corresponding github issue...

I assumed with the filename it'll link to this PR and, therefore, had intentionally explained the issue that is fixed in detail up in the description to avoid having to create a separate issue just to close it again.

vector/src/main/java/im/vector/app/features/webview/WebChromeEventListener.kt

mnaturel · 2022-05-30T12:48:51Z

vector/src/main/java/im/vector/app/features/widgets/WidgetFragment.kt

@@ -271,6 +278,20 @@ class WidgetFragment @Inject constructor() :
        viewModel.handle(WidgetAction.OnWebViewLoadingError(url, true, errorCode, description))
    }

+    private val permissionResultLauncher = registerForActivityResult(ActivityResultContracts.RequestMultiplePermissions()) { result ->


I don't know if we could use existing methods in PermissionTools file to avoid having other logic for permissions handling?

The registration methods available there, sadly don't expose the map of permission results: https://github.com/vector-im/element-android/blob/4094a66f3ce65d5fa92ac31b7749c731b3f33cf5/vector/src/main/java/im/vector/app/core/utils/PermissionsTools.kt#L67

vector/src/main/java/im/vector/app/features/widgets/webview/WebviewPermissionUtils.kt

vector/src/androidTest/java/im/vector/app/features/widgets/WebviewPermissionUtilsTest.kt

mnaturel · 2022-05-30T15:41:32Z

After some tests, I have a remark concerning the UX:
Even if I granted the microphone permission once, the dialog with checkboxes to request permission appears again. I am wondering if we could directly request Android permissions without the extra dialog with checkboxes? It will be less painful for users.

ganfra

Just small remarks otherwise LGTM

vector/src/main/java/im/vector/app/features/webview/WebChromeEventListener.kt

vector/src/main/java/im/vector/app/features/widgets/WidgetFragment.kt

Johennes · 2022-05-30T18:10:19Z

After some tests, I have a remark concerning the UX: Even if I granted the microphone permission once, the dialog with checkboxes to request permission appears again. I am wondering if we could directly request Android permissions without the extra dialog with checkboxes? It will be less painful for users.

The reason the intermediate dialog is needed is that the app currently doesn't keep track of which widget runs in the web view. So if we wouldn't always show the first prompt, then the second prompt would effectively grant the permission to all widgets – which we likely don't want.

I agree that the UX isn't particularly great. This PR was only meant to fix the bug where the web view wasn't actually granted the permission though. We already have https://element-io.atlassian.net/browse/PSF-1033 (Web view permissions are not persisted - Android) to persist the permissions per widget which can serve as the basis for generally improving the flow.

vector/src/main/java/im/vector/app/features/widgets/webview/WebviewPermissionUtils.kt

github-actions · 2022-05-31T09:44:28Z

Matrix SDK

Integration Tests Results:

[org.matrix.android.sdk.session]
= passed=20 failures=0 errors=0 skipped=3
[org.matrix.android.sdk.account]
= passed=3 failures=0 errors=0 skipped=2
[org.matrix.android.sdk.internal]
= passed=27 failures=1 errors=0 skipped=1
[org.matrix.android.sdk.ordering]
= passed=16 failures=0 errors=0 skipped=0
[org.matrix.android.sdk.PermalinkParserTest]
= passed=2 failures=0 errors=0 skipped=0

Johennes marked this pull request as draft May 25, 2022 10:51

Johennes changed the title ~~Make widget web view request system permissions for camera and microphone~~ Make widget web view request system permissions for camera and microphone (PSF-1061) May 25, 2022

Johennes added 2 commits May 25, 2022 12:53

Add changelog entry

9469027

Appease the linter

580bbd6

Johennes marked this pull request as ready for review May 25, 2022 11:39

Johennes requested review from a team, ouchadam and ganfra and removed request for a team May 25, 2022 11:39

ouchadam reviewed May 25, 2022

View reviewed changes

Make permission utils instantiable and throw when permission request …

6ec6d41

…is null

Johennes requested a review from ouchadam May 25, 2022 13:58

ouchadam reviewed May 25, 2022

View reviewed changes

ouchadam approved these changes May 25, 2022

View reviewed changes

ouchadam reviewed May 25, 2022

View reviewed changes

Johennes added 2 commits May 25, 2022 16:22

Inject permission utils

9e084ec

Add unit tests for filter/map logic

92a140b

mnaturel self-requested a review May 30, 2022 12:29

mnaturel reviewed May 30, 2022

View reviewed changes

Johennes added 3 commits May 30, 2022 16:34

Turn test into normal unit test

73d7864

Unsuppress lint rule

d757914

Remove unused imports

769b217

mnaturel reviewed May 30, 2022

View reviewed changes

vector/src/androidTest/java/im/vector/app/features/widgets/WebviewPermissionUtilsTest.kt Outdated Show resolved Hide resolved

Move test to the right folder

75da988

ganfra approved these changes May 30, 2022

View reviewed changes

vector/src/main/java/im/vector/app/features/webview/WebChromeEventListener.kt Outdated Show resolved Hide resolved

vector/src/main/java/im/vector/app/features/widgets/WidgetFragment.kt Outdated Show resolved Hide resolved

Johennes added 3 commits May 30, 2022 20:18

Remove empty default implementation

5b64946

Combine event listener interfaces

70bb2b7

Add missing file

3739919

mnaturel approved these changes May 31, 2022

View reviewed changes

Merge branch 'develop' into johannes/widget-system-permissions

4ebb26d

Johennes merged commit 7dd5b80 into develop May 31, 2022

Johennes deleted the johannes/widget-system-permissions branch May 31, 2022 08:54

bmarty reviewed May 31, 2022

View reviewed changes

vector/src/main/java/im/vector/app/features/widgets/webview/WebviewPermissionUtils.kt Show resolved Hide resolved

Johennes mentioned this pull request May 31, 2022

Use fatalError instead of NPE #6205

Merged

15 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make widget web view request system permissions for camera and microphone (PSF-1061) #6149

Make widget web view request system permissions for camera and microphone (PSF-1061) #6149

Johennes commented May 25, 2022 •

edited

Loading

github-actions bot commented May 25, 2022 •

edited

Loading

ouchadam May 25, 2022 •

edited

Loading

Johennes May 25, 2022

ouchadam May 25, 2022

Johennes May 25, 2022

ouchadam May 25, 2022

ouchadam May 25, 2022 •

edited

Loading

Johennes May 25, 2022

Johennes May 25, 2022

ouchadam May 25, 2022 •

edited

Loading

Johennes May 25, 2022

ouchadam May 25, 2022 •

edited

Loading

Johennes May 25, 2022

ouchadam May 25, 2022

mnaturel left a comment

mnaturel May 30, 2022

Johennes May 30, 2022

ganfra May 30, 2022

Johennes May 30, 2022

mnaturel May 30, 2022

Johennes May 30, 2022

mnaturel commented May 30, 2022 •

edited

Loading

ganfra left a comment

Johennes commented May 30, 2022

github-actions bot commented May 31, 2022


		object WebviewPermissionUtils {

		private var permissionRequest: PermissionRequest? = null

		@@ -0,0 +1 @@
		Make widget web view request system permissions for camera and microphone (PSF-1061)

Make widget web view request system permissions for camera and microphone (PSF-1061) #6149

Make widget web view request system permissions for camera and microphone (PSF-1061) #6149

Conversation

Johennes commented May 25, 2022 • edited Loading

Type of change

Content

Motivation and context

Screenshots / GIFs

Tests

Tested devices

Checklist

github-actions bot commented May 25, 2022 • edited Loading

Unit Test Results

ouchadam May 25, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ouchadam May 25, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ouchadam May 25, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ouchadam May 25, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mnaturel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mnaturel commented May 30, 2022 • edited Loading

ganfra left a comment

Choose a reason for hiding this comment

Johennes commented May 30, 2022

github-actions bot commented May 31, 2022

Matrix SDK

Integration Tests Results:

Johennes commented May 25, 2022 •

edited

Loading

github-actions bot commented May 25, 2022 •

edited

Loading

ouchadam May 25, 2022 •

edited

Loading

ouchadam May 25, 2022 •

edited

Loading

ouchadam May 25, 2022 •

edited

Loading

ouchadam May 25, 2022 •

edited

Loading

mnaturel commented May 30, 2022 •

edited

Loading