Skip to content

Commit 0a3bbfa

Browse files
authored
fix: bluetooth permissions check & disconnect on close (MetaMask#27358)
<!-- Please submit this PR as a draft initially. Do not mark it as "Ready for review" until the template has been completely filled out, and PR status checks have passed at least once. --> ## **Description** <!-- Write a short description of the changes included in this pull request, also include relevant motivation and context. Have in mind the following questions: 1. What is the reason for the change? 2. What is the improvement/solution? --> Introduced changes: - Permissions: New adapter method `ensurePermissions()` is called before transport checks in `retryEnsureDeviceReady`. And adds Android permission handling (Bluetooth on Android 12+, location on older Android). - Disconnect: Ledger BLE teardown uses `TransportBLE.disconnectDevice(deviceId)` instead of `transport.close()` for immediate disconnect. Cancelling “awaiting confirmation” also triggers a best-effort adapter disconnect. ## **Changelog** <!-- If this PR is not End-User-Facing and should not show up in the CHANGELOG, you can choose to either: 1. Write `CHANGELOG entry: null` 2. Label with `no-changelog` If this PR is End-User-Facing, please write a short User-Facing description in the past tense like: `CHANGELOG entry: Added a new tab for users to see their NFTs` `CHANGELOG entry: Fixed a bug that was causing some NFTs to flicker` (This helps the Release Engineer do their job more quickly and accurately) --> CHANGELOG entry: null ## **Related issues** Fixes: https://consensyssoftware.atlassian.net/browse/MUL-1580 ## **Manual testing steps** no manual testing steps ## **Screenshots/Recordings** <!-- If applicable, add screenshots and/or recordings to visualize the before and after of your change. --> ### **Before** <!-- [screenshots/recordings] --> ### **After** <!-- [screenshots/recordings] --> ## **Pre-merge author checklist** - [x] I've followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Mobile Coding Standards](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/CODING_GUIDELINES.md). - [x] I've completed the PR template to the best of my ability - [x] I've included tests if applicable - [x] I've documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [x] I've applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. ## **Pre-merge reviewer checklist** - [x] I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed). - [x] I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots. <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Medium Risk** > Changes affect BLE connection lifecycle and Android permission prompting, which can alter user flow and connection stability; coverage is included but behavior depends on platform permission APIs and device/OS quirks. > > **Overview** > Improves the hardware-wallet connection retry path by introducing a new `HardwareWalletAdapter.ensurePermissions()` contract and calling it before transport checks in `retryEnsureDeviceReady`, aborting the retry if the user must be redirected to OS Settings. > > For Ledger BLE, adds Android permission prompting (Android 12+ Bluetooth permissions, older Android location permission) and updates transport teardown to prefer `TransportBLE.disconnectDevice(deviceId)` over `transport.close()` to force immediate disconnects; the awaiting-confirmation cancel path now also triggers a best-effort adapter disconnect. Tests/mocks are updated accordingly (Ledger adapter, provider, non-hardware adapter). > > <sup>Written by [Cursor Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit 2a94bd7. This will update automatically on new commits. Configure [here](https://cursor.com/dashboard?tab=bugbot).</sup> <!-- /CURSOR_SUMMARY -->
1 parent 2eacab6 commit 0a3bbfa

9 files changed

Lines changed: 181 additions & 7 deletions

app/core/HardwareWallet/HardwareWalletProvider.test.tsx

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,7 @@ const mockAdapterInstance = {
3434
destroy: jest.fn(),
3535
startDeviceDiscovery: jest.fn(),
3636
stopDeviceDiscovery: jest.fn(),
37+
ensurePermissions: jest.fn().mockResolvedValue(true),
3738
isTransportAvailable: jest.fn().mockReturnValue(true),
3839
onTransportStateChange: jest
3940
.fn()

app/core/HardwareWallet/HardwareWalletProvider.tsx

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -122,9 +122,11 @@ export const HardwareWalletProvider: React.FC<HardwareWalletProviderProps> = ({
122122

123123
const handleAwaitingConfirmationCancel = useCallback(() => {
124124
DevLogger.log('[HardwareWallet] handleAwaitingConfirmationCancel');
125+
// eslint-disable-next-line no-empty-function
126+
refs.adapterRef.current?.disconnect().catch(() => {});
125127
awaitingConfirmationRejectRef.current?.();
126128
hideAwaitingConfirmation();
127-
}, [hideAwaitingConfirmation]);
129+
}, [hideAwaitingConfirmation, refs.adapterRef]);
128130

129131
const contextValue = useMemo(
130132
() => ({

app/core/HardwareWallet/adapters/LedgerBluetoothAdapter.test.ts

Lines changed: 103 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@ jest.mock('@ledgerhq/react-native-hw-transport-ble', () => ({
3535
__esModule: true,
3636
default: {
3737
open: jest.fn(),
38+
disconnectDevice: jest.fn().mockResolvedValue(undefined),
3839
observeState: jest.fn((observer) => {
3940
capturedBleStateObserver = observer;
4041
// Immediately trigger with PoweredOn state for most tests
@@ -66,6 +67,31 @@ jest.mock('../../Ledger/Ledger', () => ({
6667
closeRunningAppOnLedger: jest.fn(),
6768
}));
6869

70+
const mockRequestMultiple = jest.fn();
71+
const mockRequest = jest.fn();
72+
jest.mock('react-native-permissions', () => ({
73+
requestMultiple: (...args: unknown[]) => mockRequestMultiple(...args),
74+
request: (...args: unknown[]) => mockRequest(...args),
75+
PERMISSIONS: {
76+
ANDROID: {
77+
BLUETOOTH_CONNECT: 'android.permission.BLUETOOTH_CONNECT',
78+
BLUETOOTH_SCAN: 'android.permission.BLUETOOTH_SCAN',
79+
ACCESS_FINE_LOCATION: 'android.permission.ACCESS_FINE_LOCATION',
80+
},
81+
},
82+
RESULTS: {
83+
GRANTED: 'granted',
84+
DENIED: 'denied',
85+
BLOCKED: 'blocked',
86+
},
87+
}));
88+
89+
const mockGetSystemVersion = jest.fn().mockReturnValue('13');
90+
jest.mock('react-native-device-info', () => ({
91+
getSystemVersion: () => mockGetSystemVersion(),
92+
}));
93+
94+
import { Linking, Platform } from 'react-native';
6995
import { LedgerBluetoothAdapter } from './LedgerBluetoothAdapter';
7096
import { HardwareWalletType, DeviceEvent } from '@metamask/hw-wallet-sdk';
7197
import { HardwareWalletAdapterOptions } from '../types';
@@ -157,8 +183,9 @@ describe('LedgerBluetoothAdapter', () => {
157183
await adapter.connect('device-123');
158184
await adapter.connect('device-456');
159185

160-
// Should close first connection and open new one
161-
expect(mockTransportInstance.close).toHaveBeenCalled();
186+
expect(mockedTransportBLE.disconnectDevice).toHaveBeenCalledWith(
187+
'device-123',
188+
);
162189
expect(mockedTransportBLE.open).toHaveBeenCalledTimes(2);
163190
});
164191

@@ -248,7 +275,9 @@ describe('LedgerBluetoothAdapter', () => {
248275
await adapter.connect('device-123');
249276
await adapter.disconnect();
250277

251-
expect(mockTransportInstance.close).toHaveBeenCalled();
278+
expect(mockedTransportBLE.disconnectDevice).toHaveBeenCalledWith(
279+
'device-123',
280+
);
252281
expect(adapter.isConnected()).toBe(false);
253282
expect(adapter.getConnectedDeviceId()).toBeNull();
254283
});
@@ -842,12 +871,82 @@ describe('LedgerBluetoothAdapter', () => {
842871
});
843872
});
844873

874+
describe('ensurePermissions', () => {
875+
const originalOS = Platform.OS;
876+
877+
afterEach(() => {
878+
Platform.OS = originalOS;
879+
});
880+
881+
it('returns true on iOS without requesting permissions', async () => {
882+
Platform.OS = 'ios';
883+
884+
const result = await adapter.ensurePermissions();
885+
886+
expect(result).toBe(true);
887+
expect(mockRequestMultiple).not.toHaveBeenCalled();
888+
expect(mockRequest).not.toHaveBeenCalled();
889+
});
890+
891+
it('returns true on Android 12+ when BLE permissions are granted', async () => {
892+
Platform.OS = 'android';
893+
mockGetSystemVersion.mockReturnValue('12');
894+
mockRequestMultiple.mockResolvedValue({
895+
'android.permission.BLUETOOTH_CONNECT': 'granted',
896+
'android.permission.BLUETOOTH_SCAN': 'granted',
897+
});
898+
899+
const result = await adapter.ensurePermissions();
900+
901+
expect(result).toBe(true);
902+
});
903+
904+
it('opens settings on Android 12+ when BLE permissions are denied', async () => {
905+
Platform.OS = 'android';
906+
mockGetSystemVersion.mockReturnValue('13');
907+
mockRequestMultiple.mockResolvedValue({
908+
'android.permission.BLUETOOTH_CONNECT': 'denied',
909+
'android.permission.BLUETOOTH_SCAN': 'granted',
910+
});
911+
jest.spyOn(Linking, 'openSettings').mockResolvedValue(undefined);
912+
913+
const result = await adapter.ensurePermissions();
914+
915+
expect(result).toBe(false);
916+
expect(Linking.openSettings).toHaveBeenCalled();
917+
});
918+
919+
it('returns true on older Android when location permission is granted', async () => {
920+
Platform.OS = 'android';
921+
mockGetSystemVersion.mockReturnValue('11');
922+
mockRequest.mockResolvedValue('granted');
923+
924+
const result = await adapter.ensurePermissions();
925+
926+
expect(result).toBe(true);
927+
});
928+
929+
it('opens settings on older Android when location permission is denied', async () => {
930+
Platform.OS = 'android';
931+
mockGetSystemVersion.mockReturnValue('10');
932+
mockRequest.mockResolvedValue('blocked');
933+
jest.spyOn(Linking, 'openSettings').mockResolvedValue(undefined);
934+
935+
const result = await adapter.ensurePermissions();
936+
937+
expect(result).toBe(false);
938+
expect(Linking.openSettings).toHaveBeenCalled();
939+
});
940+
});
941+
845942
describe('destroy', () => {
846943
it('closes transport and marks as destroyed', async () => {
847944
await adapter.connect('device-123');
848945
adapter.destroy();
849946

850-
expect(mockTransportInstance.close).toHaveBeenCalled();
947+
expect(mockedTransportBLE.disconnectDevice).toHaveBeenCalledWith(
948+
'device-123',
949+
);
851950
});
852951

853952
it('prevents further operations', async () => {

app/core/HardwareWallet/adapters/LedgerBluetoothAdapter.ts

Lines changed: 49 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
import TransportBLE from '@ledgerhq/react-native-hw-transport-ble';
22
import { State as BleState } from 'react-native-ble-plx';
3+
import { Linking, Platform } from 'react-native';
34
import { Observable, Subscription } from 'rxjs';
45
import Eth from '@ledgerhq/hw-app-eth';
56
import {
@@ -8,6 +9,13 @@ import {
89
DeviceEventPayload,
910
ErrorCode,
1011
} from '@metamask/hw-wallet-sdk';
12+
import {
13+
PERMISSIONS,
14+
RESULTS,
15+
requestMultiple,
16+
request,
17+
} from 'react-native-permissions';
18+
import { getSystemVersion } from 'react-native-device-info';
1119
import {
1220
DiscoveredDevice,
1321
HardwareWalletAdapter,
@@ -276,6 +284,37 @@ export class LedgerBluetoothAdapter implements HardwareWalletAdapter {
276284
}
277285
}
278286

287+
async ensurePermissions(): Promise<boolean> {
288+
if (Platform.OS !== 'android') {
289+
return true;
290+
}
291+
292+
const version = Number(getSystemVersion()) || 0;
293+
294+
if (version >= 12) {
295+
const result = await requestMultiple([
296+
PERMISSIONS.ANDROID.BLUETOOTH_CONNECT,
297+
PERMISSIONS.ANDROID.BLUETOOTH_SCAN,
298+
]);
299+
const allGranted =
300+
result[PERMISSIONS.ANDROID.BLUETOOTH_CONNECT] === RESULTS.GRANTED &&
301+
result[PERMISSIONS.ANDROID.BLUETOOTH_SCAN] === RESULTS.GRANTED;
302+
303+
if (!allGranted) {
304+
await Linking.openSettings();
305+
return false;
306+
}
307+
} else {
308+
const result = await request(PERMISSIONS.ANDROID.ACCESS_FINE_LOCATION);
309+
if (result !== RESULTS.GRANTED) {
310+
await Linking.openSettings();
311+
return false;
312+
}
313+
}
314+
315+
return true;
316+
}
317+
279318
async isTransportAvailable(): Promise<boolean> {
280319
// Wait for initial BLE state if not yet received
281320
if (!this.#hasReceivedInitialBleState) {
@@ -524,12 +563,20 @@ export class LedgerBluetoothAdapter implements HardwareWalletAdapter {
524563

525564
async #closeTransport(): Promise<void> {
526565
const transport = this.#transport;
566+
const deviceId = this.#deviceId;
527567
if (transport) {
528568
this.#transport = null;
529569
try {
530-
await transport.close();
570+
if (deviceId) {
571+
// TransportBLE.close() queues a delayed disconnect (5s timeout).
572+
// Force an immediate BLE disconnection so in-flight signing is
573+
// aborted without delay.
574+
await TransportBLE.disconnectDevice(deviceId);
575+
} else {
576+
await transport.close();
577+
}
531578
} catch {
532-
// Ignore close errors
579+
// Ignore close errors — device may already be disconnected
533580
}
534581
}
535582
}

app/core/HardwareWallet/adapters/NonHardwareAdapter.test.ts

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -139,6 +139,13 @@ describe('NonHardwareAdapter', () => {
139139
});
140140
});
141141

142+
describe('ensurePermissions', () => {
143+
it('returns true', async () => {
144+
const result = await adapter.ensurePermissions();
145+
expect(result).toBe(true);
146+
});
147+
});
148+
142149
describe('isTransportAvailable', () => {
143150
it('returns true', async () => {
144151
const result = await adapter.isTransportAvailable();

app/core/HardwareWallet/adapters/NonHardwareAdapter.ts

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -60,6 +60,10 @@ export class NonHardwareAdapter implements HardwareWalletAdapter {
6060

6161
stopDeviceDiscovery(): void {}
6262

63+
async ensurePermissions(): Promise<boolean> {
64+
return true;
65+
}
66+
6367
async isTransportAvailable(): Promise<boolean> {
6468
return true;
6569
}

app/core/HardwareWallet/hooks/useDeviceConnectionFlow.ts

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -261,6 +261,10 @@ export const useDeviceConnectionFlow = ({
261261
adapter.resetFlowState();
262262
}
263263

264+
if (adapter && !(await adapter.ensurePermissions())) {
265+
return;
266+
}
267+
264268
if (adapter && (await checkTransportEnabledOrShowError(adapter))) {
265269
return;
266270
}

app/core/HardwareWallet/hooks/useDeviceEventHandlers.test.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,7 @@ describe('useDeviceEventHandlers', () => {
4040
resetFlowState: jest.fn(),
4141
startDeviceDiscovery: jest.fn().mockReturnValue(jest.fn()),
4242
stopDeviceDiscovery: jest.fn(),
43+
ensurePermissions: jest.fn(() => Promise.resolve(true)),
4344
isTransportAvailable: jest.fn(() => Promise.resolve(true)),
4445
getRequiredAppName: jest.fn().mockReturnValue('Ethereum'),
4546
getTransportDisabledErrorCode: jest

app/core/HardwareWallet/types.ts

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -105,6 +105,15 @@ export interface HardwareWalletAdapter {
105105
*/
106106
stopDeviceDiscovery(): void;
107107

108+
/**
109+
* Ensure required OS permissions are granted for this adapter.
110+
* Requests permissions if needed; opens OS Settings when permanently denied.
111+
*
112+
* @returns `true` if permissions are granted, `false` if the user was
113+
* redirected to Settings.
114+
*/
115+
ensurePermissions(): Promise<boolean>;
116+
108117
/**
109118
* Check if the underlying transport mechanism is available.
110119
* For Ledger: Bluetooth is enabled

0 commit comments

Comments
 (0)