ReviewOS

also looking at this

craft-native/craft

test(mobile-e2e): prove Android reports approximate location as granted

#214
Merged glennmichael123 wants to merge fix/android-location-granted into main
5 files +85 -5
10 days ago

Closes #192. Stacked on #213, which adds the shared permissions.location.granted case for iOS.

Where #192 stood

The report was against 26d927f (15 August). Two changes after it address it:

  • bb5c1b5, "expose native permissions bridge", added permissions.check and permissions.request backed by ContextCompat.checkSelfPermission;
  • 780e952, "accept approximate location grants", makes location count as granted when either precise or approximate is held.

Neither had run on a device. The E2E suite's Android probe did not declare location at all.

What runs now

  • The Android probe sets enableGeolocation: true. That is what puts the location permissions in the manifest, and pm grant refuses a permission the manifest does not declare. Nothing else in the Kotlin reads the flag; see #209.
  • Before launch, the harness grants ACCESS_COARSE_LOCATION only, with adb shell pm grant.
  • permissions.location.granted now runs on both platforms. permissions.check('location') and permissions.request('location') must both answer granted, the second one immediately.

Why approximate only

The report was a device with location granted. The obvious test grants both permissions, and it would still pass against a bridge that required precise location, which is the behaviour 780e952 removed. Android 12 onwards lets a person choose approximate only. Granting coarse alone is therefore the stricter test: any bridge that passes it also passes with both granted, and not the other way round.

The device state is checked, not assumed

The harness reads dumpsys package back after the grant and saves it as package-permissions.txt. It fails the leg when approximate is not granted, or when precise is. Otherwise the case could run against a weaker device state than it claims.

The parser, runtimePermissionGranted, gives three answers: true, false, and undefined when the permission is not mentioned, which usually means the manifest never declared it. It is unit tested, including against the word-boundary trap: \b matches after every dot, so permission.CAMERA would otherwise be found inside android.permission.CAMERA.

Failing without the fix

There is no Android toolchain on the machine this was written on, so the mutation runs in CI. Branch mutation/192-require-fine narrows CraftPermissionPolicy's location group back to ACCESS_FINE_LOCATION alone, and the workflow was dispatched against it:

Run 35111184463:

FAIL android-shim — 6/7 cases, no zig
       case permissions.location.granted failed: permissions.check answered "denied"
FAIL android-runtime — 6/7 cases, zig served registered:103, declines:0
       case permissions.location.granted failed: permissions.check answered "denied"

The other six cases pass on both legs, so the failure is this one assertion and not collateral damage. The mutation branch has been deleted.

Verification

bun run test, bun run typecheck, bun run verify:packages, Pickier and 42 protocol tests locally. The emulator run is this PR's CI.

5 changed files on the files tab, with 0 review threads.

Sign in to comment on this pull request.