diff --git a/config/webpack.config.renderer.base.ts b/config/webpack.config.renderer.base.ts index f1dc7e4bd..384e22993 100644 --- a/config/webpack.config.renderer.base.ts +++ b/config/webpack.config.renderer.base.ts @@ -40,10 +40,10 @@ const configuration: webpack.Configuration = { }, plugins: [ - // Development Keys - See README.md + // Development Keys - See CONTRIBUTING.md new webpack.EnvironmentPlugin({ - OAUTH_CLIENT_ID: '3fef4433a29c6ad8f22c', - OAUTH_CLIENT_SECRET: '9670de733096c15322183ff17ed0fc8704050379', + OAUTH_CLIENT_ID: 'Ov23liQIkFs5ehQLNzHF', + OAUTH_CLIENT_SECRET: '404b80632292e18419dbd2a6ed25976856e95255', }), // Extract CSS into a separate file diff --git a/package.json b/package.json index d8d883197..91d581adc 100644 --- a/package.json +++ b/package.json @@ -80,6 +80,12 @@ "package.json" ], "electronLanguages": ["en"], + "protocols": [ + { + "name": "Gitify", + "schemes": ["gitify"] + } + ], "mac": { "category": "public.app-category.developer-tools", "icon": "assets/images/app-icon.icns", diff --git a/src/main/main.ts b/src/main/main.ts index ffc8de619..4ce7e36de 100644 --- a/src/main/main.ts +++ b/src/main/main.ts @@ -20,7 +20,6 @@ const browserWindowOpts = { resizable: false, skipTaskbar: true, // Hide the app from the Windows taskbar webPreferences: { - enableRemoteModule: true, nodeIntegration: true, contextIsolation: false, }, @@ -37,11 +36,14 @@ const mb = menubar({ const menuBuilder = new MenuBuilder(mb); const contextMenu = menuBuilder.buildMenu(); -/** - * Electron Auto Updater only supports macOS and Windows - * https://github.com/electron/update-electron-app - */ +// Register your app as the handler for a custom protocol +app.setAsDefaultProtocolClient('gitify'); + if (isMacOS() || isWindows()) { + /** + * Electron Auto Updater only supports macOS and Windows + * https://github.com/electron/update-electron-app + */ const updater = new Updater(mb, menuBuilder); updater.initialize(); } @@ -54,13 +56,6 @@ app.whenReady().then(async () => { mb.on('ready', () => { mb.app.setAppUserModelId(APPLICATION.ID); - /** - * TODO: Remove @electron/remote use - see #650 - * GitHub OAuth 2 Login Flows - Enable Remote Browser Window Launch - */ - require('@electron/remote/main').initialize(); - require('@electron/remote/main').enable(mb.window.webContents); - // Tray configuration mb.tray.setToolTip(APPLICATION.NAME); mb.tray.setIgnoreDoubleClickEvents(true); @@ -172,3 +167,9 @@ app.whenReady().then(async () => { app.setLoginItemSettings(settings); }); }); + +// Handle gitify:// custom protocol URL events for OAuth 2.0 callback +app.on('open-url', (event, url) => { + event.preventDefault(); + mb.window.webContents.send(namespacedEvent('auth-callback'), url); +}); diff --git a/src/renderer/components/settings/AppearanceSettings.tsx b/src/renderer/components/settings/AppearanceSettings.tsx index 5d58ed82d..9d50ef66c 100644 --- a/src/renderer/components/settings/AppearanceSettings.tsx +++ b/src/renderer/components/settings/AppearanceSettings.tsx @@ -260,8 +260,8 @@ export const AppearanceSettings: FC = () => { updateSetting('showAccountHeader', evt.target.checked) } diff --git a/src/renderer/components/settings/NotificationSettings.tsx b/src/renderer/components/settings/NotificationSettings.tsx index 502c3949d..555ba3847 100644 --- a/src/renderer/components/settings/NotificationSettings.tsx +++ b/src/renderer/components/settings/NotificationSettings.tsx @@ -1,7 +1,7 @@ import { type FC, type MouseEvent, useContext } from 'react'; import { BellIcon } from '@primer/octicons-react'; -import { Stack, Text } from '@primer/react'; +import { Box, Stack, Text } from '@primer/react'; import { APPLICATION } from '../../../shared/constants'; import { AppContext } from '../../context/App'; @@ -61,9 +61,8 @@ export const NotificationSettings: FC = () => { tooltip={ See{' '} - {' '} + {' '} for more details. } diff --git a/src/renderer/routes/Accounts.tsx b/src/renderer/routes/Accounts.tsx index 50d542851..c6afe1abd 100644 --- a/src/renderer/routes/Accounts.tsx +++ b/src/renderer/routes/Accounts.tsx @@ -49,6 +49,10 @@ export const AccountsRoute: FC = () => { useContext(AppContext); const navigate = useNavigate(); + const [loadingStates, setLoadingStates] = useState>( + {}, + ); + const logoutAccount = useCallback( (account: Account) => { logoutFromAccount(account); @@ -65,6 +69,29 @@ export const AccountsRoute: FC = () => { navigate('/accounts', { replace: true }); }, []); + const handleRefresh = useCallback(async (account: Account) => { + const accountUUID = getAccountUUID(account); + + setLoadingStates((prev) => ({ + ...prev, + [accountUUID]: true, + })); + + await refreshAccount(account); + navigate('/accounts', { replace: true }); + + /** + * Typically the above refresh API call completes very quickly, + * so we add an brief artificial delay to allow the icon to spin a few times + */ + setTimeout(() => { + setLoadingStates((prev) => ({ + ...prev, + [accountUUID]: false, + })); + }, 500); + }, []); + const loginWithGitHub = useCallback(async () => { try { await loginWithGitHubApp(); @@ -89,11 +116,11 @@ export const AccountsRoute: FC = () => { {auth.accounts.map((account, i) => { const AuthMethodIcon = getAuthMethodIcon(account.method); const PlatformIcon = getPlatformIcon(account.platform); - const [isRefreshingAccount, setIsRefreshingAccount] = useState(false); + const accountUUID = getAccountUUID(account); return ( { title="Open account profile" onClick={() => openAccountProfile(account)} data-testid="account-profile" - className="pb-2" > { - + - + @@ -192,22 +218,9 @@ export const AccountsRoute: FC = () => { { - setIsRefreshingAccount(true); - - await refreshAccount(account); - navigate('/accounts', { replace: true }); - - /** - * Typically the above refresh API call completes very quickly, - * so we add an brief artificial delay to allow the icon to spin a few times - */ - setTimeout(() => { - setIsRefreshingAccount(false); - }, 500); - }} + onClick={() => handleRefresh(account)} size="small" - loading={isRefreshingAccount} + loading={loadingStates[accountUUID] || false} data-testid="account-refresh" /> diff --git a/src/renderer/routes/__snapshots__/Accounts.test.tsx.snap b/src/renderer/routes/__snapshots__/Accounts.test.tsx.snap index 5658d01f7..c1a1f0fc0 100644 --- a/src/renderer/routes/__snapshots__/Accounts.test.tsx.snap +++ b/src/renderer/routes/__snapshots__/Accounts.test.tsx.snap @@ -111,7 +111,7 @@ exports[`renderer/routes/Accounts.tsx Account interactions should render with PA > - + @@ -473,7 +473,7 @@ exports[`renderer/routes/Accounts.tsx Account interactions should render with PA > - + @@ -835,7 +835,7 @@ exports[`renderer/routes/Accounts.tsx Account interactions should render with PA > - + @@ -1348,7 +1348,7 @@ exports[`renderer/routes/Accounts.tsx Account interactions should set account as > - + @@ -1710,7 +1710,7 @@ exports[`renderer/routes/Accounts.tsx Account interactions should set account as > - + @@ -2072,7 +2072,7 @@ exports[`renderer/routes/Accounts.tsx Account interactions should set account as > - + @@ -2585,7 +2585,7 @@ exports[`renderer/routes/Accounts.tsx General should render itself & its childre > - + @@ -2947,7 +2947,7 @@ exports[`renderer/routes/Accounts.tsx General should render itself & its childre > - + @@ -3309,7 +3309,7 @@ exports[`renderer/routes/Accounts.tsx General should render itself & its childre > - + diff --git a/src/renderer/routes/__snapshots__/Settings.test.tsx.snap b/src/renderer/routes/__snapshots__/Settings.test.tsx.snap index e47d7138d..ea3684d94 100644 --- a/src/renderer/routes/__snapshots__/Settings.test.tsx.snap +++ b/src/renderer/routes/__snapshots__/Settings.test.tsx.snap @@ -518,30 +518,6 @@ exports[`renderer/routes/Settings.tsx should render itself & its children 1`] = -
- - -
{ describe('authGitHub', () => { - const loadURLMock = jest.spyOn(browserWindow, 'loadURL'); + const openExternalLinkMock = jest + .spyOn(comms, 'openExternalLink') + .mockImplementation(); afterEach(() => { jest.clearAllMocks(); }); - it('should call authGitHub - success', async () => { - // Casting to jest.Mock avoids Typescript errors, where the spy is expected to match all the original - // function's typing. I might fix all that if the return type of this was actually used, or if I was - // writing this test for a new feature. Since I'm just upgrading Jest, jest.Mock is a nice escape hatch - ( - jest.spyOn(browserWindow.webContents, 'on') as jest.Mock - ).mockImplementation((event, callback): void => { - if (event === 'will-redirect') { - const event = new Event('will-redirect'); - callback(event, 'https://github.com/?code=123-456'); + it('should call authGitHub - success auth flow', async () => { + const mockIpcRendererOn = ( + jest.spyOn(ipcRenderer, 'on') as jest.Mock + ).mockImplementation((event, callback) => { + if (event === 'gitify:auth-callback') { + callback(null, 'gitify://auth?code=123-456'); } }); const res = await auth.authGitHub(); + expect(openExternalLinkMock).toHaveBeenCalledTimes(1); + expect(openExternalLinkMock).toHaveBeenCalledWith( + 'https://github.com/login/oauth/authorize?client_id=FAKE_CLIENT_ID_123&scope=read%3Auser%2Cnotifications%2Crepo', + ); + + expect(mockIpcRendererOn).toHaveBeenCalledTimes(1); + expect(mockIpcRendererOn).toHaveBeenCalledWith( + 'gitify:auth-callback', + expect.any(Function), + ); + + expect(res.authMethod).toBe('GitHub App'); expect(res.authCode).toBe('123-456'); + }); - expect( - browserWindow.webContents.session.clearStorageData, - ).toHaveBeenCalledTimes(1); + it('should call authGitHub - success oauth flow', async () => { + const mockIpcRendererOn = ( + jest.spyOn(ipcRenderer, 'on') as jest.Mock + ).mockImplementation((event, callback) => { + if (event === 'gitify:auth-callback') { + callback(null, 'gitify://oauth?code=123-456'); + } + }); - expect(loadURLMock).toHaveBeenCalledTimes(1); - expect(loadURLMock).toHaveBeenCalledWith( - 'https://github.com/login/oauth/authorize?client_id=FAKE_CLIENT_ID_123&scope=read%3Auser%2Cnotifications%2Crepo', + const res = await auth.authGitHub({ + clientId: 'BYO_CLIENT_ID' as ClientID, + clientSecret: 'BYO_CLIENT_SECRET' as ClientSecret, + hostname: 'my.git.com' as Hostname, + }); + + expect(openExternalLinkMock).toHaveBeenCalledTimes(1); + expect(openExternalLinkMock).toHaveBeenCalledWith( + 'https://my.git.com/login/oauth/authorize?client_id=BYO_CLIENT_ID&scope=read%3Auser%2Cnotifications%2Crepo', + ); + + expect(mockIpcRendererOn).toHaveBeenCalledTimes(1); + expect(mockIpcRendererOn).toHaveBeenCalledWith( + 'gitify:auth-callback', + expect.any(Function), ); - expect(browserWindow.destroy).toHaveBeenCalledTimes(1); + expect(res.authMethod).toBe('OAuth App'); + expect(res.authCode).toBe('123-456'); }); it('should call authGitHub - failure', async () => { - ( - jest.spyOn(browserWindow.webContents, 'on') as jest.Mock - ).mockImplementation((event, callback): void => { - if (event === 'will-redirect') { - const event = new Event('will-redirect'); - callback(event, 'https://www.github.com/?error=Oops'); + const mockIpcRendererOn = ( + jest.spyOn(ipcRenderer, 'on') as jest.Mock + ).mockImplementation((event, callback) => { + if (event === 'gitify:auth-callback') { + callback( + null, + 'gitify://auth?error=invalid_request&error_description=The+redirect_uri+is+missing+or+invalid.&error_uri=https://docs.github.com/en/developers/apps/troubleshooting-oauth-errors', + ); } }); await expect(async () => await auth.authGitHub()).rejects.toEqual( - "Oops! Something went wrong and we couldn't log you in using GitHub. Please try again.", + "Oops! Something went wrong and we couldn't log you in using GitHub. Please try again. Reason: The redirect_uri is missing or invalid. Docs: https://docs.github.com/en/developers/apps/troubleshooting-oauth-errors", + ); + + expect(openExternalLinkMock).toHaveBeenCalledTimes(1); + expect(openExternalLinkMock).toHaveBeenCalledWith( + 'https://github.com/login/oauth/authorize?client_id=FAKE_CLIENT_ID_123&scope=read%3Auser%2Cnotifications%2Crepo', + ); + + expect(mockIpcRendererOn).toHaveBeenCalledTimes(1); + expect(mockIpcRendererOn).toHaveBeenCalledWith( + 'gitify:auth-callback', + expect.any(Function), ); - expect(loadURLMock).toHaveBeenCalledTimes(1); }); }); diff --git a/src/renderer/utils/auth/utils.ts b/src/renderer/utils/auth/utils.ts index 878ec920b..004974040 100644 --- a/src/renderer/utils/auth/utils.ts +++ b/src/renderer/utils/auth/utils.ts @@ -1,8 +1,9 @@ -import { BrowserWindow } from '@electron/remote'; import { format } from 'date-fns'; import semver from 'semver'; +import { ipcRenderer } from 'electron'; import { APPLICATION } from '../../../shared/constants'; +import { namespacedEvent } from '../../../shared/events'; import { logError, logWarn } from '../../../shared/logger'; import type { Account, @@ -17,78 +18,53 @@ import type { import type { UserDetails } from '../../typesGitHub'; import { getAuthenticatedUser } from '../api/client'; import { apiRequest } from '../api/request'; +import { openExternalLink } from '../comms'; import { Constants } from '../constants'; import { getPlatformFromHostname } from '../helpers'; import type { AuthMethod, AuthResponse, AuthTokenResponse } from './types'; -// TODO - Refactor our OAuth2 flow to use system browser and local app gitify://callback - see #485 #561 #654 export function authGitHub( authOptions = Constants.DEFAULT_AUTH_OPTIONS, ): Promise { return new Promise((resolve, reject) => { - // Build the OAuth consent page URL - const authWindow = new BrowserWindow({ - width: 548, - height: 736, - show: true, - }); - const authUrl = new URL(`https://${authOptions.hostname}`); authUrl.pathname = '/login/oauth/authorize'; authUrl.searchParams.append('client_id', authOptions.clientId); authUrl.searchParams.append('scope', Constants.AUTH_SCOPE.toString()); - const session = authWindow.webContents.session; - session.clearStorageData(); + openExternalLink(authUrl.toString() as Link); - authWindow.loadURL(authUrl.toString()); + const handleCallback = (callbackUrl: string) => { + const url = new URL(callbackUrl); - const handleCallback = (url: Link) => { - const raw_code = /code=([^&]*)/.exec(url) || null; - const authCode = - raw_code && raw_code.length > 1 ? (raw_code[1] as AuthCode) : null; - const error = /\?error=(.+)$/.exec(url); - if (authCode || error) { - // Close the browser if code found or error - authWindow.destroy(); - } - // If there is a code, proceed to get token from github - if (authCode) { - resolve({ authCode, authOptions }); + const type = url.hostname; + const code = url.searchParams.get('code'); + const error = url.searchParams.get('error'); + const errorDescription = url.searchParams.get('error_description'); + const errorUri = url.searchParams.get('error_uri'); + + if (code && (type === 'auth' || type === 'oauth')) { + const authMethod: AuthMethod = + type === 'auth' ? 'GitHub App' : 'OAuth App'; + + resolve({ + authMethod: authMethod, + authCode: code as AuthCode, + authOptions: authOptions, + }); } else if (error) { reject( - "Oops! Something went wrong and we couldn't " + - 'log you in using GitHub. Please try again.', + `Oops! Something went wrong and we couldn't log you in using GitHub. Please try again. Reason: ${errorDescription} Docs: ${errorUri}`, ); } }; - // If "Done" button is pressed, hide "Loading" - authWindow.on('close', () => { - authWindow.destroy(); - }); - - authWindow.webContents.on( - 'did-fail-load', - (_event, _errorCode, _errorDescription, validatedURL) => { - if (validatedURL.includes(authOptions.hostname)) { - authWindow.destroy(); - reject( - `Invalid Hostname. Could not load https://${authOptions.hostname}/.`, - ); - } + ipcRenderer.on( + namespacedEvent('auth-callback'), + (_, callbackUrl: string) => { + handleCallback(callbackUrl); }, ); - - authWindow.webContents.on('will-redirect', (event, url) => { - event.preventDefault(); - handleCallback(url as Link); - }); - - authWindow.webContents.on('will-navigate', (event, url) => { - event.preventDefault(); - handleCallback(url as Link); - }); }); } @@ -132,6 +108,8 @@ export async function addAccount( token: Token, hostname: Hostname, ): Promise { + const accountList = auth.accounts; + let newAccount = { hostname: hostname, method: method, @@ -140,9 +118,23 @@ export async function addAccount( } as Account; newAccount = await refreshAccount(newAccount); + const newAccountUUID = getAccountUUID(newAccount); + + const accountAlreadyExists = accountList.some( + (a) => getAccountUUID(a) === newAccountUUID, + ); + + if (accountAlreadyExists) { + logWarn( + 'addAccount', + `account for user ${newAccount.user.login} already exists`, + ); + } else { + accountList.push(newAccount); + } return { - accounts: [...auth.accounts, newAccount], + accounts: accountList, }; } @@ -249,11 +241,11 @@ export function getNewOAuthAppURL(hostname: Hostname): Link { ); newOAuthAppURL.searchParams.append( 'oauth_application[url]', - 'https://www.gitify.io', + 'https://gitify.io', ); newOAuthAppURL.searchParams.append( 'oauth_application[callback_url]', - 'https://www.gitify.io/callback', + 'gitify://oauth', ); return newOAuthAppURL.toString() as Link;