fix: correct logic for handling deleted users during oauth (#27)

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<string, any>): Promise<void> | void {
+    resolve(context: AuthContext, payload: Account | Record<string, any>): Promise<void> | 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());

packages/app/services/authFlow/CompleteState.test.ts

@@ -22,8 +22,7 @@ describe('CompleteState', () => {
         state = new CompleteState();
 
         const data = bootstrap();
-        context = data.context;
+        ({ context, mock } = data);
-        mock = data.mock;
     });
 
     afterEach(() => {
@@ -116,157 +115,188 @@ describe('CompleteState', () => {
             state.enter(context);
         });
 
-        it('should transition to finish state if code is present', () => {
+        describe('oauth', () => {
-            context.getState.returns({
+            it('should transition to finish state if code is present', () => {
-                user: {
+                context.getState.returns({
-                    isActive: true,
+                    user: {
-                    isGuest: false,
+                        isActive: true,
-                },
+                        isGuest: false,
-                auth: {
-                    oauth: {
-                        clientId: 'ely.by',
-                        code: 'XXX',
                     },
-                },
+                    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', () => {
+                    state.enter(context);
-            context.getState.returns({
+                });
-                user: {
+
-                    isActive: true,
+                it('should transition to permissions state if prompt=consent', () => {
-                    isGuest: false,
+                    context.getState.returns({
-                },
+                        user: {
-                auth: {
+                            isActive: true,
-                    oauth: {
+                            isGuest: false,
-                        clientId: 'ely.by',
+                        },
-                        acceptRequired: true,
+                        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', () => {
+                    state.enter(context);
-            context.getState.returns({
+                });
-                user: {
+
-                    isActive: true,
+                it('should transition to ChooseAccountState if user isDeleted', () => {
-                    isGuest: false,
+                    context.getState.returns({
-                },
+                        user: {
-                auth: {
+                            isActive: true,
-                    oauth: {
+                            isDeleted: true,
-                        clientId: 'ely.by',
+                            isGuest: false,
-                        prompt: ['consent'],
+                        },
-                    },
+                        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);
         });
     });
 

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) {
+        } else if (user.shouldAcceptRules && !user.isDeleted) {
-            context.navigate('/');
-        } else if (user.shouldAcceptRules) {
             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> | 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 {

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', () => {
+    describe('AccountSwitcher', () => {
-        cy.login({ accounts: ['default', 'default2'] }).then(({ accounts: [account] }) => {
+        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
+    describe('Deleted account', () => {
-    it('should prompt for permissions', () => {
+        it('should show account switcher and then abort oauth and redirect to profile', () => {
-        cy.server();
+            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({
+                cy.visit(`/oauth2/v1/ely?${new URLSearchParams(defaults)}`);
-            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.findByTestId('auth-header').should('contain', 'Choose an account');
 
-        cy.visit(
+                cy.findByTestId('auth-body').contains(account.email).click();
-            `/oauth2/v1/ely?${new URLSearchParams({
-                ...defaults,
-                client_id: 'tlauncher',
-                redirect_uri: 'http://localhost:8080',
-            })}`,
-        );
 
-        cy.wait('@complete');
+                cy.location('pathname').should('eq', '/');
-
+                cy.findByTestId('deletedAccount').should('contain', 'Account is deleted');
-        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.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.location('pathname').should('eq', '/login');
-        cy.findByTestId('deletedAccount').should('contain', 'Account is deleted');
-    });
 
-    // TODO: enable, when backend api will return correct response on auth decline
+            cy.get('[name=login]').type(`${account1.login}{enter}`);
-    xit('should redirect to error page, when permission request declined', () => {
-        cy.server();
 
-        cy.route({
+            cy.url().should('include', '/password');
-            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.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(
+            cy.get('[name=password]').type(`${account1.password}{enter}`);
-            `/oauth2/v1/ely?${new URLSearchParams({
-                ...defaults,
-                client_id: 'tlauncher',
-                redirect_uri: 'http://localhost:8080',
-            })}`,
-        );
 
-        cy.wait('@complete');
+            cy.location('pathname').should('eq', '/');
-
+            cy.findByTestId('deletedAccount').should('contain', 'Account is deleted');
-        assertPermissions();
+        });
-
-        cy.server({ enable: false });
-
-        cy.findByTestId('auth-controls-secondary').contains('Decline').click();
-
-        cy.url().should('include', 'error=access_denied');
     });
 
     describe('login_hint', () => {