From ce2242175ccdca51fa24d0ed53f333bd07093bba Mon Sep 17 00:00:00 2001 From: Omri Dan <61094771+omridan159@users.noreply.github.com> Date: Wed, 27 Sep 2023 16:12:45 +0300 Subject: [PATCH] feat: extend the time we resume the session without showing OTP (#7212) **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 Co-authored-by: Christopher Ferreira Co-authored-by: Christopher Ferreira <104831203+christopherferreira9@users.noreply.github.com> --- android/app/build.gradle | 4 +- app/components/Nav/App/index.js | 12 ++- app/core/DeeplinkManager.js | 2 + app/core/SDKConnect/SDKConnect.ts | 139 +++++++++++++++---------- bitrise.yml | 10 +- ios/MetaMask.xcodeproj/project.pbxproj | 16 +-- package.json | 2 +- yarn.lock | 8 +- 8 files changed, 116 insertions(+), 77 deletions(-) diff --git a/android/app/build.gradle b/android/app/build.gradle index 931328d02e2..9b804ffd6f8 100644 --- a/android/app/build.gradle +++ b/android/app/build.gradle @@ -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" diff --git a/app/components/Nav/App/index.js b/app/components/Nav/App/index.js index 82e11794470..563cb2d1341 100644 --- a/app/components/Nav/App/index.js +++ b/app/components/Nav/App/index.js @@ -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') { @@ -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) { @@ -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; diff --git a/app/core/DeeplinkManager.js b/app/core/DeeplinkManager.js index 419355981f7..7fa73bbc8cd 100644 --- a/app/core/DeeplinkManager.js +++ b/app/core/DeeplinkManager.js @@ -257,6 +257,7 @@ class DeeplinkManager { } SDKConnect.getInstance().reconnect({ channelId: params.channelId, + otherPublicKey: params.pubkey, context: 'deeplink (universal)', }); } else { @@ -387,6 +388,7 @@ class DeeplinkManager { } SDKConnect.getInstance().reconnect({ channelId: params.channelId, + otherPublicKey: params.pubkey, context: 'deeplink (metamask)', }); } else { diff --git a/app/core/SDKConnect/SDKConnect.ts b/app/core/SDKConnect/SDKConnect.ts index b6aba28e2fb..a1f91e63c99 100644 --- a/app/core/SDKConnect/SDKConnect.ts +++ b/app/core/SDKConnect/SDKConnect.ts @@ -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, @@ -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 { @@ -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'; @@ -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; @@ -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; @@ -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. */ @@ -191,6 +174,7 @@ export class Connection extends EventEmitter2 { rpcQueueManager, originatorInfo, approveHost, + lastAuthorized, getApprovedHosts, disapprove, revalidate, @@ -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; @@ -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', @@ -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 @@ -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(); @@ -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 { + private async checkPermissions({ + // eslint-disable-next-line + message, + lastAuthorized, + }: { + message?: CommunicationLayerMessage; + lastAuthorized?: number; + } = {}): Promise { + const channelWasActiveRecently = + !!lastAuthorized && Date.now() - lastAuthorized < HOUR_IN_MS; + // only ask approval if needed const approved = this.isApproved({ channelId: this.channelId, @@ -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, @@ -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; @@ -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; @@ -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, @@ -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); } }); @@ -971,10 +993,12 @@ export class SDKConnect extends EventEmitter2 { async reconnect({ channelId, + otherPublicKey, initialConnection, context, }: { channelId: string; + otherPublicKey: string; context?: string; initialConnection: boolean; }) { @@ -988,6 +1012,7 @@ export class SDKConnect extends EventEmitter2 { } connecting=${connecting} socketConnected=${socketConnected} existingConnection=${ existingConnection !== undefined }`, + otherPublicKey, ); let interruptReason = ''; @@ -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, @@ -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'); } @@ -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) => { @@ -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 @@ -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, diff --git a/bitrise.yml b/bitrise.yml index e3b1df6404b..1993bdded1a 100644 --- a/bitrise.yml +++ b/bitrise.yml @@ -343,7 +343,7 @@ workflows: inputs: - deploy_path: $BITRISE_DEPLOY_DIR - is_compress: true - - zip_name: "E2E_Android_Failure_Screenshots" + - zip_name: 'E2E_Android_Failure_Screenshots' meta: bitrise.io: @@ -407,7 +407,7 @@ workflows: inputs: - deploy_path: $BITRISE_DEPLOY_DIR - is_compress: true - - zip_name: "E2E_IOS_Failure_Screenshots" + - zip_name: 'E2E_IOS_Failure_Screenshots' start_e2e_tests: steps: - build-router-start@0: @@ -765,7 +765,7 @@ workflows: inputs: - deploy_path: browserstack_uploaded_apps.json title: Bitrise Deploy Browserstack Uploaded Apps - + app: envs: - opts: @@ -800,10 +800,10 @@ app: PROJECT_LOCATION_IOS: ios - opts: is_expand: false - VERSION_NAME: 7.7.0 + VERSION_NAME: 7.8.0 - opts: is_expand: false - VERSION_NUMBER: 1171 + VERSION_NUMBER: 1173 - opts: is_expand: false ANDROID_APK_LINK: '' diff --git a/ios/MetaMask.xcodeproj/project.pbxproj b/ios/MetaMask.xcodeproj/project.pbxproj index fc86561f500..821e6fa446d 100644 --- a/ios/MetaMask.xcodeproj/project.pbxproj +++ b/ios/MetaMask.xcodeproj/project.pbxproj @@ -1039,7 +1039,7 @@ CODE_SIGN_ENTITLEMENTS = MetaMask/MetaMaskDebug.entitlements; CODE_SIGN_IDENTITY = "iPhone Developer"; CODE_SIGN_STYLE = Manual; - CURRENT_PROJECT_VERSION = 1171; + CURRENT_PROJECT_VERSION = 1173; DEAD_CODE_STRIPPING = YES; DEBUG_INFORMATION_FORMAT = dwarf; DEVELOPMENT_TEAM = 48XVW22RCG; @@ -1073,7 +1073,7 @@ ); LIBRARY_SEARCH_PATHS = "$(inherited)"; LLVM_LTO = YES; - MARKETING_VERSION = 7.7.0; + MARKETING_VERSION = 7.8.0; ONLY_ACTIVE_ARCH = YES; OTHER_CFLAGS = ( "$(inherited)", @@ -1105,7 +1105,7 @@ CODE_SIGN_ENTITLEMENTS = MetaMask/MetaMask.entitlements; CODE_SIGN_IDENTITY = "iPhone Distribution"; CODE_SIGN_STYLE = Manual; - CURRENT_PROJECT_VERSION = 1171; + CURRENT_PROJECT_VERSION = 1173; DEBUG_INFORMATION_FORMAT = "dwarf-with-dsym"; DEVELOPMENT_TEAM = 48XVW22RCG; "DEVELOPMENT_TEAM[sdk=iphoneos*]" = 48XVW22RCG; @@ -1139,7 +1139,7 @@ ); LIBRARY_SEARCH_PATHS = "$(inherited)"; LLVM_LTO = YES; - MARKETING_VERSION = 7.7.0; + MARKETING_VERSION = 7.8.0; ONLY_ACTIVE_ARCH = NO; OTHER_CFLAGS = ( "$(inherited)", @@ -1251,7 +1251,7 @@ CODE_SIGN_IDENTITY = "iPhone Developer"; "CODE_SIGN_IDENTITY[sdk=iphoneos*]" = "iPhone Distribution"; CODE_SIGN_STYLE = Manual; - CURRENT_PROJECT_VERSION = 1171; + CURRENT_PROJECT_VERSION = 1173; DEAD_CODE_STRIPPING = YES; DEBUG_INFORMATION_FORMAT = dwarf; DEVELOPMENT_TEAM = 48XVW22RCG; @@ -1288,7 +1288,7 @@ "\"$(SRCROOT)/MetaMask/System/Library/Frameworks\"", ); LLVM_LTO = YES; - MARKETING_VERSION = 7.7.0; + MARKETING_VERSION = 7.8.0; ONLY_ACTIVE_ARCH = YES; OTHER_CFLAGS = ( "$(inherited)", @@ -1320,7 +1320,7 @@ CODE_SIGN_ENTITLEMENTS = MetaMask/MetaMask.entitlements; CODE_SIGN_IDENTITY = "iPhone Distribution"; CODE_SIGN_STYLE = Manual; - CURRENT_PROJECT_VERSION = 1171; + CURRENT_PROJECT_VERSION = 1173; DEBUG_INFORMATION_FORMAT = "dwarf-with-dsym"; DEVELOPMENT_TEAM = 48XVW22RCG; "DEVELOPMENT_TEAM[sdk=iphoneos*]" = 48XVW22RCG; @@ -1357,7 +1357,7 @@ "\"$(SRCROOT)/MetaMask/System/Library/Frameworks\"", ); LLVM_LTO = YES; - MARKETING_VERSION = 7.7.0; + MARKETING_VERSION = 7.8.0; ONLY_ACTIVE_ARCH = NO; OTHER_CFLAGS = ( "$(inherited)", diff --git a/package.json b/package.json index 2c2620469a2..065a83b8451 100644 --- a/package.json +++ b/package.json @@ -178,7 +178,7 @@ "@metamask/phishing-controller": "^3.0.0", "@metamask/ppom-validator": "^0.5.0", "@metamask/preferences-controller": "^3.0.0", - "@metamask/sdk-communication-layer": "^0.5.0", + "@metamask/sdk-communication-layer": "^0.7.0", "@metamask/signature-controller": "4.0.1", "@metamask/swappable-obj-proxy": "^2.1.0", "@metamask/swaps-controller": "^6.8.0", diff --git a/yarn.lock b/yarn.lock index 9c6685aaec8..b3961feffc8 100644 --- a/yarn.lock +++ b/yarn.lock @@ -4587,10 +4587,10 @@ resolved "https://registry.yarnpkg.com/@metamask/safe-event-emitter/-/safe-event-emitter-3.0.0.tgz#8c2b9073fe0722d48693143b0dc8448840daa3bd" integrity sha512-j6Z47VOmVyGMlnKXZmL0fyvWfEYtKWCA9yGZkU3FCsGZUT5lHGmvaV9JA5F2Y+010y7+ROtR3WMXIkvl/nVzqQ== -"@metamask/sdk-communication-layer@^0.5.0": - version "0.5.2" - resolved "https://registry.yarnpkg.com/@metamask/sdk-communication-layer/-/sdk-communication-layer-0.5.2.tgz#fd94d457569b7ee984ad40b1c965d509d569269b" - integrity sha512-k4v2L3E+4nROROT1/3RROSiDLUAKNkJeHsi3nN8l2ao4P0c3JuaREFwfc/u3FtXA3gALKRRGnitn2drG4Xw6rA== +"@metamask/sdk-communication-layer@^0.7.0": + version "0.7.0" + resolved "https://registry.yarnpkg.com/@metamask/sdk-communication-layer/-/sdk-communication-layer-0.7.0.tgz#b97a341d620bc3a75862b5932c9b62e98d093946" + integrity sha512-jIv/9ujLdqZPfRyAc4Qi865TWk0EhORCDeaa36+r5ED4R84BiUIh1qgyZAW5+Kp4IMlWRjrclGfvrx1exiN3Gw== dependencies: cross-fetch "^3.1.5" date-fns "^2.29.3"