Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(auth): Support 2FA via browser window auth #654

Closed
wants to merge 5 commits into from
Closed

fix(auth): Support 2FA via browser window auth #654

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
0fc4a21
chore: Replace remote for setting auto-open pref
Oct 4, 2023
29fd59c
fix(auth): Support 2FA via browser window auth
Oct 4, 2023
69e6e49
Merge branch 'main' into 10-04-fix_auth_Support_2FA_via_browser_windo…
afonsojramos Oct 18, 2023
46b2a05
chore: apply prettier
afonsojramos Oct 18, 2023
e1e7ea3
Merge branch 'main' into 10-04-fix_auth_Support_2FA_via_browser_windo…
afonsojramos Nov 22, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 6 additions & 3 deletions main.js
View file
Open in desktop
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
4 changes: 4 additions & 0 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,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
108 changes: 53 additions & 55 deletions src/utils/auth.test.ts
View file
Open in desktop
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
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
import { BrowserWindow } from '@electron/remote';

import { shell } from 'electron';
import { generateGitHubAPIUrl } from './helpers';
import { apiRequest, apiRequestAuth } from '../utils/api-requests';
import { AuthResponse, AuthState, AuthTokenResponse } from '../types';
Expand All @@ -9,68 +8,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 @@ -90,7 +50,7 @@ export const getUserData = async (
};
};

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