[MM-14081] Remove checkMFA invocations and replace with error-based M…

…FA handling (#2406) * Remove checkMFA invocations and replace with error-based MFA handling * Delete checkMfa method; Style changes * Fix unit tests
mattermost · Mar 1, 2019 · 4f2179c · 4f2179c
1 parent 4dce36d
commit 4f2179c
Show file tree

Hide file tree

Showing 11 changed files with 42 additions and 145 deletions.
diff --git a/actions/views/mfa.js b/actions/views/mfa.js
@@ -2,7 +2,6 @@
 // See LICENSE.txt for license information.
 
 import * as UserActions from 'mattermost-redux/actions/users';
-import {getConfig} from 'mattermost-redux/selectors/entities/general';
 import {getCurrentUserId} from 'mattermost-redux/selectors/entities/users';
 
 export function activateMfa(code) {
@@ -29,14 +28,3 @@ export function generateMfaSecret() {
  };
 }
 
-export function checkMfa(loginId) {
- return (dispatch, getState) => {
- const config = getConfig(getState());
-
- if (config.EnableMultifactorAuthentication !== 'true') {
- return Promise.resolve({data: false});
- }
-
- return dispatch(UserActions.checkMfa(loginId));
- };
-}
diff --git a/actions/views/mfa.test.js b/actions/views/mfa.test.js
@@ -7,7 +7,6 @@ import * as UserActions from 'mattermost-redux/actions/users';
 
 import {
  activateMfa,
- checkMfa,
  deactivateMfa,
  generateMfaSecret,
 } from 'actions/views/mfa';
@@ -71,24 +70,4 @@ describe('actions/views/mfa', () => {
  expect(UserActions.generateMfaSecret).toHaveBeenCalledWith(currentUserId);
  });
  });
-
- describe('checkMfa', () => {
- it('should skip actually checking for MFA if MFA is disabled', async () => {
- const store = configureStore({
- entities: {
- general: {
- config: {
- EnableMultifactorAuthentication: 'false',
- },
- },
- },
- });
-
- const loginId = 'user';
- const result = await store.dispatch(checkMfa(loginId));
-
- expect(result).toEqual({data: false});
- expect(UserActions.checkMfa).not.toHaveBeenCalled();
- });
- });
 });
diff --git a/components/claim/claim_controller.jsx b/components/claim/claim_controller.jsx
@@ -27,7 +27,6 @@ export default class ClaimController extends React.PureComponent {
  }).isRequired,
 
  actions: PropTypes.shape({
- checkMfa: PropTypes.func.isRequired,
  switchLdapToEmail: PropTypes.func.isRequired,
  }).isRequired,
  };
@@ -66,7 +65,6 @@ export default class ClaimController extends React.PureComponent {
  newType={newType}
  email={email}
  siteName={this.props.siteName}
- checkMfa={this.props.actions.checkMfa}
  />
  )}
  />
@@ -76,7 +74,6 @@ export default class ClaimController extends React.PureComponent {
  <LDAPToEmail
  email={email}
  passwordConfig={this.props.passwordConfig}
- checkMfa={this.props.actions.checkMfa}
  switchLdapToEmail={this.props.actions.switchLdapToEmail}
  />
  )}
@@ -88,7 +85,6 @@ export default class ClaimController extends React.PureComponent {
  email={email}
  siteName={this.props.siteName}
  ldapLoginFieldName={this.props.ldapLoginFieldName}
- checkMfa={this.props.actions.checkMfa}
  />
  )}
  />

