Skip to content

Commit

Permalink
fix(auth): Support 2FA via browser window auth
Browse files Browse the repository at this point in the history
  • Loading branch information
Brendan Mulholland committed Oct 4, 2023
1 parent 4a5ffd9 commit a3768db
Show file tree
Hide file tree
Showing 4 changed files with 85 additions and 121 deletions.
9 changes: 6 additions & 3 deletions main.js
View file
Original file line number Diff line number Diff line change
@@ -1,11 +1,10 @@
const { handleAuthCallback } = require('src/utils/auth')
const { ipcMain, app, nativeTheme } = require('electron');
const { menubar } = require('menubar');
const { autoUpdater } = require('electron-updater');
const { onFirstRunMaybe } = require('./first-run');
const path = require('path');

require('@electron/remote/main').initialize()

app.setAppUserModelId('com.electron.gitify');

const iconIdle = path.join(
Expand All @@ -23,7 +22,6 @@ const browserWindowOpts = {
minHeight: 400,
resizable: false,
webPreferences: {
enableRemoteModule: true,
overlayScrollbars: true,
nodeIntegration: true,
contextIsolation: false,
Expand Down Expand Up @@ -90,6 +88,11 @@ menubarApp.on('ready', () => {
app.setLoginItemSettings(settings);
});

app.on('open-url', function(event, url) {
event.preventDefault();
handleAuthCallback(url);
});

menubarApp.window.webContents.on('devtools-opened', () => {
menubarApp.window.setSize(800, 600);
menubarApp.window.center();
Expand Down
5 changes: 4 additions & 1 deletion package.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,10 @@
"main.js",
"first-run.js"
],
"protocols": {
"name": "Gitify",
"schemes": ["gitify"]
},
"mac": {
"category": "public.app-category.developer-tools",
"icon": "assets/images/app-icon.icns",
Expand Down Expand Up @@ -93,7 +97,6 @@
"afterSign": "scripts/notarize.js"
},
"dependencies": {
"@electron/remote": "^2.0.11",
"@primer/octicons-react": "19.8.0",
"axios": "1.5.1",
"date-fns": "2.30.0",
Expand Down
108 changes: 53 additions & 55 deletions src/utils/auth.test.ts
View file
Original file line number Diff line number Diff line change
@@ -1,65 +1,63 @@
import { AxiosPromise, AxiosResponse } from 'axios';

import remote from '@electron/remote';
const browserWindow = new remote.BrowserWindow();

import * as auth from './auth';
import * as apiRequests from './api-requests';
import { AuthState } from '../types';

describe('utils/auth.tsx', () => {
describe('authGitHub', () => {
const loadURLMock = jest.spyOn(browserWindow, 'loadURL');

beforeEach(() => {
loadURLMock.mockReset();
});

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, 'http://github.com/?code=123-456');
}
});

const res = await auth.authGitHub();

expect(res.authCode).toBe('123-456');

expect(
browserWindow.webContents.session.clearStorageData,
).toHaveBeenCalledTimes(1);

expect(loadURLMock).toHaveBeenCalledTimes(1);
expect(loadURLMock).toHaveBeenCalledWith(
'https://github.com/login/oauth/authorize?client_id=FAKE_CLIENT_ID_123&scope=read:user,notifications,repo',
);

expect(browserWindow.destroy).toHaveBeenCalledTimes(1);
});

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, 'http://www.github.com/?error=Oops');
}
});

