From 3f0565e26baf149fe25e553056a15bc83d7afb2b Mon Sep 17 00:00:00 2001 From: ErickSkrauch Date: Tue, 10 Dec 2024 20:42:06 +0100 Subject: [PATCH 1/7] Initial device code flow implementation --- packages/app/components/auth/actions.test.ts | 22 +-- packages/app/components/auth/actions.ts | 122 +++++++------ .../components/auth/deviceCode/DeviceCode.tsx | 18 ++ .../auth/deviceCode/DeviceCodeBody.tsx | 31 ++++ .../app/components/auth/deviceCode/index.ts | 1 + .../app/components/auth/finish/Finish.tsx | 160 +++++++++--------- packages/app/components/auth/finish/index.ts | 1 + packages/app/components/auth/reducer.ts | 29 +++- packages/app/containers/AuthFlowRoute.tsx | 17 +- .../app/containers/AuthFlowRouteContents.tsx | 4 +- packages/app/pages/auth/AuthPage.tsx | 10 +- packages/app/services/api/oauth.ts | 65 +++---- .../app/services/authFlow/AuthFlow.test.ts | 10 +- packages/app/services/authFlow/AuthFlow.ts | 41 ++--- .../app/services/authFlow/CompleteState.ts | 36 ++-- .../app/services/authFlow/DeviceCodeState.ts | 26 +++ ....ts => InitOAuthAuthCodeFlowState.test.ts} | 6 +- ...State.ts => InitOAuthAuthCodeFlowState.ts} | 14 +- .../authFlow/InitOAuthDeviceCodeFlowState.ts | 27 +++ .../app/services/errorsDict/errorsDict.tsx | 2 + 20 files changed, 370 insertions(+), 272 deletions(-) create mode 100644 packages/app/components/auth/deviceCode/DeviceCode.tsx create mode 100644 packages/app/components/auth/deviceCode/DeviceCodeBody.tsx create mode 100644 packages/app/components/auth/deviceCode/index.ts create mode 100644 packages/app/components/auth/finish/index.ts create mode 100644 packages/app/services/authFlow/DeviceCodeState.ts rename packages/app/services/authFlow/{OAuthState.test.ts => InitOAuthAuthCodeFlowState.test.ts} (96%) rename packages/app/services/authFlow/{OAuthState.ts => InitOAuthAuthCodeFlowState.ts} (55%) create mode 100644 packages/app/services/authFlow/InitOAuthDeviceCodeFlowState.ts diff --git a/packages/app/components/auth/actions.test.ts b/packages/app/components/auth/actions.test.ts index 2aa64ca..039576e 100644 --- a/packages/app/components/auth/actions.test.ts +++ b/packages/app/components/auth/actions.test.ts @@ -16,14 +16,18 @@ import { login, setLogin, } from 'app/components/auth/actions'; -import { OauthData, OAuthValidateResponse } from '../../services/api/oauth'; +import { OAuthValidateResponse } from 'app/services/api/oauth'; -const oauthData: OauthData = { - clientId: '', - redirectUrl: '', - responseType: '', - scope: '', - state: '', +import { OAuthState } from './reducer'; + +const oauthData: OAuthState = { + params: { + clientId: '', + redirectUrl: '', + responseType: '', + scope: '', + state: '', + }, prompt: 'none', }; @@ -64,9 +68,6 @@ describe('components/auth/actions', () => { name: '', description: '', }, - oAuth: { - state: 123, - }, session: { scopes: ['account_info'], }, @@ -86,7 +87,6 @@ describe('components/auth/actions', () => { [setClient(resp.client)], [ setOAuthRequest({ - ...resp.oAuth, prompt: 'none', loginHint: undefined, }), diff --git a/packages/app/components/auth/actions.ts b/packages/app/components/auth/actions.ts index e3c9fe1..53bbdeb 100644 --- a/packages/app/components/auth/actions.ts +++ b/packages/app/components/auth/actions.ts @@ -13,7 +13,7 @@ import { recoverPassword as recoverPasswordEndpoint, OAuthResponse, } from 'app/services/api/authentication'; -import oauth, { OauthData, Scope } from 'app/services/api/oauth'; +import oauth, { OauthRequestData, Scope } from 'app/services/api/oauth'; import { register as registerEndpoint, activate as activateEndpoint, @@ -314,38 +314,17 @@ const KNOWN_SCOPES: ReadonlyArray = [ 'account_info', 'account_email', ]; -/** - * @param {object} oauthData - * @param {string} oauthData.clientId - * @param {string} oauthData.redirectUrl - * @param {string} oauthData.responseType - * @param {string} oauthData.description - * @param {string} oauthData.scope - * @param {string} [oauthData.prompt='none'] - comma-separated list of values to adjust auth flow - * Posible values: - * * none - default behaviour - * * consent - forcibly prompt user for rules acceptance - * * select_account - force account choosage, even if user has only one - * @param {string} oauthData.loginHint - allows to choose the account, which will be used for auth - * The possible values: account id, email, username - * @param {string} oauthData.state - * - * @returns {Promise} - */ -export function oAuthValidate(oauthData: OauthData) { + +export function oAuthValidate(oauthData: Pick) { // TODO: move to oAuth actions? - // test request: /oauth?client_id=ely&redirect_uri=http%3A%2F%2Fely.by&response_type=code&scope=minecraft_server_session&description=foo + // auth code flow: /oauth?client_id=ely&redirect_uri=http%3A%2F%2Fely.by&response_type=code&scope=minecraft_server_session&description=foo + // device code flow: /code?user_code=XOXOXOXO return wrapInLoader((dispatch) => oauth - .validate(oauthData) + .validate(getOAuthRequest(oauthData)) .then((resp) => { const { scopes } = resp.session; const invalidScopes = scopes.filter((scope) => !KNOWN_SCOPES.includes(scope)); - let prompt = (oauthData.prompt || 'none').split(',').map((item) => item.trim()); - - if (prompt.includes('none')) { - prompt = ['none']; - } if (invalidScopes.length) { logger.error('Got invalid scopes after oauth validation', { @@ -353,12 +332,19 @@ export function oAuthValidate(oauthData: OauthData) { }); } + let { prompt } = oauthData; + + if (prompt && !Array.isArray(prompt)) { + prompt = prompt.split(',').map((item) => item.trim()); + } + dispatch(setClient(resp.client)); dispatch( setOAuthRequest({ - ...resp.oAuth, - prompt: oauthData.prompt || 'none', + params: oauthData.params, + description: oauthData.description, loginHint: oauthData.loginHint, + prompt, }), ); dispatch(setScopes(scopes)); @@ -375,12 +361,6 @@ export function oAuthValidate(oauthData: OauthData) { ); } -/** - * @param {object} params - * @param {bool} params.accept=false - * @param params.accept - * @returns {Promise} - */ export function oAuthComplete(params: { accept?: boolean } = {}) { return wrapInLoader( async ( @@ -388,7 +368,7 @@ export function oAuthComplete(params: { accept?: boolean } = {}) { getState, ): Promise<{ success: boolean; - redirectUri: string; + redirectUri?: string; }> => { const oauthData = getState().auth.oauth; @@ -397,11 +377,14 @@ export function oAuthComplete(params: { accept?: boolean } = {}) { } try { - const resp = await oauth.complete(oauthData, params); + const resp = await oauth.complete(getOAuthRequest(oauthData), params); + localStorage.removeItem('oauthData'); - if (resp.redirectUri.startsWith('static_page')) { - const displayCode = /static_page_with_code/.test(resp.redirectUri); + if (!resp.redirectUri) { + dispatch(setOAuthCode({ success: resp.success && params.accept })); + } else if (resp.redirectUri.startsWith('static_page')) { + const displayCode = resp.redirectUri.includes('static_page_with_code'); const [, code] = resp.redirectUri.match(/code=(.+)&/) || []; [, resp.redirectUri] = resp.redirectUri.match(/^(.+)\?/) || []; @@ -437,13 +420,41 @@ export function oAuthComplete(params: { accept?: boolean } = {}) { ); } +function getOAuthRequest({ params, description }: OAuthState): OauthRequestData { + let data: OauthRequestData; + + if ('userCode' in params) { + data = { + user_code: params.userCode, + }; + } else { + data = { + client_id: params.clientId, + redirect_uri: params.redirectUrl, + response_type: params.responseType, + scope: params.scope, + state: params.state, + }; + } + + if (description) { + data.description = description; + } + + return data; +} + function handleOauthParamsValidation( resp: { [key: string]: any; userMessage?: string; } = {}, ) { - dispatchBsod(); + // TODO: it would be better to dispatch BSOD from the initial request performers + if (resp.error !== 'invalid_user_code') { + dispatchBsod(); + } + localStorage.removeItem('oauthData'); // eslint-disable-next-line no-alert @@ -468,33 +479,16 @@ export type ClientAction = SetClientAction; interface SetOauthAction extends ReduxAction { type: 'set_oauth'; - payload: Pick; + payload: OAuthState | null; } // Input data is coming right from the query string, so the names // are the same, as used for initializing OAuth2 request -export function setOAuthRequest(data: { - client_id?: string; - redirect_uri?: string; - response_type?: string; - scope?: string; - prompt?: string; - loginHint?: string; - state?: string; -}): SetOauthAction { +// TODO: filter out allowed properties +export function setOAuthRequest(payload: OAuthState | null): SetOauthAction { return { type: 'set_oauth', - payload: { - // TODO: there is too much default empty string. Maybe we can somehow validate it - // on the level, where this action is called? - clientId: data.client_id || '', - redirectUrl: data.redirect_uri || '', - responseType: data.response_type || '', - scope: data.scope || '', - prompt: data.prompt || '', - loginHint: data.loginHint || '', - state: data.state || '', - }, + payload, }; } @@ -503,9 +497,7 @@ interface SetOAuthResultAction extends ReduxAction { payload: Pick; } -export const SET_OAUTH_RESULT = 'set_oauth_result'; // TODO: remove - -export function setOAuthCode(payload: { success: boolean; code: string; displayCode: boolean }): SetOAuthResultAction { +export function setOAuthCode(payload: Pick): SetOAuthResultAction { return { type: 'set_oauth_result', payload, @@ -515,7 +507,7 @@ export function setOAuthCode(payload: { success: boolean; code: string; displayC export function resetOAuth(): AppAction { return (dispatch): void => { localStorage.removeItem('oauthData'); - dispatch(setOAuthRequest({})); + dispatch(setOAuthRequest(null)); }; } diff --git a/packages/app/components/auth/deviceCode/DeviceCode.tsx b/packages/app/components/auth/deviceCode/DeviceCode.tsx new file mode 100644 index 0000000..5c2ec45 --- /dev/null +++ b/packages/app/components/auth/deviceCode/DeviceCode.tsx @@ -0,0 +1,18 @@ +import React from 'react'; +import { FormattedMessage as Message, defineMessages } from 'react-intl'; + +import factory from '../factory'; +import Body from './DeviceCodeBody'; + +const messages = defineMessages({ + deviceCodeTitle: 'Device code', +}); + +export default factory({ + title: messages.deviceCodeTitle, + body: Body, + footer: { + color: 'green', + children: , + }, +}); diff --git a/packages/app/components/auth/deviceCode/DeviceCodeBody.tsx b/packages/app/components/auth/deviceCode/DeviceCodeBody.tsx new file mode 100644 index 0000000..4c64884 --- /dev/null +++ b/packages/app/components/auth/deviceCode/DeviceCodeBody.tsx @@ -0,0 +1,31 @@ +import React from 'react'; +import { defineMessages } from 'react-intl'; + +import { Input } from 'app/components/ui/form'; +import BaseAuthBody from 'app/components/auth/BaseAuthBody'; + +const messages = defineMessages({ + deviceCode: 'Device code', +}); + +export default class DeviceCodeBody extends BaseAuthBody { + static displayName = 'DeviceCodeBody'; + static panelId = 'deviceCode'; + + autoFocusField = 'user_code'; + + render() { + return ( +
+ {this.renderErrors()} + + +
+ ); + } +} diff --git a/packages/app/components/auth/deviceCode/index.ts b/packages/app/components/auth/deviceCode/index.ts new file mode 100644 index 0000000..38c2691 --- /dev/null +++ b/packages/app/components/auth/deviceCode/index.ts @@ -0,0 +1 @@ +export { default } from './DeviceCode'; diff --git a/packages/app/components/auth/finish/Finish.tsx b/packages/app/components/auth/finish/Finish.tsx index cd875f9..d411e85 100644 --- a/packages/app/components/auth/finish/Finish.tsx +++ b/packages/app/components/auth/finish/Finish.tsx @@ -1,8 +1,9 @@ -import React, { MouseEventHandler } from 'react'; +import React, { FC, MouseEventHandler, useEffect } from 'react'; +import { Redirect } from 'react-router-dom'; import { FormattedMessage as Message } from 'react-intl'; import { Helmet } from 'react-helmet-async'; -import { connect } from 'app/functions'; +import { useReduxSelector } from 'app/functions'; import { Button } from 'app/components/ui/form'; import copy from 'app/services/copy'; @@ -16,102 +17,93 @@ interface Props { success?: boolean; } -class Finish extends React.Component { - render() { - const { appName, code, state, displayCode, success } = this.props; - const authData = JSON.stringify({ - auth_code: code, - state, +const Finish: FC = () => { + const { client, oauth } = useReduxSelector((state) => state.auth); + + const onCopyClick: MouseEventHandler = (event) => { + event.preventDefault(); + copy(oauth!.code!); + }; + + let authData: string | undefined; + + if (oauth && 'state' in oauth.params) { + authData = JSON.stringify({ + auth_code: oauth.code, + state: oauth.params.state, }); + } - history.pushState(null, document.title, `#${authData}`); + useEffect(() => { + if (authData) { + history.pushState(null, document.title, `#${authData}`); + } + }, []); - return ( -
- + if (!client || !oauth) { + return ; + } - {success ? ( -
-
-
- {appName}, - }} - /> -
- {displayCode ? ( -
-
- -
-
-
{code}
-
- -
- ) : ( + return ( +
+ {authData && } + + {oauth.success ? ( +
+
+
+ {client.name}, + }} + /> +
+ {oauth.displayCode ? ( +
- )} -
- ) : ( -
-
-
- {appName}, - }} - /> +
+
{oauth.code}
+
+
+ ) : (
+ )} +
+ ) : ( +
+
+
+ {client.name}, + }} + />
- )} -
- ); - } +
+ +
+
+ )} +
+ ); +}; - onCopyClick: MouseEventHandler = (event) => { - event.preventDefault(); - - const { code } = this.props; - - if (code) { - copy(code); - } - }; -} - -export default connect(({ auth }) => { - if (!auth || !auth.client || !auth.oauth) { - throw new Error('Can not connect Finish component. No auth data in state'); - } - - return { - appName: auth.client.name, - code: auth.oauth.code, - displayCode: auth.oauth.displayCode, - state: auth.oauth.state, - success: auth.oauth.success, - }; -})(Finish); +export default Finish; diff --git a/packages/app/components/auth/finish/index.ts b/packages/app/components/auth/finish/index.ts new file mode 100644 index 0000000..a5d2a36 --- /dev/null +++ b/packages/app/components/auth/finish/index.ts @@ -0,0 +1 @@ +export { default } from './Finish'; diff --git a/packages/app/components/auth/reducer.ts b/packages/app/components/auth/reducer.ts index 1862399..af398b0 100644 --- a/packages/app/components/auth/reducer.ts +++ b/packages/app/components/auth/reducer.ts @@ -38,15 +38,34 @@ export interface Client { description: string; } -export interface OAuthState { +export interface OauthAuthCodeFlowParams { clientId: string; redirectUrl: string; responseType: string; - description?: string; - scope: string; - prompt: string; - loginHint: string; state: string; + scope: string; +} + +export interface OauthDeviceCodeFlowParams { + userCode: string; +} + +export interface OAuthState { + params: OauthAuthCodeFlowParams | OauthDeviceCodeFlowParams; + description?: string; + /** + * Possible values: + * - none - default behaviour + * - consent - forcibly prompt user for rules acceptance + * - select_account - force account choosage, even if user has only one + * comma separated list of 'none' | 'consent' | 'select_account'; + */ + prompt?: string | Array; + /** + * Allows to choose the account, which will be used for auth + * The possible values: account id, email, username + */ + loginHint?: string; success?: boolean; code?: string; displayCode?: boolean; diff --git a/packages/app/containers/AuthFlowRoute.tsx b/packages/app/containers/AuthFlowRoute.tsx index 4058da3..78e777f 100644 --- a/packages/app/containers/AuthFlowRoute.tsx +++ b/packages/app/containers/AuthFlowRoute.tsx @@ -1,19 +1,18 @@ -import React from 'react'; +import React, { FC } from 'react'; import { Route, RouteProps } from 'react-router-dom'; import AuthFlowRouteContents from './AuthFlowRouteContents'; -export default function AuthFlowRoute(props: RouteProps) { - const { component: Component, ...routeProps } = props; - - if (!Component) { - throw new Error('props.component required'); - } +// Make "component" prop required +type Props = Omit & Required>; +const AuthFlowRoute: FC = ({ component: Component, ...props }) => { return ( } /> ); -} +}; + +export default AuthFlowRoute; diff --git a/packages/app/containers/AuthFlowRouteContents.tsx b/packages/app/containers/AuthFlowRouteContents.tsx index 2adcd00..f59d53a 100644 --- a/packages/app/containers/AuthFlowRouteContents.tsx +++ b/packages/app/containers/AuthFlowRouteContents.tsx @@ -4,7 +4,7 @@ import { Redirect, RouteComponentProps } from 'react-router-dom'; import authFlow from 'app/services/authFlow'; interface Props { - component: React.ComponentType> | React.ComponentType; + component: React.ComponentType | React.ComponentType; routerProps: RouteComponentProps; } @@ -74,7 +74,7 @@ export default class AuthFlowRouteContents extends React.Component }); } - onRouteAllowed(props: Props) { + onRouteAllowed(props: Props): void { if (!this.mounted) { return; } diff --git a/packages/app/pages/auth/AuthPage.tsx b/packages/app/pages/auth/AuthPage.tsx index a97dc58..4f0a9b3 100644 --- a/packages/app/pages/auth/AuthPage.tsx +++ b/packages/app/pages/auth/AuthPage.tsx @@ -1,4 +1,4 @@ -import React, { ComponentType, ReactNode, useCallback, useState } from 'react'; +import React, { FC, useCallback, useState } from 'react'; import { Route, Switch, Redirect, RouteComponentProps } from 'react-router-dom'; import AppInfo from 'app/components/auth/appInfo/AppInfo'; @@ -7,6 +7,7 @@ import Register from 'app/components/auth/register/Register'; import Login from 'app/components/auth/login/Login'; import Permissions from 'app/components/auth/permissions/Permissions'; import ChooseAccount from 'app/components/auth/chooseAccount/ChooseAccount'; +import DeviceCode from 'app/components/auth/deviceCode'; import Activation from 'app/components/auth/activation/Activation'; import ResendActivation from 'app/components/auth/resendActivation/ResendActivation'; import Password from 'app/components/auth/password/Password'; @@ -14,7 +15,7 @@ import AcceptRules from 'app/components/auth/acceptRules/AcceptRules'; import ForgotPassword from 'app/components/auth/forgotPassword/ForgotPassword'; import RecoverPassword from 'app/components/auth/recoverPassword/RecoverPassword'; import Mfa from 'app/components/auth/mfa/Mfa'; -import Finish from 'app/components/auth/finish/Finish'; +import Finish from 'app/components/auth/finish'; import { useReduxSelector } from 'app/functions'; import { Factory } from 'app/components/auth/factory'; @@ -27,7 +28,7 @@ import styles from './auth.scss'; // so that it persist disregarding remounts let isSidebarHiddenCache = false; -const AuthPage: ComponentType = () => { +const AuthPage: FC = () => { const [isSidebarHidden, setIsSidebarHidden] = useState(isSidebarHiddenCache); const client = useReduxSelector((state) => state.auth.client); @@ -54,6 +55,7 @@ const AuthPage: ComponentType = () => { + @@ -64,7 +66,7 @@ const AuthPage: ComponentType = () => { ); }; -function renderPanelTransition(factory: Factory): (props: RouteComponentProps) => ReactNode { +function renderPanelTransition(factory: Factory): FC { const { Title, Body, Footer, Links } = factory(); return (props) => ( diff --git a/packages/app/services/api/oauth.ts b/packages/app/services/api/oauth.ts index abae245..634b110 100644 --- a/packages/app/services/api/oauth.ts +++ b/packages/app/services/api/oauth.ts @@ -24,35 +24,27 @@ export interface OauthAppResponse { minecraftServerIp?: string; } -interface OauthRequestData { +interface AuthCodeFlowRequestData { client_id: string; redirect_uri: string; response_type: string; - description?: string; scope: string; - prompt: string; - login_hint?: string; state?: string; } -export interface OauthData { - clientId: string; - redirectUrl: string; - responseType: string; - description?: string; - scope: string; - // TODO: why prompt is not nullable? - prompt: string; // comma separated list of 'none' | 'consent' | 'select_account'; - loginHint?: string; - state?: string; +interface DeviceCodeFlowRequestData { + user_code: string; } +export type OauthRequestData = (AuthCodeFlowRequestData | DeviceCodeFlowRequestData) & { + description?: string; +}; + export interface OAuthValidateResponse { session: { scopes: Scope[]; }; client: Client; - oAuth: {}; // TODO: improve typing } interface FormPayloads { @@ -64,20 +56,20 @@ interface FormPayloads { } const api = { - validate(oauthData: OauthData) { + validate(oauthData: OauthRequestData) { return request - .get('/api/oauth2/v1/validate', getOAuthRequest(oauthData)) + .get('/api/oauth2/v1/validate', oauthData) .catch(handleOauthParamsValidation); }, complete( - oauthData: OauthData, + oauthData: OauthRequestData, params: { accept?: boolean } = {}, ): Promise<{ success: boolean; - redirectUri: string; + redirectUri?: string; }> { - const query = request.buildQuery(getOAuthRequest(oauthData)); + const query = request.buildQuery(oauthData); return request .post<{ @@ -146,37 +138,18 @@ if ('Cypress' in window) { export default api; -/** - * @param {object} oauthData - * @param {string} oauthData.clientId - * @param {string} oauthData.redirectUrl - * @param {string} oauthData.responseType - * @param {string} oauthData.description - * @param {string} oauthData.scope - * @param {string} oauthData.state - * - * @returns {object} - */ -function getOAuthRequest(oauthData: OauthData): OauthRequestData { - return { - client_id: oauthData.clientId, - redirect_uri: oauthData.redirectUrl, - response_type: oauthData.responseType, - description: oauthData.description, - scope: oauthData.scope, - prompt: oauthData.prompt, - login_hint: oauthData.loginHint, - state: oauthData.state, - }; -} - function handleOauthParamsValidation( resp: - | { [key: string]: any } + | Record | { statusCode: number; success: false; - error: 'invalid_request' | 'unsupported_response_type' | 'invalid_scope' | 'invalid_client'; + error: + | 'invalid_request' + | 'unsupported_response_type' + | 'invalid_scope' + | 'invalid_client' + | 'invalid_user_code'; parameter: string; } = {}, ) { diff --git a/packages/app/services/authFlow/AuthFlow.test.ts b/packages/app/services/authFlow/AuthFlow.test.ts index ab7a2f5..5b92da3 100644 --- a/packages/app/services/authFlow/AuthFlow.test.ts +++ b/packages/app/services/authFlow/AuthFlow.test.ts @@ -5,7 +5,7 @@ import AuthFlow from 'app/services/authFlow/AuthFlow'; import AbstractState from 'app/services/authFlow/AbstractState'; import localStorage from 'app/services/localStorage'; -import OAuthState from 'app/services/authFlow/OAuthState'; +import InitOAuthAuthCodeFlowState from './InitOAuthAuthCodeFlowState'; import RegisterState from 'app/services/authFlow/RegisterState'; import RecoverPasswordState from 'app/services/authFlow/RecoverPasswordState'; import ForgotPasswordState from 'app/services/authFlow/ForgotPasswordState'; @@ -314,9 +314,9 @@ describe('AuthFlow', () => { '/oauth/permissions': LoginState, '/oauth/choose-account': LoginState, '/oauth/finish': LoginState, - '/oauth2/v1/foo': OAuthState, - '/oauth2/v1': OAuthState, - '/oauth2': OAuthState, + '/oauth2/v1/foo': InitOAuthAuthCodeFlowState, + '/oauth2/v1': InitOAuthAuthCodeFlowState, + '/oauth2': InitOAuthAuthCodeFlowState, '/register': RegisterState, '/choose-account': ChooseAccountState, '/recover-password': RecoverPasswordState, @@ -379,7 +379,7 @@ describe('AuthFlow', () => { flow.handleRequest({ path, query: new URLSearchParams({}), params: {} }, () => {}, callback); expect(flow.setState, 'was called once'); - expect(flow.setState, 'to have a call satisfying', [expect.it('to be a', OAuthState)]); + expect(flow.setState, 'to have a call satisfying', [expect.it('to be a', InitOAuthAuthCodeFlowState)]); expect(callback, 'was called twice'); }); diff --git a/packages/app/services/authFlow/AuthFlow.ts b/packages/app/services/authFlow/AuthFlow.ts index cc4e1bb..9c11d22 100644 --- a/packages/app/services/authFlow/AuthFlow.ts +++ b/packages/app/services/authFlow/AuthFlow.ts @@ -10,10 +10,12 @@ import { } from 'app/components/accounts/actions'; import * as actions from 'app/components/auth/actions'; import { updateUser } from 'app/components/user/actions'; +import FinishState from './FinishState'; import RegisterState from './RegisterState'; import LoginState from './LoginState'; -import OAuthState from './OAuthState'; +import InitOAuthAuthCodeFlowState from './InitOAuthAuthCodeFlowState'; +import InitOAuthDeviceCodeFlowState from './InitOAuthDeviceCodeFlowState'; import ForgotPasswordState from './ForgotPasswordState'; import RecoverPasswordState from './RecoverPasswordState'; import ActivationState from './ActivationState'; @@ -22,11 +24,11 @@ import ChooseAccountState from './ChooseAccountState'; import ResendActivationState from './ResendActivationState'; import State from './State'; -type Request = { +interface Request { path: string; query: URLSearchParams; params: Record; -}; +} export const availableActions = { updateUser, @@ -104,7 +106,11 @@ export default class AuthFlow implements AuthContext { this.replace(route); } - browserHistory[options.replace ? 'replace' : 'push'](route); + if (options.replace) { + browserHistory.replace(route); + } else { + browserHistory.push(route); + } } this.replace = null; @@ -132,11 +138,7 @@ export default class AuthFlow implements AuthContext { } setState(state: State) { - if (!state) { - throw new Error('State is required'); - } - - this.state && this.state.leave(this); + this.state?.leave(this); this.prevState = this.state; this.state = state; const resp = this.state.enter(this); @@ -170,16 +172,8 @@ export default class AuthFlow implements AuthContext { /** * This should be called from onEnter prop of react-router Route component - * - * @param {object} request - * @param {string} request.path - * @param {object} request.params - * @param {URLSearchParams} request.query - * @param {Function} replace - * @param {Function} [callback=function() {}] - an optional callback function to be called, when state will be stabilized - * (state's enter function's promise resolved) */ - handleRequest(request: Request, replace: (path: string) => void, callback: () => void = () => {}) { + handleRequest(request: Request, replace: (path: string) => void, callback: () => void = () => {}): void { const { path } = request; this.replace = replace; this.onReady = callback; @@ -218,13 +212,20 @@ export default class AuthFlow implements AuthContext { this.setState(new ChooseAccountState()); break; + case '/code': + this.setState(new InitOAuthDeviceCodeFlowState()); + break; + + case '/oauth/finish': + this.setState(new FinishState()); + break; + case '/': case '/login': case '/password': case '/mfa': case '/accept-rules': case '/oauth/permissions': - case '/oauth/finish': case '/oauth/choose-account': this.setState(new LoginState()); break; @@ -234,7 +235,7 @@ export default class AuthFlow implements AuthContext { path.replace(/(.)\/.+/, '$1') // use only first part of an url ) { case '/oauth2': - this.setState(new OAuthState()); + this.setState(new InitOAuthAuthCodeFlowState()); break; case '/activation': this.setState(new ActivationState()); diff --git a/packages/app/services/authFlow/CompleteState.ts b/packages/app/services/authFlow/CompleteState.ts index 141aa9f..5ab296d 100644 --- a/packages/app/services/authFlow/CompleteState.ts +++ b/packages/app/services/authFlow/CompleteState.ts @@ -1,4 +1,6 @@ +import { OAuthState } from 'app/components/auth/reducer'; import { getActiveAccount } from 'app/components/accounts/reducer'; + import AbstractState from './AbstractState'; import LoginState from './LoginState'; import PermissionsState from './PermissionsState'; @@ -11,8 +13,16 @@ import { AuthContext } from './AuthFlow'; const PROMPT_ACCOUNT_CHOOSE = 'select_account'; const PROMPT_PERMISSIONS = 'consent'; +function hasPrompt(prompt: OAuthState['prompt'], needle: string): boolean { + if (Array.isArray(prompt)) { + return prompt.includes(needle); + } + + return prompt === needle; +} + export default class CompleteState extends AbstractState { - isPermissionsAccepted: boolean | void; + private readonly isPermissionsAccepted?: boolean; constructor( options: { @@ -36,7 +46,7 @@ export default class CompleteState extends AbstractState { context.setState(new LoginState()); } else if (user.shouldAcceptRules && !user.isDeleted) { context.setState(new AcceptRulesState()); - } else if (oauth && oauth.clientId) { + } else if (oauth?.params) { return this.processOAuth(context); } else { context.navigate('/'); @@ -44,6 +54,8 @@ export default class CompleteState extends AbstractState { } processOAuth(context: AuthContext): Promise | void { + console.log('process oauth', this.isPermissionsAccepted); + const { auth, accounts, user } = context.getState(); let { isSwitcherEnabled } = auth; @@ -80,7 +92,7 @@ export default class CompleteState extends AbstractState { // so that they can see, that their account was deleted // (this info is displayed on switcher) user.isDeleted || - oauth.prompt.includes(PROMPT_ACCOUNT_CHOOSE)) + hasPrompt(oauth.prompt, PROMPT_ACCOUNT_CHOOSE)) ) { context.setState(new ChooseAccountState()); } else if (user.isDeleted) { @@ -92,35 +104,35 @@ export default class CompleteState extends AbstractState { } else if (oauth.code) { context.setState(new FinishState()); } else { - const data: { [key: string]: any } = {}; + const data: Record = {}; if (typeof this.isPermissionsAccepted !== 'undefined') { data.accept = this.isPermissionsAccepted; - } else if (oauth.acceptRequired || oauth.prompt.includes(PROMPT_PERMISSIONS)) { + } else if (oauth.acceptRequired || hasPrompt(oauth.prompt, PROMPT_PERMISSIONS)) { context.setState(new PermissionsState()); return; } // TODO: it seems that oAuthComplete may be a separate state - return context.run('oAuthComplete', data).then( - (resp: { redirectUri: string }) => { + return context + .run('oAuthComplete', data) + .then((resp: { redirectUri?: string }) => { // TODO: пусть в стейт попадает флаг или тип авторизации // вместо волшебства над редирект урлой - if (resp.redirectUri.includes('static_page')) { + if (!resp.redirectUri || resp.redirectUri.includes('static_page')) { context.setState(new FinishState()); } else { return context.run('redirect', resp.redirectUri); } - }, - (resp) => { + }) + .catch((resp) => { if (resp.unauthorized) { context.setState(new LoginState()); } else if (resp.acceptRequired) { context.setState(new PermissionsState()); } - }, - ); + }); } } } diff --git a/packages/app/services/authFlow/DeviceCodeState.ts b/packages/app/services/authFlow/DeviceCodeState.ts new file mode 100644 index 0000000..4d80169 --- /dev/null +++ b/packages/app/services/authFlow/DeviceCodeState.ts @@ -0,0 +1,26 @@ +import AbstractState from './AbstractState'; +import { AuthContext } from './AuthFlow'; +import CompleteState from './CompleteState'; + +export default class DeviceCodeState extends AbstractState { + async resolve(context: AuthContext, payload: { user_code: string }): Promise { + const { query } = context.getRequest(); + + context + .run('oAuthValidate', { + params: { + userCode: payload.user_code, + }, + description: query.get('description')!, + prompt: query.get('prompt')!, + }) + .then(() => context.setState(new CompleteState())) + .catch((err) => { + if (err.error === 'invalid_user_code') { + return context.run('setErrors', { [err.parameter]: err.error }); + } + + throw err; + }); + } +} diff --git a/packages/app/services/authFlow/OAuthState.test.ts b/packages/app/services/authFlow/InitOAuthAuthCodeFlowState.test.ts similarity index 96% rename from packages/app/services/authFlow/OAuthState.test.ts rename to packages/app/services/authFlow/InitOAuthAuthCodeFlowState.test.ts index 1ae70ae..bf1ba78 100644 --- a/packages/app/services/authFlow/OAuthState.test.ts +++ b/packages/app/services/authFlow/InitOAuthAuthCodeFlowState.test.ts @@ -1,17 +1,17 @@ import sinon, { SinonMock } from 'sinon'; -import OAuthState from 'app/services/authFlow/OAuthState'; +import InitOAuthAuthCodeFlowState from './InitOAuthAuthCodeFlowState'; import CompleteState from 'app/services/authFlow/CompleteState'; import { bootstrap, expectState, expectRun, MockedAuthContext } from './helpers'; describe('OAuthState', () => { - let state: OAuthState; + let state: InitOAuthAuthCodeFlowState; let context: MockedAuthContext; let mock: SinonMock; beforeEach(() => { - state = new OAuthState(); + state = new InitOAuthAuthCodeFlowState(); const data = bootstrap(); context = data.context; diff --git a/packages/app/services/authFlow/OAuthState.ts b/packages/app/services/authFlow/InitOAuthAuthCodeFlowState.ts similarity index 55% rename from packages/app/services/authFlow/OAuthState.ts rename to packages/app/services/authFlow/InitOAuthAuthCodeFlowState.ts index fef2fab..a1bb88b 100644 --- a/packages/app/services/authFlow/OAuthState.ts +++ b/packages/app/services/authFlow/InitOAuthAuthCodeFlowState.ts @@ -2,20 +2,22 @@ import AbstractState from './AbstractState'; import { AuthContext } from './AuthFlow'; import CompleteState from './CompleteState'; -export default class OAuthState extends AbstractState { +export default class InitOAuthAuthCodeFlowState extends AbstractState { enter(context: AuthContext): Promise | void { const { query, params } = context.getRequest(); return context .run('oAuthValidate', { - clientId: query.get('client_id') || params.clientId, - redirectUrl: query.get('redirect_uri')!, - responseType: query.get('response_type')!, + params: { + clientId: query.get('client_id') || params.clientId, + redirectUrl: query.get('redirect_uri')!, + responseType: query.get('response_type')!, + scope: (query.get('scope') || '').replace(/,/g, ' '), + state: query.get('state')!, + }, description: query.get('description')!, - scope: (query.get('scope') || '').replace(/,/g, ' '), prompt: query.get('prompt')!, loginHint: query.get('login_hint')!, - state: query.get('state')!, }) .then(() => context.setState(new CompleteState())); } diff --git a/packages/app/services/authFlow/InitOAuthDeviceCodeFlowState.ts b/packages/app/services/authFlow/InitOAuthDeviceCodeFlowState.ts new file mode 100644 index 0000000..022f1db --- /dev/null +++ b/packages/app/services/authFlow/InitOAuthDeviceCodeFlowState.ts @@ -0,0 +1,27 @@ +import AbstractState from './AbstractState'; +import { AuthContext } from './AuthFlow'; +import CompleteState from './CompleteState'; +import DeviceCodeState from './DeviceCodeState'; + +export default class InitOAuthDeviceCodeFlowState extends AbstractState { + async enter(context: AuthContext): Promise { + const { query } = context.getRequest(); + + const userCode = query.get('user_code'); + + if (userCode) { + try { + await context.run('oAuthValidate', { + params: { userCode }, + description: query.get('description')!, + prompt: query.get('prompt')!, + }); + await context.setState(new CompleteState()); + } catch { + // Ok, fallback to the default + } + } + + return context.setState(new DeviceCodeState()); + } +} diff --git a/packages/app/services/errorsDict/errorsDict.tsx b/packages/app/services/errorsDict/errorsDict.tsx index 7095cf5..12fdad6 100644 --- a/packages/app/services/errorsDict/errorsDict.tsx +++ b/packages/app/services/errorsDict/errorsDict.tsx @@ -127,6 +127,8 @@ const errorsMap: Record) => ReactElement> = 'error.redirectUri_required': () => , 'error.redirectUri_invalid': () => , + + 'invalid_user_code': () => , }; interface ErrorLiteral { From af59cc033f04d63e03ee91896b33d74ebd55fbea Mon Sep 17 00:00:00 2001 From: ErickSkrauch Date: Sat, 14 Dec 2024 13:16:29 +0100 Subject: [PATCH 2/7] Extract device code into a separate view. Convert more components from class components to functional. Fix invalid finish state when client was auto approved --- packages/app/components/auth/AuthTitle.tsx | 12 +- .../app/components/auth/PanelTransition.tsx | 11 +- packages/app/components/auth/actions.test.ts | 3 + packages/app/components/auth/actions.ts | 7 +- .../components/auth/deviceCode/DeviceCode.tsx | 38 +++-- .../auth/deviceCode/DeviceCodeBody.tsx | 28 ++-- .../auth/deviceCode/deviceCode.scss | 5 + packages/app/components/ui/Panel.tsx | 144 +++++++----------- packages/app/components/ui/form/Form.tsx | 40 ++--- packages/app/components/ui/panel.scss | 1 + packages/app/containers/AuthFlowRoute.tsx | 13 +- .../containers/AuthFlowRouteContents.test.tsx | 2 +- .../app/containers/AuthFlowRouteContents.tsx | 99 +++--------- packages/app/hooks/index.ts | 1 + packages/app/hooks/useIsMounted.ts | 17 +++ packages/app/pages/auth/AuthPage.tsx | 9 +- packages/app/pages/auth/auth.scss | 2 - packages/app/pages/root/RootPage.tsx | 118 ++++++-------- .../app/services/authFlow/CompleteState.ts | 4 +- .../app/services/errorsDict/errorsDict.tsx | 2 +- 20 files changed, 241 insertions(+), 315 deletions(-) create mode 100644 packages/app/components/auth/deviceCode/deviceCode.scss create mode 100644 packages/app/hooks/index.ts create mode 100644 packages/app/hooks/useIsMounted.ts diff --git a/packages/app/components/auth/AuthTitle.tsx b/packages/app/components/auth/AuthTitle.tsx index cafdb35..2baf384 100644 --- a/packages/app/components/auth/AuthTitle.tsx +++ b/packages/app/components/auth/AuthTitle.tsx @@ -1,8 +1,12 @@ -import React from 'react'; +import React, { FC } from 'react'; import { Helmet } from 'react-helmet-async'; import { FormattedMessage as Message, MessageDescriptor } from 'react-intl'; -export default function AuthTitle({ title }: { title: MessageDescriptor }) { +interface Props { + title: MessageDescriptor; +} + +const AuthTitle: FC = ({ title }) => { return ( {(msg) => ( @@ -13,4 +17,6 @@ export default function AuthTitle({ title }: { title: MessageDescriptor }) { )} ); -} +}; + +export default AuthTitle; diff --git a/packages/app/components/auth/PanelTransition.tsx b/packages/app/components/auth/PanelTransition.tsx index 86a3468..370132c 100644 --- a/packages/app/components/auth/PanelTransition.tsx +++ b/packages/app/components/auth/PanelTransition.tsx @@ -94,7 +94,8 @@ interface OwnProps { Title: ReactElement; Body: ReactElement; Footer: ReactElement; - Links: ReactNode; + Links?: ReactNode; + className?: string; } interface Props extends OwnProps { @@ -255,6 +256,7 @@ class PanelTransition extends React.PureComponent { onSubmit={this.onFormSubmit} onInvalid={this.onFormInvalid} isLoading={this.props.auth.isLoading} + className={this.props.className} > {panels.map((config) => this.getHeader(config))} @@ -285,10 +287,7 @@ class PanelTransition extends React.PureComponent { onFormSubmit = (): void => { this.props.clearErrors(); - - if (this.body) { - this.body.onFormSubmit(); - } + this.body?.onFormSubmit(); }; onFormInvalid = (errors: Record): void => this.props.setErrors(errors); @@ -377,7 +376,7 @@ class PanelTransition extends React.PureComponent { if (length === 1) { if (!this.wasAutoFocused) { - this.body.autoFocus(); + this.body?.autoFocus(); } this.wasAutoFocused = true; diff --git a/packages/app/components/auth/actions.test.ts b/packages/app/components/auth/actions.test.ts index 039576e..77d2a22 100644 --- a/packages/app/components/auth/actions.test.ts +++ b/packages/app/components/auth/actions.test.ts @@ -87,6 +87,9 @@ describe('components/auth/actions', () => { [setClient(resp.client)], [ setOAuthRequest({ + params: { + userCode: '', + }, prompt: 'none', loginHint: undefined, }), diff --git a/packages/app/components/auth/actions.ts b/packages/app/components/auth/actions.ts index 53bbdeb..ed870ce 100644 --- a/packages/app/components/auth/actions.ts +++ b/packages/app/components/auth/actions.ts @@ -382,7 +382,12 @@ export function oAuthComplete(params: { accept?: boolean } = {}) { localStorage.removeItem('oauthData'); if (!resp.redirectUri) { - dispatch(setOAuthCode({ success: resp.success && params.accept })); + dispatch( + setOAuthCode({ + // if accept is undefined, then it was auto approved + success: resp.success && (typeof params.accept === 'undefined' || params.accept), + }), + ); } else if (resp.redirectUri.startsWith('static_page')) { const displayCode = resp.redirectUri.includes('static_page_with_code'); diff --git a/packages/app/components/auth/deviceCode/DeviceCode.tsx b/packages/app/components/auth/deviceCode/DeviceCode.tsx index 5c2ec45..cae7191 100644 --- a/packages/app/components/auth/deviceCode/DeviceCode.tsx +++ b/packages/app/components/auth/deviceCode/DeviceCode.tsx @@ -1,18 +1,32 @@ -import React from 'react'; +import React, { FC } from 'react'; +import { RouteComponentProps } from 'react-router-dom'; import { FormattedMessage as Message, defineMessages } from 'react-intl'; -import factory from '../factory'; -import Body from './DeviceCodeBody'; +import { Button } from 'app/components/ui/form'; +import AuthTitle from 'app/components/auth/AuthTitle'; +import PanelTransition from 'app/components/auth/PanelTransition'; + +import style from './deviceCode.scss'; +import DeviceCodeBody from './DeviceCodeBody'; const messages = defineMessages({ - deviceCodeTitle: 'Device code', + deviceCodeTitle: 'Device Code', }); -export default factory({ - title: messages.deviceCodeTitle, - body: Body, - footer: { - color: 'green', - children: , - }, -}); +const DeviceCode: FC = (props) => { + return ( + } + Body={} + Footer={ + + } + /> + ); +}; + +export default DeviceCode; diff --git a/packages/app/components/auth/deviceCode/DeviceCodeBody.tsx b/packages/app/components/auth/deviceCode/DeviceCodeBody.tsx index 4c64884..ddbaa33 100644 --- a/packages/app/components/auth/deviceCode/DeviceCodeBody.tsx +++ b/packages/app/components/auth/deviceCode/DeviceCodeBody.tsx @@ -1,13 +1,9 @@ import React from 'react'; -import { defineMessages } from 'react-intl'; +import { FormattedMessage as Message } from 'react-intl'; import { Input } from 'app/components/ui/form'; import BaseAuthBody from 'app/components/auth/BaseAuthBody'; -const messages = defineMessages({ - deviceCode: 'Device code', -}); - export default class DeviceCodeBody extends BaseAuthBody { static displayName = 'DeviceCodeBody'; static panelId = 'deviceCode'; @@ -16,16 +12,22 @@ export default class DeviceCodeBody extends BaseAuthBody { render() { return ( -
+ <> {this.renderErrors()} - -
+ + {(nodes) => ( + + )} + + ); } } diff --git a/packages/app/components/auth/deviceCode/deviceCode.scss b/packages/app/components/auth/deviceCode/deviceCode.scss new file mode 100644 index 0000000..f90e318 --- /dev/null +++ b/packages/app/components/auth/deviceCode/deviceCode.scss @@ -0,0 +1,5 @@ +.form { + max-width: 340px; + margin: 0 auto; + padding: 55px 13px 0; +} diff --git a/packages/app/components/ui/Panel.tsx b/packages/app/components/ui/Panel.tsx index ce151fc..479f446 100644 --- a/packages/app/components/ui/Panel.tsx +++ b/packages/app/components/ui/Panel.tsx @@ -1,121 +1,91 @@ -import React from 'react'; +import React, { FC, PropsWithChildren, useState, useCallback } from 'react'; import clsx from 'clsx'; -import { omit } from 'app/functions'; import styles from './panel.scss'; import icons from './icons.scss'; -export function Panel(props: { title?: string; icon?: string; children: React.ReactNode }) { - const { title: titleText, icon: iconType } = props; - let icon: React.ReactElement | undefined; - let title: React.ReactElement | undefined; - - if (iconType) { - icon = ( - - ); - } - - if (titleText) { - title = ( - - {icon} - {titleText} - - ); - } +interface PanelProps extends PropsWithChildren { + title?: string; + icon?: string; +} +export const Panel: FC = ({ title, icon, children }) => { return (
- {title} + {title && ( + + {icon && ( + + )} + {title} + + )} - {props.children} + {children}
); -} +}; -export function PanelHeader(props: { children: React.ReactNode }) { +export const PanelHeader: FC> = ({ children }) => { return ( -
- {props.children} +
+ {children}
); -} +}; -export function PanelBody(props: { children: React.ReactNode }) { +export const PanelBody: FC> = ({ children }) => { return ( -
- {props.children} +
+ {children}
); -} +}; -export function PanelFooter(props: { children: React.ReactNode }) { +export const PanelFooter: FC> = ({ children }) => { return ( -
- {props.children} +
+ {children}
); +}; + +interface PanelBodyHeaderProps extends PropsWithChildren { + type?: 'default' | 'error'; + onClose?: () => void; } -export class PanelBodyHeader extends React.Component< - { - type?: 'default' | 'error'; - onClose?: () => void; - children: React.ReactNode; - }, - { - isClosed: boolean; - } -> { - state: { - isClosed: boolean; - } = { - isClosed: false, - }; +export const PanelBodyHeader: FC = ({ type = 'default', onClose, children }) => { + const [isClosed, setIsClosed] = useState(false); + const handleCloseClick = useCallback(() => { + setIsClosed(true); + onClose?.(); + }, [onClose]); - render() { - const { type = 'default', children } = this.props; + return ( +
+ {type === 'error' && } + {children} +
+ ); +}; - let close; - - if (type === 'error') { - close = ; - } - - const className = clsx(styles[`${type}BodyHeader`], { - [styles.isClosed]: this.state.isClosed, - }); - - const extraProps = omit(this.props, ['type', 'onClose']); - - return ( -
- {close} - {children} -
- ); - } - - onClose = (event: React.MouseEvent) => { - event.preventDefault(); - - const { onClose } = this.props; - - this.setState({ isClosed: true }); - - if (onClose) { - onClose(); - } - }; +interface PanelIconProps { + icon: string; } -export function PanelIcon({ icon }: { icon: string }) { +export const PanelIcon: FC = ({ icon }) => { return (
); -} +}; diff --git a/packages/app/components/ui/form/Form.tsx b/packages/app/components/ui/form/Form.tsx index a68a6f5..0811428 100644 --- a/packages/app/components/ui/form/Form.tsx +++ b/packages/app/components/ui/form/Form.tsx @@ -1,4 +1,4 @@ -import React from 'react'; +import React, { ReactNode } from 'react'; import clsx from 'clsx'; import logger from 'app/services/logger'; @@ -10,7 +10,8 @@ interface BaseProps { id: string; isLoading: boolean; onInvalid: (errors: Record) => void; - children: React.ReactNode; + className?: string; + children?: ReactNode; } interface PropsWithoutForm extends BaseProps { @@ -24,10 +25,6 @@ interface PropsWithForm extends BaseProps { type Props = PropsWithoutForm | PropsWithForm; -function hasForm(props: Props): props is PropsWithForm { - return 'form' in props; -} - interface State { id: string; // just to track value for derived updates isTouched: boolean; @@ -54,10 +51,7 @@ export default class Form extends React.Component { mounted = false; componentDidMount() { - if (hasForm(this.props)) { - this.props.form.addLoadingListener(this.onLoading); - } - + (this.props as PropsWithForm).form?.addLoadingListener(this.onLoading); this.mounted = true; } @@ -77,8 +71,8 @@ export default class Form extends React.Component { } componentDidUpdate(prevProps: Props) { - const nextForm = hasForm(this.props) ? this.props.form : undefined; - const prevForm = hasForm(prevProps) ? prevProps.form : undefined; + const nextForm = (this.props as PropsWithForm).form; + const prevForm = (prevProps as PropsWithForm).form; if (nextForm !== prevForm) { if (prevForm) { @@ -92,10 +86,7 @@ export default class Form extends React.Component { } componentWillUnmount() { - if (hasForm(this.props)) { - this.props.form.removeLoadingListener(this.onLoading); - } - + (this.props as PropsWithForm).form?.removeLoadingListener(this.onLoading); this.mounted = false; } @@ -104,7 +95,7 @@ export default class Form extends React.Component { return (
{ this.clearErrors(); let result: Promise | void; - if (hasForm(this.props)) { - // @ts-ignore this prop has default value - result = this.props.onSubmit(this.props.form); + if ((this.props as PropsWithForm).form) { + result = (this.props as PropsWithForm).onSubmit!((this.props as PropsWithForm).form); } else { - // @ts-ignore this prop has default value - result = this.props.onSubmit(new FormData(form)); + result = (this.props as PropsWithoutForm).onSubmit!(new FormData(form)); } if (result && result.then) { @@ -181,14 +170,11 @@ export default class Form extends React.Component { } setErrors(errors: { [key: string]: string }) { - if (hasForm(this.props)) { - this.props.form.setErrors(errors); - } - + (this.props as PropsWithForm).form?.setErrors(errors); this.props.onInvalid(errors); } - clearErrors = () => hasForm(this.props) && this.props.form.clearErrors(); + clearErrors = () => (this.props as PropsWithForm).form?.clearErrors(); onFormSubmit = (event: React.FormEvent) => { event.preventDefault(); diff --git a/packages/app/components/ui/panel.scss b/packages/app/components/ui/panel.scss index aafb27a..efefea0 100644 --- a/packages/app/components/ui/panel.scss +++ b/packages/app/components/ui/panel.scss @@ -76,6 +76,7 @@ $bodyTopBottomPadding: 15px; padding: 10px 20px; margin: (-$bodyTopBottomPadding) (-$bodyLeftRightPadding) 15px; max-height: 200px; + text-align: center; transition: 0.4s ease; } diff --git a/packages/app/containers/AuthFlowRoute.tsx b/packages/app/containers/AuthFlowRoute.tsx index 78e777f..e881a4a 100644 --- a/packages/app/containers/AuthFlowRoute.tsx +++ b/packages/app/containers/AuthFlowRoute.tsx @@ -1,17 +1,16 @@ -import React, { FC } from 'react'; -import { Route, RouteProps } from 'react-router-dom'; +import React, { FC, ComponentType } from 'react'; +import { Route, RouteProps, RouteComponentProps } from 'react-router-dom'; import AuthFlowRouteContents from './AuthFlowRouteContents'; // Make "component" prop required -type Props = Omit & Required>; +type Props = Omit & { + component: ComponentType; +}; const AuthFlowRoute: FC = ({ component: Component, ...props }) => { return ( - } - /> + } /> ); }; diff --git a/packages/app/containers/AuthFlowRouteContents.test.tsx b/packages/app/containers/AuthFlowRouteContents.test.tsx index 0be6915..3d9b95a 100644 --- a/packages/app/containers/AuthFlowRouteContents.test.tsx +++ b/packages/app/containers/AuthFlowRouteContents.test.tsx @@ -43,7 +43,7 @@ describe('AuthFlowRouteContents', () => { (authFlow.handleRequest as any).callsArg(2); - render(); + render(); const component = screen.getByTestId('test-component'); diff --git a/packages/app/containers/AuthFlowRouteContents.tsx b/packages/app/containers/AuthFlowRouteContents.tsx index f59d53a..ab3e526 100644 --- a/packages/app/containers/AuthFlowRouteContents.tsx +++ b/packages/app/containers/AuthFlowRouteContents.tsx @@ -1,89 +1,34 @@ -import React from 'react'; -import { Redirect, RouteComponentProps } from 'react-router-dom'; +import React, { FC, ReactElement, ComponentType, useEffect, useState } from 'react'; +import { RouteComponentProps } from 'react-router-dom'; +import { useIsMounted } from 'app/hooks'; import authFlow from 'app/services/authFlow'; -interface Props { - component: React.ComponentType | React.ComponentType; - routerProps: RouteComponentProps; +interface Props extends RouteComponentProps { + component: ComponentType; } -interface State { - access: null | 'rejected' | 'allowed'; - component: React.ReactElement | null; -} - -export default class AuthFlowRouteContents extends React.Component { - state: State = { - access: null, - component: null, - }; - - mounted = false; - - shouldComponentUpdate({ routerProps: nextRoute, component: nextComponent }: Props, state: State) { - const { component: prevComponent, routerProps: prevRoute } = this.props; - - return ( - prevRoute.location.pathname !== nextRoute.location.pathname || - prevRoute.location.search !== nextRoute.location.search || - prevComponent !== nextComponent || - this.state.access !== state.access - ); - } - - componentDidMount() { - this.mounted = true; - this.handleProps(this.props); - } - - componentDidUpdate() { - this.handleProps(this.props); - } - - componentWillUnmount() { - this.mounted = false; - } - - render() { - return this.state.component; - } - - handleProps(props: Props) { - const { routerProps } = props; +const AuthFlowRouteContents: FC = ({ component: WantedComponent, location, match, history }) => { + const isMounted = useIsMounted(); + const [component, setComponent] = useState(null); + useEffect(() => { authFlow.handleRequest( { - path: routerProps.location.pathname, - params: routerProps.match.params, - query: new URLSearchParams(routerProps.location.search), + path: location.pathname, + params: match.params, + query: new URLSearchParams(location.search), + }, + history.push, + () => { + if (isMounted()) { + setComponent(); + } }, - this.onRedirect.bind(this), - this.onRouteAllowed.bind(this, props), ); - } + }, [location.pathname, location.search]); - onRedirect(path: string) { - if (!this.mounted) { - return; - } + return component; +}; - this.setState({ - access: 'rejected', - component: , - }); - } - - onRouteAllowed(props: Props): void { - if (!this.mounted) { - return; - } - - const { component: Component } = props; - - this.setState({ - access: 'allowed', - component: , - }); - } -} +export default AuthFlowRouteContents; diff --git a/packages/app/hooks/index.ts b/packages/app/hooks/index.ts new file mode 100644 index 0000000..e312e05 --- /dev/null +++ b/packages/app/hooks/index.ts @@ -0,0 +1 @@ +export { default as useIsMounted } from './useIsMounted'; diff --git a/packages/app/hooks/useIsMounted.ts b/packages/app/hooks/useIsMounted.ts new file mode 100644 index 0000000..598a338 --- /dev/null +++ b/packages/app/hooks/useIsMounted.ts @@ -0,0 +1,17 @@ +import { useCallback, useEffect, useRef } from 'react'; + +// This code is copied from the usehooks-ts: https://usehooks-ts.com/react-hook/use-is-mounted +// Replace it with the library when the Node.js version of the project will be updated at least to 16. +export default function useIsMounted(): () => boolean { + const isMounted = useRef(false); + + useEffect(() => { + isMounted.current = true; + + return () => { + isMounted.current = false; + }; + }, []); + + return useCallback(() => isMounted.current, []); +} diff --git a/packages/app/pages/auth/AuthPage.tsx b/packages/app/pages/auth/AuthPage.tsx index 4f0a9b3..7450674 100644 --- a/packages/app/pages/auth/AuthPage.tsx +++ b/packages/app/pages/auth/AuthPage.tsx @@ -1,5 +1,6 @@ import React, { FC, useCallback, useState } from 'react'; import { Route, Switch, Redirect, RouteComponentProps } from 'react-router-dom'; +import clsx from 'clsx'; import AppInfo from 'app/components/auth/appInfo/AppInfo'; import PanelTransition from 'app/components/auth/PanelTransition'; @@ -7,7 +8,6 @@ import Register from 'app/components/auth/register/Register'; import Login from 'app/components/auth/login/Login'; import Permissions from 'app/components/auth/permissions/Permissions'; import ChooseAccount from 'app/components/auth/chooseAccount/ChooseAccount'; -import DeviceCode from 'app/components/auth/deviceCode'; import Activation from 'app/components/auth/activation/Activation'; import ResendActivation from 'app/components/auth/resendActivation/ResendActivation'; import Password from 'app/components/auth/password/Password'; @@ -38,8 +38,8 @@ const AuthPage: FC = () => { }, []); return ( -
-
+ <> +
@@ -55,14 +55,13 @@ const AuthPage: FC = () => { -
-
+ ); }; diff --git a/packages/app/pages/auth/auth.scss b/packages/app/pages/auth/auth.scss index 0261ac5..5a227ec 100644 --- a/packages/app/pages/auth/auth.scss +++ b/packages/app/pages/auth/auth.scss @@ -14,8 +14,6 @@ $sidebar-width: 320px; } .hiddenSidebar { - composes: sidebar; - display: none; } diff --git a/packages/app/pages/root/RootPage.tsx b/packages/app/pages/root/RootPage.tsx index c9ba816..51a2a7b 100644 --- a/packages/app/pages/root/RootPage.tsx +++ b/packages/app/pages/root/RootPage.tsx @@ -1,8 +1,8 @@ -import React from 'react'; +import React, { FC, useCallback, useEffect } from 'react'; import { Route, Switch } from 'react-router-dom'; import clsx from 'clsx'; -import { connect } from 'app/functions'; +import { useReduxSelector, useReduxDispatch } from 'app/functions'; import { resetAuth } from 'app/components/auth/actions'; import { ScrollIntoView } from 'app/components/ui/scroll'; import PrivateRoute from 'app/containers/PrivateRoute'; @@ -10,14 +10,12 @@ import AuthFlowRoute from 'app/containers/AuthFlowRoute'; import { PopupStack } from 'app/components/ui/popup'; import * as loader from 'app/services/loader'; import { getActiveAccount } from 'app/components/accounts/reducer'; -import { User } from 'app/components/user'; -import { Account } from 'app/components/accounts/reducer'; import { ComponentLoader } from 'app/components/ui/loader'; -import Toolbar from './Toolbar'; +import PageNotFound from 'app/pages/404/PageNotFound'; +import DeviceCode from 'app/components/auth/deviceCode'; import styles from './root.scss'; - -import PageNotFound from 'app/pages/404/PageNotFound'; +import Toolbar from './Toolbar'; const ProfileController = React.lazy( () => import(/* webpackChunkName: "page-profile-all" */ 'app/pages/profile/ProfileController'), @@ -26,76 +24,56 @@ const RulesPage = React.lazy(() => import(/* webpackChunkName: "page-rules" */ ' const DevPage = React.lazy(() => import(/* webpackChunkName: "page-dev-applications" */ 'app/pages/dev/DevPage')); const AuthPage = React.lazy(() => import(/* webpackChunkName: "page-auth" */ 'app/pages/auth/AuthPage')); -class RootPage extends React.PureComponent<{ - account: Account | null; - user: User; - isPopupActive: boolean; - onLogoClick: (event: React.MouseEvent) => void; -}> { - componentDidMount() { - this.onPageUpdate(); - } +const RootPage: FC = () => { + const dispatch = useReduxDispatch(); + const user = useReduxSelector((state) => state.user); + const account = useReduxSelector(getActiveAccount); + const isPopupActive = useReduxSelector((state) => state.popup.popups.length > 0); - componentDidUpdate() { - this.onPageUpdate(); - } + const onLogoClick = useCallback(() => { + dispatch(resetAuth()); + }, []); - onPageUpdate() { + useEffect(() => { loader.hide(); - } + }); // No deps, effect must be called on every update - render() { - const { user, account, isPopupActive, onLogoClick } = this.props; + return ( +
+ - if (document && document.body) { - document.body.style.overflow = isPopupActive ? 'hidden' : ''; - } +
+ +
+ }> + + + + + - return ( -
- + + + -
- -
- }> - - - - - - - - - - - - -
+ + +
-
- ); - } -} + +
+ ); +}; -export default connect( - (state) => ({ - user: state.user, - account: getActiveAccount(state), - isPopupActive: state.popup.popups.length > 0, - }), - { - onLogoClick: resetAuth, - }, -)(RootPage); +export default RootPage; diff --git a/packages/app/services/authFlow/CompleteState.ts b/packages/app/services/authFlow/CompleteState.ts index 5ab296d..95a46ee 100644 --- a/packages/app/services/authFlow/CompleteState.ts +++ b/packages/app/services/authFlow/CompleteState.ts @@ -22,7 +22,7 @@ function hasPrompt(prompt: OAuthState['prompt'], needle: string): boolean { } export default class CompleteState extends AbstractState { - private readonly isPermissionsAccepted?: boolean; + isPermissionsAccepted?: boolean; constructor( options: { @@ -54,8 +54,6 @@ export default class CompleteState extends AbstractState { } processOAuth(context: AuthContext): Promise | void { - console.log('process oauth', this.isPermissionsAccepted); - const { auth, accounts, user } = context.getState(); let { isSwitcherEnabled } = auth; diff --git a/packages/app/services/errorsDict/errorsDict.tsx b/packages/app/services/errorsDict/errorsDict.tsx index 12fdad6..c19fc8b 100644 --- a/packages/app/services/errorsDict/errorsDict.tsx +++ b/packages/app/services/errorsDict/errorsDict.tsx @@ -128,7 +128,7 @@ const errorsMap: Record) => ReactElement> = 'error.redirectUri_required': () => , 'error.redirectUri_invalid': () => , - 'invalid_user_code': () => , + invalid_user_code: () => , }; interface ErrorLiteral { From b0975f0b0fe233258fc488bc2a5c2a7d424ba8cc Mon Sep 17 00:00:00 2001 From: ErickSkrauch Date: Tue, 17 Dec 2024 21:49:05 +0100 Subject: [PATCH 3/7] Fix E2E tests, related to the OAuth flow --- packages/app/components/auth/actions.ts | 2 +- packages/app/services/api/oauth.ts | 11 ++++---- .../cypress/integration/auth/oauth.test.ts | 26 ++++++++++--------- 3 files changed, 21 insertions(+), 18 deletions(-) diff --git a/packages/app/components/auth/actions.ts b/packages/app/components/auth/actions.ts index ed870ce..12a85ee 100644 --- a/packages/app/components/auth/actions.ts +++ b/packages/app/components/auth/actions.ts @@ -391,7 +391,7 @@ export function oAuthComplete(params: { accept?: boolean } = {}) { } else if (resp.redirectUri.startsWith('static_page')) { const displayCode = resp.redirectUri.includes('static_page_with_code'); - const [, code] = resp.redirectUri.match(/code=(.+)&/) || []; + const [, code] = resp.redirectUri.match(/code=([^&]+)/) || []; [, resp.redirectUri] = resp.redirectUri.match(/^(.+)\?/) || []; dispatch( diff --git a/packages/app/services/api/oauth.ts b/packages/app/services/api/oauth.ts index 634b110..8686ffb 100644 --- a/packages/app/services/api/oauth.ts +++ b/packages/app/services/api/oauth.ts @@ -69,16 +69,17 @@ const api = { success: boolean; redirectUri?: string; }> { - const query = request.buildQuery(oauthData); + const data: Record = {}; + + if (typeof params.accept !== 'undefined') { + data.accept = params.accept; + } return request .post<{ success: boolean; redirectUri: string; - }>( - `/api/oauth2/v1/complete?${query}`, - typeof params.accept === 'undefined' ? {} : { accept: params.accept }, - ) + }>(`/api/oauth2/v1/complete?${request.buildQuery(oauthData)}`, data) .catch((resp = {}) => { if (resp.statusCode === 401 && resp.error === 'access_denied') { // user declined permissions diff --git a/tests-e2e/cypress/integration/auth/oauth.test.ts b/tests-e2e/cypress/integration/auth/oauth.test.ts index df5061a..fbae030 100644 --- a/tests-e2e/cypress/integration/auth/oauth.test.ts +++ b/tests-e2e/cypress/integration/auth/oauth.test.ts @@ -1,4 +1,5 @@ import { account1 } from '../../fixtures/accounts.json'; +import { OAuthState } from 'app/components/auth/reducer'; import { UserResponse } from 'app/services/api/accounts'; const defaults = { @@ -23,14 +24,14 @@ describe('OAuth', () => { JSON.stringify({ timestamp: Date.now() - 3600, payload: { - clientId: 'ely', - redirectUrl: 'https://dev.ely.by/authorization/oauth', - responseType: 'code', - description: null, - scope: 'account_info account_email', - loginHint: null, - state: null, - }, + params: { + clientId: 'ely', + redirectUrl: 'https://dev.ely.by/authorization/oauth', + responseType: 'code', + state: '', + scope: 'account_info account_email', + }, + } as OAuthState, }), ); cy.login({ accounts: ['default'] }); @@ -81,6 +82,7 @@ describe('OAuth', () => { ...defaults, client_id: 'tlauncher', redirect_uri: 'http://localhost:8080', + state: '123', })}`, ); @@ -92,7 +94,7 @@ describe('OAuth', () => { cy.findByTestId('auth-controls').contains('Approve').click(); - cy.url().should('match', /^http:\/\/localhost:8080\/?\?code=[^&]+&state=$/); + cy.url().should('match', /^http:\/\/localhost:8080\/?\?code=[^&]+&state=123$/); }); it('should redirect to error page, when permission request declined', () => { @@ -334,7 +336,7 @@ describe('OAuth', () => { cy.findByTestId('auth-controls').contains('Approve').click(); - cy.url().should('match', /^http:\/\/localhost:8080\/?\?code=[^&]+&state=$/); + cy.url().should('match', /^http:\/\/localhost:8080\/?\?code=[^&]+$/); }); it('should redirect to error page, when permission request declined', () => { @@ -377,7 +379,7 @@ describe('OAuth', () => { cy.findByTestId('auth-controls').contains('Approve').click(); - cy.url().should('match', /^http:\/\/localhost:8080\/?\?code=[^&]+&state=$/); + cy.url().should('match', /^http:\/\/localhost:8080\/?\?code=[^&]+$/); }); }); @@ -403,7 +405,7 @@ describe('OAuth', () => { cy.findByTestId('auth-controls').contains('Approve').click(); - cy.url().should('match', /^http:\/\/localhost:8080\/?\?code=[^&]+&state=$/); + cy.url().should('match', /^http:\/\/localhost:8080\/?\?code=[^&]+$/); }); }); From be08857edc6d8cd4c19d7f2c0fdd14d2a1661099 Mon Sep 17 00:00:00 2001 From: ErickSkrauch Date: Tue, 17 Dec 2024 23:11:39 +0100 Subject: [PATCH 4/7] Add E2E tests for device code grant flow. Handle more errors for device code. Dispatch a BSOD for an any unhandled exception from auth flow state --- packages/app/components/auth/actions.ts | 6 - .../components/auth/authError/AuthError.tsx | 2 +- .../auth/deviceCode/DeviceCodeBody.tsx | 1 - packages/app/components/ui/Panel.tsx | 3 +- .../app/containers/AuthFlowRouteContents.tsx | 10 +- .../app/services/authFlow/AcceptRulesState.ts | 7 +- .../app/services/authFlow/ActivationState.ts | 6 +- packages/app/services/authFlow/AuthFlow.ts | 25 +- .../services/authFlow/ChooseAccountState.ts | 15 +- .../app/services/authFlow/CompleteState.ts | 74 +++--- .../app/services/authFlow/DeviceCodeState.ts | 4 +- .../services/authFlow/ForgotPasswordState.ts | 13 +- .../authFlow/InitOAuthDeviceCodeFlowState.ts | 3 +- packages/app/services/authFlow/LoginState.ts | 2 +- .../app/services/authFlow/PermissionsState.ts | 6 +- .../services/authFlow/RecoverPasswordState.ts | 7 +- .../app/services/authFlow/RegisterState.ts | 7 +- .../authFlow/ResendActivationState.ts | 6 +- .../app/services/errorsDict/errorsDict.tsx | 7 + .../cypress/integration/auth/oauth.test.ts | 239 ++++++++++++------ 20 files changed, 267 insertions(+), 176 deletions(-) diff --git a/packages/app/components/auth/actions.ts b/packages/app/components/auth/actions.ts index 12a85ee..c84ce29 100644 --- a/packages/app/components/auth/actions.ts +++ b/packages/app/components/auth/actions.ts @@ -19,7 +19,6 @@ import { activate as activateEndpoint, resendActivation as resendActivationEndpoint, } from 'app/services/api/signup'; -import dispatchBsod from 'app/components/ui/bsod/dispatchBsod'; import { create as createPopup } from 'app/components/ui/popup/actions'; import ContactForm from 'app/components/contact'; import { Account } from 'app/components/accounts/reducer'; @@ -455,11 +454,6 @@ function handleOauthParamsValidation( userMessage?: string; } = {}, ) { - // TODO: it would be better to dispatch BSOD from the initial request performers - if (resp.error !== 'invalid_user_code') { - dispatchBsod(); - } - localStorage.removeItem('oauthData'); // eslint-disable-next-line no-alert diff --git a/packages/app/components/auth/authError/AuthError.tsx b/packages/app/components/auth/authError/AuthError.tsx index b3582e8..369c946 100644 --- a/packages/app/components/auth/authError/AuthError.tsx +++ b/packages/app/components/auth/authError/AuthError.tsx @@ -31,7 +31,7 @@ const AuthError: ComponentType = ({ error, onClose }) => { }, [error, onClose]); return ( - + {resolveError(error)} ); diff --git a/packages/app/components/auth/deviceCode/DeviceCodeBody.tsx b/packages/app/components/auth/deviceCode/DeviceCodeBody.tsx index ddbaa33..e0e97d4 100644 --- a/packages/app/components/auth/deviceCode/DeviceCodeBody.tsx +++ b/packages/app/components/auth/deviceCode/DeviceCodeBody.tsx @@ -20,7 +20,6 @@ export default class DeviceCodeBody extends BaseAuthBody { { onClose?: () => void; } -export const PanelBodyHeader: FC = ({ type = 'default', onClose, children }) => { +export const PanelBodyHeader: FC = ({ type = 'default', onClose, children, ...props }) => { const [isClosed, setIsClosed] = useState(false); const handleCloseClick = useCallback(() => { setIsClosed(true); @@ -71,6 +71,7 @@ export const PanelBodyHeader: FC = ({ type = 'default', on [styles.errorBodyHeader]: type === 'error', [styles.isClosed]: isClosed, })} + {...props} > {type === 'error' && } {children} diff --git a/packages/app/containers/AuthFlowRouteContents.tsx b/packages/app/containers/AuthFlowRouteContents.tsx index ab3e526..ca03d16 100644 --- a/packages/app/containers/AuthFlowRouteContents.tsx +++ b/packages/app/containers/AuthFlowRouteContents.tsx @@ -13,6 +13,10 @@ const AuthFlowRouteContents: FC = ({ component: WantedComponent, location const [component, setComponent] = useState(null); useEffect(() => { + // Promise that will be resolved after handleRequest might contain already non-actual component to render, + // so set it to false in the effect's clear function to prevent unwanted UI state + let isActual = true; + authFlow.handleRequest( { path: location.pathname, @@ -21,11 +25,15 @@ const AuthFlowRouteContents: FC = ({ component: WantedComponent, location }, history.push, () => { - if (isMounted()) { + if (isActual && isMounted()) { setComponent(); } }, ); + + return () => { + isActual = false; + }; }, [location.pathname, location.search]); return component; diff --git a/packages/app/services/authFlow/AcceptRulesState.ts b/packages/app/services/authFlow/AcceptRulesState.ts index d5c8308..3acb4b2 100644 --- a/packages/app/services/authFlow/AcceptRulesState.ts +++ b/packages/app/services/authFlow/AcceptRulesState.ts @@ -1,5 +1,3 @@ -import logger from 'app/services/logger'; - import AbstractState from './AbstractState'; import { AuthContext } from './AuthFlow'; import CompleteState from './CompleteState'; @@ -16,10 +14,7 @@ export default class AcceptRulesState extends AbstractState { } resolve(context: AuthContext): Promise | void { - context - .run('acceptRules') - .then(() => context.setState(new CompleteState())) - .catch((err = {}) => err.errors || logger.warn('Error accepting rules', err)); + return context.run('acceptRules').then(() => context.setState(new CompleteState())); } reject(context: AuthContext, payload: Record): void { diff --git a/packages/app/services/authFlow/ActivationState.ts b/packages/app/services/authFlow/ActivationState.ts index 8581b20..9112af5 100644 --- a/packages/app/services/authFlow/ActivationState.ts +++ b/packages/app/services/authFlow/ActivationState.ts @@ -1,4 +1,3 @@ -import logger from 'app/services/logger'; import { AuthContext } from 'app/services/authFlow'; import AbstractState from './AbstractState'; @@ -12,10 +11,7 @@ export default class ActivationState extends AbstractState { } resolve(context: AuthContext, payload: { key: string }): Promise | void { - context - .run('activate', payload.key) - .then(() => context.setState(new CompleteState())) - .catch((err = {}) => err.errors || logger.warn('Error activating account', err)); + return context.run('activate', payload.key).then(() => context.setState(new CompleteState())); } reject(context: AuthContext): void { diff --git a/packages/app/services/authFlow/AuthFlow.ts b/packages/app/services/authFlow/AuthFlow.ts index 9c11d22..2611a17 100644 --- a/packages/app/services/authFlow/AuthFlow.ts +++ b/packages/app/services/authFlow/AuthFlow.ts @@ -10,8 +10,9 @@ import { } from 'app/components/accounts/actions'; import * as actions from 'app/components/auth/actions'; import { updateUser } from 'app/components/user/actions'; -import FinishState from './FinishState'; +import dispatchBsod from 'app/components/ui/bsod/dispatchBsod'; +import FinishState from './FinishState'; import RegisterState from './RegisterState'; import LoginState from './LoginState'; import InitOAuthAuthCodeFlowState from './InitOAuthAuthCodeFlowState'; @@ -121,11 +122,23 @@ export default class AuthFlow implements AuthContext { } resolve(payload: Record = {}) { - this.state.resolve(this, payload); + const maybePromise = this.state.resolve(this, payload); + + if (maybePromise && maybePromise.catch) { + maybePromise.catch((err) => { + dispatchBsod(); + throw err; + }); + } } reject(payload: Record = {}) { - this.state.reject(this, payload); + try { + this.state.reject(this, payload); + } catch (err) { + dispatchBsod(); + throw err; + } } goBack() { @@ -133,11 +146,11 @@ export default class AuthFlow implements AuthContext { } run(actionId: T, payload?: Parameters[0]): Promise { - // @ts-ignore the extended version of redux with thunk will return the correct promise + // @ts-expect-error the extended version of redux with thunk will return the correct promise return Promise.resolve(this.dispatch(this.actions[actionId](payload))); } - setState(state: State) { + setState(state: State): Promise | void { this.state?.leave(this); this.prevState = this.state; this.state = state; @@ -256,8 +269,6 @@ export default class AuthFlow implements AuthContext { /** * Tries to restore last oauth request, if it was stored in localStorage * in last 2 hours - * - * @returns {bool} - whether oauth state is being restored */ private restoreOAuthState(): boolean { if (this.oAuthStateRestored) { diff --git a/packages/app/services/authFlow/ChooseAccountState.ts b/packages/app/services/authFlow/ChooseAccountState.ts index a04879b..143341e 100644 --- a/packages/app/services/authFlow/ChooseAccountState.ts +++ b/packages/app/services/authFlow/ChooseAccountState.ts @@ -1,4 +1,5 @@ import type { Account } from 'app/components/accounts/reducer'; +import logger from 'app/services/logger'; import AbstractState from './AbstractState'; import { AuthContext } from './AuthFlow'; @@ -21,10 +22,16 @@ export default class ChooseAccountState extends AbstractState { // So if there is no `id` property, it's an empty object resolve(context: AuthContext, payload: Account): Promise | void { if (payload.id) { - return context - .run('authenticate', payload) - .then(() => context.run('setAccountSwitcher', false)) - .then(() => context.setState(new CompleteState())); + return ( + context + .run('authenticate', payload) + .then(() => context.run('setAccountSwitcher', false)) + .then(() => context.setState(new CompleteState())) + // By default, this error must cause a BSOD. But by I don't know why reasons it shouldn't, + // because somebody somewhere catches an invalid authentication result and routes the user + // to the password entering form. To keep this behavior we catch all errors, log it and suppress + .catch((err) => err.errors || logger.warn('Error choosing an account', err)) + ); } // log in to another account diff --git a/packages/app/services/authFlow/CompleteState.ts b/packages/app/services/authFlow/CompleteState.ts index 95a46ee..3ea52d5 100644 --- a/packages/app/services/authFlow/CompleteState.ts +++ b/packages/app/services/authFlow/CompleteState.ts @@ -92,45 +92,49 @@ export default class CompleteState extends AbstractState { user.isDeleted || hasPrompt(oauth.prompt, PROMPT_ACCOUNT_CHOOSE)) ) { - context.setState(new ChooseAccountState()); - } else if (user.isDeleted) { + return context.setState(new ChooseAccountState()); + } + + if (user.isDeleted) { // you shall not pass // if we are here, this means that user have already seen account // switcher and now we should redirect him to his profile, // because oauth is not available for deleted accounts - context.navigate('/'); - } else if (oauth.code) { - context.setState(new FinishState()); - } else { - const data: Record = {}; - - if (typeof this.isPermissionsAccepted !== 'undefined') { - data.accept = this.isPermissionsAccepted; - } else if (oauth.acceptRequired || hasPrompt(oauth.prompt, PROMPT_PERMISSIONS)) { - context.setState(new PermissionsState()); - - return; - } - - // TODO: it seems that oAuthComplete may be a separate state - return context - .run('oAuthComplete', data) - .then((resp: { redirectUri?: string }) => { - // TODO: пусть в стейт попадает флаг или тип авторизации - // вместо волшебства над редирект урлой - if (!resp.redirectUri || resp.redirectUri.includes('static_page')) { - context.setState(new FinishState()); - } else { - return context.run('redirect', resp.redirectUri); - } - }) - .catch((resp) => { - if (resp.unauthorized) { - context.setState(new LoginState()); - } else if (resp.acceptRequired) { - context.setState(new PermissionsState()); - } - }); + return context.navigate('/'); } + + if (oauth.code) { + return context.setState(new FinishState()); + } + + const data: Record = {}; + + if (typeof this.isPermissionsAccepted !== 'undefined') { + data.accept = this.isPermissionsAccepted; + } else if (oauth.acceptRequired || hasPrompt(oauth.prompt, PROMPT_PERMISSIONS)) { + context.setState(new PermissionsState()); + + return; + } + + // TODO: it seems that oAuthComplete may be a separate state + return context + .run('oAuthComplete', data) + .then((resp: { redirectUri?: string }) => { + // TODO: пусть в стейт попадает флаг или тип авторизации + // вместо волшебства над редирект урлой + if (!resp.redirectUri || resp.redirectUri.includes('static_page')) { + context.setState(new FinishState()); + } else { + return context.run('redirect', resp.redirectUri); + } + }) + .catch((resp) => { + if (resp.unauthorized) { + context.setState(new LoginState()); + } else if (resp.acceptRequired) { + context.setState(new PermissionsState()); + } + }); } } diff --git a/packages/app/services/authFlow/DeviceCodeState.ts b/packages/app/services/authFlow/DeviceCodeState.ts index 4d80169..a4adb50 100644 --- a/packages/app/services/authFlow/DeviceCodeState.ts +++ b/packages/app/services/authFlow/DeviceCodeState.ts @@ -6,7 +6,7 @@ export default class DeviceCodeState extends AbstractState { async resolve(context: AuthContext, payload: { user_code: string }): Promise { const { query } = context.getRequest(); - context + return context .run('oAuthValidate', { params: { userCode: payload.user_code, @@ -16,7 +16,7 @@ export default class DeviceCodeState extends AbstractState { }) .then(() => context.setState(new CompleteState())) .catch((err) => { - if (err.error === 'invalid_user_code') { + if (['invalid_user_code', 'expired_token', 'used_user_code'].includes(err.error)) { return context.run('setErrors', { [err.parameter]: err.error }); } diff --git a/packages/app/services/authFlow/ForgotPasswordState.ts b/packages/app/services/authFlow/ForgotPasswordState.ts index d323b4f..2e5920f 100644 --- a/packages/app/services/authFlow/ForgotPasswordState.ts +++ b/packages/app/services/authFlow/ForgotPasswordState.ts @@ -1,5 +1,3 @@ -import logger from 'app/services/logger'; - import AbstractState from './AbstractState'; import { AuthContext } from './AuthFlow'; import LoginState from './LoginState'; @@ -11,13 +9,10 @@ export default class ForgotPasswordState extends AbstractState { } resolve(context: AuthContext, payload: { login: string; captcha: string }): Promise | void { - context - .run('forgotPassword', payload) - .then(() => { - context.run('setLogin', payload.login); - context.setState(new RecoverPasswordState()); - }) - .catch((err = {}) => err.errors || logger.warn('Error requesting password recoverage', err)); + return context.run('forgotPassword', payload).then(() => { + context.run('setLogin', payload.login); + context.setState(new RecoverPasswordState()); + }); } goBack(context: AuthContext): void { diff --git a/packages/app/services/authFlow/InitOAuthDeviceCodeFlowState.ts b/packages/app/services/authFlow/InitOAuthDeviceCodeFlowState.ts index 022f1db..74d14ee 100644 --- a/packages/app/services/authFlow/InitOAuthDeviceCodeFlowState.ts +++ b/packages/app/services/authFlow/InitOAuthDeviceCodeFlowState.ts @@ -16,7 +16,8 @@ export default class InitOAuthDeviceCodeFlowState extends AbstractState { description: query.get('description')!, prompt: query.get('prompt')!, }); - await context.setState(new CompleteState()); + + return context.setState(new CompleteState()); } catch { // Ok, fallback to the default } diff --git a/packages/app/services/authFlow/LoginState.ts b/packages/app/services/authFlow/LoginState.ts index b67b2b9..3f6b583 100644 --- a/packages/app/services/authFlow/LoginState.ts +++ b/packages/app/services/authFlow/LoginState.ts @@ -30,7 +30,7 @@ export default class LoginState extends AbstractState { login: string; }, ): Promise | void { - context + return context .run('login', payload) .then(() => context.setState(new PasswordState())) .catch((err = {}) => err.errors || logger.warn('Error validating login', err)); diff --git a/packages/app/services/authFlow/PermissionsState.ts b/packages/app/services/authFlow/PermissionsState.ts index 6aa70a8..2d59a23 100644 --- a/packages/app/services/authFlow/PermissionsState.ts +++ b/packages/app/services/authFlow/PermissionsState.ts @@ -12,15 +12,15 @@ export default class PermissionsState extends AbstractState { } resolve(context: AuthContext): Promise | void { - this.process(context, true); + return this.process(context, true); } reject(context: AuthContext): void { this.process(context, false); } - process(context: AuthContext, accept: boolean): void { - context.setState( + process(context: AuthContext, accept: boolean): Promise | void { + return context.setState( new CompleteState({ accept, }), diff --git a/packages/app/services/authFlow/RecoverPasswordState.ts b/packages/app/services/authFlow/RecoverPasswordState.ts index 79f9bfd..88803c6 100644 --- a/packages/app/services/authFlow/RecoverPasswordState.ts +++ b/packages/app/services/authFlow/RecoverPasswordState.ts @@ -1,5 +1,3 @@ -import logger from 'app/services/logger'; - import AbstractState from './AbstractState'; import { AuthContext } from './AuthFlow'; import LoginState from './LoginState'; @@ -17,10 +15,7 @@ export default class RecoverPasswordState extends AbstractState { context: AuthContext, payload: { key: string; newPassword: string; newRePassword: string }, ): Promise | void { - context - .run('recoverPassword', payload) - .then(() => context.setState(new CompleteState())) - .catch((err = {}) => err.errors || logger.warn('Error recovering password', err)); + return context.run('recoverPassword', payload).then(() => context.setState(new CompleteState())); } goBack(context: AuthContext): void { diff --git a/packages/app/services/authFlow/RegisterState.ts b/packages/app/services/authFlow/RegisterState.ts index fbc0561..a41b850 100644 --- a/packages/app/services/authFlow/RegisterState.ts +++ b/packages/app/services/authFlow/RegisterState.ts @@ -1,5 +1,3 @@ -import logger from 'app/services/logger'; - import AbstractState from './AbstractState'; import { AuthContext } from './AuthFlow'; import CompleteState from './CompleteState'; @@ -22,10 +20,7 @@ export default class RegisterState extends AbstractState { rulesAgreement: boolean; }, ): Promise | void { - context - .run('register', payload) - .then(() => context.setState(new CompleteState())) - .catch((err = {}) => err.errors || logger.warn('Error registering', err)); + return context.run('register', payload).then(() => context.setState(new CompleteState())); } reject(context: AuthContext, payload: Record): void { diff --git a/packages/app/services/authFlow/ResendActivationState.ts b/packages/app/services/authFlow/ResendActivationState.ts index 5796976..1e1dd1b 100644 --- a/packages/app/services/authFlow/ResendActivationState.ts +++ b/packages/app/services/authFlow/ResendActivationState.ts @@ -1,5 +1,4 @@ import { AuthContext } from 'app/services/authFlow'; -import logger from 'app/services/logger'; import AbstractState from './AbstractState'; import ActivationState from './ActivationState'; @@ -11,10 +10,7 @@ export default class ResendActivationState extends AbstractState { } resolve(context: AuthContext, payload: { email: string; captcha: string }): Promise | void { - context - .run('resendActivation', payload) - .then(() => context.setState(new ActivationState())) - .catch((err = {}) => err.errors || logger.warn('Error resending activation', err)); + return context.run('resendActivation', payload).then(() => context.setState(new ActivationState())); } reject(context: AuthContext): void { diff --git a/packages/app/services/errorsDict/errorsDict.tsx b/packages/app/services/errorsDict/errorsDict.tsx index c19fc8b..87775bc 100644 --- a/packages/app/services/errorsDict/errorsDict.tsx +++ b/packages/app/services/errorsDict/errorsDict.tsx @@ -129,6 +129,13 @@ const errorsMap: Record) => ReactElement> = 'error.redirectUri_invalid': () => , invalid_user_code: () => , + expired_token: () => ( + + ), + used_user_code: () => , }; interface ErrorLiteral { diff --git a/tests-e2e/cypress/integration/auth/oauth.test.ts b/tests-e2e/cypress/integration/auth/oauth.test.ts index fbae030..3f54909 100644 --- a/tests-e2e/cypress/integration/auth/oauth.test.ts +++ b/tests-e2e/cypress/integration/auth/oauth.test.ts @@ -10,35 +10,175 @@ const defaults = { }; describe('OAuth', () => { - it('should complete oauth', () => { - cy.login({ accounts: ['default'] }); + describe('AuthCode grant flow', () => { + it('should complete oauth', () => { + cy.login({ accounts: ['default'] }); - cy.visit(`/oauth2/v1/ely?${new URLSearchParams(defaults)}`); + cy.visit(`/oauth2/v1/ely?${new URLSearchParams(defaults)}`); - cy.url().should('equal', 'https://dev.ely.by/'); + cy.url().should('equal', 'https://dev.ely.by/'); + }); + + it('should restore previous oauthData if any', () => { + localStorage.setItem( + 'oauthData', + JSON.stringify({ + timestamp: Date.now() - 3600, + payload: { + params: { + clientId: 'ely', + redirectUrl: 'https://dev.ely.by/authorization/oauth', + responseType: 'code', + state: '', + scope: 'account_info account_email', + }, + } as OAuthState, + }), + ); + cy.login({ accounts: ['default'] }); + + cy.visit('/'); + + cy.url().should('equal', 'https://dev.ely.by/'); + }); + + describe('static pages', () => { + it('should authenticate using static page', () => { + cy.server(); + cy.route({ + method: 'POST', + url: '/api/oauth2/v1/complete**', + }).as('complete'); + + cy.login({ accounts: ['default'] }); + + cy.visit( + `/oauth2/v1/ely?${new URLSearchParams({ + ...defaults, + client_id: 'tlauncher', + redirect_uri: 'static_page', + })}`, + ); + + cy.wait('@complete'); + + cy.url().should('include', 'oauth/finish#{%22auth_code%22:'); + }); + + it('should authenticate using static page with code', () => { + cy.server(); + cy.route({ + method: 'POST', + url: '/api/oauth2/v1/complete**', + }).as('complete'); + + cy.login({ accounts: ['default'] }); + + cy.visit( + `/oauth2/v1/ely?${new URLSearchParams({ + ...defaults, + client_id: 'tlauncher', + redirect_uri: 'static_page_with_code', + })}`, + ); + + cy.wait('@complete'); + + cy.url().should('include', 'oauth/finish#{%22auth_code%22:'); + + cy.findByTestId('oauth-code-container').should('contain', 'provide the following code'); + + // just click on copy, but we won't assert if the string was copied + // because it is a little bit complicated + // https://github.com/cypress-io/cypress/issues/2752 + cy.findByTestId('oauth-code-container').contains('Copy').click(); + }); + }); }); - it('should restore previous oauthData if any', () => { - localStorage.setItem( - 'oauthData', - JSON.stringify({ - timestamp: Date.now() - 3600, - payload: { - params: { - clientId: 'ely', - redirectUrl: 'https://dev.ely.by/authorization/oauth', - responseType: 'code', - state: '', - scope: 'account_info account_email', - }, - } as OAuthState, - }), - ); - cy.login({ accounts: ['default'] }); + describe('DeviceCode grant flow', () => { + it('should complete flow by complete uri', () => { + cy.login({ accounts: ['default'] }); - cy.visit('/'); + cy.visit('/code?user_code=E2E-APPROVED'); - cy.url().should('equal', 'https://dev.ely.by/'); + cy.location('pathname').should('eq', '/oauth/finish'); + cy.get('[data-e2e-content]').contains('successfully completed'); + }); + + it('should complete flow with manual approve', () => { + cy.login({ accounts: ['default'] }); + + cy.visit('/code'); + + cy.get('[name=user_code]').type('E2E-UNAPPROVED{enter}'); + + cy.location('pathname').should('eq', '/oauth/permissions'); + + cy.findByTestId('auth-controls').contains('Approve').click(); + + cy.location('pathname').should('eq', '/oauth/finish'); + cy.get('[data-e2e-content]').contains('successfully completed'); + }); + + it('should complete flow with auto approve', () => { + cy.login({ accounts: ['default'] }); + + cy.visit('/code'); + + cy.get('[name=user_code]').type('E2E-APPROVED{enter}'); + + cy.location('pathname').should('eq', '/oauth/finish'); + cy.get('[data-e2e-content]').contains('successfully completed'); + }); + + it('should complete flow by declining the code', () => { + cy.login({ accounts: ['default'] }); + + cy.visit('/code'); + + cy.get('[name=user_code]').type('E2E-UNAPPROVED{enter}'); + + cy.location('pathname').should('eq', '/oauth/permissions'); + + cy.findByTestId('auth-controls-secondary').contains('Decline').click(); + + cy.location('pathname').should('eq', '/oauth/finish'); + cy.get('[data-e2e-content]').contains('was failed'); + }); + + it('should show an error for an unknown code', () => { + cy.login({ accounts: ['default'] }); + + cy.visit('/code'); + + cy.get('[name=user_code]').type('UNKNOWN-CODE{enter}'); + + cy.location('pathname').should('eq', '/code'); + cy.findByTestId('auth-error').contains('Invalid Device Code'); + }); + + it('should show an error for an expired code', () => { + cy.login({ accounts: ['default'] }); + + cy.visit('/code'); + + cy.get('[name=user_code]').type('E2E-EXPIRED{enter}'); + + cy.location('pathname').should('eq', '/code'); + cy.findByTestId('auth-error').contains('The code has expired'); + }); + + it('should show an error for an expired code', () => { + cy.login({ accounts: ['default'] }); + + cy.visit('/code'); + + cy.get('[name=user_code]').type('E2E-COMPLETED{enter}'); + + cy.location('pathname').should('eq', '/code'); + cy.findByTestId('auth-error').contains('This code has been already used'); + }); }); describe('AccountSwitcher', () => { @@ -408,59 +548,6 @@ describe('OAuth', () => { cy.url().should('match', /^http:\/\/localhost:8080\/?\?code=[^&]+$/); }); }); - - describe('static pages', () => { - it('should authenticate using static page', () => { - cy.server(); - cy.route({ - method: 'POST', - url: '/api/oauth2/v1/complete**', - }).as('complete'); - - cy.login({ accounts: ['default'] }); - - cy.visit( - `/oauth2/v1/ely?${new URLSearchParams({ - ...defaults, - client_id: 'tlauncher', - redirect_uri: 'static_page', - })}`, - ); - - cy.wait('@complete'); - - cy.url().should('include', 'oauth/finish#{%22auth_code%22:'); - }); - - it('should authenticate using static page with code', () => { - cy.server(); - cy.route({ - method: 'POST', - url: '/api/oauth2/v1/complete**', - }).as('complete'); - - cy.login({ accounts: ['default'] }); - - cy.visit( - `/oauth2/v1/ely?${new URLSearchParams({ - ...defaults, - client_id: 'tlauncher', - redirect_uri: 'static_page_with_code', - })}`, - ); - - cy.wait('@complete'); - - cy.url().should('include', 'oauth/finish#{%22auth_code%22:'); - - cy.findByTestId('oauth-code-container').should('contain', 'provide the following code'); - - // just click on copy, but we won't assert if the string was copied - // because it is a little bit complicated - // https://github.com/cypress-io/cypress/issues/2752 - cy.findByTestId('oauth-code-container').contains('Copy').click(); - }); - }); }); function assertPermissions() { From a6932255b0af2eba4662ca2b24ea054260e7836e Mon Sep 17 00:00:00 2001 From: ErickSkrauch Date: Mon, 23 Dec 2024 12:33:41 +0100 Subject: [PATCH 5/7] Some tests progress [skip ci] --- packages/app/components/auth/actions.test.ts | 10 +- packages/app/components/auth/actions.ts | 5 +- .../auth/deviceCode/DeviceCodeBody.tsx | 2 +- .../containers/AuthFlowRouteContents.test.tsx | 62 ----------- .../app/services/authFlow/AuthFlow.test.ts | 8 +- .../services/authFlow/CompleteState.test.ts | 105 +++++++++++------- .../InitOAuthAuthCodeFlowState.test.ts | 48 ++++---- 7 files changed, 106 insertions(+), 134 deletions(-) delete mode 100644 packages/app/containers/AuthFlowRouteContents.test.tsx diff --git a/packages/app/components/auth/actions.test.ts b/packages/app/components/auth/actions.test.ts index 77d2a22..02537f4 100644 --- a/packages/app/components/auth/actions.test.ts +++ b/packages/app/components/auth/actions.test.ts @@ -88,9 +88,13 @@ describe('components/auth/actions', () => { [ setOAuthRequest({ params: { - userCode: '', + clientId: '', + redirectUrl: '', + responseType: '', + state: '', + scope: '', }, - prompt: 'none', + prompt: ['none'], loginHint: undefined, }), ], @@ -117,7 +121,7 @@ describe('components/auth/actions', () => { return callThunk(oAuthComplete).then(() => { expect(request.post, 'to have a call satisfying', [ - '/api/oauth2/v1/complete?client_id=&redirect_uri=&response_type=&description=&scope=&prompt=none&login_hint=&state=', + '/api/oauth2/v1/complete?client_id=&redirect_uri=&response_type=&scope=&state=', {}, ]); }); diff --git a/packages/app/components/auth/actions.ts b/packages/app/components/auth/actions.ts index c84ce29..eb49d47 100644 --- a/packages/app/components/auth/actions.ts +++ b/packages/app/components/auth/actions.ts @@ -481,10 +481,11 @@ interface SetOauthAction extends ReduxAction { payload: OAuthState | null; } +type SetOauthRequestPayload = Pick; + // Input data is coming right from the query string, so the names // are the same, as used for initializing OAuth2 request -// TODO: filter out allowed properties -export function setOAuthRequest(payload: OAuthState | null): SetOauthAction { +export function setOAuthRequest(payload: SetOauthRequestPayload | null): SetOauthAction { return { type: 'set_oauth', payload, diff --git a/packages/app/components/auth/deviceCode/DeviceCodeBody.tsx b/packages/app/components/auth/deviceCode/DeviceCodeBody.tsx index e0e97d4..912f982 100644 --- a/packages/app/components/auth/deviceCode/DeviceCodeBody.tsx +++ b/packages/app/components/auth/deviceCode/DeviceCodeBody.tsx @@ -15,7 +15,7 @@ export default class DeviceCodeBody extends BaseAuthBody { <> {this.renderErrors()} - + {(nodes) => ( { - beforeEach(() => { - sinon.stub(authFlow, 'handleRequest'); - }); - - afterEach(() => { - (authFlow.handleRequest as any).restore(); - }); - - let componentProps: { [key: string]: any }; - - function Component(props: { [key: string]: any }) { - componentProps = props; - - return
; - } - - it('should render component if route allowed', () => { - const authRequest = { - path: '/path', - params: { foo: 1 }, - query: new URLSearchParams(), - }; - - const routerProps: any = { - location: { - pathname: authRequest.path, - search: '', - query: new URLSearchParams(), - }, - match: { - params: authRequest.params, - }, - }; - - (authFlow.handleRequest as any).callsArg(2); - - render(); - - const component = screen.getByTestId('test-component'); - - uxpect(authFlow.handleRequest, 'to have a call satisfying', [ - { - ...authRequest, - query: uxpect.it('to be a', URLSearchParams), - }, - uxpect.it('to be a function'), - uxpect.it('to be a function'), - ]); - - expect(component).toBeInTheDocument(); - uxpect(componentProps, 'to equal', routerProps); - }); -}); diff --git a/packages/app/services/authFlow/AuthFlow.test.ts b/packages/app/services/authFlow/AuthFlow.test.ts index 5b92da3..7a90627 100644 --- a/packages/app/services/authFlow/AuthFlow.test.ts +++ b/packages/app/services/authFlow/AuthFlow.test.ts @@ -14,6 +14,7 @@ import ResendActivationState from 'app/services/authFlow/ResendActivationState'; import LoginState from 'app/services/authFlow/LoginState'; import CompleteState from 'app/services/authFlow/CompleteState'; import ChooseAccountState from 'app/services/authFlow/ChooseAccountState'; +import FinishState from 'app/services/authFlow/FinishState'; import { Store } from 'redux'; describe('AuthFlow', () => { @@ -211,11 +212,6 @@ describe('AuthFlow', () => { expect(actual, 'to be', expected); }); - - it('should throw if no state', () => { - // @ts-ignore - expect(() => flow.setState(), 'to throw', 'State is required'); - }); }); describe('#run', () => { @@ -313,7 +309,7 @@ describe('AuthFlow', () => { '/accept-rules': LoginState, '/oauth/permissions': LoginState, '/oauth/choose-account': LoginState, - '/oauth/finish': LoginState, + '/oauth/finish': FinishState, '/oauth2/v1/foo': InitOAuthAuthCodeFlowState, '/oauth2/v1': InitOAuthAuthCodeFlowState, '/oauth2': InitOAuthAuthCodeFlowState, diff --git a/packages/app/services/authFlow/CompleteState.test.ts b/packages/app/services/authFlow/CompleteState.test.ts index 85d3576..f0d625e 100644 --- a/packages/app/services/authFlow/CompleteState.test.ts +++ b/packages/app/services/authFlow/CompleteState.test.ts @@ -137,7 +137,9 @@ describe('CompleteState', () => { auth: { credentials: {}, oauth: { - clientId: 'ely.by', + params: { + clientId: 'ely.by', + }, code: 'XXX', }, }, @@ -157,7 +159,9 @@ describe('CompleteState', () => { auth: { credentials: {}, oauth: { - clientId: 'ely.by', + params: { + clientId: 'ely.by', + }, acceptRequired: true, }, }, @@ -176,7 +180,9 @@ describe('CompleteState', () => { auth: { credentials: {}, oauth: { - clientId: 'ely.by', + params: { + clientId: 'ely.by', + }, prompt: ['consent'], }, }, @@ -202,7 +208,9 @@ describe('CompleteState', () => { credentials: {}, isSwitcherEnabled: true, oauth: { - clientId: 'ely.by', + params: { + clientId: 'ely.by', + }, prompt: [], }, }, @@ -227,7 +235,9 @@ describe('CompleteState', () => { isSwitcherEnabled: true, credentials: {}, oauth: { - clientId: 'ely.by', + params: { + clientId: 'ely.by', + }, prompt: [], }, }, @@ -251,13 +261,15 @@ describe('CompleteState', () => { isSwitcherEnabled: false, credentials: {}, oauth: { - clientId: 'ely.by', + params: { + clientId: 'ely.by', + }, prompt: [], }, }, }); - expectRun(mock, 'oAuthComplete', {}).returns({ then() {} }); + expectRun(mock, 'oAuthComplete', {}).returns(Promise.resolve()); state.enter(context); }); @@ -275,7 +287,9 @@ describe('CompleteState', () => { isSwitcherEnabled: true, credentials: {}, oauth: { - clientId: 'ely.by', + params: { + clientId: 'ely.by', + }, prompt: ['select_account'], }, }, @@ -299,13 +313,15 @@ describe('CompleteState', () => { isSwitcherEnabled: false, credentials: {}, oauth: { - clientId: 'ely.by', + params: { + clientId: 'ely.by', + }, prompt: ['select_account'], }, }, }); - expectRun(mock, 'oAuthComplete', {}).returns({ then() {} }); + expectRun(mock, 'oAuthComplete', {}).returns(Promise.resolve()); state.enter(context); }); @@ -322,15 +338,15 @@ describe('CompleteState', () => { auth: { credentials: {}, oauth: { - clientId: 'ely.by', + params: { + clientId: 'ely.by', + }, prompt: [], }, }, }); - expectRun(mock, 'oAuthComplete', sinon.match.object).returns({ - then() {}, - }); + expectRun(mock, 'oAuthComplete', sinon.match.object).returns(Promise.resolve()); state.enter(context); }); @@ -343,7 +359,9 @@ describe('CompleteState', () => { auth: { credentials: {}, oauth: { - clientId: 'ely.by', + params: { + clientId: 'ely.by', + }, prompt: [], }, }, @@ -370,7 +388,9 @@ describe('CompleteState', () => { auth: { credentials: {}, oauth: { - clientId: 'ely.by', + params: { + clientId: 'ely.by', + }, prompt: [], }, }, @@ -399,7 +419,9 @@ describe('CompleteState', () => { auth: { credentials: {}, oauth: { - clientId: 'ely.by', + params: { + clientId: 'ely.by', + }, prompt: [], }, }, @@ -447,7 +469,9 @@ describe('CompleteState', () => { auth: { credentials: {}, oauth: { - clientId: 'ely.by', + params: { + clientId: 'ely.by', + }, loginHint: account[field], prompt: [], }, @@ -485,7 +509,9 @@ describe('CompleteState', () => { auth: { credentials: {}, oauth: { - clientId: 'ely.by', + params: { + clientId: 'ely.by', + }, loginHint: account.id, prompt: [], }, @@ -518,16 +544,16 @@ describe('CompleteState', () => { auth: { credentials: {}, oauth: { - clientId: 'ely.by', + params: { + clientId: 'ely.by', + }, loginHint: account.id, prompt: [], }, }, }); - expectRun(mock, 'oAuthComplete', {}).returns({ - then: () => Promise.resolve(), - }); + expectRun(mock, 'oAuthComplete', {}).returns(Promise.resolve()); return expect(state.enter(context), 'to be fulfilled'); }); @@ -560,16 +586,15 @@ describe('CompleteState', () => { auth: { credentials: {}, oauth: { - clientId: 'ely.by', + params: { + clientId: 'ely.by', + }, prompt: [], }, }, }); - mock.expects('run') - .once() - .withExactArgs('oAuthComplete', sinon.match(expected)) - .returns({ then() {} }); + mock.expects('run').once().withExactArgs('oAuthComplete', sinon.match(expected)).returns(Promise.resolve()); state.enter(context); }); @@ -585,15 +610,15 @@ describe('CompleteState', () => { auth: { credentials: {}, oauth: { - clientId: 'ely.by', + params: { + clientId: 'ely.by', + }, prompt: [], }, }, }); - expectRun(mock, 'oAuthComplete', sinon.match(expected)).returns({ - then() {}, - }); + expectRun(mock, 'oAuthComplete', sinon.match(expected)).returns(Promise.resolve()); state.enter(context); }); @@ -611,16 +636,16 @@ describe('CompleteState', () => { auth: { credentials: {}, oauth: { - clientId: 'ely.by', + params: { + clientId: 'ely.by', + }, prompt: [], acceptRequired: true, }, }, }); - expectRun(mock, 'oAuthComplete', sinon.match(expected)).returns({ - then() {}, - }); + expectRun(mock, 'oAuthComplete', sinon.match(expected)).returns(Promise.resolve()); state.enter(context); }); @@ -638,16 +663,16 @@ describe('CompleteState', () => { auth: { credentials: {}, oauth: { - clientId: 'ely.by', + params: { + clientId: 'ely.by', + }, prompt: [], acceptRequired: true, }, }, }); - expectRun(mock, 'oAuthComplete', sinon.match(expected)).returns({ - then() {}, - }); + expectRun(mock, 'oAuthComplete', sinon.match(expected)).returns(Promise.resolve()); state.enter(context); }); diff --git a/packages/app/services/authFlow/InitOAuthAuthCodeFlowState.test.ts b/packages/app/services/authFlow/InitOAuthAuthCodeFlowState.test.ts index bf1ba78..f80f86a 100644 --- a/packages/app/services/authFlow/InitOAuthAuthCodeFlowState.test.ts +++ b/packages/app/services/authFlow/InitOAuthAuthCodeFlowState.test.ts @@ -44,14 +44,16 @@ describe('OAuthState', () => { mock, 'oAuthValidate', sinon.match({ - clientId: query.client_id, - redirectUrl: query.redirect_uri, - responseType: query.response_type, + params: { + clientId: query.client_id, + redirectUrl: query.redirect_uri, + responseType: query.response_type, + scope: query.scope, + state: query.state, + }, description: query.description, - scope: query.scope, prompt: query.prompt, loginHint: query.login_hint, - state: query.state, }), ).returns({ then() {} }); @@ -76,11 +78,13 @@ describe('OAuthState', () => { mock, 'oAuthValidate', sinon.match({ - clientId, - redirectUrl: query.redirect_uri, - responseType: query.response_type, - scope: query.scope, - state: query.state, + params: { + clientId, + redirectUrl: query.redirect_uri, + responseType: query.response_type, + scope: query.scope, + state: query.state, + }, }), ).returns({ then() {} }); @@ -106,11 +110,13 @@ describe('OAuthState', () => { mock, 'oAuthValidate', sinon.match({ - clientId: query.client_id, - redirectUrl: query.redirect_uri, - responseType: query.response_type, - scope: query.scope, - state: query.state, + params: { + clientId: query.client_id, + redirectUrl: query.redirect_uri, + responseType: query.response_type, + scope: query.scope, + state: query.state, + }, }), ).returns({ then() {} }); @@ -134,11 +140,13 @@ describe('OAuthState', () => { mock, 'oAuthValidate', sinon.match({ - clientId: query.client_id, - redirectUrl: query.redirect_uri, - responseType: query.response_type, - scope: 'scope1 scope2 scope3', - state: query.state, + params: { + clientId: query.client_id, + redirectUrl: query.redirect_uri, + responseType: query.response_type, + scope: 'scope1 scope2 scope3', + state: query.state, + }, }), ).returns({ then() {} }); From cf5973c8767f22eaacc7eec6eb22a1957f226bef Mon Sep 17 00:00:00 2001 From: ErickSkrauch Date: Mon, 23 Dec 2024 14:53:49 +0100 Subject: [PATCH 6/7] Fix unit tests --- packages/app/package.json | 1 + .../authFlow/AuthFlow.functional.test.ts | 18 +++++----- .../services/authFlow/CompleteState.test.ts | 34 ++----------------- yarn.lock | 5 +++ 4 files changed, 19 insertions(+), 39 deletions(-) diff --git a/packages/app/package.json b/packages/app/package.json index b733761..9562ddb 100644 --- a/packages/app/package.json +++ b/packages/app/package.json @@ -49,6 +49,7 @@ "@types/rtl-detect": "^1.0.0", "@types/webfontloader": "^1.6.30", "@types/webpack-env": "^1.15.2", + "synchronous-promise": "^2.0.17", "utility-types": "^3.10.0" } } diff --git a/packages/app/services/authFlow/AuthFlow.functional.test.ts b/packages/app/services/authFlow/AuthFlow.functional.test.ts index 376b379..16f6fdd 100644 --- a/packages/app/services/authFlow/AuthFlow.functional.test.ts +++ b/packages/app/services/authFlow/AuthFlow.functional.test.ts @@ -1,5 +1,6 @@ import expect from 'app/test/unexpected'; import sinon from 'sinon'; +import { SynchronousPromise } from 'synchronous-promise'; import { Store } from 'redux'; @@ -100,21 +101,22 @@ describe('AuthFlow.functional', () => { auth: { credentials: {}, oauth: { - clientId: 123, + params: { + clientId: 123, + }, prompt: [], }, }, }); // @ts-ignore - flow.run.onCall(0).returns({ then: (fn) => fn() }); + flow.run.onCall(0).returns(SynchronousPromise.resolve()); // @ts-ignore - flow.run.onCall(1).returns({ - then: (fn: Function) => - fn({ - redirectUri: expectedRedirect, - }), - }); + flow.run.onCall(1).returns( + SynchronousPromise.resolve({ + redirectUri: expectedRedirect, + }), + ); navigate('/oauth2'); diff --git a/packages/app/services/authFlow/CompleteState.test.ts b/packages/app/services/authFlow/CompleteState.test.ts index f0d625e..7cb25c3 100644 --- a/packages/app/services/authFlow/CompleteState.test.ts +++ b/packages/app/services/authFlow/CompleteState.test.ts @@ -1,5 +1,6 @@ import expect from 'app/test/unexpected'; import sinon, { SinonMock } from 'sinon'; +import { SynchronousPromise } from 'synchronous-promise'; import CompleteState from 'app/services/authFlow/CompleteState'; import LoginState from 'app/services/authFlow/LoginState'; @@ -351,32 +352,6 @@ describe('CompleteState', () => { state.enter(context); }); - it('should listen for auth success/failure', () => { - context.getState.returns({ - user: { - isGuest: false, - }, - auth: { - credentials: {}, - oauth: { - params: { - clientId: 'ely.by', - }, - prompt: [], - }, - }, - }); - - expectRun(mock, 'oAuthComplete', sinon.match.object).returns({ - then(success: Function, fail: Function) { - expect(success, 'to be a', 'function'); - expect(fail, 'to be a', 'function'); - }, - }); - - state.enter(context); - }); - it('should run redirect by default', () => { const expectedUrl = 'foo/bar'; const promise = Promise.resolve({ redirectUri: expectedUrl }); @@ -409,8 +384,7 @@ describe('CompleteState', () => { resp: Record, expectedInstance: typeof AbstractState, ) => { - // @ts-ignore - const promise = Promise[type](resp); + const promise = SynchronousPromise[type](resp); context.getState.returns({ user: { @@ -519,9 +493,7 @@ describe('CompleteState', () => { }); expectRun(mock, 'setAccountSwitcher', false); - expectRun(mock, 'oAuthComplete', {}).returns({ - then: () => Promise.resolve(), - }); + expectRun(mock, 'oAuthComplete', {}).returns(SynchronousPromise.resolve()); return expect(state.enter(context), 'to be fulfilled'); }); diff --git a/yarn.lock b/yarn.lock index aa95157..0c12799 100644 --- a/yarn.lock +++ b/yarn.lock @@ -13829,6 +13829,11 @@ symbol.prototype.description@^1.0.0: has-symbols "^1.0.1" object.getownpropertydescriptors "^2.1.2" +synchronous-promise@^2.0.17: + version "2.0.17" + resolved "https://registry.yarnpkg.com/synchronous-promise/-/synchronous-promise-2.0.17.tgz#38901319632f946c982152586f2caf8ddc25c032" + integrity sha512-AsS729u2RHUfEra9xJrE39peJcc2stq2+poBXX8bcM08Y6g9j/i/PUzwNQqkaJde7Ntg1TO7bSREbR5sdosQ+g== + table@^6.0.9: version "6.7.1" resolved "https://registry.yarnpkg.com/table/-/table-6.7.1.tgz#ee05592b7143831a8c94f3cee6aae4c1ccef33e2" From f96e5318120b6f3532113a14bf3d1fa611b278dd Mon Sep 17 00:00:00 2001 From: ErickSkrauch Date: Mon, 23 Dec 2024 15:13:10 +0100 Subject: [PATCH 7/7] Update the design --- .../auth/deviceCode/DeviceCodeBody.tsx | 13 ++++++++++++- .../components/auth/deviceCode/deviceCode.scss | 17 +++++++++++++++++ packages/app/icons/webfont/hash.svg | 3 +++ 3 files changed, 32 insertions(+), 1 deletion(-) create mode 100644 packages/app/icons/webfont/hash.svg diff --git a/packages/app/components/auth/deviceCode/DeviceCodeBody.tsx b/packages/app/components/auth/deviceCode/DeviceCodeBody.tsx index 912f982..9356c10 100644 --- a/packages/app/components/auth/deviceCode/DeviceCodeBody.tsx +++ b/packages/app/components/auth/deviceCode/DeviceCodeBody.tsx @@ -4,6 +4,8 @@ import { FormattedMessage as Message } from 'react-intl'; import { Input } from 'app/components/ui/form'; import BaseAuthBody from 'app/components/auth/BaseAuthBody'; +import style from './deviceCode.scss'; + export default class DeviceCodeBody extends BaseAuthBody { static displayName = 'DeviceCodeBody'; static panelId = 'deviceCode'; @@ -15,12 +17,21 @@ export default class DeviceCodeBody extends BaseAuthBody { <> {this.renderErrors()} +
+ +
+ +
+ {(nodes) => ( diff --git a/packages/app/components/auth/deviceCode/deviceCode.scss b/packages/app/components/auth/deviceCode/deviceCode.scss index f90e318..ac9e2af 100644 --- a/packages/app/components/auth/deviceCode/deviceCode.scss +++ b/packages/app/components/auth/deviceCode/deviceCode.scss @@ -3,3 +3,20 @@ margin: 0 auto; padding: 55px 13px 0; } + +.description { + font-size: 15px; + color: #aaa; + text-align: center; + margin-bottom: 15px; +} + +.icon { + composes: hash from '~app/components/ui/icons.scss'; + + display: block; + text-align: center; + font-size: 69px; + color: #ccc; + margin-block: 5px 15px; +} diff --git a/packages/app/icons/webfont/hash.svg b/packages/app/icons/webfont/hash.svg new file mode 100644 index 0000000..2f35f8c --- /dev/null +++ b/packages/app/icons/webfont/hash.svg @@ -0,0 +1,3 @@ + + +