Skip to content

Commit

Permalink
feat: extend the time we resume the session without showing OTP (#7212)
Browse files Browse the repository at this point in the history 
<!--
Thanks for your contribution!

Please ensure that any applicable requirements below are satisfied
before submitting this pull request. This will help ensure a quick and
efficient review cycle.
-->

**Development & PR Process**
1. Follow MetaMask [Mobile Coding
Standards](https://github.com/MetaMask/metamask-mobile/blob/main/.github/coding_guidelines/CODING_GUIDELINES.md)
2. Add `release-xx` label to identify the PR slated for a upcoming
release (will be used in release discussion)
3. Add `needs-dev-review` label when work is completed
4. Add the appropiate QA label when dev review is completed
    - `needs-qa`: PR requires manual QA.
- `No QA/E2E only`: PR does not require any manual QA effort. Prior to
merging, ensure that you have successful end-to-end test runs in
Bitrise.
- `Spot check on release build`: PR does not require feature QA but
needs non-automated verification. In the description section, provide
test scenarios. Add screenshots, and or recordings of what was tested.
5. Add `QA Passed` label when QA has signed off (Only required if the PR
was labeled with `needs-qa`)
6. Add your team's label, i.e. label starting with `team-` (or
`external-contributor` label if your not a MetaMask employee)

**Description**

Extended the session persistence time to one hour without showing OTP.

For the first hour after connecting, the session will remain active
without requesting an OTP.

All of the examples SDK apps that use the OTP modal tested on IOS and
Android with a new wallet build that includes the new OTP changes.

The cases that have been checked were `when the channel was active
recently` and `when the channel was NOT active recently` in order to
test both flows.

Everything appears to be working as expected.
**Screenshots/Recordings**

_If applicable, add screenshots and/or recordings to visualize the
before and after of your change_

**Issue**

fixes #???

**Checklist**

* [x] There is a related GitHub issue
* [x] Tests are included if applicable
* [x] Any added code is fully documented

---------

Co-authored-by: abretonc7s <arthur.breton@consensys.net>
Co-authored-by: Christopher Ferreira <christopher.ferreira@consensys.net>
Co-authored-by: Christopher Ferreira <104831203+christopherferreira9@users.noreply.github.com>
  • Loading branch information
@omridan159 @abretonc7s @christopherferreira9
4 people authored Sep 27, 2023
1 parent 3635e72 commit ce22421
Show file tree
Hide file tree
Showing 8 changed files with 116 additions and 77 deletions.
4 changes: 2 additions & 2 deletions android/app/build.gradle
View file
Original file line number Diff line number Diff line change
Expand Up @@ -137,8 +137,8 @@ android {
applicationId "io.metamask"
minSdkVersion rootProject.ext.minSdkVersion
targetSdkVersion rootProject.ext.targetSdkVersion
versionCode 1171
versionName "7.7.0"
versionCode 1173
versionName "7.8.0"
testBuildType System.getProperty('testBuildType', 'debug')
missingDimensionStrategy 'react-native-camera', 'general'
testInstrumentationRunner "androidx.test.runner.AndroidJUnitRunner"
Expand Down
12 changes: 8 additions & 4 deletions app/components/Nav/App/index.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -231,6 +231,8 @@ const App = ({ userLoggedIn }) => {
const { colors } = useTheme();
const { toastRef } = useContext(ToastContext);
const dispatch = useDispatch();
const sdkInit = useRef(false);
const [onboarded, setOnboarded] = useState(false);
const triggerSetCurrentRoute = (route) => {
dispatch(setCurrentRoute(route));
if (route === 'Wallet' || route === 'BrowserView') {
Expand Down Expand Up @@ -349,17 +351,18 @@ const App = ({ userLoggedIn }) => {
initAnalytics();
}, []);

const sdkInit = useRef(false);
useEffect(() => {
if (navigator && !sdkInit.current) {
sdkInit.current = true;
if (navigator?.getCurrentRoute && !sdkInit.current && onboarded) {
SDKConnect.getInstance()
.init({ navigation: navigator })
.then(() => {
sdkInit.current = true;
})
.catch((err) => {
console.error(`Cannot initialize SDKConnect`, err);
});
}
}, [navigator]);
}, [navigator, onboarded]);

useEffect(() => {
if (isWC2Enabled) {
Expand All @@ -372,6 +375,7 @@ const App = ({ userLoggedIn }) => {
useEffect(() => {
async function checkExisting() {
const existingUser = await AsyncStorage.getItem(EXISTING_USER);
setOnboarded(!!existingUser);
const route = !existingUser
? Routes.ONBOARDING.ROOT_NAV
: Routes.ONBOARDING.LOGIN;
Expand Down
2 changes: 2 additions & 0 deletions app/core/DeeplinkManager.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -257,6 +257,7 @@ class DeeplinkManager {
}
SDKConnect.getInstance().reconnect({
channelId: params.channelId,
otherPublicKey: params.pubkey,
context: 'deeplink (universal)',
});
} else {
Expand Down Expand Up @@ -387,6 +388,7 @@ class DeeplinkManager {
}
SDKConnect.getInstance().reconnect({
channelId: params.channelId,
otherPublicKey: params.pubkey,
context: 'deeplink (metamask)',
});
} else {
Expand Down
139 changes: 86 additions & 53 deletions app/core/SDKConnect/SDKConnect.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ import { StackNavigationProp } from '@react-navigation/stack';
import BackgroundTimer from 'react-native-background-timer';
import DefaultPreference from 'react-native-default-preference';
import AppConstants from '../AppConstants';

import {
TransactionController,
WalletDevice,
Expand Down Expand Up @@ -35,16 +34,6 @@ import {
} from '@metamask/sdk-communication-layer';
import { ethErrors } from 'eth-rpc-errors';
import { EventEmitter2 } from 'eventemitter2';
import {
MediaStream,
MediaStreamTrack,
RTCIceCandidate,
RTCPeerConnection,
RTCSessionDescription,
RTCView,
mediaDevices,
registerGlobals,
} from 'react-native-webrtc';
import Routes from '../../../app/constants/navigation/Routes';
import generateOTP from './utils/generateOTP.util';
import {
Expand All @@ -53,7 +42,6 @@ import {
waitForEmptyRPCQueue,
waitForKeychainUnlocked,
} from './utils/wait.util';

import { Json } from '@metamask/controller-utils';
import { PROTOCOLS } from '../../constants/deeplinks';
import { Minimizer } from '../NativeModules';
Expand All @@ -65,17 +53,6 @@ export const HOUR_IN_MS = MIN_IN_MS * 60;
export const DAY_IN_MS = HOUR_IN_MS * 24;
export const DEFAULT_SESSION_TIMEOUT_MS = 7 * DAY_IN_MS;

const webrtc = {
RTCPeerConnection,
RTCIceCandidate,
RTCSessionDescription,
RTCView,
MediaStream,
MediaStreamTrack,
mediaDevices,
registerGlobals,
};

export interface ConnectionProps {
id: string;
otherPublicKey: string;
Expand All @@ -84,6 +61,7 @@ export interface ConnectionProps {
initialConnection?: boolean;
originatorInfo?: OriginatorInfo;
validUntil: number;
lastAuthorized: number; // timestamp of last received activity
}
export interface ConnectedSessions {
[id: string]: Connection;
Expand Down Expand Up @@ -152,6 +130,11 @@ export class Connection extends EventEmitter2 {
isResumed = false;
initialConnection: boolean;

/*
* Timestamp of last activity, used to check if channel is still active and to prevent showing OTP approval modal too often.
*/
lastAuthorized: number;

/**
* Prevent double sending 'authorized' message.
*/
Expand Down Expand Up @@ -191,6 +174,7 @@ export class Connection extends EventEmitter2 {
rpcQueueManager,
originatorInfo,
approveHost,
lastAuthorized,
getApprovedHosts,
disapprove,
revalidate,
Expand All @@ -213,6 +197,7 @@ export class Connection extends EventEmitter2 {
super();
this.origin = origin;
this.channelId = id;
this.lastAuthorized = lastAuthorized;
this.reconnect = reconnect || false;
this.isResumed = false;
this.originatorInfo = originatorInfo;
Expand All @@ -233,7 +218,6 @@ export class Connection extends EventEmitter2 {
communicationServerUrl: AppConstants.MM_SDK.SERVER_URL,
communicationLayerPreference: CommunicationLayerPreference.SOCKET,
otherPublicKey,
webRTCLib: webrtc,
reconnect,
walletInfo: {
type: 'MetaMask Mobile',
Expand Down Expand Up @@ -324,30 +308,50 @@ export class Connection extends EventEmitter2 {
!this.initialConnection &&
this.origin === AppConstants.DEEPLINKS.ORIGIN_QR_CODE
) {
if (approvalController.get(this.channelId)) {
// cleaning previous pending approval
approvalController.reject(
this.channelId,
ethErrors.provider.userRejectedRequest(),
);
}
this.approvalPromise = undefined;
const currentTime = Date.now();
const channelWasActiveRecently =
!!this.lastAuthorized &&
currentTime - this.lastAuthorized < HOUR_IN_MS;

if (!this.otps) {
this.otps = generateOTP();
}
this.sendMessage({
type: MessageType.OTP,
otpAnswer: this.otps?.[0],
}).catch((err) => {
Logger.log(err, `SDKConnect:: Connection failed to send otp`);
});
// Prevent auto approval if metamask is killed and restarted
disapprove(this.channelId);
if (channelWasActiveRecently) {
this.approvalPromise = undefined;

// Always need to re-approve connection first.
await this.checkPermissions();
this.sendAuthorized(true);
// Prevent auto approval if metamask is killed and restarted
disapprove(this.channelId);

// Always need to re-approve connection first.
await this.checkPermissions({
lastAuthorized: this.lastAuthorized,
});

this.sendAuthorized(true);
} else {
if (approvalController.get(this.channelId)) {
// cleaning previous pending approval
approvalController.reject(
this.channelId,
ethErrors.provider.userRejectedRequest(),
);
}
this.approvalPromise = undefined;

if (!this.otps) {
this.otps = generateOTP();
}
this.sendMessage({
type: MessageType.OTP,
otpAnswer: this.otps?.[0],
}).catch((err) => {
Logger.log(err, `SDKConnect:: Connection failed to send otp`);
});
// Prevent auto approval if metamask is killed and restarted
disapprove(this.channelId);

// Always need to re-approve connection first.
await this.checkPermissions();
this.sendAuthorized(true);
this.lastAuthorized = Date.now();
}
} else if (
!this.initialConnection &&
this.origin === AppConstants.DEEPLINKS.ORIGIN_DEEPLINK
Expand Down Expand Up @@ -424,7 +428,7 @@ export class Connection extends EventEmitter2 {
// Wait for bridge to be ready before handling messages.
// It will wait until user accept/reject the connection request.
try {
await this.checkPermissions(message);
await this.checkPermissions({ message });
if (!this.receivedDisconnect) {
await waitForConnectionReadiness({ connection: this });
this.sendAuthorized();
Expand Down Expand Up @@ -630,9 +634,17 @@ export class Connection extends EventEmitter2 {
* @returns {boolean} true when host is approved or user approved the request.
* @throws error if the user reject approval request.
*/
private async checkPermissions(
_message?: CommunicationLayerMessage,
): Promise<boolean> {
private async checkPermissions({
// eslint-disable-next-line
message,
lastAuthorized,
}: {
message?: CommunicationLayerMessage;
lastAuthorized?: number;
} = {}): Promise<boolean> {
const channelWasActiveRecently =
!!lastAuthorized && Date.now() - lastAuthorized < HOUR_IN_MS;

// only ask approval if needed
const approved = this.isApproved({
channelId: this.channelId,
Expand Down Expand Up @@ -663,6 +675,10 @@ export class Connection extends EventEmitter2 {
this.revalidate({ channelId: this.channelId });
}

if (channelWasActiveRecently) {
return true;
}

const approvalRequest = {
origin: this.origin,
type: ApprovalTypes.CONNECT_ACCOUNTS,
Expand Down Expand Up @@ -812,6 +828,8 @@ export class SDKConnect extends EventEmitter2 {
await this.reconnect({
channelId: id,
initialConnection: false,
otherPublicKey:
this.connected[id].remote.getKeyInfo()?.ecies.otherPubKey ?? '',
context: 'connectToChannel',
});
return;
Expand All @@ -825,6 +843,7 @@ export class SDKConnect extends EventEmitter2 {
otherPublicKey,
origin,
validUntil: Date.now() + DEFAULT_SESSION_TIMEOUT_MS,
lastAuthorized: Date.now(),
};

const initialConnection = this.approvedHosts[id] === undefined;
Expand Down Expand Up @@ -876,7 +895,8 @@ export class SDKConnect extends EventEmitter2 {
connection.remote.on(EventType.CLIENTS_DISCONNECTED, () => {
const host = AppConstants.MM_SDK.SDK_REMOTE_ORIGIN + connection.channelId;
// Prevent disabled connection ( if user chose do not remember session )
if (this.disabledHosts[host] !== undefined) {
const isDisabled = this.disabledHosts[host]; // should be 0 when disabled.
if (isDisabled !== undefined) {
this.updateSDKLoadingState({
channelId: connection.channelId,
loading: false,
Expand All @@ -886,6 +906,8 @@ export class SDKConnect extends EventEmitter2 {
`SDKConnect::watchConnection can't update SDK loading state`,
);
});
// Force terminate connection since it was disabled (do not remember)
this.removeChannel(connection.channelId, true);
}
});

Expand Down Expand Up @@ -971,10 +993,12 @@ export class SDKConnect extends EventEmitter2 {

async reconnect({
channelId,
otherPublicKey,
initialConnection,
context,
}: {
channelId: string;
otherPublicKey: string;
context?: string;
initialConnection: boolean;
}) {
Expand All @@ -988,6 +1012,7 @@ export class SDKConnect extends EventEmitter2 {
} connecting=${connecting} socketConnected=${socketConnected} existingConnection=${
existingConnection !== undefined
}`,
otherPublicKey,
);

let interruptReason = '';
Expand Down Expand Up @@ -1026,6 +1051,7 @@ export class SDKConnect extends EventEmitter2 {
this.connecting[channelId] = true;
this.connected[channelId] = new Connection({
...connection,
otherPublicKey,
reconnect: true,
initialConnection,
rpcQueueManager: this.rpcqueueManager,
Expand All @@ -1044,7 +1070,9 @@ export class SDKConnect extends EventEmitter2 {
withKeyExchange: true,
});
this.watchConnection(this.connected[channelId]);
this.connecting[channelId] = false;
const afterConnected =
this.connected[channelId].remote.isConnected() ?? false;
this.connecting[channelId] = !afterConnected; // If not connected, it means it's connecting.
this.emit('refresh');
}

Expand All @@ -1058,6 +1086,7 @@ export class SDKConnect extends EventEmitter2 {
if (channelId) {
this.reconnect({
channelId,
otherPublicKey: this.connections[channelId].otherPublicKey,
initialConnection: false,
context: 'reconnectAll',
}).catch((err) => {
Expand Down Expand Up @@ -1142,7 +1171,7 @@ export class SDKConnect extends EventEmitter2 {

/**
* Invalidate a channel/session by preventing future connection to be established.
* Instead of removing the channel, it creates sets the session to timeout on next
* Instead of removing the channel, it sets the session to timeout on next
* connection which will remove it while conitnuing current session.
*
* @param channelId
Expand Down Expand Up @@ -1251,11 +1280,15 @@ export class SDKConnect extends EventEmitter2 {
}

private _approveHost({ host }: approveHostProps) {
const channelId = host.replace(AppConstants.MM_SDK.SDK_REMOTE_ORIGIN, '');
if (this.disabledHosts[host]) {
// Might be useful for future feature.
} else {
// Host is approved for 24h.
this.approvedHosts[host] = Date.now() + DAY_IN_MS;
if (this.connections[channelId]) {
this.connections[channelId].lastAuthorized = Date.now();
}
// Prevent disabled hosts from being persisted.
DefaultPreference.set(
AppConstants.MM_SDK.SDK_APPROVEDHOSTS,
Expand Down
Loading

0 comments on commit ce22421

Please sign in to comment.