From c53def3a00c9ba8149d2fa4bffc484fb5771183b Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Mon, 21 Sep 2020 14:53:29 -0700 Subject: [PATCH 01/10] webview: Prepare to "revive" realm in `handleInitialLoad`. In an upcoming commit, we'll change the `Auth` type to have a URL object for the realm. We call `JSON.stringify` on the arguments passed to `handleInitialLoad`. For the `realm` property, this means it'll be a string URL, not a URL object. Here, we pick it out in preparation for "reviving" it back into its URL form in the body of `handleInitialLoad`. --- src/webview/js/generatedEs3.js | 55 +++++++++++++++++++++++++++++++++- src/webview/js/js.js | 6 +++- 2 files changed, 59 insertions(+), 2 deletions(-) diff --git a/src/webview/js/generatedEs3.js b/src/webview/js/generatedEs3.js index bcd5acfe35a..3404fec6839 100644 --- a/src/webview/js/generatedEs3.js +++ b/src/webview/js/generatedEs3.js @@ -36,6 +36,55 @@ var compiledWebviewJs = (function (exports) { return Constructor; } + function _defineProperty(obj, key, value) { + if (key in obj) { + Object.defineProperty(obj, key, { + value: value, + enumerable: true, + configurable: true, + writable: true + }); + } else { + obj[key] = value; + } + + return obj; + } + + function ownKeys(object, enumerableOnly) { + var keys = Object.keys(object); + + if (Object.getOwnPropertySymbols) { + var symbols = Object.getOwnPropertySymbols(object); + if (enumerableOnly) symbols = symbols.filter(function (sym) { + return Object.getOwnPropertyDescriptor(object, sym).enumerable; + }); + keys.push.apply(keys, symbols); + } + + return keys; + } + + function _objectSpread2(target) { + for (var i = 1; i < arguments.length; i++) { + var source = arguments[i] != null ? arguments[i] : {}; + + if (i % 2) { + ownKeys(Object(source), true).forEach(function (key) { + _defineProperty(target, key, source[key]); + }); + } else if (Object.getOwnPropertyDescriptors) { + Object.defineProperties(target, Object.getOwnPropertyDescriptors(source)); + } else { + ownKeys(Object(source)).forEach(function (key) { + Object.defineProperty(target, key, Object.getOwnPropertyDescriptor(source, key)); + }); + } + } + + return target; + } + var sendMessage = (function (msg) { window.ReactNativeWebView.postMessage(JSON.stringify(msg)); }); @@ -610,7 +659,11 @@ var compiledWebviewJs = (function (exports) { sendScrollMessageIfListShort(); }; - var handleInitialLoad = function handleInitialLoad(platformOS, scrollMessageId, auth) { + var handleInitialLoad = function handleInitialLoad(platformOS, scrollMessageId, rawAuth) { + var auth = _objectSpread2(_objectSpread2({}, rawAuth), {}, { + realm: rawAuth.realm + }); + if (platformOS === 'ios') { window.addEventListener('message', handleMessageEvent); } else { diff --git a/src/webview/js/js.js b/src/webview/js/js.js index 30621d659f9..8789b419004 100644 --- a/src/webview/js/js.js +++ b/src/webview/js/js.js @@ -544,8 +544,12 @@ const handleUpdateEventContent = (uevent: WebViewUpdateEventContent) => { export const handleInitialLoad = ( platformOS: string, scrollMessageId: number | null, - auth: Auth, + // In an upcoming commit, the `realm` part of an `Auth` object will + // be a URL object. It'll be passed here in its stringified form. + rawAuth: {| ...$Diff, realm: string |}, ) => { + const auth: Auth = { ...rawAuth, realm: rawAuth.realm }; + // Since its version 5.x, the `react-native-webview` library dispatches our // `message` events at `window` on iOS but `document` on Android. if (platformOS === 'ios') { From 565e23e7479c21c235e835fe70ab66ae098c206a Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Thu, 20 Aug 2020 20:56:28 -0700 Subject: [PATCH 02/10] store: Replace/revive URL instances. Like we did in bfe794955, for ZulipVersion instances. We don't store any of these in Redux yet, but we will soon. --- src/boot/store.js | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/boot/store.js b/src/boot/store.js index 1104540fb2d..8eedfc7c138 100644 --- a/src/boot/store.js +++ b/src/boot/store.js @@ -271,6 +271,8 @@ const SERIALIZED_TYPE_FIELD_NAME: '__serializedType__' = '__serializedType__'; const customReplacer = (key, value, defaultReplacer) => { if (value instanceof ZulipVersion) { return { data: value.raw(), [SERIALIZED_TYPE_FIELD_NAME]: 'ZulipVersion' }; + } else if (value instanceof URL) { + return { data: value.toString(), [SERIALIZED_TYPE_FIELD_NAME]: 'URL' }; } return defaultReplacer(key, value); }; @@ -281,6 +283,8 @@ const customReviver = (key, value, defaultReviver) => { switch (value[SERIALIZED_TYPE_FIELD_NAME]) { case 'ZulipVersion': return new ZulipVersion(data); + case 'URL': + return new URL(data); default: // Fall back to defaultReviver, below } From 865914f0e3f00071257d544f1d164207a2907b66 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Thu, 20 Aug 2020 12:29:13 -0700 Subject: [PATCH 03/10] accounts: Make `Auth.realm` a URL object, including in `state.accounts`. Changing the `Auth` type implicitly changes the `Account` and `Identity` types, too. Done with an effort to minimize unnecessary follow-on changes in this commit. Here, we don't propagate the realm in its URL form as far toward the eventual consumers of the realm as we'll eventually want to; we'll do that in upcoming commits. Wherever two realm values are compared for equality with `===`, we make sure they are both stringified first. Flow didn't warn about these cases, so we caught them in test failures and by scanning through a full-project search for the term 'realm'. --- src/__tests__/lib/exampleData.js | 8 +++---- src/account-info/AccountDetails.js | 2 +- src/account/AccountList.js | 4 ++-- src/account/AccountPickScreen.js | 2 +- src/account/__tests__/accountsReducer-test.js | 24 +++++++++---------- src/account/accountMisc.js | 3 ++- src/account/accountsReducer.js | 18 ++++++++++---- src/api/apiFetch.js | 2 +- src/api/settings/getServerSettings.js | 2 +- src/api/transportTypes.js | 2 +- src/boot/store.js | 12 ++++++++++ src/common/OwnAvatar.js | 2 +- src/common/UserAvatarWithPresence.js | 2 +- src/common/WebLink.js | 2 +- src/emoji/__tests__/emojiSelectors-test.js | 8 +++---- src/lightbox/Lightbox.js | 2 +- src/message/fetchActions.js | 2 +- src/message/messagesActions.js | 6 ++--- src/nav/__tests__/navReducer-test.js | 10 ++++---- src/notification/index.js | 4 ++-- src/settings/LegalScreen.js | 2 +- src/start/AuthScreen.js | 4 ++-- src/start/DevAuthScreen.js | 2 +- src/start/PasswordAuthScreen.js | 2 +- src/start/__tests__/webAuth-test.js | 8 +++++-- src/start/webAuth.js | 2 +- src/utils/__tests__/url-test.js | 4 ++-- src/utils/url.js | 4 +++- src/webview/html/__tests__/render-test.js | 2 +- src/webview/html/messageAsHtml.js | 2 +- src/webview/js/__tests__/rewriteHtml.js | 4 ++-- src/webview/js/generatedEs3.js | 4 ++-- src/webview/js/js.js | 6 ++--- src/webview/js/rewriteHtml.js | 2 +- src/webview/webViewHandleUpdates.js | 2 +- 35 files changed, 98 insertions(+), 69 deletions(-) diff --git a/src/__tests__/lib/exampleData.js b/src/__tests__/lib/exampleData.js index 578ae3cc09b..a400c9c706c 100644 --- a/src/__tests__/lib/exampleData.js +++ b/src/__tests__/lib/exampleData.js @@ -123,7 +123,7 @@ export const makeCrossRealmBot = (args: { name?: string } = {}): CrossRealmBot = is_bot: true, }); -export const realm = 'https://zulip.example.org'; +export const realm = new URL('https://zulip.example.org'); export const zulipVersion = new ZulipVersion('2.1.0-234-g7c3acf4'); @@ -133,7 +133,7 @@ export const makeAccount = ( args: { user?: User, email?: string, - realm?: string, + realm?: URL, apiKey?: string, zulipFeatureLevel?: number | null, zulipVersion?: ZulipVersion | null, @@ -423,7 +423,7 @@ export const action = deepFreeze({ }, login_success: { type: LOGIN_SUCCESS, - realm: selfAccount.realm, + realm: selfAccount.realm.toString(), email: selfAccount.email, apiKey: selfAccount.apiKey, }, @@ -475,7 +475,7 @@ export const action = deepFreeze({ realm_send_welcome_emails: true, realm_show_digest_email: true, realm_signup_notifications_stream_id: 3, - realm_uri: selfAccount.realm, + realm_uri: selfAccount.realm.toString(), realm_video_chat_provider: 1, realm_waiting_period_threshold: 3, zulip_feature_level: 1, diff --git a/src/account-info/AccountDetails.js b/src/account-info/AccountDetails.js index 6bb38e002b5..ae06454b6d6 100644 --- a/src/account-info/AccountDetails.js +++ b/src/account-info/AccountDetails.js @@ -80,6 +80,6 @@ class AccountDetails extends PureComponent { } export default connect((state, props) => ({ - realm: getCurrentRealm(state), + realm: getCurrentRealm(state).toString(), userStatusText: getUserStatusTextForUser(state, props.user.user_id), }))(AccountDetails); diff --git a/src/account/AccountList.js b/src/account/AccountList.js index 7d7f187f795..b3e55426a3e 100644 --- a/src/account/AccountList.js +++ b/src/account/AccountList.js @@ -20,14 +20,14 @@ export default class AccountList extends PureComponent { `${item.email}${item.realm}`} + keyExtractor={item => `${item.email}${item.realm.toString()}`} ItemSeparatorComponent={() => } renderItem={({ item, index }) => ( diff --git a/src/account/AccountPickScreen.js b/src/account/AccountPickScreen.js index 1907de29c04..20a588f090c 100644 --- a/src/account/AccountPickScreen.js +++ b/src/account/AccountPickScreen.js @@ -25,7 +25,7 @@ class AccountPickScreen extends PureComponent { dispatch(switchAccount(index)); }); } else { - dispatch(navigateToRealmScreen(realm)); + dispatch(navigateToRealmScreen(realm.toString())); } }; diff --git a/src/account/__tests__/accountsReducer-test.js b/src/account/__tests__/accountsReducer-test.js index 062d78f3ecc..14782bedc0f 100644 --- a/src/account/__tests__/accountsReducer-test.js +++ b/src/account/__tests__/accountsReducer-test.js @@ -17,8 +17,8 @@ import * as eg from '../../__tests__/lib/exampleData'; describe('accountsReducer', () => { describe('REALM_ADD', () => { describe('on list of identities', () => { - const account1 = eg.makeAccount({ realm: 'https://realm.one.org', apiKey: '' }); - const account2 = eg.makeAccount({ realm: 'https://realm.two.org', apiKey: '' }); + const account1 = eg.makeAccount({ realm: new URL('https://realm.one.org'), apiKey: '' }); + const account2 = eg.makeAccount({ realm: new URL('https://realm.two.org'), apiKey: '' }); const prevState = deepFreeze([account1, account2]); const baseAction = deepFreeze({ type: REALM_ADD, @@ -27,8 +27,8 @@ describe('accountsReducer', () => { }); test('if no account with this realm exists, prepend new one, with empty email/apiKey', () => { - const newRealm = 'https://new.realm.org'; - const action = deepFreeze({ ...baseAction, realm: newRealm }); + const newRealm = new URL('https://new.realm.org'); + const action = deepFreeze({ ...baseAction, realm: newRealm.toString() }); expect(accountsReducer(prevState, action)).toEqual([ eg.makeAccount({ realm: newRealm, email: '', apiKey: '' }), account1, @@ -37,7 +37,7 @@ describe('accountsReducer', () => { }); test('if account with this realm exists, move to front of list', () => { - const action = deepFreeze({ ...baseAction, realm: account2.realm }); + const action = deepFreeze({ ...baseAction, realm: account2.realm.toString() }); expect(accountsReducer(prevState, action)).toEqual([account2, account1]); }); }); @@ -46,7 +46,7 @@ describe('accountsReducer', () => { const existingAccountBase = eg.makeAccount({}); const baseAction = deepFreeze({ type: REALM_ADD, - realm: existingAccountBase.realm, + realm: existingAccountBase.realm.toString(), zulipFeatureLevel: eg.zulipFeatureLevel, zulipVersion: eg.zulipVersion, }); @@ -153,8 +153,8 @@ describe('accountsReducer', () => { }); describe('LOGIN_SUCCESS', () => { - const account1 = eg.makeAccount({ email: '', realm: 'https://one.example.org' }); - const account2 = eg.makeAccount({ realm: 'https://two.example.org' }); + const account1 = eg.makeAccount({ email: '', realm: new URL('https://one.example.org') }); + const account2 = eg.makeAccount({ realm: new URL('https://two.example.org') }); const prevState = deepFreeze([account1, account2]); @@ -168,7 +168,7 @@ describe('accountsReducer', () => { type: LOGIN_SUCCESS, apiKey: newAccount.apiKey, email: newAccount.email, - realm: newAccount.realm, + realm: newAccount.realm.toString(), }); const expectedState = [{ ...newAccount, zulipVersion: account1.zulipVersion }, account2]; @@ -181,7 +181,7 @@ describe('accountsReducer', () => { test('on login, if account does not exist, add as first item', () => { const newAccount = eg.makeAccount({ email: 'newaccount@example.com', - realm: 'https://new.realm.org', + realm: new URL('https://new.realm.org'), zulipVersion: null, zulipFeatureLevel: null, }); @@ -190,7 +190,7 @@ describe('accountsReducer', () => { type: LOGIN_SUCCESS, apiKey: newAccount.apiKey, email: newAccount.email, - realm: newAccount.realm, + realm: newAccount.realm.toString(), }); const expectedState = [newAccount, account1, account2]; @@ -209,7 +209,7 @@ describe('accountsReducer', () => { const action = deepFreeze({ type: LOGIN_SUCCESS, apiKey: newAccount.apiKey, - realm: newAccount.realm, + realm: newAccount.realm.toString(), email: newAccount.email, }); diff --git a/src/account/accountMisc.js b/src/account/accountMisc.js index 5f2252906ff..0a535b9d170 100644 --- a/src/account/accountMisc.js +++ b/src/account/accountMisc.js @@ -8,7 +8,8 @@ export const identityOfAuth: Auth => Identity = identitySlice; export const identityOfAccount: Account => Identity = identitySlice; /** A string corresponding uniquely to an identity, for use in `Map`s. */ -export const keyOfIdentity = ({ realm, email }: Identity): string => `${realm}\0${email}`; +export const keyOfIdentity = ({ realm, email }: Identity): string => + `${realm.toString()}\0${email}`; export const authOfAccount = (account: Account): Auth => { const { realm, email, apiKey } = account; diff --git a/src/account/accountsReducer.js b/src/account/accountsReducer.js index b4cba842e05..102cb769ccd 100644 --- a/src/account/accountsReducer.js +++ b/src/account/accountsReducer.js @@ -16,7 +16,7 @@ import { NULL_ARRAY } from '../nullObjects'; const initialState = NULL_ARRAY; const realmAdd = (state, action) => { - const accountIndex = state.findIndex(account => account.realm === action.realm); + const accountIndex = state.findIndex(account => account.realm.toString() === action.realm); if (accountIndex !== -1) { const newAccount = { @@ -29,7 +29,7 @@ const realmAdd = (state, action) => { return [ { - realm: action.realm, + realm: new URL(action.realm), apiKey: '', email: '', ackedPushToken: null, @@ -60,16 +60,24 @@ const accountSwitch = (state, action) => { const findAccount = (state: AccountsState, identity: Identity): number => { const { realm, email } = identity; return state.findIndex( - account => account.realm === realm && (!account.email || account.email === email), + account => + account.realm.toString() === realm.toString() && (!account.email || account.email === email), ); }; const loginSuccess = (state, action) => { const { realm, email, apiKey } = action; - const accountIndex = findAccount(state, { realm, email }); + const accountIndex = findAccount(state, { realm: new URL(realm), email }); if (accountIndex === -1) { return [ - { realm, email, apiKey, ackedPushToken: null, zulipVersion: null, zulipFeatureLevel: null }, + { + realm: new URL(realm), + email, + apiKey, + ackedPushToken: null, + zulipVersion: null, + zulipFeatureLevel: null, + }, ...state, ]; } diff --git a/src/api/apiFetch.js b/src/api/apiFetch.js index 95e16b02189..a571464b3ed 100644 --- a/src/api/apiFetch.js +++ b/src/api/apiFetch.js @@ -46,7 +46,7 @@ export const apiFetch = async ( auth: Auth, route: string, params: $Diff<$Exact, {| headers: mixed |}>, -) => fetchWithAuth(auth, `${auth.realm}/${apiVersion}/${route}`, params); +) => fetchWithAuth(auth, `${auth.realm.toString()}/${apiVersion}/${route}`, params); export const apiCall = async ( auth: Auth, diff --git a/src/api/settings/getServerSettings.js b/src/api/settings/getServerSettings.js index 4afa0394e63..7f14ca8a6f6 100644 --- a/src/api/settings/getServerSettings.js +++ b/src/api/settings/getServerSettings.js @@ -43,4 +43,4 @@ export type ApiResponseServerSettings = {| /** See https://zulip.com/api/server-settings */ export default async (realm: string): Promise => - apiGet({ apiKey: '', email: '', realm }, 'server_settings'); + apiGet({ apiKey: '', email: '', realm: new URL(realm) }, 'server_settings'); diff --git a/src/api/transportTypes.js b/src/api/transportTypes.js index 25121027c1d..00776b00746 100644 --- a/src/api/transportTypes.js +++ b/src/api/transportTypes.js @@ -16,7 +16,7 @@ * an `Auth`. */ export type Auth = {| - realm: string, + realm: URL, apiKey: string, email: string, |}; diff --git a/src/boot/store.js b/src/boot/store.js index 8eedfc7c138..e3193d753ef 100644 --- a/src/boot/store.js +++ b/src/boot/store.js @@ -156,6 +156,8 @@ const migrations: { [string]: (GlobalState) => GlobalState } = { ...state, accounts: state.accounts.map(a => ({ ...a, + // `a.realm` is a string until migration 15 + // $FlowMigrationFudge realm: a.realm.replace(/\/+$/, ''), })), }), @@ -187,6 +189,16 @@ const migrations: { [string]: (GlobalState) => GlobalState } = { })), }), + // Convert Accounts[].realm from `string` to `URL` + '15': state => ({ + ...state, + accounts: state.accounts.map(a => ({ + ...a, + // $FlowMigrationFudge - `a.realm` will be a string here + realm: new URL(a.realm), + })), + }), + // TIP: When adding a migration, consider just using `dropCache`. }; diff --git a/src/common/OwnAvatar.js b/src/common/OwnAvatar.js index 93c2d472cd2..e3e85d33cbf 100644 --- a/src/common/OwnAvatar.js +++ b/src/common/OwnAvatar.js @@ -28,6 +28,6 @@ class OwnAvatar extends PureComponent { } export default connect(state => ({ - realm: getCurrentRealm(state), + realm: getCurrentRealm(state).toString(), user: getSelfUserDetail(state), }))(OwnAvatar); diff --git a/src/common/UserAvatarWithPresence.js b/src/common/UserAvatarWithPresence.js index 3ce7fa54a1e..c5168511b8d 100644 --- a/src/common/UserAvatarWithPresence.js +++ b/src/common/UserAvatarWithPresence.js @@ -60,5 +60,5 @@ class UserAvatarWithPresence extends PureComponent { } export default connect(state => ({ - realm: getCurrentRealm(state), + realm: getCurrentRealm(state).toString(), }))(UserAvatarWithPresence); diff --git a/src/common/WebLink.js b/src/common/WebLink.js index 957833617e9..98ddc91236d 100644 --- a/src/common/WebLink.js +++ b/src/common/WebLink.js @@ -44,5 +44,5 @@ class WebLink extends PureComponent { } export default connect(state => ({ - realm: getCurrentRealm(state), + realm: getCurrentRealm(state).toString(), }))(WebLink); diff --git a/src/emoji/__tests__/emojiSelectors-test.js b/src/emoji/__tests__/emojiSelectors-test.js index e4ac90cb710..b545860ba5f 100644 --- a/src/emoji/__tests__/emojiSelectors-test.js +++ b/src/emoji/__tests__/emojiSelectors-test.js @@ -11,7 +11,7 @@ describe('getActiveImageEmojiById', () => { const state = { accounts: [ { - realm: 'https://example.com', + realm: new URL('https://example.com'), }, ], realm: { @@ -56,7 +56,7 @@ describe('getActiveImageEmojiById', () => { describe('getAllImageEmojiById', () => { test('get realm emojis with absolute url', () => { const state = { - accounts: [{ realm: 'https://example.com' }], + accounts: [{ realm: new URL('https://example.com') }], realm: { emoji: { 1: { @@ -87,7 +87,7 @@ describe('getAllImageEmojiById', () => { describe('getAllImageEmojiByCode', () => { test('get realm emoji object with emoji names as the keys', () => { const state = { - accounts: [{ realm: 'https://example.com' }], + accounts: [{ realm: new URL('https://example.com') }], realm: { emoji: { 1: { @@ -129,7 +129,7 @@ describe('getAllImageEmojiByCode', () => { describe('getActiveImageEmojiByName', () => { test('get realm emoji object with emoji names as the keys', () => { const state = { - accounts: [{ realm: 'https://example.com' }], + accounts: [{ realm: new URL('https://example.com') }], realm: { emoji: { 1: { diff --git a/src/lightbox/Lightbox.js b/src/lightbox/Lightbox.js index c5d3132ded6..cd86cc397d3 100644 --- a/src/lightbox/Lightbox.js +++ b/src/lightbox/Lightbox.js @@ -115,7 +115,7 @@ class Lightbox extends PureComponent { diff --git a/src/message/fetchActions.js b/src/message/fetchActions.js index 07d2d4c355d..d223222278e 100644 --- a/src/message/fetchActions.js +++ b/src/message/fetchActions.js @@ -311,7 +311,7 @@ export const doInitialFetch = () => async (dispatch: Dispatch, getState: GetStat }, }), ), - tryFetch(() => api.getServerSettings(auth.realm)), + tryFetch(() => api.getServerSettings(auth.realm.toString())), ]); } catch (e) { // This should only happen on a 4xx HTTP status, which should only diff --git a/src/message/messagesActions.js b/src/message/messagesActions.js index 469296bae1f..f90ce348fa7 100644 --- a/src/message/messagesActions.js +++ b/src/message/messagesActions.js @@ -44,11 +44,11 @@ export const messageLinkPress = (href: string) => async ( const auth = getAuth(state); const usersById = getUsersById(state); const streamsById = getStreamsById(state); - const narrow = getNarrowFromLink(href, auth.realm, usersById, streamsById); + const narrow = getNarrowFromLink(href, auth.realm.toString(), usersById, streamsById); if (narrow) { - const anchor = getMessageIdFromLink(href, auth.realm); + const anchor = getMessageIdFromLink(href, auth.realm.toString()); dispatch(doNarrow(narrow, anchor)); - } else if (!isUrlOnRealm(href, auth.realm)) { + } else if (!isUrlOnRealm(href, auth.realm.toString())) { openLink(href); } else { const url = diff --git a/src/nav/__tests__/navReducer-test.js b/src/nav/__tests__/navReducer-test.js index 9aa83f6d44f..add69acfc58 100644 --- a/src/nav/__tests__/navReducer-test.js +++ b/src/nav/__tests__/navReducer-test.js @@ -130,7 +130,7 @@ describe('navReducer', () => { const action = deepFreeze({ type: REHYDRATE, payload: { - accounts: [{ apiKey: '', realm: 'https://example.com' }], + accounts: [{ apiKey: '', realm: new URL('https://example.com') }], users: [], realm: {}, }, @@ -147,8 +147,8 @@ describe('navReducer', () => { type: REHYDRATE, payload: { accounts: [ - { apiKey: '', realm: 'https://example.com', email: 'johndoe@example.com' }, - { apiKey: '', realm: 'https://example.com', email: 'janedoe@example.com' }, + { apiKey: '', realm: new URL('https://example.com'), email: 'johndoe@example.com' }, + { apiKey: '', realm: new URL('https://example.com'), email: 'janedoe@example.com' }, ], users: [], realm: {}, @@ -165,7 +165,9 @@ describe('navReducer', () => { const action = deepFreeze({ type: REHYDRATE, payload: { - accounts: [{ apiKey: '', realm: 'https://example.com', email: 'johndoe@example.com' }], + accounts: [ + { apiKey: '', realm: new URL('https://example.com'), email: 'johndoe@example.com' }, + ], users: [], realm: {}, }, diff --git a/src/notification/index.js b/src/notification/index.js index 3ed221f27b6..eab6e18b538 100644 --- a/src/notification/index.js +++ b/src/notification/index.js @@ -38,7 +38,7 @@ export const getAccountFromNotificationData = ( const urlMatches = []; identities.forEach((account, i) => { - if (account.realm === realm_uri) { + if (account.realm.toString() === realm_uri) { urlMatches.push(i); } }); @@ -49,7 +49,7 @@ export const getAccountFromNotificationData = ( // just a race -- this notification was sent before the logout); or // there's some confusion where the realm_uri we have is different from // the one the server sends in notifications. - const knownUrls = identities.map(({ realm }) => realm); + const knownUrls = identities.map(({ realm }) => realm.toString()); logging.warn('notification realm_uri not found in accounts', { realm_uri, known_urls: knownUrls, diff --git a/src/settings/LegalScreen.js b/src/settings/LegalScreen.js index 5306dc471f1..13689a3eb26 100644 --- a/src/settings/LegalScreen.js +++ b/src/settings/LegalScreen.js @@ -35,5 +35,5 @@ class LegalScreen extends PureComponent { } export default connect(state => ({ - realm: getCurrentRealm(state), + realm: getCurrentRealm(state).toString(), }))(LegalScreen); diff --git a/src/start/AuthScreen.js b/src/start/AuthScreen.js index 3d8c9e9b2a2..63c35412bfe 100644 --- a/src/start/AuthScreen.js +++ b/src/start/AuthScreen.js @@ -226,7 +226,7 @@ class AuthScreen extends PureComponent { const { dispatch, realm } = this.props; const auth = webAuth.authFromCallbackUrl(event.url, otp, realm); if (auth) { - dispatch(loginSuccess(auth.realm, auth.email, auth.apiKey)); + dispatch(loginSuccess(auth.realm.toString(), auth.email, auth.apiKey)); } }; @@ -347,5 +347,5 @@ class AuthScreen extends PureComponent { } export default connect(state => ({ - realm: getCurrentRealm(state), + realm: getCurrentRealm(state).toString(), }))(AuthScreen); diff --git a/src/start/DevAuthScreen.js b/src/start/DevAuthScreen.js index 10259c570a8..3b2cf1fd50d 100644 --- a/src/start/DevAuthScreen.js +++ b/src/start/DevAuthScreen.js @@ -73,7 +73,7 @@ class DevAuthScreen extends PureComponent { try { const { api_key } = await api.devFetchApiKey(partialAuth, email); - this.props.dispatch(loginSuccess(partialAuth.realm, email, api_key)); + this.props.dispatch(loginSuccess(partialAuth.realm.toString(), email, api_key)); this.setState({ progress: false }); } catch (err) { this.setState({ progress: false, error: err.data && err.data.msg }); diff --git a/src/start/PasswordAuthScreen.js b/src/start/PasswordAuthScreen.js index 21a4c7d4b52..6d90687f27d 100644 --- a/src/start/PasswordAuthScreen.js +++ b/src/start/PasswordAuthScreen.js @@ -57,7 +57,7 @@ class PasswordAuthScreen extends PureComponent { try { const fetchedKey = await api.fetchApiKey(partialAuth, email, password); this.setState({ progress: false }); - dispatch(loginSuccess(partialAuth.realm, fetchedKey.email, fetchedKey.api_key)); + dispatch(loginSuccess(partialAuth.realm.toString(), fetchedKey.email, fetchedKey.api_key)); } catch (err) { this.setState({ progress: false, diff --git a/src/start/__tests__/webAuth-test.js b/src/start/__tests__/webAuth-test.js index 4f64b856c88..2c7e654a858 100644 --- a/src/start/__tests__/webAuth-test.js +++ b/src/start/__tests__/webAuth-test.js @@ -3,11 +3,15 @@ import { authFromCallbackUrl } from '../webAuth'; describe('authFromCallbackUrl', () => { const otp = '13579bdf'; - const realm = 'https://chat.example'; + const realm = 'https://chat.example/'; test('success', () => { const url = `zulip://login?realm=${realm}&email=a@b&otp_encrypted_api_key=2636fdeb`; - expect(authFromCallbackUrl(url, otp, realm)).toEqual({ realm, email: 'a@b', apiKey: '5af4' }); + expect(authFromCallbackUrl(url, otp, realm)).toEqual({ + realm: new URL(realm), + email: 'a@b', + apiKey: '5af4', + }); }); test('wrong realm', () => { diff --git a/src/start/webAuth.js b/src/start/webAuth.js index 57bb861f314..568e151e6ac 100644 --- a/src/start/webAuth.js +++ b/src/start/webAuth.js @@ -98,7 +98,7 @@ export const authFromCallbackUrl = ( && otpEncryptedApiKey.length === otp.length ) { const apiKey = extractApiKey(otpEncryptedApiKey, otp); - return { realm, email, apiKey }; + return { realm: new URL(realm), email, apiKey }; } return null; diff --git a/src/utils/__tests__/url-test.js b/src/utils/__tests__/url-test.js index 57a0bed50fb..deac7ba6586 100644 --- a/src/utils/__tests__/url-test.js +++ b/src/utils/__tests__/url-test.js @@ -14,7 +14,7 @@ import type { AutocompletionDefaults } from '../url'; describe('getResource', () => { test('when uri contains domain, do not change, add auth headers', () => { const auth: Auth = { - realm: 'https://example.com/', + realm: new URL('https://example.com/'), apiKey: 'someApiKey', email: 'johndoe@example.com', }; @@ -31,7 +31,7 @@ describe('getResource', () => { }); const exampleAuth: Auth = { - realm: 'https://example.com', + realm: new URL('https://example.com'), email: 'nobody@example.org', apiKey: 'someApiKey', }; diff --git a/src/utils/url.js b/src/utils/url.js index 1266e98b183..dcccf230fb3 100644 --- a/src/utils/url.js +++ b/src/utils/url.js @@ -56,7 +56,9 @@ export const getResource = ( uri: string, auth: Auth, ): {| uri: string, headers?: { [string]: string } |} => - isUrlOnRealm(uri, auth.realm) ? getResourceWithAuth(uri, auth) : getResourceNoAuth(uri); + isUrlOnRealm(uri, auth.realm.toString()) + ? getResourceWithAuth(uri, auth) + : getResourceNoAuth(uri); export type Protocol = 'https://' | 'http://'; diff --git a/src/webview/html/__tests__/render-test.js b/src/webview/html/__tests__/render-test.js index 365af1b6fa9..8f429363c4b 100644 --- a/src/webview/html/__tests__/render-test.js +++ b/src/webview/html/__tests__/render-test.js @@ -11,6 +11,6 @@ describe('typing', () => { email: `${name}@example.com`, }; - expect(messageTypingAsHtml(eg.realm, [user])).not.toContain('&<'); + expect(messageTypingAsHtml(eg.realm.toString(), [user])).not.toContain('&<'); }); }); diff --git a/src/webview/html/messageAsHtml.js b/src/webview/html/messageAsHtml.js index 67c4e73c486..8b4b9e45641 100644 --- a/src/webview/html/messageAsHtml.js +++ b/src/webview/html/messageAsHtml.js @@ -118,7 +118,7 @@ $!${divOpenHtml} const { sender_full_name } = message; const sender_id = message.isOutbox ? backgroundData.ownUser.user_id : message.sender_id; - const avatarUrl = getAvatarFromMessage(message, backgroundData.auth.realm); + const avatarUrl = getAvatarFromMessage(message, backgroundData.auth.realm.toString()); const subheaderHtml = template`
diff --git a/src/webview/js/__tests__/rewriteHtml.js b/src/webview/js/__tests__/rewriteHtml.js index 0833a6bb5dc..2f48d81e8e1 100644 --- a/src/webview/js/__tests__/rewriteHtml.js +++ b/src/webview/js/__tests__/rewriteHtml.js @@ -5,7 +5,7 @@ import rewriteHtml from '../rewriteHtml'; import type { Auth } from '../../../types'; describe('rewriteHtml', () => { - const realm = 'https://realm.example.com'; + const realm = new URL('https://realm.example.com'); global.jsdom.reconfigure({ url: 'file:///nowhere_land/index.html' }); const auth: Auth = { @@ -27,7 +27,7 @@ describe('rewriteHtml', () => { const prefixes = { relative: '', 'root-relative': '/', - 'absolute on-realm': realm, + 'absolute on-realm': realm.toString(), 'absolute off-realm': 'https://example.org', }; diff --git a/src/webview/js/generatedEs3.js b/src/webview/js/generatedEs3.js index 3404fec6839..98f7042bf30 100644 --- a/src/webview/js/generatedEs3.js +++ b/src/webview/js/generatedEs3.js @@ -231,7 +231,7 @@ var compiledWebviewJs = (function (exports) { }); var rewriteImageUrls = function rewriteImageUrls(auth, element) { - var realm = new URL(auth.realm); + var realm = auth.realm; var imageTags = [].concat(element instanceof HTMLImageElement ? [element] : [], Array.from(element.getElementsByTagName('img'))); imageTags.forEach(function (img) { var actualSrc = img.getAttribute('src'); @@ -661,7 +661,7 @@ var compiledWebviewJs = (function (exports) { var handleInitialLoad = function handleInitialLoad(platformOS, scrollMessageId, rawAuth) { var auth = _objectSpread2(_objectSpread2({}, rawAuth), {}, { - realm: rawAuth.realm + realm: new URL(rawAuth.realm) }); if (platformOS === 'ios') { diff --git a/src/webview/js/js.js b/src/webview/js/js.js index 8789b419004..3760e9c40d2 100644 --- a/src/webview/js/js.js +++ b/src/webview/js/js.js @@ -544,11 +544,11 @@ const handleUpdateEventContent = (uevent: WebViewUpdateEventContent) => { export const handleInitialLoad = ( platformOS: string, scrollMessageId: number | null, - // In an upcoming commit, the `realm` part of an `Auth` object will - // be a URL object. It'll be passed here in its stringified form. + // The `realm` part of an `Auth` object is a URL object. It's passed + // in its stringified form. rawAuth: {| ...$Diff, realm: string |}, ) => { - const auth: Auth = { ...rawAuth, realm: rawAuth.realm }; + const auth: Auth = { ...rawAuth, realm: new URL(rawAuth.realm) }; // Since its version 5.x, the `react-native-webview` library dispatches our // `message` events at `window` on iOS but `document` on Android. diff --git a/src/webview/js/rewriteHtml.js b/src/webview/js/rewriteHtml.js index ec877e7aa8d..2ea2cc745e0 100644 --- a/src/webview/js/rewriteHtml.js +++ b/src/webview/js/rewriteHtml.js @@ -16,7 +16,7 @@ const inlineApiRoutes: RegExp[] = ['^/user_uploads/', '^/thumbnail$', '^/avatar/ * inject an API key into its query parameters. */ const rewriteImageUrls = (auth: Auth, element: Element | Document) => { - const realm = new URL(auth.realm); + const realm = auth.realm; // Find the image elements to act on. const imageTags: $ReadOnlyArray = [].concat( diff --git a/src/webview/webViewHandleUpdates.js b/src/webview/webViewHandleUpdates.js index 05a3e359d4c..eb7a07ec9a9 100644 --- a/src/webview/webViewHandleUpdates.js +++ b/src/webview/webViewHandleUpdates.js @@ -73,7 +73,7 @@ const updateTyping = (prevProps: Props, nextProps: Props): WebViewUpdateEventTyp type: 'typing', content: nextProps.typingUsers.length > 0 - ? messageTypingAsHtml(nextProps.backgroundData.auth.realm, nextProps.typingUsers) + ? messageTypingAsHtml(nextProps.backgroundData.auth.realm.toString(), nextProps.typingUsers) : '', }); From a3a83176758a0af81d7e0fcf075bdb8940785dd4 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Thu, 20 Aug 2020 14:05:08 -0700 Subject: [PATCH 04/10] webAuth: Pipe `Account[0].realm` as URL down to `authFromCallbackUrl`. As we'll do in some upcoming commits, stringify the realm exactly where we need it as a string. This slightly expands the area of our codebase in which it's easy to see that the realm is well-formed data. --- src/start/AuthScreen.js | 15 +++++---------- src/start/__tests__/webAuth-test.js | 14 +++++++------- src/start/webAuth.js | 11 +++-------- 3 files changed, 15 insertions(+), 25 deletions(-) diff --git a/src/start/AuthScreen.js b/src/start/AuthScreen.js index 63c35412bfe..985f182ba50 100644 --- a/src/start/AuthScreen.js +++ b/src/start/AuthScreen.js @@ -27,7 +27,7 @@ import styles from '../styles'; import { Centerer, Screen, ZulipButton } from '../common'; import { getCurrentRealm } from '../selectors'; import RealmInfo from './RealmInfo'; -import { encodeParamsForUrl, tryParseUrl } from '../utils/url'; +import { encodeParamsForUrl } from '../utils/url'; import * as webAuth from './webAuth'; import { loginSuccess, navigateToDev, navigateToPassword } from '../actions'; import IosCompliantAppleAuthButton from './IosCompliantAppleAuthButton'; @@ -168,7 +168,7 @@ export const activeAuthentications = ( type Props = $ReadOnly<{| dispatch: Dispatch, - realm: string, + realm: URL, navigation: NavigationScreenProp<{ params: {| serverSettings: ApiResponseServerSettings |} }>, |}>; @@ -260,7 +260,7 @@ class AuthScreen extends PureComponent { id_token: credential.identityToken, }); - openLink(`${this.props.realm}/complete/apple/?${params}`); + openLink(`${this.props.realm.toString()}/complete/apple/?${params}`); // Currently, the rest is handled with the `zulip://` redirect, // same as in the web flow. @@ -275,12 +275,7 @@ class AuthScreen extends PureComponent { return false; } - const host = tryParseUrl(this.props.realm)?.host; - if (host === undefined) { - // `this.props.realm` invalid. - // TODO: Check this much sooner. - return false; - } + const { host } = this.props.realm; // The native flow for Apple auth assumes that the app and the server // are operated by the same organization, so that for a user to @@ -347,5 +342,5 @@ class AuthScreen extends PureComponent { } export default connect(state => ({ - realm: getCurrentRealm(state).toString(), + realm: getCurrentRealm(state), }))(AuthScreen); diff --git a/src/start/__tests__/webAuth-test.js b/src/start/__tests__/webAuth-test.js index 2c7e654a858..d3395e955f5 100644 --- a/src/start/__tests__/webAuth-test.js +++ b/src/start/__tests__/webAuth-test.js @@ -1,14 +1,14 @@ /* @flow strict-local */ import { authFromCallbackUrl } from '../webAuth'; +import * as eg from '../../__tests__/lib/exampleData'; describe('authFromCallbackUrl', () => { const otp = '13579bdf'; - const realm = 'https://chat.example/'; test('success', () => { - const url = `zulip://login?realm=${realm}&email=a@b&otp_encrypted_api_key=2636fdeb`; - expect(authFromCallbackUrl(url, otp, realm)).toEqual({ - realm: new URL(realm), + const url = `zulip://login?realm=${eg.realm.toString()}&email=a@b&otp_encrypted_api_key=2636fdeb`; + expect(authFromCallbackUrl(url, otp, eg.realm)).toEqual({ + realm: eg.realm, email: 'a@b', apiKey: '5af4', }); @@ -17,13 +17,13 @@ describe('authFromCallbackUrl', () => { test('wrong realm', () => { const url = 'zulip://login?realm=https://other.example.org&email=a@b&otp_encrypted_api_key=2636fdeb'; - expect(authFromCallbackUrl(url, otp, realm)).toEqual(null); + expect(authFromCallbackUrl(url, otp, eg.realm)).toEqual(null); }); test('not login', () => { // Hypothetical link that isn't a login... but somehow with all the same // query params, for extra confusion for good measure. - const url = `zulip://message?realm=${realm}&email=a@b&otp_encrypted_api_key=2636fdeb`; - expect(authFromCallbackUrl(url, otp, realm)).toEqual(null); + const url = `zulip://message?realm=${eg.realm.toString()}&email=a@b&otp_encrypted_api_key=2636fdeb`; + expect(authFromCallbackUrl(url, otp, eg.realm)).toEqual(null); }); }); diff --git a/src/start/webAuth.js b/src/start/webAuth.js index 568e151e6ac..d818708675b 100644 --- a/src/start/webAuth.js +++ b/src/start/webAuth.js @@ -66,11 +66,7 @@ export const closeBrowser = () => { */ const extractApiKey = (encoded: string, otp: string) => hexToAscii(xorHexStrings(encoded, otp)); -export const authFromCallbackUrl = ( - callbackUrl: string, - otp: string, - realm: string, -): Auth | null => { +export const authFromCallbackUrl = (callbackUrl: string, otp: string, realm: URL): Auth | null => { // callback format expected: zulip://login?realm={}&email={}&otp_encrypted_api_key={} const url = tryParseUrl(callbackUrl); if (!url) { @@ -82,8 +78,7 @@ export const authFromCallbackUrl = ( return null; } const callbackRealm = tryParseUrl(callbackRealmStr); - // TODO: Check validity of `realm` much sooner - if (!callbackRealm || callbackRealm.origin !== new URL(realm).origin) { + if (!callbackRealm || callbackRealm.origin !== realm.origin) { return null; } @@ -98,7 +93,7 @@ export const authFromCallbackUrl = ( && otpEncryptedApiKey.length === otp.length ) { const apiKey = extractApiKey(otpEncryptedApiKey, otp); - return { realm: new URL(realm), email, apiKey }; + return { realm, email, apiKey }; } return null; From e5972a2cdf31fcd006bd5580f9f447daac0b3e52 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Thu, 20 Aug 2020 14:08:46 -0700 Subject: [PATCH 05/10] react: Pipe `Account[].realm` as URL to some React-component consumers. As in the previous and next commits, stringify the realm exactly where we need it as a string. This slightly expands the area of our codebase in which it's easy to see that the realm is well-formed data. --- src/account/AccountItem.js | 4 ++-- src/account/AccountList.js | 2 +- src/common/WebLink.js | 4 ++-- src/settings/LegalScreen.js | 4 ++-- 4 files changed, 7 insertions(+), 7 deletions(-) diff --git a/src/account/AccountItem.js b/src/account/AccountItem.js index a6ca72a55cc..9d7fca77fef 100644 --- a/src/account/AccountItem.js +++ b/src/account/AccountItem.js @@ -39,7 +39,7 @@ const styles = createStyleSheet({ type Props = $ReadOnly<{| index: number, email: string, - realm: string, + realm: URL, onSelect: (index: number) => void, onRemove: (index: number) => void, showDoneIcon: boolean, @@ -58,7 +58,7 @@ export default class AccountItem extends PureComponent { - + {!showDoneIcon ? ( diff --git a/src/account/AccountList.js b/src/account/AccountList.js index b3e55426a3e..01ef1585a1a 100644 --- a/src/account/AccountList.js +++ b/src/account/AccountList.js @@ -27,7 +27,7 @@ export default class AccountList extends PureComponent { index={index} showDoneIcon={index === 0 && item.isLoggedIn} email={item.email} - realm={item.realm.toString()} + realm={item.realm} onSelect={onAccountSelect} onRemove={onAccountRemove} /> diff --git a/src/common/WebLink.js b/src/common/WebLink.js index 98ddc91236d..3ae256f5b4a 100644 --- a/src/common/WebLink.js +++ b/src/common/WebLink.js @@ -13,7 +13,7 @@ type Props = $ReadOnly<{| dispatch: Dispatch, label: string, href: string, - realm: string, + realm: URL, |}>; /** @@ -44,5 +44,5 @@ class WebLink extends PureComponent { } export default connect(state => ({ - realm: getCurrentRealm(state).toString(), + realm: getCurrentRealm(state), }))(WebLink); diff --git a/src/settings/LegalScreen.js b/src/settings/LegalScreen.js index 13689a3eb26..b0a49a65131 100644 --- a/src/settings/LegalScreen.js +++ b/src/settings/LegalScreen.js @@ -10,7 +10,7 @@ import { getCurrentRealm } from '../selectors'; type Props = $ReadOnly<{| dispatch: Dispatch, - realm: string, + realm: URL, |}>; class LegalScreen extends PureComponent { @@ -35,5 +35,5 @@ class LegalScreen extends PureComponent { } export default connect(state => ({ - realm: getCurrentRealm(state).toString(), + realm: getCurrentRealm(state), }))(LegalScreen); From c852dd8208c0d76970434dd64a4bb5abf29cf785 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Thu, 20 Aug 2020 14:14:53 -0700 Subject: [PATCH 06/10] avatar: Pipe `Account[0].realm` as URL down to `getAvatarUrl`. As in the previous and next commits, stringify the realm exactly where we need it as a string. This slightly expands the area of our codebase in which it's easy to see that the realm is well-formed data. `getAvatarUrl` itself will likely disappear soon, when we make the shell crunchier for handling `.avatar_url` on users, cross-realm bots, and messages. But the changes we make to its callers, here, will continue to be beneficial after that. --- src/account-info/AccountDetails.js | 4 ++-- src/common/OwnAvatar.js | 4 ++-- src/common/UserAvatarWithPresence.js | 5 ++--- src/lightbox/Lightbox.js | 2 +- src/utils/avatar.js | 6 +++--- src/webview/html/__tests__/render-test.js | 2 +- src/webview/html/messageAsHtml.js | 2 +- src/webview/html/messageTypingAsHtml.js | 4 ++-- src/webview/webViewHandleUpdates.js | 2 +- 9 files changed, 15 insertions(+), 16 deletions(-) diff --git a/src/account-info/AccountDetails.js b/src/account-info/AccountDetails.js index ae06454b6d6..46150cf4ce2 100644 --- a/src/account-info/AccountDetails.js +++ b/src/account-info/AccountDetails.js @@ -28,7 +28,7 @@ const componentStyles = createStyleSheet({ const AVATAR_SIZE = 200; type SelectorProps = {| - realm: string, + realm: URL, userStatusText: string | void, |}; @@ -80,6 +80,6 @@ class AccountDetails extends PureComponent { } export default connect((state, props) => ({ - realm: getCurrentRealm(state).toString(), + realm: getCurrentRealm(state), userStatusText: getUserStatusTextForUser(state, props.user.user_id), }))(AccountDetails); diff --git a/src/common/OwnAvatar.js b/src/common/OwnAvatar.js index e3e85d33cbf..d767ea8cea1 100644 --- a/src/common/OwnAvatar.js +++ b/src/common/OwnAvatar.js @@ -11,7 +11,7 @@ type Props = $ReadOnly<{| dispatch: Dispatch, user: User, size: number, - realm: string, + realm: URL, |}>; /** @@ -28,6 +28,6 @@ class OwnAvatar extends PureComponent { } export default connect(state => ({ - realm: getCurrentRealm(state).toString(), + realm: getCurrentRealm(state), user: getSelfUserDetail(state), }))(OwnAvatar); diff --git a/src/common/UserAvatarWithPresence.js b/src/common/UserAvatarWithPresence.js index c5168511b8d..2b5cae6d049 100644 --- a/src/common/UserAvatarWithPresence.js +++ b/src/common/UserAvatarWithPresence.js @@ -23,7 +23,7 @@ type Props = $ReadOnly<{| avatarUrl: ?string, email: string, size: number, - realm: string, + realm: URL, shape: 'rounded' | 'square', onPress?: () => void, |}>; @@ -43,7 +43,6 @@ class UserAvatarWithPresence extends PureComponent { avatarUrl: '', email: '', size: 32, - realm: '', shape: 'rounded', }; @@ -60,5 +59,5 @@ class UserAvatarWithPresence extends PureComponent { } export default connect(state => ({ - realm: getCurrentRealm(state).toString(), + realm: getCurrentRealm(state), }))(UserAvatarWithPresence); diff --git a/src/lightbox/Lightbox.js b/src/lightbox/Lightbox.js index cd86cc397d3..c5d3132ded6 100644 --- a/src/lightbox/Lightbox.js +++ b/src/lightbox/Lightbox.js @@ -115,7 +115,7 @@ class Lightbox extends PureComponent { diff --git a/src/utils/avatar.js b/src/utils/avatar.js index 24d7b73b707..87d553374d3 100644 --- a/src/utils/avatar.js +++ b/src/utils/avatar.js @@ -24,7 +24,7 @@ export const getGravatarFromEmail = (email: string = '', size: number): string = export const getAvatarUrl = ( avatarUrl: ?string, email: string, - realm: string, + realm: URL, size: number = 80, ): string => { if (typeof avatarUrl !== 'string') { @@ -36,11 +36,11 @@ export const getAvatarUrl = ( return size > 100 ? getMediumAvatar(fullUrl) : fullUrl; }; -export const getAvatarFromUser = (user: UserOrBot, realm: string, size?: number): string => +export const getAvatarFromUser = (user: UserOrBot, realm: URL, size?: number): string => getAvatarUrl(user.avatar_url, user.email, realm, size); export const getAvatarFromMessage = ( message: Message | Outbox, - realm: string, + realm: URL, size?: number, ): string => getAvatarUrl(message.avatar_url, message.sender_email, realm, size); diff --git a/src/webview/html/__tests__/render-test.js b/src/webview/html/__tests__/render-test.js index 8f429363c4b..365af1b6fa9 100644 --- a/src/webview/html/__tests__/render-test.js +++ b/src/webview/html/__tests__/render-test.js @@ -11,6 +11,6 @@ describe('typing', () => { email: `${name}@example.com`, }; - expect(messageTypingAsHtml(eg.realm.toString(), [user])).not.toContain('&<'); + expect(messageTypingAsHtml(eg.realm, [user])).not.toContain('&<'); }); }); diff --git a/src/webview/html/messageAsHtml.js b/src/webview/html/messageAsHtml.js index 8b4b9e45641..67c4e73c486 100644 --- a/src/webview/html/messageAsHtml.js +++ b/src/webview/html/messageAsHtml.js @@ -118,7 +118,7 @@ $!${divOpenHtml} const { sender_full_name } = message; const sender_id = message.isOutbox ? backgroundData.ownUser.user_id : message.sender_id; - const avatarUrl = getAvatarFromMessage(message, backgroundData.auth.realm.toString()); + const avatarUrl = getAvatarFromMessage(message, backgroundData.auth.realm); const subheaderHtml = template`
diff --git a/src/webview/html/messageTypingAsHtml.js b/src/webview/html/messageTypingAsHtml.js index 1e2db871951..8360767b1d2 100644 --- a/src/webview/html/messageTypingAsHtml.js +++ b/src/webview/html/messageTypingAsHtml.js @@ -3,7 +3,7 @@ import template from './template'; import type { UserOrBot } from '../../types'; import { getAvatarFromUser } from '../../utils/avatar'; -const typingAvatar = (realm: string, user: UserOrBot): string => template` +const typingAvatar = (realm: URL, user: UserOrBot): string => template`
template`
`; -export default (realm: string, users: $ReadOnlyArray): string => template` +export default (realm: URL, users: $ReadOnlyArray): string => template` $!${users.map(user => typingAvatar(realm, user)).join('')}
diff --git a/src/webview/webViewHandleUpdates.js b/src/webview/webViewHandleUpdates.js index eb7a07ec9a9..05a3e359d4c 100644 --- a/src/webview/webViewHandleUpdates.js +++ b/src/webview/webViewHandleUpdates.js @@ -73,7 +73,7 @@ const updateTyping = (prevProps: Props, nextProps: Props): WebViewUpdateEventTyp type: 'typing', content: nextProps.typingUsers.length > 0 - ? messageTypingAsHtml(nextProps.backgroundData.auth.realm.toString(), nextProps.typingUsers) + ? messageTypingAsHtml(nextProps.backgroundData.auth.realm, nextProps.typingUsers) : '', }); From d04af7520211ad0e1dc4cd8b823a9d21bad3a53f Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Thu, 20 Aug 2020 14:40:40 -0700 Subject: [PATCH 07/10] getServerSettings: Pipe realm as URL down. As in the previous and next commits, stringify the realm exactly where we need it as a string. This slightly expands the area of our codebase in which it's easy to see that the realm is well-formed data. Also rename `this.state.realm` to `this.state.realmInputValue`, to be more descriptive. --- src/account/AccountPickScreen.js | 2 +- src/api/settings/getServerSettings.js | 4 ++-- src/message/fetchActions.js | 2 +- src/nav/navActions.js | 2 +- src/start/RealmScreen.js | 22 +++++++++++----------- 5 files changed, 16 insertions(+), 16 deletions(-) diff --git a/src/account/AccountPickScreen.js b/src/account/AccountPickScreen.js index 20a588f090c..1907de29c04 100644 --- a/src/account/AccountPickScreen.js +++ b/src/account/AccountPickScreen.js @@ -25,7 +25,7 @@ class AccountPickScreen extends PureComponent { dispatch(switchAccount(index)); }); } else { - dispatch(navigateToRealmScreen(realm.toString())); + dispatch(navigateToRealmScreen(realm)); } }; diff --git a/src/api/settings/getServerSettings.js b/src/api/settings/getServerSettings.js index 7f14ca8a6f6..f4a78ef44de 100644 --- a/src/api/settings/getServerSettings.js +++ b/src/api/settings/getServerSettings.js @@ -42,5 +42,5 @@ export type ApiResponseServerSettings = {| |}; /** See https://zulip.com/api/server-settings */ -export default async (realm: string): Promise => - apiGet({ apiKey: '', email: '', realm: new URL(realm) }, 'server_settings'); +export default async (realm: URL): Promise => + apiGet({ apiKey: '', email: '', realm }, 'server_settings'); diff --git a/src/message/fetchActions.js b/src/message/fetchActions.js index d223222278e..07d2d4c355d 100644 --- a/src/message/fetchActions.js +++ b/src/message/fetchActions.js @@ -311,7 +311,7 @@ export const doInitialFetch = () => async (dispatch: Dispatch, getState: GetStat }, }), ), - tryFetch(() => api.getServerSettings(auth.realm.toString())), + tryFetch(() => api.getServerSettings(auth.realm)), ]); } catch (e) { // This should only happen on a 4xx HTTP status, which should only diff --git a/src/nav/navActions.js b/src/nav/navActions.js index 1b5ec43e06b..5e9234b7215 100644 --- a/src/nav/navActions.js +++ b/src/nav/navActions.js @@ -51,7 +51,7 @@ export const navigateToAccountDetails = (userId: number): NavigationAction => export const navigateToGroupDetails = (recipients: UserOrBot[]): NavigationAction => StackActions.push({ routeName: 'group-details', params: { recipients } }); -export const navigateToRealmScreen = (realm?: string): NavigationAction => +export const navigateToRealmScreen = (realm?: URL): NavigationAction => StackActions.push({ routeName: 'realm', params: { realm } }); export const navigateToLightbox = (src: string, message: Message): NavigationAction => diff --git a/src/start/RealmScreen.js b/src/start/RealmScreen.js index 68eb3f69881..aecf8478253 100644 --- a/src/start/RealmScreen.js +++ b/src/start/RealmScreen.js @@ -18,7 +18,7 @@ type SelectorProps = {| type Props = $ReadOnly<{| navigation: NavigationScreenProp<{ params: ?{| - realm: string | void, + realm: URL | void, initial?: boolean, |}, }>, @@ -28,7 +28,7 @@ type Props = $ReadOnly<{| |}>; type State = {| - realm: string, + realmInputValue: string, error: string | null, progress: boolean, |}; @@ -36,16 +36,16 @@ type State = {| class RealmScreen extends PureComponent { state = { progress: false, - realm: this.props.initialRealm, + realmInputValue: this.props.initialRealm, error: null, }; scrollView: ScrollView; tryRealm = async () => { - const { realm } = this.state; + const { realmInputValue } = this.state; - const parsedRealm = tryParseUrl(realm); + const parsedRealm = tryParseUrl(realmInputValue); if (!parsedRealm) { this.setState({ error: 'Please enter a valid URL' }); return; @@ -61,10 +61,10 @@ class RealmScreen extends PureComponent { error: null, }); try { - const serverSettings: ApiResponseServerSettings = await api.getServerSettings(realm); + const serverSettings: ApiResponseServerSettings = await api.getServerSettings(parsedRealm); dispatch( realmAdd( - realm, + realmInputValue, serverSettings.zulip_feature_level ?? 0, new ZulipVersion(serverSettings.zulip_version), ), @@ -81,7 +81,7 @@ class RealmScreen extends PureComponent { } }; - handleRealmChange = (value: string) => this.setState({ realm: value }); + handleRealmChange = (value: string) => this.setState({ realmInputValue: value }); componentDidMount() { const { initialRealm } = this.props; @@ -92,7 +92,7 @@ class RealmScreen extends PureComponent { render() { const { initialRealm, navigation } = this.props; - const { progress, error, realm } = this.state; + const { progress, error, realmInputValue } = this.state; const styles = { input: { marginTop: 16, marginBottom: 8 }, @@ -131,7 +131,7 @@ class RealmScreen extends PureComponent { text="Enter" progress={progress} onPress={this.tryRealm} - disabled={!isValidUrl(realm)} + disabled={!isValidUrl(realmInputValue)} /> ); @@ -139,5 +139,5 @@ class RealmScreen extends PureComponent { } export default connect((state, props) => ({ - initialRealm: props.navigation.state.params?.realm ?? '', + initialRealm: props.navigation.state.params?.realm?.toString() ?? '', }))(RealmScreen); From 458dee2def4e95be4f8414e771833b95ab18dfc2 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Thu, 20 Aug 2020 17:30:32 -0700 Subject: [PATCH 08/10] url, internalLinks: Pipe realm as URL to `isUrlOnRealm`, `getPathsFromUrl`. As in the previous and next commits, stringify the realm exactly where we need it as a string. This slightly expands the area of our codebase in which it's easy to see that the realm is well-formed data. --- src/message/messagesActions.js | 6 +- src/utils/__tests__/internalLinks-test.js | 153 +++++++--------------- src/utils/__tests__/url-test.js | 14 +- src/utils/internalLinks.js | 20 +-- src/utils/url.js | 10 +- 5 files changed, 73 insertions(+), 130 deletions(-) diff --git a/src/message/messagesActions.js b/src/message/messagesActions.js index f90ce348fa7..469296bae1f 100644 --- a/src/message/messagesActions.js +++ b/src/message/messagesActions.js @@ -44,11 +44,11 @@ export const messageLinkPress = (href: string) => async ( const auth = getAuth(state); const usersById = getUsersById(state); const streamsById = getStreamsById(state); - const narrow = getNarrowFromLink(href, auth.realm.toString(), usersById, streamsById); + const narrow = getNarrowFromLink(href, auth.realm, usersById, streamsById); if (narrow) { - const anchor = getMessageIdFromLink(href, auth.realm.toString()); + const anchor = getMessageIdFromLink(href, auth.realm); dispatch(doNarrow(narrow, anchor)); - } else if (!isUrlOnRealm(href, auth.realm.toString())) { + } else if (!isUrlOnRealm(href, auth.realm)) { openLink(href); } else { const url = diff --git a/src/utils/__tests__/internalLinks-test.js b/src/utils/__tests__/internalLinks-test.js index 6af79ffa69c..80cd605e905 100644 --- a/src/utils/__tests__/internalLinks-test.js +++ b/src/utils/__tests__/internalLinks-test.js @@ -12,148 +12,95 @@ import { } from '../internalLinks'; import * as eg from '../../__tests__/lib/exampleData'; +const realm = new URL('https://example.com'); + describe('isInternalLink', () => { test('when link is external, return false', () => { - expect(isInternalLink('https://example.com', 'https://another.com')).toBe(false); + expect(isInternalLink('https://example.com', new URL('https://another.com'))).toBe(false); }); test('when link is internal, but not in app, return false', () => { - expect(isInternalLink('https://example.com/user_uploads', 'https://example.com')).toBe(false); + expect(isInternalLink('https://example.com/user_uploads', realm)).toBe(false); }); test('when link is internal and in app, return true', () => { - expect(isInternalLink('https://example.com/#narrow/stream/jest', 'https://example.com')).toBe( - true, - ); + expect(isInternalLink('https://example.com/#narrow/stream/jest', realm)).toBe(true); }); test('when link is relative and in app, return true', () => { - expect(isInternalLink('#narrow/stream/jest/topic/topic1', 'https://example.com')).toBe(true); - expect(isInternalLink('/#narrow/stream/jest', 'https://example.com')).toBe(true); + expect(isInternalLink('#narrow/stream/jest/topic/topic1', realm)).toBe(true); + expect(isInternalLink('/#narrow/stream/jest', realm)).toBe(true); }); test('links including IDs are also recognized', () => { - expect(isInternalLink('#narrow/stream/123-jest/topic/topic1', 'https://example.com')).toBe( - true, - ); - expect(isInternalLink('/#narrow/stream/123-jest', 'https://example.com')).toBe(true); - expect(isInternalLink('/#narrow/pm-with/123-mark', 'https://example.com')).toBe(true); + expect(isInternalLink('#narrow/stream/123-jest/topic/topic1', realm)).toBe(true); + expect(isInternalLink('/#narrow/stream/123-jest', realm)).toBe(true); + expect(isInternalLink('/#narrow/pm-with/123-mark', realm)).toBe(true); }); }); describe('isMessageLink', () => { test('only in-app link containing "near/" is a message link', () => { - expect(isMessageLink('https://example.com/#narrow/stream/jest', 'https://example.com')).toBe( - false, - ); - expect(isMessageLink('https://example.com/#narrow/#near/1', 'https://example.com')).toBe(true); + expect(isMessageLink('https://example.com/#narrow/stream/jest', realm)).toBe(false); + expect(isMessageLink('https://example.com/#narrow/#near/1', realm)).toBe(true); }); }); describe('getLinkType', () => { test('links to a different domain are of "external" type', () => { - expect(getLinkType('https://google.com/some-path', 'https://example.com')).toBe('external'); + expect(getLinkType('https://google.com/some-path', realm)).toBe('external'); }); test('only in-app link containing "stream" is a stream link', () => { - expect( - getLinkType('https://example.com/#narrow/pm-with/1,2-group', 'https://example.com'), - ).toBe('pm'); - expect(getLinkType('https://example.com/#narrow/stream/jest', 'https://example.com')).toBe( - 'stream', - ); - expect(getLinkType('https://example.com/#narrow/stream/stream/', 'https://example.com')).toBe( - 'stream', - ); + expect(getLinkType('https://example.com/#narrow/pm-with/1,2-group', realm)).toBe('pm'); + expect(getLinkType('https://example.com/#narrow/stream/jest', realm)).toBe('stream'); + expect(getLinkType('https://example.com/#narrow/stream/stream/', realm)).toBe('stream'); }); test('when a url is not a topic narrow return false', () => { - expect( - getLinkType('https://example.com/#narrow/pm-with/1,2-group', 'https://example.com'), - ).toBe('pm'); - expect(getLinkType('https://example.com/#narrow/stream/jest', 'https://example.com')).toBe( - 'stream', - ); - expect( - getLinkType( - 'https://example.com/#narrow/stream/stream/topic/topic/near/', - 'https://example.com', - ), - ).toBe('home'); - expect(getLinkType('https://example.com/#narrow/stream/topic/', 'https://example.com')).toBe( - 'stream', + expect(getLinkType('https://example.com/#narrow/pm-with/1,2-group', realm)).toBe('pm'); + expect(getLinkType('https://example.com/#narrow/stream/jest', realm)).toBe('stream'); + expect(getLinkType('https://example.com/#narrow/stream/stream/topic/topic/near/', realm)).toBe( + 'home', ); + expect(getLinkType('https://example.com/#narrow/stream/topic/', realm)).toBe('stream'); }); test('when a url is a topic narrow return true', () => { + expect(getLinkType('https://example.com/#narrow/stream/jest/topic/test', realm)).toBe('topic'); expect( - getLinkType('https://example.com/#narrow/stream/jest/topic/test', 'https://example.com'), - ).toBe('topic'); - expect( - getLinkType( - 'https://example.com/#narrow/stream/mobile/subject/topic/near/378333', - 'https://example.com', - ), - ).toBe('topic'); - expect( - getLinkType('https://example.com/#narrow/stream/mobile/topic/topic/', 'https://example.com'), - ).toBe('topic'); - expect( - getLinkType( - 'https://example.com/#narrow/stream/stream/topic/topic/near/1', - 'https://example.com', - ), + getLinkType('https://example.com/#narrow/stream/mobile/subject/topic/near/378333', realm), ).toBe('topic'); + expect(getLinkType('https://example.com/#narrow/stream/mobile/topic/topic/', realm)).toBe( + 'topic', + ); + expect(getLinkType('https://example.com/#narrow/stream/stream/topic/topic/near/1', realm)).toBe( + 'topic', + ); expect( - getLinkType( - 'https://example.com/#narrow/stream/stream/subject/topic/near/1', - 'https://example.com', - ), + getLinkType('https://example.com/#narrow/stream/stream/subject/topic/near/1', realm), ).toBe('topic'); - expect(getLinkType('/#narrow/stream/stream/subject/topic', 'https://example.com')).toBe( - 'topic', - ); + expect(getLinkType('/#narrow/stream/stream/subject/topic', realm)).toBe('topic'); }); test('only in-app link containing "pm-with" is a group link', () => { + expect(getLinkType('https://example.com/#narrow/stream/jest/topic/test', realm)).toBe('topic'); + expect(getLinkType('https://example.com/#narrow/pm-with/1,2-group', realm)).toBe('pm'); + expect(getLinkType('https://example.com/#narrow/pm-with/1,2-group/near/1', realm)).toBe('pm'); expect( - getLinkType('https://example.com/#narrow/stream/jest/topic/test', 'https://example.com'), - ).toBe('topic'); - expect( - getLinkType('https://example.com/#narrow/pm-with/1,2-group', 'https://example.com'), - ).toBe('pm'); - expect( - getLinkType('https://example.com/#narrow/pm-with/1,2-group/near/1', 'https://example.com'), - ).toBe('pm'); - expect( - getLinkType( - 'https://example.com/#narrow/pm-with/a.40b.2Ecom.2Ec.2Ed.2Ecom/near/3', - 'https://example.com', - ), + getLinkType('https://example.com/#narrow/pm-with/a.40b.2Ecom.2Ec.2Ed.2Ecom/near/3', realm), ).toBe('pm'); }); test('only in-app link containing "is" is a special link', () => { - expect( - getLinkType('https://example.com/#narrow/stream/jest/topic/test', 'https://example.com'), - ).toBe('topic'); - expect(getLinkType('https://example.com/#narrow/is/private', 'https://example.com')).toBe( - 'special', - ); - expect(getLinkType('https://example.com/#narrow/is/starred', 'https://example.com')).toBe( - 'special', - ); - expect(getLinkType('https://example.com/#narrow/is/mentioned', 'https://example.com')).toBe( - 'special', - ); - expect(getLinkType('https://example.com/#narrow/is/men', 'https://example.com')).toBe('home'); - expect(getLinkType('https://example.com/#narrow/is/men/stream', 'https://example.com')).toBe( - 'home', - ); - expect(getLinkType('https://example.com/#narrow/are/men/stream', 'https://example.com')).toBe( - 'home', - ); + expect(getLinkType('https://example.com/#narrow/stream/jest/topic/test', realm)).toBe('topic'); + expect(getLinkType('https://example.com/#narrow/is/private', realm)).toBe('special'); + expect(getLinkType('https://example.com/#narrow/is/starred', realm)).toBe('special'); + expect(getLinkType('https://example.com/#narrow/is/mentioned', realm)).toBe('special'); + expect(getLinkType('https://example.com/#narrow/is/men', realm)).toBe('home'); + expect(getLinkType('https://example.com/#narrow/is/men/stream', realm)).toBe('home'); + expect(getLinkType('https://example.com/#narrow/are/men/stream', realm)).toBe('home'); }); }); @@ -189,7 +136,7 @@ describe('getNarrowFromLink', () => { const get = (url, streams = []) => getNarrowFromLink( url, - 'https://example.com', + new URL('https://example.com'), usersById, new Map(streams.map(s => [s.stream_id, s])), ); @@ -343,26 +290,18 @@ describe('getNarrowFromLink', () => { describe('getMessageIdFromLink', () => { test('not message link', () => { - expect( - getMessageIdFromLink('https://example.com/#narrow/is/private', 'https://example.com'), - ).toBe(0); + expect(getMessageIdFromLink('https://example.com/#narrow/is/private', realm)).toBe(0); }); test('when link is a group link, return anchor message id', () => { expect( - getMessageIdFromLink( - 'https://example.com/#narrow/pm-with/1,3-group/near/1/', - 'https://example.com', - ), + getMessageIdFromLink('https://example.com/#narrow/pm-with/1,3-group/near/1/', realm), ).toBe(1); }); test('when link is a topic link, return anchor message id', () => { expect( - getMessageIdFromLink( - 'https://example.com/#narrow/stream/jest/topic/test/near/1', - 'https://example.com', - ), + getMessageIdFromLink('https://example.com/#narrow/stream/jest/topic/test/near/1', realm), ).toBe(1); }); }); diff --git a/src/utils/__tests__/url-test.js b/src/utils/__tests__/url-test.js index deac7ba6586..b27c5c197f0 100644 --- a/src/utils/__tests__/url-test.js +++ b/src/utils/__tests__/url-test.js @@ -59,20 +59,20 @@ describe('getResource', () => { }); describe('isUrlOnRealm', () => { + const realm = new URL('https://example.com'); + test('when link is on realm, return true', () => { - expect(isUrlOnRealm('/#narrow/stream/jest', 'https://example.com')).toBe(true); + expect(isUrlOnRealm('/#narrow/stream/jest', realm)).toBe(true); - expect(isUrlOnRealm('https://example.com/#narrow/stream/jest', 'https://example.com')).toBe( - true, - ); + expect(isUrlOnRealm('https://example.com/#narrow/stream/jest', realm)).toBe(true); - expect(isUrlOnRealm('#narrow/#near/1', 'https://example.com')).toBe(true); + expect(isUrlOnRealm('#narrow/#near/1', realm)).toBe(true); }); test('when link is on not realm, return false', () => { - expect(isUrlOnRealm('https://demo.example.com', 'https://example.com')).toBe(false); + expect(isUrlOnRealm('https://demo.example.com', realm)).toBe(false); - expect(isUrlOnRealm('www.google.com', 'https://example.com')).toBe(false); + expect(isUrlOnRealm('www.google.com', realm)).toBe(false); }); }); diff --git a/src/utils/internalLinks.js b/src/utils/internalLinks.js index b4a9d6fe614..9de0c7f0d94 100644 --- a/src/utils/internalLinks.js +++ b/src/utils/internalLinks.js @@ -4,9 +4,11 @@ import type { Narrow, Stream, User } from '../types'; import { topicNarrow, streamNarrow, groupNarrow, specialNarrow } from './narrow'; import { isUrlOnRealm } from './url'; -const getPathsFromUrl = (url: string = '', realm: string) => { +// TODO: Work out what this does, write a jsdoc for its interface, and +// reimplement using URL object (not just for the realm) +const getPathsFromUrl = (url: string = '', realm: URL) => { const paths = url - .split(realm) + .split(realm.toString()) .pop() .split('#narrow/') .pop() @@ -20,16 +22,18 @@ const getPathsFromUrl = (url: string = '', realm: string) => { }; /** PRIVATE -- exported only for tests. */ -export const isInternalLink = (url: string, realm: string): boolean => - isUrlOnRealm(url, realm) ? /^(\/#narrow|#narrow)/i.test(url.split(realm).pop()) : false; +export const isInternalLink = (url: string, realm: URL): boolean => + isUrlOnRealm(url, realm) + ? /^(\/#narrow|#narrow)/i.test(url.split(realm.toString()).pop()) + : false; /** PRIVATE -- exported only for tests. */ -export const isMessageLink = (url: string, realm: string): boolean => +export const isMessageLink = (url: string, realm: URL): boolean => isInternalLink(url, realm) && url.includes('near'); type LinkType = 'external' | 'home' | 'pm' | 'topic' | 'stream' | 'special'; -export const getLinkType = (url: string, realm: string): LinkType => { +export const getLinkType = (url: string, realm: URL): LinkType => { if (!isInternalLink(url, realm)) { return 'external'; } @@ -115,7 +119,7 @@ const parsePmOperand = (operand, usersById) => { export const getNarrowFromLink = ( url: string, - realm: string, + realm: URL, usersById: Map, streamsById: Map, ): Narrow | null => { @@ -141,7 +145,7 @@ export const getNarrowFromLink = ( } }; -export const getMessageIdFromLink = (url: string, realm: string): number => { +export const getMessageIdFromLink = (url: string, realm: URL): number => { const paths = getPathsFromUrl(url, realm); return isMessageLink(url, realm) ? parseInt(paths[paths.lastIndexOf('near') + 1], 10) : 0; diff --git a/src/utils/url.js b/src/utils/url.js index dcccf230fb3..bc06075992b 100644 --- a/src/utils/url.js +++ b/src/utils/url.js @@ -40,8 +40,10 @@ export const tryParseUrl = (url: string, base?: string | URL): URL | void => { } }; -export const isUrlOnRealm = (url: string = '', realm: string): boolean => - url.startsWith('/') || url.startsWith(realm) || !/^(http|www.)/i.test(url); +// TODO: Work out what this does, write a jsdoc for its interface, and +// reimplement using URL object (not just for the realm) +export const isUrlOnRealm = (url: string = '', realm: URL): boolean => + url.startsWith('/') || url.startsWith(realm.toString()) || !/^(http|www.)/i.test(url); const getResourceWithAuth = (uri: string, auth: Auth) => ({ uri: new URL(uri, auth.realm).toString(), @@ -56,9 +58,7 @@ export const getResource = ( uri: string, auth: Auth, ): {| uri: string, headers?: { [string]: string } |} => - isUrlOnRealm(uri, auth.realm.toString()) - ? getResourceWithAuth(uri, auth) - : getResourceNoAuth(uri); + isUrlOnRealm(uri, auth.realm) ? getResourceWithAuth(uri, auth) : getResourceNoAuth(uri); export type Protocol = 'https://' | 'http://'; From ed1be642807ac317833a582ef7556b8f3a8f7aef Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Thu, 20 Aug 2020 17:43:25 -0700 Subject: [PATCH 09/10] redux: In REALM_ADD and LOGIN_SUCCESS, make `realm` be a URL object. As in the preceding handful of commits, stringify the realm exactly where we need it as a string. These should be the last sites in which it's not totally clear that the realm is well-formed data. --- src/__tests__/lib/exampleData.js | 2 +- src/account/__tests__/accountsReducer-test.js | 12 ++++++------ src/account/accountActions.js | 4 ++-- src/account/accountsReducer.js | 17 ++++++----------- src/actionTypes.js | 4 ++-- src/start/AuthScreen.js | 2 +- src/start/DevAuthScreen.js | 2 +- src/start/PasswordAuthScreen.js | 2 +- src/start/RealmScreen.js | 2 +- 9 files changed, 21 insertions(+), 26 deletions(-) diff --git a/src/__tests__/lib/exampleData.js b/src/__tests__/lib/exampleData.js index a400c9c706c..5f2885600fb 100644 --- a/src/__tests__/lib/exampleData.js +++ b/src/__tests__/lib/exampleData.js @@ -423,7 +423,7 @@ export const action = deepFreeze({ }, login_success: { type: LOGIN_SUCCESS, - realm: selfAccount.realm.toString(), + realm: selfAccount.realm, email: selfAccount.email, apiKey: selfAccount.apiKey, }, diff --git a/src/account/__tests__/accountsReducer-test.js b/src/account/__tests__/accountsReducer-test.js index 14782bedc0f..ef8596e27dc 100644 --- a/src/account/__tests__/accountsReducer-test.js +++ b/src/account/__tests__/accountsReducer-test.js @@ -28,7 +28,7 @@ describe('accountsReducer', () => { test('if no account with this realm exists, prepend new one, with empty email/apiKey', () => { const newRealm = new URL('https://new.realm.org'); - const action = deepFreeze({ ...baseAction, realm: newRealm.toString() }); + const action = deepFreeze({ ...baseAction, realm: newRealm }); expect(accountsReducer(prevState, action)).toEqual([ eg.makeAccount({ realm: newRealm, email: '', apiKey: '' }), account1, @@ -37,7 +37,7 @@ describe('accountsReducer', () => { }); test('if account with this realm exists, move to front of list', () => { - const action = deepFreeze({ ...baseAction, realm: account2.realm.toString() }); + const action = deepFreeze({ ...baseAction, realm: account2.realm }); expect(accountsReducer(prevState, action)).toEqual([account2, account1]); }); }); @@ -46,7 +46,7 @@ describe('accountsReducer', () => { const existingAccountBase = eg.makeAccount({}); const baseAction = deepFreeze({ type: REALM_ADD, - realm: existingAccountBase.realm.toString(), + realm: existingAccountBase.realm, zulipFeatureLevel: eg.zulipFeatureLevel, zulipVersion: eg.zulipVersion, }); @@ -168,7 +168,7 @@ describe('accountsReducer', () => { type: LOGIN_SUCCESS, apiKey: newAccount.apiKey, email: newAccount.email, - realm: newAccount.realm.toString(), + realm: newAccount.realm, }); const expectedState = [{ ...newAccount, zulipVersion: account1.zulipVersion }, account2]; @@ -190,7 +190,7 @@ describe('accountsReducer', () => { type: LOGIN_SUCCESS, apiKey: newAccount.apiKey, email: newAccount.email, - realm: newAccount.realm.toString(), + realm: newAccount.realm, }); const expectedState = [newAccount, account1, account2]; @@ -209,7 +209,7 @@ describe('accountsReducer', () => { const action = deepFreeze({ type: LOGIN_SUCCESS, apiKey: newAccount.apiKey, - realm: newAccount.realm.toString(), + realm: newAccount.realm, email: newAccount.email, }); diff --git a/src/account/accountActions.js b/src/account/accountActions.js index ab948cdb4aa..37e40e99b91 100644 --- a/src/account/accountActions.js +++ b/src/account/accountActions.js @@ -15,7 +15,7 @@ export const switchAccount = (index: number): Action => ({ }); export const realmAdd = ( - realm: string, + realm: URL, zulipFeatureLevel: number, zulipVersion: ZulipVersion, ): Action => ({ @@ -30,7 +30,7 @@ export const removeAccount = (index: number): Action => ({ index, }); -export const loginSuccess = (realm: string, email: string, apiKey: string): Action => ({ +export const loginSuccess = (realm: URL, email: string, apiKey: string): Action => ({ type: LOGIN_SUCCESS, realm, email, diff --git a/src/account/accountsReducer.js b/src/account/accountsReducer.js index 102cb769ccd..c9c06e6705e 100644 --- a/src/account/accountsReducer.js +++ b/src/account/accountsReducer.js @@ -16,7 +16,9 @@ import { NULL_ARRAY } from '../nullObjects'; const initialState = NULL_ARRAY; const realmAdd = (state, action) => { - const accountIndex = state.findIndex(account => account.realm.toString() === action.realm); + const accountIndex = state.findIndex( + account => account.realm.toString() === action.realm.toString(), + ); if (accountIndex !== -1) { const newAccount = { @@ -29,7 +31,7 @@ const realmAdd = (state, action) => { return [ { - realm: new URL(action.realm), + realm: action.realm, apiKey: '', email: '', ackedPushToken: null, @@ -67,17 +69,10 @@ const findAccount = (state: AccountsState, identity: Identity): number => { const loginSuccess = (state, action) => { const { realm, email, apiKey } = action; - const accountIndex = findAccount(state, { realm: new URL(realm), email }); + const accountIndex = findAccount(state, { realm, email }); if (accountIndex === -1) { return [ - { - realm: new URL(realm), - email, - apiKey, - ackedPushToken: null, - zulipVersion: null, - zulipFeatureLevel: null, - }, + { realm, email, apiKey, ackedPushToken: null, zulipVersion: null, zulipFeatureLevel: null }, ...state, ]; } diff --git a/src/actionTypes.js b/src/actionTypes.js index 2a03fbb0b3e..db6394f565a 100644 --- a/src/actionTypes.js +++ b/src/actionTypes.js @@ -142,7 +142,7 @@ type AccountSwitchAction = {| type RealmAddAction = {| type: typeof REALM_ADD, - realm: string, + realm: URL, zulipFeatureLevel: number, zulipVersion: ZulipVersion, |}; @@ -154,7 +154,7 @@ type AccountRemoveAction = {| type LoginSuccessAction = {| type: typeof LOGIN_SUCCESS, - realm: string, + realm: URL, email: string, apiKey: string, |}; diff --git a/src/start/AuthScreen.js b/src/start/AuthScreen.js index 985f182ba50..a8a4a718d7a 100644 --- a/src/start/AuthScreen.js +++ b/src/start/AuthScreen.js @@ -226,7 +226,7 @@ class AuthScreen extends PureComponent { const { dispatch, realm } = this.props; const auth = webAuth.authFromCallbackUrl(event.url, otp, realm); if (auth) { - dispatch(loginSuccess(auth.realm.toString(), auth.email, auth.apiKey)); + dispatch(loginSuccess(auth.realm, auth.email, auth.apiKey)); } }; diff --git a/src/start/DevAuthScreen.js b/src/start/DevAuthScreen.js index 3b2cf1fd50d..10259c570a8 100644 --- a/src/start/DevAuthScreen.js +++ b/src/start/DevAuthScreen.js @@ -73,7 +73,7 @@ class DevAuthScreen extends PureComponent { try { const { api_key } = await api.devFetchApiKey(partialAuth, email); - this.props.dispatch(loginSuccess(partialAuth.realm.toString(), email, api_key)); + this.props.dispatch(loginSuccess(partialAuth.realm, email, api_key)); this.setState({ progress: false }); } catch (err) { this.setState({ progress: false, error: err.data && err.data.msg }); diff --git a/src/start/PasswordAuthScreen.js b/src/start/PasswordAuthScreen.js index 6d90687f27d..21a4c7d4b52 100644 --- a/src/start/PasswordAuthScreen.js +++ b/src/start/PasswordAuthScreen.js @@ -57,7 +57,7 @@ class PasswordAuthScreen extends PureComponent { try { const fetchedKey = await api.fetchApiKey(partialAuth, email, password); this.setState({ progress: false }); - dispatch(loginSuccess(partialAuth.realm.toString(), fetchedKey.email, fetchedKey.api_key)); + dispatch(loginSuccess(partialAuth.realm, fetchedKey.email, fetchedKey.api_key)); } catch (err) { this.setState({ progress: false, diff --git a/src/start/RealmScreen.js b/src/start/RealmScreen.js index aecf8478253..73194af3606 100644 --- a/src/start/RealmScreen.js +++ b/src/start/RealmScreen.js @@ -64,7 +64,7 @@ class RealmScreen extends PureComponent { const serverSettings: ApiResponseServerSettings = await api.getServerSettings(parsedRealm); dispatch( realmAdd( - realmInputValue, + parsedRealm, serverSettings.zulip_feature_level ?? 0, new ZulipVersion(serverSettings.zulip_version), ), From 05e24eeae0588d743ea97f646ad3b6edcc4f45bf Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Mon, 21 Sep 2020 17:46:46 -0700 Subject: [PATCH 10/10] url handling [nfc]: Add a few TODOs for clearer, better code. --- src/start/AuthScreen.js | 1 + src/utils/internalLinks.js | 6 ++++++ 2 files changed, 7 insertions(+) diff --git a/src/start/AuthScreen.js b/src/start/AuthScreen.js index a8a4a718d7a..695968ea839 100644 --- a/src/start/AuthScreen.js +++ b/src/start/AuthScreen.js @@ -260,6 +260,7 @@ class AuthScreen extends PureComponent { id_token: credential.identityToken, }); + // TODO: Use a `URL` computation, for #4146. openLink(`${this.props.realm.toString()}/complete/apple/?${params}`); // Currently, the rest is handled with the `zulip://` redirect, diff --git a/src/utils/internalLinks.js b/src/utils/internalLinks.js index 9de0c7f0d94..8d2c4c00f83 100644 --- a/src/utils/internalLinks.js +++ b/src/utils/internalLinks.js @@ -21,18 +21,24 @@ const getPathsFromUrl = (url: string = '', realm: URL) => { return paths; }; +// TODO: Work out what this does, write a jsdoc for its interface, and +// reimplement using URL object (not just for the realm) /** PRIVATE -- exported only for tests. */ export const isInternalLink = (url: string, realm: URL): boolean => isUrlOnRealm(url, realm) ? /^(\/#narrow|#narrow)/i.test(url.split(realm.toString()).pop()) : false; +// TODO: Work out what this does, write a jsdoc for its interface, and +// reimplement using URL object (not just for the realm) /** PRIVATE -- exported only for tests. */ export const isMessageLink = (url: string, realm: URL): boolean => isInternalLink(url, realm) && url.includes('near'); type LinkType = 'external' | 'home' | 'pm' | 'topic' | 'stream' | 'special'; +// TODO: Work out what this does, write a jsdoc for its interface, and +// reimplement using URL object (not just for the realm) export const getLinkType = (url: string, realm: URL): LinkType => { if (!isInternalLink(url, realm)) { return 'external';