From 831ab42155a11fb4bef8a883ff22b1addea6ddcc Mon Sep 17 00:00:00 2001 From: SleepWalker Date: Sun, 11 Oct 2020 20:15:53 +0300 Subject: [PATCH] fix: correct logic for handling deleted users during oauth (#27) --- .../services/authFlow/ChooseAccountState.ts | 6 +- .../services/authFlow/CompleteState.test.ts | 316 ++++++++++-------- .../app/services/authFlow/CompleteState.ts | 22 +- .../cypress/integration/auth/oauth.test.ts | 306 +++++++++-------- 4 files changed, 354 insertions(+), 296 deletions(-) diff --git a/packages/app/services/authFlow/ChooseAccountState.ts b/packages/app/services/authFlow/ChooseAccountState.ts index 3f514d3..9982149 100644 --- a/packages/app/services/authFlow/ChooseAccountState.ts +++ b/packages/app/services/authFlow/ChooseAccountState.ts @@ -1,3 +1,5 @@ +import type { Account } from 'app/components/accounts/reducer'; + import AbstractState from './AbstractState'; import { AuthContext } from './AuthFlow'; import LoginState from './LoginState'; @@ -14,10 +16,12 @@ export default class ChooseAccountState extends AbstractState { } } - resolve(context: AuthContext, payload: Record): Promise | void { + resolve(context: AuthContext, payload: Account | Record): Promise | void { if (payload.id) { + // payload is Account context.setState(new CompleteState()); } else { + // log in to another account context.navigate('/login'); context.run('setLogin', null); context.setState(new LoginState()); diff --git a/packages/app/services/authFlow/CompleteState.test.ts b/packages/app/services/authFlow/CompleteState.test.ts index e718dcb..bf51f86 100644 --- a/packages/app/services/authFlow/CompleteState.test.ts +++ b/packages/app/services/authFlow/CompleteState.test.ts @@ -22,8 +22,7 @@ describe('CompleteState', () => { state = new CompleteState(); const data = bootstrap(); - context = data.context; - mock = data.mock; + ({ context, mock } = data); }); afterEach(() => { @@ -116,157 +115,188 @@ describe('CompleteState', () => { state.enter(context); }); - it('should transition to finish state if code is present', () => { - context.getState.returns({ - user: { - isActive: true, - isGuest: false, - }, - auth: { - oauth: { - clientId: 'ely.by', - code: 'XXX', + describe('oauth', () => { + it('should transition to finish state if code is present', () => { + context.getState.returns({ + user: { + isActive: true, + isGuest: false, }, - }, + auth: { + oauth: { + clientId: 'ely.by', + code: 'XXX', + }, + }, + }); + + expectState(mock, FinishState); + + state.enter(context); }); - expectState(mock, FinishState); + describe('permissions', () => { + it('should transition to permissions state if acceptRequired', () => { + context.getState.returns({ + user: { + isActive: true, + isGuest: false, + }, + auth: { + oauth: { + clientId: 'ely.by', + acceptRequired: true, + }, + }, + }); - state.enter(context); - }); + expectState(mock, PermissionsState); - it('should transition to permissions state if acceptRequired', () => { - context.getState.returns({ - user: { - isActive: true, - isGuest: false, - }, - auth: { - oauth: { - clientId: 'ely.by', - acceptRequired: true, - }, - }, + state.enter(context); + }); + + it('should transition to permissions state if prompt=consent', () => { + context.getState.returns({ + user: { + isActive: true, + isGuest: false, + }, + auth: { + oauth: { + clientId: 'ely.by', + prompt: ['consent'], + }, + }, + }); + + expectState(mock, PermissionsState); + + state.enter(context); + }); }); - expectState(mock, PermissionsState); + describe('account switcher', () => { + it('should transition to ChooseAccountState if user has multiple accs and switcher enabled', () => { + context.getState.returns({ + user: { + isActive: true, + isGuest: false, + }, + accounts: { + available: [{ id: 1 }, { id: 2 }], + active: 1, + }, + auth: { + isSwitcherEnabled: true, + oauth: { + clientId: 'ely.by', + prompt: [], + }, + }, + }); - state.enter(context); - }); + expectState(mock, ChooseAccountState); - it('should transition to permissions state if prompt=consent', () => { - context.getState.returns({ - user: { - isActive: true, - isGuest: false, - }, - auth: { - oauth: { - clientId: 'ely.by', - prompt: ['consent'], - }, - }, + state.enter(context); + }); + + it('should transition to ChooseAccountState if user isDeleted', () => { + context.getState.returns({ + user: { + isActive: true, + isDeleted: true, + isGuest: false, + }, + accounts: { + available: [{ id: 1 }], + active: 1, + }, + auth: { + isSwitcherEnabled: true, + oauth: { + clientId: 'ely.by', + prompt: [], + }, + }, + }); + + expectState(mock, ChooseAccountState); + + state.enter(context); + }); + + it('should NOT transition to ChooseAccountState if user has multiple accs and switcher disabled', () => { + context.getState.returns({ + user: { + isActive: true, + isGuest: false, + }, + accounts: { + available: [{ id: 1 }, { id: 2 }], + active: 1, + }, + auth: { + isSwitcherEnabled: false, + oauth: { + clientId: 'ely.by', + prompt: [], + }, + }, + }); + + expectRun(mock, 'oAuthComplete', {}).returns({ then() {} }); + + state.enter(context); + }); + + it('should transition to ChooseAccountState if prompt=select_account and switcher enabled', () => { + context.getState.returns({ + user: { + isActive: true, + isGuest: false, + }, + accounts: { + available: [{ id: 1 }], + active: 1, + }, + auth: { + isSwitcherEnabled: true, + oauth: { + clientId: 'ely.by', + prompt: ['select_account'], + }, + }, + }); + + expectState(mock, ChooseAccountState); + + state.enter(context); + }); + + it('should NOT transition to ChooseAccountState if prompt=select_account and switcher disabled', () => { + context.getState.returns({ + user: { + isActive: true, + isGuest: false, + }, + accounts: { + available: [{ id: 1 }], + active: 1, + }, + auth: { + isSwitcherEnabled: false, + oauth: { + clientId: 'ely.by', + prompt: ['select_account'], + }, + }, + }); + + expectRun(mock, 'oAuthComplete', {}).returns({ then() {} }); + + state.enter(context); + }); }); - - expectState(mock, PermissionsState); - - state.enter(context); - }); - - it('should transition to ChooseAccountState if user has multiple accs and switcher enabled', () => { - context.getState.returns({ - user: { - isActive: true, - isGuest: false, - }, - accounts: { - available: [{ id: 1 }, { id: 2 }], - active: 1, - }, - auth: { - isSwitcherEnabled: true, - oauth: { - clientId: 'ely.by', - prompt: [], - }, - }, - }); - - expectState(mock, ChooseAccountState); - - state.enter(context); - }); - - it('should NOT transition to ChooseAccountState if user has multiple accs and switcher disabled', () => { - context.getState.returns({ - user: { - isActive: true, - isGuest: false, - }, - accounts: { - available: [{ id: 1 }, { id: 2 }], - active: 1, - }, - auth: { - isSwitcherEnabled: false, - oauth: { - clientId: 'ely.by', - prompt: [], - }, - }, - }); - - expectRun(mock, 'oAuthComplete', {}).returns({ then() {} }); - - state.enter(context); - }); - - it('should transition to ChooseAccountState if prompt=select_account and switcher enabled', () => { - context.getState.returns({ - user: { - isActive: true, - isGuest: false, - }, - accounts: { - available: [{ id: 1 }], - active: 1, - }, - auth: { - isSwitcherEnabled: true, - oauth: { - clientId: 'ely.by', - prompt: ['select_account'], - }, - }, - }); - - expectState(mock, ChooseAccountState); - - state.enter(context); - }); - - it('should NOT transition to ChooseAccountState if prompt=select_account and switcher disabled', () => { - context.getState.returns({ - user: { - isActive: true, - isGuest: false, - }, - accounts: { - available: [{ id: 1 }], - active: 1, - }, - auth: { - isSwitcherEnabled: false, - oauth: { - clientId: 'ely.by', - prompt: ['select_account'], - }, - }, - }); - - expectRun(mock, 'oAuthComplete', {}).returns({ then() {} }); - - state.enter(context); }); }); diff --git a/packages/app/services/authFlow/CompleteState.ts b/packages/app/services/authFlow/CompleteState.ts index 19a8027..a432e9f 100644 --- a/packages/app/services/authFlow/CompleteState.ts +++ b/packages/app/services/authFlow/CompleteState.ts @@ -34,9 +34,7 @@ export default class CompleteState extends AbstractState { context.setState(new LoginState()); } else if (!user.isActive) { context.setState(new ActivationState()); - } else if (user.isDeleted) { - context.navigate('/'); - } else if (user.shouldAcceptRules) { + } else if (user.shouldAcceptRules && !user.isDeleted) { context.setState(new AcceptRulesState()); } else if (oauth && oauth.clientId) { return this.processOAuth(context); @@ -46,7 +44,7 @@ export default class CompleteState extends AbstractState { } processOAuth(context: AuthContext): Promise | void { - const { auth, accounts } = context.getState(); + const { auth, accounts, user } = context.getState(); let { isSwitcherEnabled } = auth; const { oauth } = auth; @@ -75,8 +73,22 @@ export default class CompleteState extends AbstractState { } } - if (isSwitcherEnabled && (accounts.available.length > 1 || oauth.prompt.includes(PROMPT_ACCOUNT_CHOOSE))) { + if ( + isSwitcherEnabled && + (accounts.available.length > 1 || + // we are always showing account switcher for deleted users + // so that they can see, that their account was deleted + // (this info is displayed on switcher) + user.isDeleted || + oauth.prompt.includes(PROMPT_ACCOUNT_CHOOSE)) + ) { context.setState(new ChooseAccountState()); + } else 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 { diff --git a/tests-e2e/cypress/integration/auth/oauth.test.ts b/tests-e2e/cypress/integration/auth/oauth.test.ts index 753d852..68d8ecb 100644 --- a/tests-e2e/cypress/integration/auth/oauth.test.ts +++ b/tests-e2e/cypress/integration/auth/oauth.test.ts @@ -17,36 +17,6 @@ describe('OAuth', () => { cy.url().should('equal', 'https://dev.ely.by/'); }); - it('should not complete oauth if account is deleted', () => { - cy.login({ accounts: ['default'] }); - - cy.server(); - cy.route({ - method: 'GET', - url: `/api/v1/accounts/${account1.id}`, - response: { - id: 7, - uuid: '522e8c19-89d8-4a6d-a2ec-72ebb58c2dbe', - username: 'SleepWalker', - isOtpEnabled: false, - registeredAt: 1475568334, - lang: 'en', - elyProfileLink: 'http://ely.by/u7', - email: 'danilenkos@auroraglobal.com', - isActive: true, - isDeleted: true, // force user into the deleted state - passwordChangedAt: 1476075696, - hasMojangUsernameCollision: true, - shouldAcceptRules: false, - } as UserResponse, - }); - - cy.visit(`/oauth2/v1/ely?${new URLSearchParams(defaults)}`); - - cy.location('pathname').should('eq', '/'); - cy.findByTestId('deletedAccount').should('contain', 'Account is deleted'); - }); - it('should restore previous oauthData if any', () => { localStorage.setItem( 'oauthData', @@ -70,144 +40,186 @@ describe('OAuth', () => { cy.url().should('equal', 'https://dev.ely.by/'); }); - it('should ask to choose an account if user has multiple', () => { - cy.login({ accounts: ['default', 'default2'] }).then(({ accounts: [account] }) => { + describe('AccountSwitcher', () => { + it('should ask to choose an account if user has multiple', () => { + cy.login({ accounts: ['default', 'default2'] }).then(({ accounts: [account] }) => { + cy.visit(`/oauth2/v1/ely?${new URLSearchParams(defaults)}`); + + cy.url().should('include', '/oauth/choose-account'); + + cy.findByTestId('auth-header').should('contain', 'Choose an account'); + + cy.findByTestId('auth-body').contains(account.email).click(); + + cy.url().should('equal', 'https://dev.ely.by/'); + }); + }); + }); + + describe('Permissions prompt', () => { + // TODO: remove api mocks, when we will be able to revoke permissions + it('should prompt for permissions', () => { + cy.server(); + + cy.route({ + method: 'POST', + // NOTE: can not use cypress glob syntax, because it will break due to + // '%2F%2F' (//) in redirect_uri + // url: '/api/oauth2/v1/complete/*', + url: new RegExp('/api/oauth2/v1/complete'), + response: { + statusCode: 401, + error: 'accept_required', + }, + status: 401, + }).as('complete'); + + cy.login({ accounts: ['default'] }); + + cy.visit( + `/oauth2/v1/ely?${new URLSearchParams({ + ...defaults, + client_id: 'tlauncher', + redirect_uri: 'http://localhost:8080', + })}`, + ); + + cy.wait('@complete'); + + assertPermissions(); + + cy.server({ enable: false }); + + cy.findByTestId('auth-controls').contains('Approve').click(); + + cy.url().should('match', /^http:\/\/localhost:8080\/?\?code=[^&]+&state=$/); + }); + + // TODO: enable, when backend api will return correct response on auth decline + xit('should redirect to error page, when permission request declined', () => { + cy.server(); + + cy.route({ + method: 'POST', + // NOTE: can not use cypress glob syntax, because it will break due to + // '%2F%2F' (//) in redirect_uri + // url: '/api/oauth2/v1/complete/*', + url: new RegExp('/api/oauth2/v1/complete'), + response: { + statusCode: 401, + error: 'accept_required', + }, + status: 401, + }).as('complete'); + + cy.login({ accounts: ['default'] }); + + cy.visit( + `/oauth2/v1/ely?${new URLSearchParams({ + ...defaults, + client_id: 'tlauncher', + redirect_uri: 'http://localhost:8080', + })}`, + ); + + cy.wait('@complete'); + + assertPermissions(); + + cy.server({ enable: false }); + + cy.findByTestId('auth-controls-secondary').contains('Decline').click(); + + cy.url().should('include', 'error=access_denied'); + }); + }); + + describe('Sign-in during oauth', () => { + it('should allow sign in during oauth (guest oauth)', () => { cy.visit(`/oauth2/v1/ely?${new URLSearchParams(defaults)}`); - cy.url().should('include', '/oauth/choose-account'); + cy.location('pathname').should('eq', '/login'); - cy.findByTestId('auth-header').should('contain', 'Choose an account'); + cy.get('[name=login]').type(`${account1.login}{enter}`); - cy.findByTestId('auth-body').contains(account.email).click(); + cy.url().should('include', '/password'); + + cy.get('[name=password]').type(`${account1.password}{enter}`); cy.url().should('equal', 'https://dev.ely.by/'); }); }); - // TODO: remove api mocks, when we will be able to revoke permissions - it('should prompt for permissions', () => { - cy.server(); + describe('Deleted account', () => { + it('should show account switcher and then abort oauth and redirect to profile', () => { + cy.login({ accounts: ['default'] }).then(({ accounts: [account] }) => { + cy.server(); + cy.route({ + method: 'GET', + url: `/api/v1/accounts/${account1.id}`, + response: { + id: account.id, + uuid: '522e8c19-89d8-4a6d-a2ec-72ebb58c2dbe', + username: account.username, + isOtpEnabled: false, + registeredAt: 1475568334, + lang: 'en', + elyProfileLink: 'http://ely.by/u7', + email: account.email, + isActive: true, + isDeleted: true, // force user into the deleted state + passwordChangedAt: 1476075696, + hasMojangUsernameCollision: true, + shouldAcceptRules: false, + } as UserResponse, + }); - cy.route({ - method: 'POST', - // NOTE: can not use cypress glob syntax, because it will break due to - // '%2F%2F' (//) in redirect_uri - // url: '/api/oauth2/v1/complete/*', - url: new RegExp('/api/oauth2/v1/complete'), - response: { - statusCode: 401, - error: 'accept_required', - }, - status: 401, - }).as('complete'); + cy.visit(`/oauth2/v1/ely?${new URLSearchParams(defaults)}`); - cy.login({ accounts: ['default'] }); + cy.findByTestId('auth-header').should('contain', 'Choose an account'); - cy.visit( - `/oauth2/v1/ely?${new URLSearchParams({ - ...defaults, - client_id: 'tlauncher', - redirect_uri: 'http://localhost:8080', - })}`, - ); + cy.findByTestId('auth-body').contains(account.email).click(); - cy.wait('@complete'); - - assertPermissions(); - - cy.server({ enable: false }); - - cy.findByTestId('auth-controls').contains('Approve').click(); - - cy.url().should('match', /^http:\/\/localhost:8080\/?\?code=[^&]+&state=$/); - }); - - it('should allow sign in during oauth (guest oauth)', () => { - cy.visit(`/oauth2/v1/ely?${new URLSearchParams(defaults)}`); - - cy.location('pathname').should('eq', '/login'); - - cy.get('[name=login]').type(`${account1.login}{enter}`); - - cy.url().should('include', '/password'); - - cy.get('[name=password]').type(`${account1.password}{enter}`); - - cy.url().should('equal', 'https://dev.ely.by/'); - }); - - it('should allow sign in during oauth and not finish process if the account is deleted', () => { - cy.visit(`/oauth2/v1/ely?${new URLSearchParams(defaults)}`); - - cy.location('pathname').should('eq', '/login'); - - cy.get('[name=login]').type(`${account1.login}{enter}`); - - cy.url().should('include', '/password'); - - cy.server(); - cy.route({ - method: 'GET', - url: `/api/v1/accounts/${account1.id}`, - response: { - id: 7, - uuid: '522e8c19-89d8-4a6d-a2ec-72ebb58c2dbe', - username: 'SleepWalker', - isOtpEnabled: false, - registeredAt: 1475568334, - lang: 'en', - elyProfileLink: 'http://ely.by/u7', - email: 'danilenkos@auroraglobal.com', - isActive: true, - isDeleted: true, // force user into the deleted state - passwordChangedAt: 1476075696, - hasMojangUsernameCollision: true, - shouldAcceptRules: false, - } as UserResponse, + cy.location('pathname').should('eq', '/'); + cy.findByTestId('deletedAccount').should('contain', 'Account is deleted'); + }); }); - cy.get('[name=password]').type(`${account1.password}{enter}`); + it('should allow sign and then abort oauth and redirect to profile', () => { + cy.visit(`/oauth2/v1/ely?${new URLSearchParams(defaults)}`); - cy.location('pathname').should('eq', '/'); - cy.findByTestId('deletedAccount').should('contain', 'Account is deleted'); - }); + cy.location('pathname').should('eq', '/login'); - // TODO: enable, when backend api will return correct response on auth decline - xit('should redirect to error page, when permission request declined', () => { - cy.server(); + cy.get('[name=login]').type(`${account1.login}{enter}`); - cy.route({ - method: 'POST', - // NOTE: can not use cypress glob syntax, because it will break due to - // '%2F%2F' (//) in redirect_uri - // url: '/api/oauth2/v1/complete/*', - url: new RegExp('/api/oauth2/v1/complete'), - response: { - statusCode: 401, - error: 'accept_required', - }, - status: 401, - }).as('complete'); + cy.url().should('include', '/password'); - cy.login({ accounts: ['default'] }); + cy.server(); + cy.route({ + method: 'GET', + url: `/api/v1/accounts/${account1.id}`, + response: { + id: 7, + uuid: '522e8c19-89d8-4a6d-a2ec-72ebb58c2dbe', + username: 'SleepWalker', + isOtpEnabled: false, + registeredAt: 1475568334, + lang: 'en', + elyProfileLink: 'http://ely.by/u7', + email: 'danilenkos@auroraglobal.com', + isActive: true, + isDeleted: true, // force user into the deleted state + passwordChangedAt: 1476075696, + hasMojangUsernameCollision: true, + shouldAcceptRules: false, + } as UserResponse, + }); - cy.visit( - `/oauth2/v1/ely?${new URLSearchParams({ - ...defaults, - client_id: 'tlauncher', - redirect_uri: 'http://localhost:8080', - })}`, - ); + cy.get('[name=password]').type(`${account1.password}{enter}`); - cy.wait('@complete'); - - assertPermissions(); - - cy.server({ enable: false }); - - cy.findByTestId('auth-controls-secondary').contains('Decline').click(); - - cy.url().should('include', 'error=access_denied'); + cy.location('pathname').should('eq', '/'); + cy.findByTestId('deletedAccount').should('contain', 'Account is deleted'); + }); }); describe('login_hint', () => {