diff --git a/components/claim/components/email_to_ldap.jsx b/components/claim/components/email_to_ldap.jsx
@@ -15,7 +15,6 @@ export default class EmailToLDAP extends React.Component {
  email: PropTypes.string,
  siteName: PropTypes.string,
  ldapLoginFieldName: PropTypes.string,
- checkMfa: PropTypes.func.isRequired,
  };
 
  constructor(props) {
@@ -69,21 +68,7 @@ export default class EmailToLDAP extends React.Component {
  state.ldapPassword = ldapPassword;
  this.setState(state);
 
- this.props.checkMfa(this.props.email).then((result) => {
- if (result.error) {
- this.setState({
- error: result.error.message,
- });
- return;
- }
-
- const requiresMfa = result.data;
- if (requiresMfa) {
- this.setState({showMfa: true});
- } else {
- this.submit(this.props.email, password, '', ldapId, ldapPassword);
- }
- });
+ this.submit(this.props.email, password, '', ldapId, ldapPassword);
  }
 
  submit(loginId, password, token, ldapId, ldapPassword) {
@@ -99,20 +84,24 @@ export default class EmailToLDAP extends React.Component {
  }
  },
  (err) => {
- switch (err.id) {
- case 'ent.ldap.do_login.user_not_registered.app_error':
- case 'ent.ldap.do_login.user_filtered.app_error':
- case 'ent.ldap.do_login.matched_to_many_users.app_error':
- this.setState({ldapError: err.message, showMfa: false});
- break;
- case 'ent.ldap.do_login.invalid_password.app_error':
- this.setState({ldapPasswordError: err.message, showMfa: false});
- break;
- case 'api.user.check_user_password.invalid.app_error':
- this.setState({passwordError: err.message, showMfa: false});
- break;
- default:
- this.setState({serverError: err.message, showMfa: false});
+ if (!this.state.showMfa && err.server_error_id === 'mfa.validate_token.authenticate.app_error') {
+ this.setState({showMfa: true});
+ } else {
+ switch (err.id) {
+ case 'ent.ldap.do_login.user_not_registered.app_error':
+ case 'ent.ldap.do_login.user_filtered.app_error':
+ case 'ent.ldap.do_login.matched_to_many_users.app_error':
+ this.setState({ldapError: err.message, showMfa: false});
+ break;
+ case 'ent.ldap.do_login.invalid_password.app_error':
+ this.setState({ldapPasswordError: err.message, showMfa: false});
+ break;
+ case 'api.user.check_user_password.invalid.app_error':
+ this.setState({passwordError: err.message, showMfa: false});
+ break;
+ default:
+ this.setState({serverError: err.message, showMfa: false});
+ }
  }
  }
  );

diff --git a/components/claim/components/email_to_oauth.jsx b/components/claim/components/email_to_oauth.jsx
@@ -17,7 +17,6 @@ export default class EmailToOAuth extends React.PureComponent {
  newType: PropTypes.string,
  email: PropTypes.string,
  siteName: PropTypes.string,
- checkMfa: PropTypes.func.isRequired,
  };
 
  constructor(props) {
@@ -45,21 +44,7 @@ export default class EmailToOAuth extends React.PureComponent {
  state.error = null;
  this.setState(state);
 
- this.props.checkMfa(this.props.email).then((result) => {
- if (result.error) {
- this.setState({
- error: result.error.message,
- });
- return;
- }
-
- const requiresMfa = result.data;
- if (requiresMfa) {
- this.setState({showMfa: true});
- } else {
- this.submit(this.props.email, password, '');
- }
- });
+ this.submit(this.props.email, password, '');
  }
 
  submit(loginId, password, token) {
@@ -74,7 +59,11 @@ export default class EmailToOAuth extends React.PureComponent {
  }
  },
  (err) => {
- this.setState({error: err.message, showMfa: false});
+ if (!this.state.showMfa && err.server_error_id === 'mfa.validate_token.authenticate.app_error') {
+ this.setState({showMfa: true});
+ } else {
+ this.setState({error: err.message, showMfa: false});
+ }
  }
  );
  }

