Skip to content

Commit 4e1f024

Browse files
DaanHooglandDaanHoogland
committed
servlet: fix login NPE for users created on first login
1 parent cf64acb commit 4e1f024

5 files changed

Lines changed: 167 additions & 2 deletions

File tree

‎server/src/main/java/com/cloud/api/ApiServlet.java‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -313,6 +313,11 @@ void processRequestInContext(final HttpServletRequest req, final HttpServletResp
313313
if (!DefaultForgotPasswordAPIAuthenticatorCmd.APINAME.equalsIgnoreCase(command) || StringUtils.isNotBlank(username)) {
314314
LOGGER.debug("Authentication failure: {}", e.getMessage());
315315
}
316+
} catch (RuntimeException e) {
317+
if (apiAuthenticator.getAPIType() == APIAuthenticationType.LOGIN_API) {
318+
invalidateHttpSession(session, "invalidating session after unexpected login failure");
319+
}
320+
throw e;
316321
}
317322

318323
if (apiAuthenticator.getAPIType() == APIAuthenticationType.LOGOUT_API) {
@@ -426,6 +431,11 @@ void processRequestInContext(final HttpServletRequest req, final HttpServletResp
426431
} catch (final Exception ex) {
427432
LOGGER.error("unknown exception writing api response", ex);
428433
auditTrailSb.append(" unknown exception writing api response");
434+
if (!resp.isCommitted()) {
435+
final String serializedResponseText = apiServer.getSerializedApiError(HttpServletResponse.SC_INTERNAL_SERVER_ERROR,
436+
"Internal error processing the request", params, responseType);
437+
HttpUtils.writeHttpResponse(resp, serializedResponseText, HttpServletResponse.SC_INTERNAL_SERVER_ERROR, responseType, ApiServer.JSONContentType.value());
438+
}
429439
} finally {
430440
ACCESSLOGGER.info(auditTrailSb.toString());
431441
if (LOGGER.isDebugEnabled()) {

‎server/src/main/java/com/cloud/api/auth/DefaultLoginAPIAuthenticatorCmd.java‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -155,7 +155,10 @@ public String authenticate(String command, Map<String, Object[]> params, HttpSes
155155
}
156156
serializedResponse = ApiResponseSerializer.toSerializedString(_apiServer.loginUser(session, username[0], pwd, domainId, domain, remoteAddress, params),
157157
responseType);
158-
userDetailsDao.removeDetail(userAccount.getId(), UserDetailVO.OauthLogin);
158+
final Long userId = (Long)session.getAttribute("userid");
159+
if (userId != null) {
160+
userDetailsDao.removeDetail(userId, UserDetailVO.OauthLogin);
161+
}
159162
return serializedResponse;
160163
} catch (final CloudAuthenticationException ex) {
161164
ApiServlet.invalidateHttpSession(session, "fall through to API key,");

‎server/src/test/java/com/cloud/api/ApiServletTest.java‎

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -318,6 +318,31 @@ public void processRequestInContextLogin() throws UnknownHostException {
318318
Mockito.eq(InetAddress.getByName("127.0.0.1")), Mockito.anyString(), Mockito.isA(StringBuilder.class), Mockito.isA(HttpServletRequest.class), Mockito.isA(HttpServletResponse.class));
319319
}
320320

321+
@SuppressWarnings("unchecked")
322+
@Test
323+
public void processRequestInContextLoginUnexpectedExceptionInvalidatesSessionAndReturnsError() throws UnknownHostException {
324+
Mockito.when(request.getMethod()).thenReturn("POST");
325+
Mockito.when(request.getSession(false)).thenReturn(null);
326+
Mockito.when(request.getSession(true)).thenReturn(session);
327+
HashMap<String, String[]> params = new HashMap<String, String[]>();
328+
params.put(ApiConstants.COMMAND, new String[] { "login" });
329+
params.put(ApiConstants.USERNAME, new String[] { "TEST" });
330+
params.put(ApiConstants.PASSWORD, new String[] { "TEST-PWD" });
331+
Mockito.when(request.getParameterMap()).thenReturn(params);
332+
Mockito.when(authenticator.getAPIType()).thenReturn(APIAuthenticationType.LOGIN_API);
333+
Mockito.when(authenticator.authenticate(Mockito.anyString(), Mockito.anyMap(), Mockito.isA(HttpSession.class),
334+
Mockito.eq(InetAddress.getByName("127.0.0.1")), Mockito.anyString(), Mockito.isA(StringBuilder.class), Mockito.isA(HttpServletRequest.class), Mockito.isA(HttpServletResponse.class)))
335+
.thenThrow(new NullPointerException("test"));
336+
Mockito.when(apiServer.getSerializedApiError(Mockito.eq(HttpServletResponse.SC_INTERNAL_SERVER_ERROR), Mockito.anyString(), Mockito.anyMap(), Mockito.anyString()))
337+
.thenReturn("{\"errorresponse\":{}}");
338+
339+
servlet.processRequestInContext(request, response);
340+
341+
Mockito.verify(session).invalidate();
342+
Mockito.verify(response).setStatus(HttpServletResponse.SC_INTERNAL_SERVER_ERROR);
343+
Assert.assertEquals("{\"errorresponse\":{}}", responseWriter.toString());
344+
}
345+
321346
@Test
322347
public void getClientAddressWithXForwardedFor() throws UnknownHostException {
323348
setConfigValue("proxy.header.verify", "true");

‎server/src/test/java/com/cloud/api/auth/DefaultLoginAPIAuthenticatorCmdTest.java‎

Lines changed: 128 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,128 @@
1+
// Licensed to the Apache Software Foundation (ASF) under one
2+
// or more contributor license agreements. See the NOTICE file
3+
// distributed with this work for additional information
4+
// regarding copyright ownership. The ASF licenses this file
5+
// to you under the Apache License, Version 2.0 (the
6+
// "License"); you may not use this file except in compliance
7+
// with the License. You may obtain a copy of the License at
8+
//
9+
// http://www.apache.org/licenses/LICENSE-2.0
10+
//
11+
// Unless required by applicable law or agreed to in writing,
12+
// software distributed under the License is distributed on an
13+
// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
14+
// KIND, either express or implied. See the License for the
15+
// specific language governing permissions and limitations
16+
// under the License.
17+
package com.cloud.api.auth;
18+
19+
import java.net.InetAddress;
20+
import java.util.HashMap;
21+
import java.util.Map;
22+
23+
import javax.servlet.http.HttpServletRequest;
24+
import javax.servlet.http.HttpServletResponse;
25+
import javax.servlet.http.HttpSession;
26+
27+
import org.apache.cloudstack.api.ApiConstants;
28+
import org.apache.cloudstack.api.ApiServerService;
29+
import org.apache.cloudstack.api.ResponseObject;
30+
import org.apache.cloudstack.resourcedetail.UserDetailVO;
31+
import org.apache.cloudstack.resourcedetail.dao.UserDetailsDao;
32+
import org.junit.After;
33+
import org.junit.Assert;
34+
import org.junit.Before;
35+
import org.junit.Test;
36+
import org.junit.runner.RunWith;
37+
import org.mockito.Mock;
38+
import org.mockito.MockedStatic;
39+
import org.mockito.Mockito;
40+
import org.mockito.junit.MockitoJUnitRunner;
41+
42+
import com.cloud.api.response.ApiResponseSerializer;
43+
import com.cloud.domain.Domain;
44+
import com.cloud.user.AccountService;
45+
import com.cloud.user.DomainService;
46+
import com.cloud.user.UserAccount;
47+
48+
@RunWith(MockitoJUnitRunner.class)
49+
public class DefaultLoginAPIAuthenticatorCmdTest {
50+
51+
private static final String USERNAME = "user@example.com";
52+
private static final long DOMAIN_ID = 1L;
53+
private static final long USER_ID = 42L;
54+
private static final String SERIALIZED_RESPONSE = "{\"loginresponse\":{}}";
55+
56+
@Mock
57+
ApiServerService apiServer;
58+
@Mock
59+
UserDetailsDao userDetailsDao;
60+
@Mock
61+
AccountService accountService;
62+
@Mock
63+
DomainService domainService;
64+
@Mock
65+
HttpSession session;
66+
@Mock
67+
HttpServletRequest req;
68+
@Mock
69+
HttpServletResponse resp;
70+
@Mock
71+
Domain domain;
72+
@Mock
73+
ResponseObject loginResponse;
74+
75+
DefaultLoginAPIAuthenticatorCmd cmd = new DefaultLoginAPIAuthenticatorCmd();
76+
77+
private MockedStatic<ApiResponseSerializer> serializerMock;
78+
private Map<String, Object[]> params;
79+
80+
@Before
81+
public void setUp() throws Exception {
82+
cmd._apiServer = apiServer;
83+
cmd._accountService = accountService;
84+
cmd._domainService = domainService;
85+
cmd.userDetailsDao = userDetailsDao;
86+
87+
params = new HashMap<>();
88+
params.put(ApiConstants.USERNAME, new String[] {USERNAME});
89+
params.put(ApiConstants.PASSWORD, new String[] {"password"});
90+
params.put(ApiConstants.DOMAIN, new String[] {"/"});
91+
92+
Mockito.when(req.getMethod()).thenReturn("POST");
93+
Mockito.when(domain.getId()).thenReturn(DOMAIN_ID);
94+
Mockito.when(domainService.findDomainByIdOrPath(Mockito.any(), Mockito.anyString())).thenReturn(domain);
95+
Mockito.when(apiServer.loginUser(Mockito.eq(session), Mockito.eq(USERNAME), Mockito.anyString(), Mockito.eq(DOMAIN_ID),
96+
Mockito.anyString(), Mockito.any(), Mockito.any())).thenReturn(loginResponse);
97+
Mockito.when(session.getAttribute("userid")).thenReturn(USER_ID);
98+
99+
serializerMock = Mockito.mockStatic(ApiResponseSerializer.class);
100+
serializerMock.when(() -> ApiResponseSerializer.toSerializedString(loginResponse, "json")).thenReturn(SERIALIZED_RESPONSE);
101+
}
102+
103+
@After
104+
public void tearDown() {
105+
serializerMock.close();
106+
}
107+
108+
private String authenticate() throws Exception {
109+
return cmd.authenticate("login", params, session, InetAddress.getLoopbackAddress(), "json", new StringBuilder(), req, resp);
110+
}
111+
112+
@Test
113+
public void authenticateUserCreatedDuringLoginReturnsLoginResponse() throws Exception {
114+
Mockito.when(accountService.getActiveUserAccount(USERNAME, DOMAIN_ID)).thenReturn(null);
115+
116+
Assert.assertEquals(SERIALIZED_RESPONSE, authenticate());
117+
Mockito.verify(userDetailsDao).removeDetail(USER_ID, UserDetailVO.OauthLogin);
118+
}
119+
120+
@Test
121+
public void authenticateExistingUserRemovesOauthLoginDetail() throws Exception {
122+
UserAccount userAccount = Mockito.mock(UserAccount.class);
123+
Mockito.when(accountService.getActiveUserAccount(USERNAME, DOMAIN_ID)).thenReturn(userAccount);
124+
125+
Assert.assertEquals(SERIALIZED_RESPONSE, authenticate());
126+
Mockito.verify(userDetailsDao).removeDetail(USER_ID, UserDetailVO.OauthLogin);
127+
}
128+
}

‎ui/src/main.js‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -68,7 +68,6 @@ vueApp.use(backupUtilPlugin)
6868
vueApp.use(extensions)
6969
vueApp.use(directives)
7070

71-
// drop stored values that are not valid JSON (e.g. 'undefined'), as reading them throws
7271
vueProps.$localStorage.keys().forEach(key => {
7372
try {
7473
vueProps.$localStorage.get(key)

0 commit comments

Comments
 (0)