Skip to content

Commit c2e8732

Browse files
zhx828zhx828
authored
Merge pull request #386 from zhx828/update-error-message
Give more informative message when user failed to login or register
2 parents 5aa98f7 + 18baf17 commit c2e8732

11 files changed

+94
-56
lines changed

‎src/main/java/org/mskcc/cbio/oncokb/security/DomainUserDetailsService.java

+6-1
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
package org.mskcc.cbio.oncokb.security;
22

3+
import org.apache.commons.lang3.StringUtils;
34
import org.mskcc.cbio.oncokb.domain.User;
45
import org.mskcc.cbio.oncokb.repository.UserRepository;
56
import org.hibernate.validator.internal.constraintvalidators.hv.EmailValidator;
@@ -50,7 +51,11 @@ public UserDetails loadUserByUsername(final String login) {
5051

5152
private org.springframework.security.core.userdetails.User createSpringSecurityUser(String lowercaseLogin, User user) {
5253
if (!user.getActivated()) {
53-
throw new UserNotActivatedException("User " + lowercaseLogin + " was not activated");
54+
if (StringUtils.isNotEmpty(user.getActivationKey())) {
55+
throw new UserNotActivatedException(lowercaseLogin);
56+
} else {
57+
throw new UserNotApprovedException(lowercaseLogin);
58+
}
5459
}
5560
List<GrantedAuthority> grantedAuthorities = user.getAuthorities().stream()
5661
.map(authority -> new SimpleGrantedAuthority(authority.getName()))

‎src/main/java/org/mskcc/cbio/oncokb/security/UserNotActivatedException.java

+2-2
Original file line numberDiff line numberDiff line change
@@ -9,8 +9,8 @@ public class UserNotActivatedException extends AuthenticationException {
99

1010
private static final long serialVersionUID = 1L;
1111

12-
public UserNotActivatedException(String message) {
13-
super(message);
12+
public UserNotActivatedException(String userEmail) {
13+
super("User " + userEmail + " is not activated");
1414
}
1515

1616
public UserNotActivatedException(String message, Throwable t) {

‎src/main/java/org/mskcc/cbio/oncokb/security/UserNotApprovedException.java

+19
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,19 @@
1+
package org.mskcc.cbio.oncokb.security;
2+
3+
import org.springframework.security.core.AuthenticationException;
4+
5+
/**
6+
* This exception is thrown in case of a not activated user trying to authenticate.
7+
*/
8+
public class UserNotApprovedException extends AuthenticationException {
9+
10+
private static final long serialVersionUID = 1L;
11+
12+
public UserNotApprovedException(String userEmail) {
13+
super("User " + userEmail + " is not approved.");
14+
}
15+
16+
public UserNotApprovedException(String message, Throwable t) {
17+
super(message, t);
18+
}
19+
}

‎src/main/java/org/mskcc/cbio/oncokb/service/EmailAlreadyUsedException.java

+1-1
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@
33
public class EmailAlreadyUsedException extends RuntimeException {
44

55
public EmailAlreadyUsedException() {
6-
super("Email is already in use!");
6+
super("Email is already in use.");
77
}
88

99
}

‎src/main/java/org/mskcc/cbio/oncokb/service/UserService.java

+1-10
Original file line numberDiff line numberDiff line change
@@ -99,17 +99,8 @@ public Optional<User> requestPasswordReset(String mail) {
9999
}
100100

101101
public User registerUser(UserDTO userDTO, String password) {
102-
userRepository.findOneByLogin(userDTO.getLogin().toLowerCase()).ifPresent(existingUser -> {
103-
boolean removed = removeNonActivatedUser(existingUser);
104-
if (!removed) {
105-
throw new UsernameAlreadyUsedException();
106-
}
107-
});
108102
userRepository.findOneByEmailIgnoreCase(userDTO.getEmail()).ifPresent(existingUser -> {
109-
boolean removed = removeNonActivatedUser(existingUser);
110-
if (!removed) {
111-
throw new EmailAlreadyUsedException();
112-
}
103+
throw new EmailAlreadyUsedException();
113104
});
114105
User newUser = new User();
115106
String encryptedPassword = passwordEncoder.encode(password);

‎src/main/java/org/mskcc/cbio/oncokb/service/UsernameAlreadyUsedException.java

+1-1
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@
33
public class UsernameAlreadyUsedException extends RuntimeException {
44

55
public UsernameAlreadyUsedException() {
6-
super("Login name already used!");
6+
super("Login name already used.");
77
}
88

99
}

‎src/main/java/org/mskcc/cbio/oncokb/web/rest/errors/EmailAlreadyUsedException.java

+1-1
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,6 @@ public class EmailAlreadyUsedException extends BadRequestAlertException {
55
private static final long serialVersionUID = 1L;
66

77
public EmailAlreadyUsedException() {
8-
super(ErrorConstants.EMAIL_ALREADY_USED_TYPE, "Email is already in use!", "userManagement", "emailexists");
8+
super(ErrorConstants.EMAIL_ALREADY_USED_TYPE, "Email is already in use.", "userManagement", "emailexists");
99
}
1010
}

‎src/main/webapp/app/components/login/LoginPage.tsx

+5-12
Original file line numberDiff line numberDiff line change
@@ -15,15 +15,16 @@ import {
1515
} from 'availity-reactstrap-validation';
1616
import { Alert, Button, Col, Row } from 'react-bootstrap';
1717
import LoadingIndicator from 'app/components/loadingIndicator/LoadingIndicator';
18-
import {LoadingButton} from "app/shared/button/LoadingButton";
18+
import { LoadingButton } from 'app/shared/button/LoadingButton';
19+
import { ErrorAlert, OncoKBError } from 'app/shared/alert/ErrorAlert';
1920

2021
export interface ILoginProps {
2122
authenticationStore: AuthenticationStore;
2223
routing: RouterStore;
2324
}
2425

2526
const LoginContent: React.FunctionComponent<{
26-
loginError: boolean;
27+
loginError?: OncoKBError;
2728
loading: boolean;
2829
handleLogin: Function;
2930
}> = props => {
@@ -32,12 +33,7 @@ const LoginContent: React.FunctionComponent<{
3233
<AvForm onSubmit={props.handleLogin}>
3334
<Row>
3435
<Col md="12">
35-
{props.loginError ? (
36-
<Alert variant="danger">
37-
<strong>Failed to log in!</strong> Please check your credentials
38-
and try again.
39-
</Alert>
40-
) : null}
36+
{props.loginError ? <ErrorAlert error={props.loginError} /> : null}
4137
</Col>
4238
<Col md="12">
4339
<AvField
@@ -66,10 +62,7 @@ const LoginContent: React.FunctionComponent<{
6662
<span>You don&apos;t have an account yet?</span>{' '}
6763
<Link to="/account/register">Register a new account</Link>
6864
</Alert>
69-
<LoadingButton
70-
variant="primary"
71-
type="submit"
72-
loading={props.loading}>
65+
<LoadingButton variant="primary" type="submit" loading={props.loading}>
7366
<span>Log in</span>
7467
</LoadingButton>
7568
</AvForm>

‎src/main/webapp/app/pages/RegisterPage.tsx

+17-14
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,8 @@ import { LicenseInquireLink } from 'app/shared/links/LicenseInquireLink';
3939
import WindowStore from 'app/store/WindowStore';
4040
import SmallPageContainer from 'app/components/SmallPageContainer';
4141
import MessageToContact from 'app/shared/texts/MessageToContact';
42-
import * as XRegExp from "xregexp";
42+
import * as XRegExp from 'xregexp';
43+
import { ErrorAlert, OncoKBError } from 'app/shared/alert/ErrorAlert';
4344

4445
export type NewUserRequiredFields = {
4546
username: string;
@@ -69,7 +70,7 @@ export const LICENSE_HASH_KEY = 'license';
6970
export class RegisterPage extends React.Component<IRegisterProps> {
7071
@observable password = '';
7172
@observable registerStatus: RegisterStatus = RegisterStatus.NA;
72-
@observable registerError: any;
73+
@observable registerError: OncoKBError;
7374
@observable selectedLicense: LicenseType | undefined;
7475

7576
private newAccount: Partial<ManagedUserVM>;
@@ -146,7 +147,7 @@ export class RegisterPage extends React.Component<IRegisterProps> {
146147
}
147148

148149
@action.bound
149-
failedToRegistered(error: any) {
150+
failedToRegistered(error: OncoKBError) {
150151
this.registerStatus = RegisterStatus.NOT_SUCCESS;
151152
this.registerError = error;
152153
window.scrollTo(0, 0);
@@ -237,11 +238,7 @@ export class RegisterPage extends React.Component<IRegisterProps> {
237238

238239
return (
239240
<div className={'registerPage'}>
240-
{this.registerStatus === RegisterStatus.NOT_SUCCESS ? (
241-
<div>
242-
<Alert variant="danger">{this.errorRegisterMessage}</Alert>
243-
</div>
244-
) : null}
241+
{this.registerError ? <ErrorAlert error={this.registerError} /> : null}
245242
<AvForm id="register-form" onValidSubmit={this.handleValidSubmit}>
246243
<Row className={getSectionClassName(true)}>
247244
<Col xs={12}>
@@ -312,7 +309,8 @@ export class RegisterPage extends React.Component<IRegisterProps> {
312309
},
313310
pattern: {
314311
value: XRegExp('^[\\p{Latin}\\s]+$'),
315-
errorMessage: 'Sorry, we only support Latin letters for now.'
312+
errorMessage:
313+
'Sorry, we only support Latin letters for now.'
316314
},
317315
minLength: {
318316
value: 1,
@@ -334,7 +332,8 @@ export class RegisterPage extends React.Component<IRegisterProps> {
334332
},
335333
pattern: {
336334
value: XRegExp('^[\\p{Latin}\\s]+$'),
337-
errorMessage: 'Sorry, we only support Latin letters for now.'
335+
errorMessage:
336+
'Sorry, we only support Latin letters for now.'
338337
},
339338
minLength: {
340339
value: 1,
@@ -422,7 +421,8 @@ export class RegisterPage extends React.Component<IRegisterProps> {
422421
},
423422
pattern: {
424423
value: XRegExp('^[\\p{Latin}\\p{Common}\\s]+$'),
425-
errorMessage: 'Sorry, we only support Latin letters for now.'
424+
errorMessage:
425+
'Sorry, we only support Latin letters for now.'
426426
},
427427
maxLength: {
428428
value: 50,
@@ -445,7 +445,8 @@ export class RegisterPage extends React.Component<IRegisterProps> {
445445
},
446446
pattern: {
447447
value: XRegExp('^[\\p{Latin}\\p{Common}\\s]+$'),
448-
errorMessage: 'Sorry, we only support Latin letters for now.'
448+
errorMessage:
449+
'Sorry, we only support Latin letters for now.'
449450
},
450451
maxLength: {
451452
value: 50,
@@ -468,7 +469,8 @@ export class RegisterPage extends React.Component<IRegisterProps> {
468469
},
469470
pattern: {
470471
value: XRegExp('^[\\p{Latin}\\p{Common}\\s]+$'),
471-
errorMessage: 'Sorry, we only support Latin letters for now.'
472+
errorMessage:
473+
'Sorry, we only support Latin letters for now.'
472474
},
473475
maxLength: {
474476
value: 50,
@@ -487,7 +489,8 @@ export class RegisterPage extends React.Component<IRegisterProps> {
487489
required: { value: true, errorMessage: 'Required.' },
488490
pattern: {
489491
value: XRegExp('^[\\p{Latin}\\p{Common}\\s]+$'),
490-
errorMessage: 'Sorry, we only support Latin letters for now.'
492+
errorMessage:
493+
'Sorry, we only support Latin letters for now.'
491494
},
492495
minLength: {
493496
value: 1,

‎src/main/webapp/app/shared/alert/ErrorAlert.tsx

+36
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,36 @@
1+
import { Alert } from 'react-bootstrap';
2+
import React from 'react';
3+
4+
type OncoKBErrorResponseBody = {
5+
type: string;
6+
title: string;
7+
status: number;
8+
detail: string;
9+
path: string;
10+
message: string;
11+
};
12+
type OncoKBResponse = Response & {
13+
body: OncoKBErrorResponseBody;
14+
};
15+
export type OncoKBError = Error & {
16+
response: OncoKBResponse;
17+
};
18+
19+
export const ErrorAlert: React.FunctionComponent<{
20+
error: OncoKBError;
21+
}> = props => {
22+
const error = props.error;
23+
let errorMessage = error.message;
24+
if (error.response && error.response.body) {
25+
if (error.response.body.detail) {
26+
errorMessage = error.response.body.detail;
27+
} else if (error.response.body.title) {
28+
errorMessage = error.response.body.title;
29+
}
30+
}
31+
return (
32+
<Alert variant="danger">
33+
<strong>{errorMessage}</strong>
34+
</Alert>
35+
);
36+
};

‎src/main/webapp/app/store/AuthenticationStore.ts

+5-14
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ import {
1818
getStoredToken
1919
} from 'app/indexUtils';
2020
import { notifyError, notifySuccess } from 'app/shared/utils/NotificationUtils';
21+
import { OncoKBError } from 'app/shared/alert/ErrorAlert';
2122

2223
export const ACTION_TYPES = {
2324
LOGIN: 'authentication/LOGIN',
@@ -33,8 +34,7 @@ export const AUTH_WEBSITE_TOKEN_KEY = 'oncokb-website-token';
3334
class AuthenticationStore {
3435
@observable loading = false;
3536
@observable loginSuccess = false;
36-
@observable loginError = false; // Errors returned from server side
37-
@observable loginErrorMessage = ''; // Errors returned from server side
37+
@observable loginError: OncoKBError | undefined; // Errors returned from server side
3838
@observable showModalLogin = false;
3939
@observable errorMessage = ''; // Errors returned from server side
4040
@observable redirectMessage = '';
@@ -184,9 +184,8 @@ class AuthenticationStore {
184184
// we should fetch the account info when the user is successfully logged in.
185185
this.getAccount().finally(() => {
186186
this.loginSuccess = true;
187-
this.loginError = false;
188-
this.loginErrorMessage = '';
189187
this.loading = false;
188+
this.loginError = undefined;
190189
});
191190
}
192191

@@ -197,10 +196,9 @@ class AuthenticationStore {
197196
}
198197

199198
@action.bound
200-
loginErrorCallback(error: Error) {
199+
loginErrorCallback(error: OncoKBError) {
201200
this.loginSuccess = false;
202-
this.loginError = true;
203-
this.loginErrorMessage = error.message;
201+
this.loginError = error;
204202
this.loading = false;
205203
}
206204

@@ -224,13 +222,6 @@ class AuthenticationStore {
224222
this.idToken = getPublicWebsiteToken();
225223
}
226224

227-
@action
228-
initialLoginStatus() {
229-
this.loginSuccess = false;
230-
this.loginError = false;
231-
this.loginErrorMessage = '';
232-
}
233-
234225
destroy() {
235226
for (const item of this.reactions) {
236227
item();

0 commit comments

Comments
 (0)