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

Jump to bottom

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

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from
+85 −120
Draft

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.
Jump to
Jump to file
Failed to load files.
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 All @@ -72,7 +70,7 @@

apiRequestMock.mockResolvedValueOnce(requestPromise);

const res = await auth.getToken(authCode);

Check failure on line 73 in src/utils/auth.test.ts

View workflow job for this annotation

GitHub Actions / run-unit-tests 

Property 'getToken' does not exist on type 'typeof import("/home/runner/work/gitify/gitify/src/utils/auth")'.

expect(apiRequests.apiRequest).toHaveBeenCalledWith(
'https://github.com/login/oauth/access_token',
Expand All @@ -96,7 +94,7 @@

apiRequestMock.mockResolvedValueOnce(requestPromise);

const call = async () => await auth.getToken(authCode);

Check failure on line 97 in src/utils/auth.test.ts

View workflow job for this annotation

GitHub Actions / run-unit-tests 

Property 'getToken' does not exist on type 'typeof import("/home/runner/work/gitify/gitify/src/utils/auth")'.

await expect(call).rejects.toEqual({ data: { message } });
});
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 @@ -8,69 +7,30 @@

export const authGitHub = (
authOptions = Constants.DEFAULT_AUTH_OPTIONS,
): Promise<AuthResponse> => {

Check failure on line 10 in src/utils/auth.ts

View workflow job for this annotation

GitHub Actions / run-unit-tests 

A function whose declared type is neither 'undefined', 'void', nor 'any' must return a value.
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);

Check failure on line 26 in src/utils/auth.ts

View workflow job for this annotation

GitHub Actions / run-unit-tests 

'token' is declared but its value is never read.

Check failure on line 26 in src/utils/auth.ts

View workflow job for this annotation

GitHub Actions / run-unit-tests 

'await' expressions are only allowed within async functions and at the top levels of modules.
} 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 getToken = async (
const getToken = async (
authCode: string,
authOptions = Constants.DEFAULT_AUTH_OPTIONS,
): Promise<AuthTokenResponse> => {
Expand Down