await expect(async () => await auth.authGitHub()).rejects.toEqual(
"Oops! Something went wrong and we couldn't log you in using Github. Please try again.",
);
expect(loadURLMock).toHaveBeenCalledTimes(1);
});
});
// TODO: how to test?
// describe('authGitHub', () => {
// const loadURLMock = jest.spyOn(browserWindow, 'loadURL');
//
// beforeEach(() => {
// loadURLMock.mockReset();
// });
//
// 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, 'http://github.com/?code=123-456');
// }
// });
//
// const res = await auth.authGitHub();
//
// expect(res.authCode).toBe('123-456');
//
// expect(
// browserWindow.webContents.session.clearStorageData,
// ).toHaveBeenCalledTimes(1);
//
// expect(loadURLMock).toHaveBeenCalledTimes(1);
// expect(loadURLMock).toHaveBeenCalledWith(
// 'https://github.com/login/oauth/authorize?client_id=FAKE_CLIENT_ID_123&scope=read:user,notifications,repo',
// );
//
// expect(browserWindow.destroy).toHaveBeenCalledTimes(1);
// });
//
// 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, 'http://www.github.com/?error=Oops');
// }
// });
//
// await expect(async () => await auth.authGitHub()).rejects.toEqual(
// "Oops! Something went wrong and we couldn't log you in using Github. Please try again.",
// );
// expect(loadURLMock).toHaveBeenCalledTimes(1);
// });
// });

describe('getToken', () => {
const authCode = '123-456';
Expand Down
84 changes: 22 additions & 62 deletions src/utils/auth.ts
View file
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
import { shell } from 'electron';
import { generateGitHubAPIUrl } from './helpers';

const { BrowserWindow } = require('@electron/remote');

import { apiRequest, apiRequestAuth } from '../utils/api-requests';
import { AuthResponse, AuthState, AuthTokenResponse } from '../types';
import { Constants } from '../utils/constants';
Expand All @@ -10,68 +9,29 @@ import { User } from '../typesGithub';
export const authGitHub = (
authOptions = Constants.DEFAULT_AUTH_OPTIONS,
): Promise<AuthResponse> => {
return new Promise((resolve, reject) => {
// Build the OAuth consent page URL
const authWindow = new BrowserWindow({
width: 548,
height: 736,
show: true,
});

const githubUrl = `https://${authOptions.hostname}/login/oauth/authorize`;
const authUrl = `${githubUrl}?client_id=${authOptions.clientId}&scope=${Constants.AUTH_SCOPE}`;

const session = authWindow.webContents.session;
session.clearStorageData();

authWindow.loadURL(authUrl);

const handleCallback = (url: string) => {
const raw_code = /code=([^&]*)/.exec(url) || null;
const authCode = raw_code && raw_code.length > 1 ? raw_code[1] : 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 });
} else if (error) {
reject(
"Oops! Something went wrong and we couldn't " +
'log you in using Github. Please try again.',
);
}
};

// If "Done" button is pressed, hide "Loading"
authWindow.on('close', () => {
authWindow.destroy();
});
const callbackUrl = encodeURIComponent("gitify://oauth-callback")

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}/.`,
);
}
},
);
const githubUrl = `https://${authOptions.hostname}/login/oauth/authorize`;
const authUrl = `${githubUrl}?client_id=${authOptions.clientId}&scope=${Constants.AUTH_SCOPE}&redirect_uri=${callbackUrl}`;

authWindow.webContents.on('will-redirect', (event, url) => {
event.preventDefault();
handleCallback(url);
});
shell.openExternal(authUrl);
};

authWindow.webContents.on('will-navigate', (event, url) => {
event.preventDefault();
handleCallback(url);
});
});
export const handleAuthCallback = (url: string) => {
const raw_code = /code=([^&]*)/.exec(url) || null;
const authCode = raw_code && raw_code.length > 1 ? raw_code[1] : null;
const error = /\?error=(.+)$/.exec(url);

// If there is a code, proceed to get token from github
if (authCode) {
const { token } = await getToken(authCode);
} else if (error) {
// TODO: Error handling
// reject(
// "Oops! Something went wrong and we couldn't " +
// 'log you in using Github. Please try again.',
// );
}
};

export const getUserData = async (
Expand All @@ -91,7 +51,7 @@ export const getUserData = async (
};
};

export const getToken = async (
const getToken = async (
authCode: string,
authOptions = Constants.DEFAULT_AUTH_OPTIONS,
): Promise<AuthTokenResponse> => {
Expand Down

0 comments on commit a3768db

Please sign in to comment.