diff --git a/components/claim/components/ldap_to_email.jsx b/components/claim/components/ldap_to_email.jsx
@@ -13,7 +13,6 @@ export default class LDAPToEmail extends React.Component {
  static propTypes = {
  email: PropTypes.string,
  passwordConfig: PropTypes.object,
- checkMfa: PropTypes.func.isRequired,
  switchLdapToEmail: PropTypes.func.isRequired,
  };
 
@@ -74,49 +73,25 @@ export default class LDAPToEmail extends React.Component {
  state.ldapPassword = ldapPassword;
  this.setState(state);
 
- this.props.checkMfa(this.props.email).then((result) => {
- if (result.error) {
- this.setState({
- error: result.error.message,
- });
- return;
- }
-
- const requiresMfa = result.data;
- if (requiresMfa) {
- this.setState({
- showMfa: true,
- });
- } else {
- this.submit(this.props.email, password, '', ldapPassword);
- }
- });
+ this.submit(this.props.email, password, '', ldapPassword);
  }
 
  submit(loginId, password, token, ldapPassword) {
- this.props.switchLdapToEmail(
- ldapPassword || this.state.ldapPassword,
- this.props.email,
- password,
- token).
- then(({data, error: err}) => {
- if (data && data.follow_link) {
- window.location.href = data.follow_link;
- } else if (err) {
- if (err.server_error_id.startsWith('model.user.is_valid.pwd')) {
- this.setState({passwordError: err.message, showMfa: false});
- } else {
- switch (err.server_error_id) {
- case 'ent.ldap.do_login.invalid_password.app_error':
- this.setState({ldapPasswordError: err.message, showMfa: false});
- break;
- default:
- this.setState({serverError: err.message, showMfa: false});
- }
- }
+ this.props.switchLdapToEmail(ldapPassword || this.state.ldapPassword, this.props.email, password, token).then(({data, error: err}) => {
+ if (data && data.follow_link) {
+ window.location.href = data.follow_link;
+ } else if (err) {
+ if (err.server_error_id.startsWith('model.user.is_valid.pwd')) {
+ this.setState({passwordError: err.message, showMfa: false});
+ } else if (err.server_error_id === 'ent.ldap.do_login.invalid_password.app_error') {
+ this.setState({ldapPasswordError: err.message, showMfa: false});
+ } else if (!this.state.showMfa && err.server_error_id === 'mfa.validate_token.authenticate.app_error') {
+ this.setState({showMfa: true});
+ } else {
+ this.setState({serverError: err.message, showMfa: false});
  }
  }
-  );
+ });
  }
 
  render() {

diff --git a/components/claim/components/ldap_to_email.test.js b/components/claim/components/ldap_to_email.test.js
@@ -10,7 +10,6 @@ describe('components/claim/components/ldap_to_email.jsx', () => {
  const requiredProps = {
  email: '',
  passwordConfig: {},
- checkMfa: jest.fn(),
  switchLdapToEmail: jest.fn(() => Promise.resolve({data: true})),
  };
 

diff --git a/components/claim/index.js b/components/claim/index.js
@@ -7,7 +7,6 @@ import {bindActionCreators} from 'redux';
 import {switchLdapToEmail} from 'mattermost-redux/actions/users';
 import {getConfig} from 'mattermost-redux/selectors/entities/general';
 
-import {checkMfa} from 'actions/views/mfa';
 import {getPasswordConfig} from 'utils/utils.jsx';
 
 import ClaimController from './claim_controller.jsx';
@@ -27,7 +26,6 @@ function mapStateToProps(state) {
 function mapDispatchToProps(dispatch) {
  return {
  actions: bindActionCreators({
- checkMfa,
  switchLdapToEmail,
  }, dispatch),
  };

diff --git a/components/login/login_controller/index.js b/components/login/login_controller/index.js
@@ -12,7 +12,6 @@ import {RequestStatus} from 'mattermost-redux/constants';
 import {addUserToTeamFromInvite} from 'actions/team_actions';
 
 import {login} from 'actions/views/login';
-import {checkMfa} from 'actions/views/mfa';
 
 import LoginController from './login_controller.jsx';
 
@@ -79,7 +78,6 @@ function mapStateToProps(state) {
 function mapDispatchToProps(dispatch) {
  return {
  actions: bindActionCreators({
- checkMfa,
  login,
  addUserToTeamFromInvite,
  }, dispatch),

diff --git a/components/login/login_controller/login_controller.jsx b/components/login/login_controller/login_controller.jsx
@@ -59,7 +59,6 @@ class LoginController extends React.Component {
  siteName: PropTypes.string,
  initializing: PropTypes.bool,
  actions: PropTypes.shape({
- checkMfa: PropTypes.func.isRequired,
  login: PropTypes.func.isRequired,
  addUserToTeamFromInvite: PropTypes.func.isRequired,
  }).isRequired,
@@ -256,21 +255,7 @@ class LoginController extends React.Component {
  return;
  }
 
- this.props.actions.checkMfa(loginId).then((result) => {
- if (result.error) {
- this.setState({
- serverError: result.error.message,
- });
- return;
- }
-
- const requiresMfa = result.data;
- if (requiresMfa) {
- this.setState({showMfa: true});
- } else {
- this.submit(loginId, password, '');
- }
- });
+ this.submit(loginId, password, '');
  }
 
  submit = (loginId, password, token) => {
@@ -303,6 +288,8 @@ class LoginController extends React.Component {
  />
  ),
  });
+ } else if (!this.state.showMfa && error.server_error_id === 'mfa.validate_token.authenticate.app_error') {
+ this.setState({showMfa: true});
  } else {
  this.setState({showMfa: false, serverError: error.message, loading: false});
  }

diff --git a/components/login/login_controller/login_controller.test.jsx b/components/login/login_controller/login_controller.test.jsx
@@ -32,7 +32,6 @@ describe('components/login/LoginController', () => {
  samlLoginButtonText: '',
  siteName: '',
  actions: {
- checkMfa: jest.fn(),
  login: jest.fn(),
  addUserToTeamFromInvite: jest.fn(),
  },