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.

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): restore oauth2 flow functionality #1456

Merged
merged 4 commits into from
Aug 14, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
11 changes: 7 additions & 4 deletions src/electron/main.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,6 @@ const { autoUpdater } = require('electron-updater');
const { updateElectronApp } = require('update-electron-app');

log.initialize();

// TODO: Remove @electron/remote use - see #650
require('@electron/remote/main').initialize();

// Tray Icons
const idleIcon = path.resolve(
`${__dirname}/../../assets/images/tray-idleTemplate.png`,
Expand Down Expand Up @@ -124,6 +120,13 @@ app.whenReady().then(async () => {
mb.on('ready', () => {
mb.app.setAppUserModelId('com.electron.gitify');

/**
* 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('Gitify');
mb.tray.setIgnoreDoubleClickEvents(true);
Expand Down
32 changes: 15 additions & 17 deletions src/routes/Login.tsx
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { KeyIcon, PersonIcon } from '@primer/octicons-react';
import { type FC, useContext, useEffect } from 'react';
import { KeyIcon, MarkGithubIcon, PersonIcon } from '@primer/octicons-react';
import log from 'electron-log';
import { type FC, useCallback, useContext, useEffect } from 'react';
import { useNavigate } from 'react-router-dom';
import { Button } from '../components/buttons/Button';
import { LogoIcon } from '../components/icons/LogoIcon';
Expand All @@ -9,7 +10,7 @@ import { showWindow } from '../utils/comms';

export const LoginRoute: FC = () => {
const navigate = useNavigate();
const { isLoggedIn } = useContext(AppContext);
const { loginWithGitHubApp, isLoggedIn } = useContext(AppContext);

useEffect(() => {
if (isLoggedIn) {
Expand All @@ -18,14 +19,13 @@ export const LoginRoute: FC = () => {
}
}, [isLoggedIn]);

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

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

<div className="text-center text-sm font-semibold italic">Login with</div>
{
// FIXME: Temporarily disable Login with GitHub (OAuth) as it's currently broken and requires a rewrite - see #485 #561 #747
/*
<Button

<Button
name="GitHub"
icon={MarkGitHubIcon}
icon={{ icon: MarkGithubIcon }}
label="Login with GitHub"
class="w-50 px-2 py-2 my-2 text-xs"
onClick={loginUser}
/>
*/
}
className="mt-2 py-2"
onClick={() => loginUser()}
>
GitHub
</Button>

<Button
icon={{ icon: KeyIcon }}
Expand Down
37 changes: 36 additions & 1 deletion src/routes/LoginWithOAuthApp.test.tsx
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { fireEvent, render, screen } from '@testing-library/react';
import { fireEvent, render, screen, waitFor } from '@testing-library/react';
import { MemoryRouter } from 'react-router-dom';
import { AppContext } from '../context/App';
import type { AuthState, ClientID, ClientSecret, Hostname } from '../types';
Expand All @@ -12,6 +12,8 @@ jest.mock('react-router-dom', () => ({
}));

describe('routes/LoginWithOAuthApp.tsx', () => {
const mockLoginWithOAuthApp = jest.fn();

const openExternalLinkMock = jest
.spyOn(comms, 'openExternalLink')
.mockImplementation();
Expand Down Expand Up @@ -108,6 +110,39 @@ describe('routes/LoginWithOAuthApp.tsx', () => {
});
});

it('should login using a token - success', async () => {
mockLoginWithOAuthApp.mockResolvedValueOnce(null);

render(
<AppContext.Provider
value={{
loginWithOAuthApp: mockLoginWithOAuthApp,
}}
>
<MemoryRouter>
<LoginWithOAuthApp />
</MemoryRouter>
</AppContext.Provider>,
);

fireEvent.change(screen.getByLabelText('Hostname'), {
target: { value: 'github.com' },
});
fireEvent.change(screen.getByLabelText('Client ID'), {
target: { value: '1234567890_ASDFGHJKL' },
});
fireEvent.change(screen.getByLabelText('Client Secret'), {
target: { value: '1234567890_asdfghjklPOIUYTREWQ0987654321' },
});

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

await waitFor(() => expect(mockLoginWithOAuthApp).toHaveBeenCalledTimes(1));

expect(mockLoginWithOAuthApp).toHaveBeenCalledTimes(1);
expect(mockNavigate).toHaveBeenNthCalledWith(1, -1);
});

it('should render the form with errors', () => {
render(
<AppContext.Provider value={{ auth: mockAuth }}>
Expand Down
5 changes: 4 additions & 1 deletion src/routes/LoginWithOAuthApp.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import { BookIcon, PersonIcon, SignInIcon } from '@primer/octicons-react';
import log from 'electron-log';
import { type FC, useCallback, useContext } from 'react';
import { Form, type FormRenderProps } from 'react-final-form';
import { useNavigate } from 'react-router-dom';
import { Header } from '../components/Header';
import { Button } from '../components/buttons/Button';
import { FieldInput } from '../components/fields/FieldInput';
Expand Down Expand Up @@ -59,6 +60,8 @@ export const validate = (values: IValues): IFormErrors => {
};

export const LoginWithOAuthApp: FC = () => {
const navigate = useNavigate();

const { loginWithOAuthApp } = useContext(AppContext);

const renderForm = (formProps: FormRenderProps) => {
Expand Down Expand Up @@ -126,9 +129,9 @@ export const LoginWithOAuthApp: FC = () => {
async (data: IValues) => {
try {
await loginWithOAuthApp(data as LoginOAuthAppOptions);
navigate(-1);
} catch (err) {
log.error('Auth: Failed to login with oauth app', err);
// Skip
}
},
[loginWithOAuthApp],
Expand Down
38 changes: 26 additions & 12 deletions src/routes/LoginWithPersonalAccessToken.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ jest.mock('react-router-dom', () => ({
}));

describe('routes/LoginWithPersonalAccessToken.tsx', () => {
const mockValidateToken = jest.fn();
const mockLoginWithPersonalAccessToken = jest.fn();
const openExternalLinkMock = jest
.spyOn(comms, 'openExternalLink')
.mockImplementation();
Expand Down Expand Up @@ -75,7 +75,9 @@ describe('routes/LoginWithPersonalAccessToken.tsx', () => {
it('should be disabled if no hostname configured', async () => {
render(
<AppContext.Provider
value={{ loginWithPersonalAccessToken: mockValidateToken }}
value={{
loginWithPersonalAccessToken: mockLoginWithPersonalAccessToken,
}}
>
<MemoryRouter>
<LoginWithPersonalAccessToken />
Expand All @@ -95,7 +97,9 @@ describe('routes/LoginWithPersonalAccessToken.tsx', () => {
it('should open in browser if hostname configured', async () => {
render(
<AppContext.Provider
value={{ loginWithPersonalAccessToken: mockValidateToken }}
value={{
loginWithPersonalAccessToken: mockLoginWithPersonalAccessToken,
}}
>
<MemoryRouter>
<LoginWithPersonalAccessToken />
Expand All @@ -110,11 +114,13 @@ describe('routes/LoginWithPersonalAccessToken.tsx', () => {
});

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

render(
<AppContext.Provider
value={{ loginWithPersonalAccessToken: mockValidateToken }}
value={{
loginWithPersonalAccessToken: mockLoginWithPersonalAccessToken,
}}
>
<MemoryRouter>
<LoginWithPersonalAccessToken />
Expand All @@ -131,18 +137,22 @@ describe('routes/LoginWithPersonalAccessToken.tsx', () => {

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

await waitFor(() => expect(mockValidateToken).toHaveBeenCalledTimes(1));
await waitFor(() =>
expect(mockLoginWithPersonalAccessToken).toHaveBeenCalledTimes(1),
);

expect(mockValidateToken).toHaveBeenCalledTimes(1);
expect(mockLoginWithPersonalAccessToken).toHaveBeenCalledTimes(1);
expect(mockNavigate).toHaveBeenNthCalledWith(1, -1);
});

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

render(
<AppContext.Provider
value={{ loginWithPersonalAccessToken: mockValidateToken }}
value={{
loginWithPersonalAccessToken: mockLoginWithPersonalAccessToken,
}}
>
<MemoryRouter>
<LoginWithPersonalAccessToken />
Expand All @@ -160,9 +170,11 @@ describe('routes/LoginWithPersonalAccessToken.tsx', () => {
fireEvent.submit(screen.getByLabelText('Login'));
});

await waitFor(() => expect(mockValidateToken).toHaveBeenCalledTimes(1));
await waitFor(() =>
expect(mockLoginWithPersonalAccessToken).toHaveBeenCalledTimes(1),
);

expect(mockValidateToken).toHaveBeenCalledTimes(1);
expect(mockLoginWithPersonalAccessToken).toHaveBeenCalledTimes(1);
expect(mockNavigate).toHaveBeenCalledTimes(0);
});

Expand All @@ -189,7 +201,9 @@ describe('routes/LoginWithPersonalAccessToken.tsx', () => {
it('should open help docs in the browser', async () => {
render(
<AppContext.Provider
value={{ loginWithPersonalAccessToken: mockValidateToken }}
value={{
loginWithPersonalAccessToken: mockLoginWithPersonalAccessToken,
}}
>
<MemoryRouter>
<LoginWithPersonalAccessToken />
Expand Down
44 changes: 44 additions & 0 deletions src/routes/__snapshots__/Login.test.tsx.snap

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions src/utils/auth/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ 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<AuthResponse> {
Expand Down