Skip to content

Commit fd834c3

Browse files
setchysetchy
authored
fix(auth): restore oauth2 flow functionality (#1456)
* fix(auth): restore oauth2 flow functionality Signed-off-by: Adam Setch <[email protected]> * fix(auth): restore oauth2 flow functionality Signed-off-by: Adam Setch <[email protected]> * test coverage Signed-off-by: Adam Setch <[email protected]> --------- Signed-off-by: Adam Setch <[email protected]>
1 parent 331ba10 commit fd834c3

File tree

7 files changed

+133
-35
lines changed

7 files changed

+133
-35
lines changed

‎src/electron/main.js‎

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -17,10 +17,6 @@ const { autoUpdater } = require('electron-updater');
1717
const { updateElectronApp } = require('update-electron-app');
1818

1919
log.initialize();
20-
21-
// TODO: Remove @electron/remote use - see #650
22-
require('@electron/remote/main').initialize();
23-
2420
// Tray Icons
2521
const idleIcon = path.resolve(
2622
`${__dirname}/../../assets/images/tray-idleTemplate.png`,
@@ -124,6 +120,13 @@ app.whenReady().then(async () => {
124120
mb.on('ready', () => {
125121
mb.app.setAppUserModelId('com.electron.gitify');
126122

123+
/**
124+
* TODO: Remove @electron/remote use - see #650
125+
* GitHub OAuth 2 Login Flows - Enable Remote Browser Window Launch
126+
*/
127+
require('@electron/remote/main').initialize();
128+
require('@electron/remote/main').enable(mb.window.webContents);
129+
127130
// Tray configuration
128131
mb.tray.setToolTip('Gitify');
129132
mb.tray.setIgnoreDoubleClickEvents(true);

‎src/routes/Login.tsx‎

Lines changed: 15 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
1-
import { KeyIcon, PersonIcon } from '@primer/octicons-react';
2-
import { type FC, useContext, useEffect } from 'react';
1+
import { KeyIcon, MarkGithubIcon, PersonIcon } from '@primer/octicons-react';
2+
import log from 'electron-log';
3+
import { type FC, useCallback, useContext, useEffect } from 'react';
34
import { useNavigate } from 'react-router-dom';
45
import { Button } from '../components/buttons/Button';
56
import { LogoIcon } from '../components/icons/LogoIcon';
@@ -9,7 +10,7 @@ import { showWindow } from '../utils/comms';
910

1011
export const LoginRoute: FC = () => {
1112
const navigate = useNavigate();
12-
const { isLoggedIn } = useContext(AppContext);
13+
const { loginWithGitHubApp, isLoggedIn } = useContext(AppContext);
1314

1415
useEffect(() => {
1516
if (isLoggedIn) {
@@ -18,14 +19,13 @@ export const LoginRoute: FC = () => {
1819
}
1920
}, [isLoggedIn]);
2021

21-
// FIXME: Temporarily disable Login with GitHub (OAuth) as it's currently broken and requires a rewrite - see #485 #561 #747
22-
/* const loginUser = useCallback(async () => {
22+
const loginUser = useCallback(async () => {
2323
try {
24-
await login();
24+
await loginWithGitHubApp();
2525
} catch (err) {
2626
log.error('Auth: failed to login with GitHub', err);
2727
}
28-
}, []); */
28+
}, [loginWithGitHubApp]);
2929

3030
return (
3131
<div className="flex flex-1 flex-col items-center justify-center p-4">
@@ -36,18 +36,16 @@ export const LoginRoute: FC = () => {
3636
</div>
3737

3838
<div className="text-center text-sm font-semibold italic">Login with</div>
39-
{
40-
// FIXME: Temporarily disable Login with GitHub (OAuth) as it's currently broken and requires a rewrite - see #485 #561 #747
41-
/*
42-
<Button
39+
40+
<Button
4341
name="GitHub"
44-
icon={MarkGitHubIcon}
42+
icon={{ icon: MarkGithubIcon }}
4543
label="Login with GitHub"
46-
class="w-50 px-2 py-2 my-2 text-xs"
47-
onClick={loginUser}
48-
/>
49-
*/
50-
}
44+
className="mt-2 py-2"
45+
onClick={() => loginUser()}
46+
>
47+
GitHub
48+
</Button>
5149

5250
<Button
5351
icon={{ icon: KeyIcon }}

‎src/routes/LoginWithOAuthApp.test.tsx‎

Lines changed: 36 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { fireEvent, render, screen } from '@testing-library/react';
1+
import { fireEvent, render, screen, waitFor } from '@testing-library/react';
22
import { MemoryRouter } from 'react-router-dom';
33
import { AppContext } from '../context/App';
44
import type { AuthState, ClientID, ClientSecret, Hostname } from '../types';
@@ -12,6 +12,8 @@ jest.mock('react-router-dom', () => ({
1212
}));
1313

1414
describe('routes/LoginWithOAuthApp.tsx', () => {
15+
const mockLoginWithOAuthApp = jest.fn();
16+
1517
const openExternalLinkMock = jest
1618
.spyOn(comms, 'openExternalLink')
1719
.mockImplementation();
@@ -108,6 +110,39 @@ describe('routes/LoginWithOAuthApp.tsx', () => {
108110
});
109111
});
110112

113+
it('should login using a token - success', async () => {
114+
mockLoginWithOAuthApp.mockResolvedValueOnce(null);
115+
116+
render(
117+
<AppContext.Provider
118+
value={{
119+
loginWithOAuthApp: mockLoginWithOAuthApp,
120+
}}
121+
>
122+
<MemoryRouter>
123+
<LoginWithOAuthApp />
124+
</MemoryRouter>
125+
</AppContext.Provider>,
126+
);
127+
128+
fireEvent.change(screen.getByLabelText('Hostname'), {
129+
target: { value: 'github.com' },
130+
});
131+
fireEvent.change(screen.getByLabelText('Client ID'), {
132+
target: { value: '1234567890_ASDFGHJKL' },
133+
});
134+
fireEvent.change(screen.getByLabelText('Client Secret'), {
135+
target: { value: '1234567890_asdfghjklPOIUYTREWQ0987654321' },
136+
});
137+
138+
fireEvent.submit(screen.getByLabelText('Login'));
139+
140+
await waitFor(() => expect(mockLoginWithOAuthApp).toHaveBeenCalledTimes(1));
141+
142+
expect(mockLoginWithOAuthApp).toHaveBeenCalledTimes(1);
143+
expect(mockNavigate).toHaveBeenNthCalledWith(1, -1);
144+
});
145+
111146
it('should render the form with errors', () => {
112147
render(
113148
<AppContext.Provider value={{ auth: mockAuth }}>

‎src/routes/LoginWithOAuthApp.tsx‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ import { BookIcon, PersonIcon, SignInIcon } from '@primer/octicons-react';
22
import log from 'electron-log';
33
import { type FC, useCallback, useContext } from 'react';
44
import { Form, type FormRenderProps } from 'react-final-form';
5+
import { useNavigate } from 'react-router-dom';
56
import { Header } from '../components/Header';
67
import { Button } from '../components/buttons/Button';
78
import { FieldInput } from '../components/fields/FieldInput';
@@ -59,6 +60,8 @@ export const validate = (values: IValues): IFormErrors => {
5960
};
6061

6162
export const LoginWithOAuthApp: FC = () => {
63+
const navigate = useNavigate();
64+
6265
const { loginWithOAuthApp } = useContext(AppContext);
6366

6467
const renderForm = (formProps: FormRenderProps) => {
@@ -126,9 +129,9 @@ export const LoginWithOAuthApp: FC = () => {
126129
async (data: IValues) => {
127130
try {
128131
await loginWithOAuthApp(data as LoginOAuthAppOptions);
132+
navigate(-1);
129133
} catch (err) {
130134
log.error('Auth: Failed to login with oauth app', err);
131-
// Skip
132135
}
133136
},
134137
[loginWithOAuthApp],

‎src/routes/LoginWithPersonalAccessToken.test.tsx‎

Lines changed: 26 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,7 @@ jest.mock('react-router-dom', () => ({
2020
}));
2121

2222
describe('routes/LoginWithPersonalAccessToken.tsx', () => {
23-
const mockValidateToken = jest.fn();
23+
const mockLoginWithPersonalAccessToken = jest.fn();
2424
const openExternalLinkMock = jest
2525
.spyOn(comms, 'openExternalLink')
2626
.mockImplementation();
@@ -75,7 +75,9 @@ describe('routes/LoginWithPersonalAccessToken.tsx', () => {
7575
it('should be disabled if no hostname configured', async () => {
7676
render(
7777
<AppContext.Provider
78-
value={{ loginWithPersonalAccessToken: mockValidateToken }}
78+
value={{
79+
loginWithPersonalAccessToken: mockLoginWithPersonalAccessToken,
80+
}}
7981
>
8082
<MemoryRouter>
8183
<LoginWithPersonalAccessToken />
@@ -95,7 +97,9 @@ describe('routes/LoginWithPersonalAccessToken.tsx', () => {
9597
it('should open in browser if hostname configured', async () => {
9698
render(
9799
<AppContext.Provider
98-
value={{ loginWithPersonalAccessToken: mockValidateToken }}
100+
value={{
101+
loginWithPersonalAccessToken: mockLoginWithPersonalAccessToken,
102+
}}
99103
>
100104
<MemoryRouter>
101105
<LoginWithPersonalAccessToken />
@@ -110,11 +114,13 @@ describe('routes/LoginWithPersonalAccessToken.tsx', () => {
110114
});
111115

112116
it('should login using a token - success', async () => {
113-
mockValidateToken.mockResolvedValueOnce(null);
117+
mockLoginWithPersonalAccessToken.mockResolvedValueOnce(null);
114118

115119
render(
116120
<AppContext.Provider
117-
value={{ loginWithPersonalAccessToken: mockValidateToken }}
121+
value={{
122+
loginWithPersonalAccessToken: mockLoginWithPersonalAccessToken,
123+
}}
118124
>
119125
<MemoryRouter>
120126
<LoginWithPersonalAccessToken />
@@ -131,18 +137,22 @@ describe('routes/LoginWithPersonalAccessToken.tsx', () => {
131137

132138
fireEvent.submit(screen.getByLabelText('Login'));
133139

134-
await waitFor(() => expect(mockValidateToken).toHaveBeenCalledTimes(1));
140+
await waitFor(() =>
141+
expect(mockLoginWithPersonalAccessToken).toHaveBeenCalledTimes(1),
142+
);
135143

136-
expect(mockValidateToken).toHaveBeenCalledTimes(1);
144+
expect(mockLoginWithPersonalAccessToken).toHaveBeenCalledTimes(1);
137145
expect(mockNavigate).toHaveBeenNthCalledWith(1, -1);
138146
});
139147

140148
it('should login using a token - failure', async () => {
141-
mockValidateToken.mockRejectedValueOnce(null);
149+
mockLoginWithPersonalAccessToken.mockRejectedValueOnce(null);
142150

143151
render(
144152
<AppContext.Provider
145-
value={{ loginWithPersonalAccessToken: mockValidateToken }}
153+
value={{
154+
loginWithPersonalAccessToken: mockLoginWithPersonalAccessToken,
155+
}}
146156
>
147157
<MemoryRouter>
148158
<LoginWithPersonalAccessToken />
@@ -160,9 +170,11 @@ describe('routes/LoginWithPersonalAccessToken.tsx', () => {
160170
fireEvent.submit(screen.getByLabelText('Login'));
161171
});
162172

163-
await waitFor(() => expect(mockValidateToken).toHaveBeenCalledTimes(1));
173+
await waitFor(() =>
174+
expect(mockLoginWithPersonalAccessToken).toHaveBeenCalledTimes(1),
175+
);
164176

165-
expect(mockValidateToken).toHaveBeenCalledTimes(1);
177+
expect(mockLoginWithPersonalAccessToken).toHaveBeenCalledTimes(1);
166178
expect(mockNavigate).toHaveBeenCalledTimes(0);
167179
});
168180

@@ -189,7 +201,9 @@ describe('routes/LoginWithPersonalAccessToken.tsx', () => {
189201
it('should open help docs in the browser', async () => {
190202
render(
191203
<AppContext.Provider
192-
value={{ loginWithPersonalAccessToken: mockValidateToken }}
204+
value={{
205+
loginWithPersonalAccessToken: mockLoginWithPersonalAccessToken,
206+
}}
193207
>
194208
<MemoryRouter>
195209
<LoginWithPersonalAccessToken />

‎src/routes/__snapshots__/Login.test.tsx.snap‎

Lines changed: 44 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎src/utils/auth/utils.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ import { Constants } from '../constants';
1818
import { getPlatformFromHostname } from '../helpers';
1919
import type { AuthMethod, AuthResponse, AuthTokenResponse } from './types';
2020

21+
// TODO - Refactor our OAuth2 flow to use system browser and local app gitify://callback - see #485 #561 #654
2122
export function authGitHub(
2223
authOptions = Constants.DEFAULT_AUTH_OPTIONS,
2324
): Promise<AuthResponse> {

0 commit comments

Comments
 (0)