-
Notifications
You must be signed in to change notification settings - Fork 2.2k
feat(rtn-web-browser): add auth support for chromebook #14523
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 10 commits
cf5cbb5
4663a61
fafcea2
5f009b5
14780ce
eb7ffeb
393712f
a7d23d1
48ec841
9dabafb
47bca61
6cd4bb9
e0e0237
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,78 @@ | ||
// Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. | ||
// SPDX-License-Identifier: Apache-2.0 | ||
|
||
import { Platform } from 'react-native'; | ||
|
||
import { nativeModule } from '../../nativeModule'; | ||
import { isChromebook, openAuthSessionAsync } from '../openAuthSessionAsync'; | ||
|
||
jest.mock('react-native', () => ({ | ||
Platform: { OS: 'ios' }, | ||
AppState: { addEventListener: jest.fn() }, | ||
Linking: { addEventListener: jest.fn() }, | ||
NativeModules: {}, | ||
})); | ||
|
||
jest.mock('../../nativeModule', () => ({ | ||
nativeModule: { openAuthSessionAsync: jest.fn() }, | ||
})); | ||
|
||
const mockPlatform = Platform as jest.Mocked<typeof Platform>; | ||
const mockNativeModule = nativeModule as jest.Mocked<typeof nativeModule>; | ||
|
||
describe('openAuthSessionAsync', () => { | ||
beforeEach(() => { | ||
jest.clearAllMocks(); | ||
mockPlatform.OS = 'ios'; | ||
}); | ||
|
||
describe('isChromebook', () => { | ||
soberm marked this conversation as resolved.
Show resolved
Hide resolved
|
||
it('returns false by default', async () => { | ||
const result = await isChromebook(); | ||
expect(result).toBe(false); | ||
}); | ||
}); | ||
|
||
describe('openAuthSessionAsync', () => { | ||
it('enforces HTTPS on URLs', async () => { | ||
mockNativeModule.openAuthSessionAsync.mockResolvedValue('result'); | ||
|
||
await openAuthSessionAsync('http://example.com', ['myapp://callback']); | ||
|
||
expect(mockNativeModule.openAuthSessionAsync).toHaveBeenCalledWith( | ||
'https://example.com', | ||
'myapp://callback', | ||
false, | ||
); | ||
}); | ||
|
||
it('calls iOS implementation', async () => { | ||
mockNativeModule.openAuthSessionAsync.mockResolvedValue('result'); | ||
|
||
const result = await openAuthSessionAsync( | ||
'https://example.com', | ||
['myapp://callback'], | ||
true, | ||
); | ||
|
||
expect(result).toBe('result'); | ||
expect(mockNativeModule.openAuthSessionAsync).toHaveBeenCalledWith( | ||
'https://example.com', | ||
'myapp://callback', | ||
true, | ||
); | ||
}); | ||
|
||
it('finds first non-web URL for redirect', async () => { | ||
const redirectUrls = ['https://web.com', 'myapp://deep']; | ||
|
||
await openAuthSessionAsync('https://example.com', redirectUrls); | ||
|
||
expect(mockNativeModule.openAuthSessionAsync).toHaveBeenCalledWith( | ||
'https://example.com', | ||
'myapp://deep', | ||
false, | ||
); | ||
}); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,6 +5,7 @@ import { | |
AppState, | ||
Linking, | ||
NativeEventSubscription, | ||
NativeModules, | ||
Platform, | ||
} from 'react-native'; | ||
|
||
|
@@ -13,6 +14,32 @@ import { nativeModule } from '../nativeModule'; | |
let appStateListener: NativeEventSubscription | undefined; | ||
let redirectListener: NativeEventSubscription | undefined; | ||
|
||
export async function isChromebook(): Promise<boolean> { | ||
// expo go | ||
try { | ||
const Device = require('expo-device'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't understand. do we expect users to install why not include it as a dependency? and if we cannot include it as a dependency, why is this logic needed at all? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We don't require There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. got it. thanks for the explanation |
||
if (Device?.hasPlatformFeatureAsync) { | ||
if (await Device.hasPlatformFeatureAsync('org.chromium.arc')) return true; | ||
if ( | ||
await Device.hasPlatformFeatureAsync( | ||
'org.chromium.arc.device_management', | ||
) | ||
) | ||
return true; | ||
} | ||
} catch { | ||
// not using Expo | ||
} | ||
|
||
// fallback to native module | ||
try { | ||
const nm = (NativeModules as any)?.ChromeOS; | ||
if (nm?.isChromeOS) return !!(await nm.isChromeOS()); | ||
} catch {} | ||
|
||
return false; | ||
} | ||
|
||
export const openAuthSessionAsync = async ( | ||
url: string, | ||
redirectUrls: string[], | ||
|
@@ -25,6 +52,15 @@ export const openAuthSessionAsync = async ( | |
} | ||
|
||
if (Platform.OS === 'android') { | ||
try { | ||
const isChromebookRes = await isChromebook(); | ||
if (isChromebookRes) { | ||
return openAuthSessionChromeOs(httpsUrl, redirectUrls); | ||
} | ||
} catch { | ||
// ignore and fallback to android | ||
} | ||
|
||
return openAuthSessionAndroid(httpsUrl, redirectUrls); | ||
} | ||
}; | ||
|
@@ -66,6 +102,27 @@ const openAuthSessionAndroid = async (url: string, redirectUrls: string[]) => { | |
} | ||
}; | ||
|
||
const openAuthSessionChromeOs = async (url: string, redirectUrls: string[]) => { | ||
try { | ||
const [redirectUrl] = await Promise.all([ | ||
Promise.race([ | ||
// wait for app to redirect, resulting in a redirectUrl | ||
getRedirectPromise(redirectUrls), | ||
// wait for app to return some other way, resulting in null | ||
getAppStatePromise(), | ||
]), | ||
Linking.openURL(url), | ||
]); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is hard to follow. this races between, 2 promises
and then this race is part of an all, which resolves when the race resolved and an url has been opened. doesn't
as
so that.
and then this makes another call to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. resolved this offline. the solution uses |
||
|
||
if (redirectUrl) Linking.openURL(redirectUrl); | ||
|
||
return redirectUrl; | ||
} finally { | ||
removeAppStateListener(); | ||
removeRedirectListener(); | ||
} | ||
}; | ||
|
||
const getAppStatePromise = (): Promise<null> => | ||
new Promise(resolve => { | ||
// remove any stray listeners before creating new ones | ||
|
Uh oh!
There was an error while loading. Please reload this page.