ReviewOS
Reload

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

Review threads live on the whole diff, not on one commit, so none are shown here - a thread's line means something in the branch's final form, and painting it into an intermediate step would put it on code it is not about.

.github/workflows/README.mdmodified+2-2
@@ -56,8 +56,8 @@ bun run test:mobile-e2e:android
5656```
5757
5858```
59ok android-shim — 6/6 cases, no zig
60ok android-runtime — 6/6 cases, zig served registered:103, declines:0
59ok android-shim — 7/7 cases, no zig
60ok android-runtime — 7/7 cases, zig served registered:103, declines:0
6161```
6262
6363One Android case needs a person: `share.dismissed.resolvesFalse` opens the
scripts/mobile-e2e/android.tsmodified+32-1
@@ -2,7 +2,7 @@ import type { LegOutcome, RunnerOptions } from './types'
22import { copyFileSync, existsSync, mkdirSync, readFileSync, rmSync, writeFileSync } from 'node:fs'
33import { join } from 'node:path'
44import { init } from '../../packages/android/src/index'
5import { androidDeclines, awaitedNeeds, DISMISS_SHARE_MENU, evaluateRun, hasTerminated, shareMenuInFront } from './protocol'
5import { androidDeclines, awaitedNeeds, DISMISS_SHARE_MENU, evaluateRun, hasTerminated, runtimePermissionGranted, shareMenuInFront } from './protocol'
66import { command, driverPage, waitForFile } from './support'
77
88/**
@@ -15,11 +15,29 @@ import { command, driverPage, waitForFile } from './support'
1515 * Clipboard is not a configurable capability on Android - CraftBridge serves
1616 * clipboardRead/clipboardWrite unconditionally - so the success case needs no
1717 * flag, and asking for one would imply a gate that is not there.
18 *
19 * Geolocation on, because that is what puts the location permissions in the
20 * manifest, and `pm grant` refuses a permission the manifest does not declare.
21 * The Kotlin does not otherwise read the flag (#209).
1822 */
1923const CONFIG = {
2024 enablePushNotifications: false,
25 enableGeolocation: true,
2126}
2227
28/**
29 * The one location permission the harness grants: approximate, and not
30 * precise.
31 *
32 * #192 was a device with location granted that still reported something
33 * else. Granting both permissions would pass against a bridge that required
34 * either one, and that is not the contract. Android 12 onwards lets a person
35 * choose approximate only, and CraftPermissionPolicy counts that as `granted`
36 * (780e952). Coarse alone is the stricter of the two tests: anything this
37 * passes, a device with both permissions granted passes too.
38 */
39const GRANTED_LOCATION_PERMISSION = 'android.permission.ACCESS_COARSE_LOCATION'
40
2341const PACKAGE = 'dev.craft.e2e.probe'
2442const APP_NAME = 'CraftE2EProbe'
2543
@@ -142,6 +160,19 @@ async function runLeg(leg: Leg, options: RunnerOptions): Promise<LegOutcome> {
142160 await adb(['uninstall', PACKAGE], { serial, allowFailure: true })
143161 await adb(['install', '-r', apk], { serial, logPath: join(evidence, 'adb.log') })
144162
163 // Granted from the host before launch, so no dialog waits on a person.
164 await adb(['shell', 'pm', 'grant', PACKAGE, GRANTED_LOCATION_PERMISSION], { serial, logPath: join(evidence, 'adb.log') })
165
166 // And read back, because the case is only as strict as the device state it
167 // ran against: approximate granted, precise not. A device that had precise
168 // granted as well would pass against a bridge requiring precise.
169 const packageState = (await adb(['shell', 'dumpsys', 'package', PACKAGE], { serial })).stdout
170 writeFileSync(join(evidence, 'package-permissions.txt'), packageState)
171 if (runtimePermissionGranted(packageState, GRANTED_LOCATION_PERMISSION) !== true)
172 failures.push(`${GRANTED_LOCATION_PERMISSION} is not granted after pm grant; see ${label}/package-permissions.txt`)
173 if (runtimePermissionGranted(packageState, 'android.permission.ACCESS_FINE_LOCATION') === true)
174 failures.push('ACCESS_FINE_LOCATION is granted too, so the location case cannot tell approximate-only from precise')
175
145176 // Not best-effort: the ring buffer outlives an uninstall, and a transcript
146177 // left in it would otherwise be read as this run's. The plan event carries
147178 // this run's nonce as a second guard.
scripts/mobile-e2e/driver.htmlmodified+6-1
@@ -170,8 +170,13 @@
170170 // already holds used to wait for an authorization change that never
171171 // comes, because nothing changes. The harness grants it with
172172 // `simctl privacy` before launch, so both calls must answer at once.
173 //
174 // And #192 on Android, where a device with location granted reported
175 // something other than `granted`. The harness grants approximate
176 // location only, with `pm grant`, which is the case a bridge
177 // requiring precise location gets wrong.
173178 name: 'permissions.location.granted',
174 platforms: ['ios'],
179 platforms: ['ios', 'android'],
175180 run: function () {
176181 return withTimeout(function () { return window.craft.permissions.check('location') }).then(function (checked) {
177182 if (checked.state !== 'resolved') return 'permissions.check ' + checked.state + ' ' + (checked.error ? checked.error.message : '')
scripts/mobile-e2e/protocol.test.tsmodified+27-1
@@ -1,7 +1,7 @@
11import { describe, expect, it } from 'bun:test'
22import { readFileSync } from 'node:fs'
33import { join } from 'node:path'
4import { ANDROID_DECLINE_PHRASES, androidDeclines, awaitedNeeds, DISMISS_SHARE_MENU, evaluateRun, hasTerminated, parseDriverOutput, REQUIRED_CASES, requiredCaseProblems, shareMenuInFront, ZIG_REFUSED_ACTIONS, ZIG_SERVED_ACTIONS, ZIG_TESTED_ACTIONS, zigDispatchedActions, zigHandBacks, zigRefusals } from './protocol'
4import { ANDROID_DECLINE_PHRASES, androidDeclines, awaitedNeeds, DISMISS_SHARE_MENU, evaluateRun, hasTerminated, parseDriverOutput, REQUIRED_CASES, requiredCaseProblems, runtimePermissionGranted, shareMenuInFront, ZIG_REFUSED_ACTIONS, ZIG_SERVED_ACTIONS, ZIG_TESTED_ACTIONS, zigDispatchedActions, zigHandBacks, zigRefusals } from './protocol'
55
66const ESC = String.fromCharCode(27)
77
@@ -334,6 +334,32 @@ describe('share menu', () => {
334334 })
335335})
336336
337describe('runtime permission state', () => {
338 // As `dumpsys package` prints a coarse-only grant on Android 14.
339 const dumpsys = [
340 ' runtime permissions:',
341 ' android.permission.ACCESS_FINE_LOCATION: granted=false, flags=[ USER_SENSITIVE_WHEN_GRANTED|USER_SENSITIVE_WHEN_DENIED]',
342 ' android.permission.ACCESS_COARSE_LOCATION: granted=true, flags=[ USER_SET|USER_SENSITIVE_WHEN_GRANTED|USER_SENSITIVE_WHEN_DENIED]',
343 ' android.permission.ACCESS_BACKGROUND_LOCATION: granted=false, flags=[ RESTRICTION_INSTALLER_EXEMPT]',
344 ].join('\n')
345
346 it('reads granted and not granted apart', () => {
347 expect(runtimePermissionGranted(dumpsys, 'android.permission.ACCESS_COARSE_LOCATION')).toBe(true)
348 expect(runtimePermissionGranted(dumpsys, 'android.permission.ACCESS_FINE_LOCATION')).toBe(false)
349 })
350
351 it('says nothing about a permission the output never mentions', () => {
352 expect(runtimePermissionGranted(dumpsys, 'android.permission.CAMERA')).toBeUndefined()
353 })
354
355 it('does not read one permission as another that ends the same way', () => {
356 // A dot left unescaped would match any character, and a name without a
357 // boundary would match inside a longer one.
358 expect(runtimePermissionGranted(dumpsys, 'permission.ACCESS_COARSE_LOCATION')).toBeUndefined()
359 expect(runtimePermissionGranted('androidXpermissionXCAMERA: granted=true', 'android.permission.CAMERA')).toBeUndefined()
360 })
361})
362
337363describe('android declines', () => {
338364 // The lines the first Android runtime run actually produced, plus the two
339365 // decline kinds it did not hit, so all three are read the way they are
scripts/mobile-e2e/protocol.tsmodified+18-0
@@ -89,6 +89,7 @@ export const REQUIRED_CASES: Record<MobilePlatform, string[]> = {
8989 'push.disabled.rejects',
9090 'share.empty.rejects',
9191 'share.dismissed.resolvesFalse',
92 'permissions.location.granted',
9293 ],
9394}
9495
@@ -354,6 +355,23 @@ export function shareMenuInFront(dumpsysWindow: string): boolean {
354355 .some(line => /mCurrentFocus=/.test(line) && /ChooserActivity/.test(line))
355356}
356357
358/**
359 * Whether `dumpsys package` says a runtime permission is granted, or
360 * `undefined` when the output does not mention it at all.
361 *
362 * Three answers rather than two, because "not mentioned" usually means the
363 * manifest never declared the permission, and reading that as "not granted"
364 * would hide the cause.
365 */
366export function runtimePermissionGranted(dumpsysPackage: string, permission: string): boolean | undefined {
367 const escaped = permission.replaceAll('.', '\\.')
368 // Whitespace or line start before the name, not a word boundary, which sits
369 // after every dot, so `permission.CAMERA` would match inside
370 // `android.permission.CAMERA`.
371 const match = dumpsysPackage.match(new RegExp(`(?:^|\\s)${escaped}: granted=(true|false)`, 'm'))
372 return match ? match[1] === 'true' : undefined
373}
374
357375/**
358376 * The actions the Zig dispatcher saw, by name.
359377 *