Skip to content

npe guard on missing session attribute #13871

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
DaanHoogland wants to merge 2 commits into
base: 4.20
Choose a base branch
from
+75 −57
Open

npe guard on missing session attribute #13871

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e3074a0
npe guard on missing session attribute
Aug 13, 2026
0ab78ce
deal with sonarcube/-cloud warnings
Aug 14, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion server/src/main/java/com/cloud/api/ApiServlet.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -422,7 +422,7 @@ protected boolean skip2FAcheckForAPIs(String command) {
protected boolean skip2FAcheckForUser(HttpSession session) {
boolean skip2FAcheck = false;
Long userId = (Long) session.getAttribute("userid");
boolean is2FAverified = (boolean) session.getAttribute(ApiConstants.IS_2FA_VERIFIED);
boolean is2FAverified = Boolean.TRUE.equals(session.getAttribute(ApiConstants.IS_2FA_VERIFIED));
if (is2FAverified) {
LOGGER.debug(String.format("Two factor authentication is already verified for the user %d, so skipping", userId));
skip2FAcheck = true;
Expand Down
130 changes: 74 additions & 56 deletions server/src/test/java/com/cloud/api/ApiServletTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@
import java.util.Map;

import static org.mockito.ArgumentMatchers.nullable;
import static org.mockito.Mockito.when;

@RunWith(MockitoJUnitRunner.class)
public class ApiServletTest {
Expand Down Expand Up @@ -109,17 +110,17 @@
servlet = new ApiServlet();
spyServlet = Mockito.spy(servlet);
responseWriter = new StringWriter();
Mockito.when(response.getWriter()).thenReturn(
when(response.getWriter()).thenReturn(
new PrintWriter(responseWriter));
Mockito.when(request.getRemoteAddr()).thenReturn("127.0.0.1");
Mockito.when(accountService.getSystemUser()).thenReturn(user);
Mockito.when(accountService.getSystemAccount()).thenReturn(account);
when(request.getRemoteAddr()).thenReturn("127.0.0.1");
when(accountService.getSystemUser()).thenReturn(user);
when(accountService.getSystemAccount()).thenReturn(account);

Field accountMgrField = ApiServlet.class.getDeclaredField("accountMgr");
accountMgrField.setAccessible(true);
accountMgrField.set(servlet, accountService);

Mockito.when(authManager.getAPIAuthenticator(Mockito.anyString())).thenReturn(authenticator);
when(authManager.getAPIAuthenticator(Mockito.anyString())).thenReturn(authenticator);
Mockito.lenient().when(authenticator.authenticate(Mockito.anyString(), Mockito.anyMap(), Mockito.isA(HttpSession.class),
Mockito.same(InetAddress.getByName("127.0.0.1")), Mockito.anyString(), Mockito.isA(StringBuilder.class), Mockito.isA(HttpServletRequest.class), Mockito.isA(HttpServletResponse.class))).thenReturn("{\"loginresponse\":{}");

Expand Down Expand Up @@ -155,7 +156,7 @@

@Test
public void utf8Fixup() {
Mockito.when(request.getQueryString()).thenReturn(
when(request.getQueryString()).thenReturn(
"foo=12345&bar=blah&baz=&param=param");
HashMap<String, Object[]> params = new HashMap<String, Object[]>();
servlet.utf8Fixup(request, params);
Expand All @@ -165,21 +166,21 @@

@Test
public void utf8FixupNull() {
Mockito.when(request.getQueryString()).thenReturn("&&=a&=&&a&a=a=a=a");
when(request.getQueryString()).thenReturn("&&=a&=&&a&a=a=a=a");
servlet.utf8Fixup(request, new HashMap<String, Object[]>());
}

@Test
public void utf8FixupStrangeInputs() {
Mockito.when(request.getQueryString()).thenReturn("&&=a&=&&a&a=a=a=a");
when(request.getQueryString()).thenReturn("&&=a&=&&a&a=a=a=a");
HashMap<String, Object[]> params = new HashMap<String, Object[]>();
servlet.utf8Fixup(request, params);
Assert.assertTrue(params.containsKey(""));
}

@Test
public void utf8FixupUtf() throws UnsupportedEncodingException {
Mockito.when(request.getQueryString()).thenReturn(
when(request.getQueryString()).thenReturn(
URLEncoder.encode("防水镜钻孔机", "UTF-8") + "="
+ URLEncoder.encode("árvíztűrőtükörfúró", "UTF-8"));
HashMap<String, Object[]> params = new HashMap<String, Object[]>();
Expand All @@ -190,7 +191,7 @@
@SuppressWarnings("unchecked")
@Test
public void processRequestInContextUnauthorizedGET() {
Mockito.when(request.getMethod()).thenReturn("GET");
when(request.getMethod()).thenReturn("GET");
Mockito.lenient().when(
apiServer.verifyRequest(Mockito.anyMap(), Mockito.anyLong(), Mockito.any(InetAddress.class)))
.thenReturn(false);
Expand All @@ -204,8 +205,8 @@
@SuppressWarnings("unchecked")
@Test
public void processRequestInContextAuthorizedGet() {
Mockito.when(request.getMethod()).thenReturn("GET");
Mockito.when(
when(request.getMethod()).thenReturn("GET");
when(
apiServer.verifyRequest(nullable(Map.class), nullable(Long.class), nullable(InetAddress.class)))
.thenReturn(true);
servlet.processRequestInContext(request, response);
Expand All @@ -218,16 +219,16 @@
@SuppressWarnings("unchecked")
@Test
public void processRequestInContextLogout() throws UnknownHostException {
Mockito.when(request.getMethod()).thenReturn("GET");
Mockito.when(request.getSession(Mockito.anyBoolean())).thenReturn(
when(request.getMethod()).thenReturn("GET");
when(request.getSession(Mockito.anyBoolean())).thenReturn(
session);
Mockito.when(session.getAttribute("userid")).thenReturn(1l);
Mockito.when(session.getAttribute("accountobj")).thenReturn(account);
when(session.getAttribute("userid")).thenReturn(1l);
when(session.getAttribute("accountobj")).thenReturn(account);
HashMap<String, String[]> params = new HashMap<String, String[]>();
params.put(ApiConstants.COMMAND, new String[] { "logout" });
Mockito.when(request.getParameterMap()).thenReturn(params);
when(request.getParameterMap()).thenReturn(params);

Mockito.when(authenticator.getAPIType()).thenReturn(APIAuthenticationType.LOGOUT_API);
when(authenticator.getAPIType()).thenReturn(APIAuthenticationType.LOGOUT_API);

servlet.processRequestInContext(request, response);

Expand All @@ -241,16 +242,16 @@
@SuppressWarnings("unchecked")
@Test
public void processRequestInContextLogin() throws UnknownHostException {
Mockito.when(request.getMethod()).thenReturn("GET");
Mockito.when(request.getSession(Mockito.anyBoolean())).thenReturn(
when(request.getMethod()).thenReturn("GET");
when(request.getSession(Mockito.anyBoolean())).thenReturn(
session);
HashMap<String, String[]> params = new HashMap<String, String[]>();
params.put(ApiConstants.COMMAND, new String[] { "login" });
params.put(ApiConstants.USERNAME, new String[] { "TEST" });
params.put(ApiConstants.PASSWORD, new String[] { "TEST-PWD" });
params.put(ApiConstants.DOMAIN_ID, new String[] { "42" });
params.put(ApiConstants.DOMAIN, new String[] { "TEST-DOMAIN" });
Mockito.when(request.getParameterMap()).thenReturn(params);
when(request.getParameterMap()).thenReturn(params);

servlet.processRequestInContext(request, response);

Expand All @@ -260,43 +261,43 @@
}

@Test
public void getClientAddressWithXForwardedFor() throws UnknownHostException {

Check warning on line 264 in server/src/test/java/com/cloud/api/ApiServletTest.java

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Replace these 3 tests with a single Parameterized one. 


See more on https://sonarcloud.io/project/issues?id=apache_cloudstack&issues=AZ__u_NPDoP9WiI4fUg1&open=AZ__u_NPDoP9WiI4fUg1&pullRequest=13871
String[] proxynet = {"127.0.0.0/8"};
Mockito.when(spyServlet.proxyNets()).thenReturn(proxynet);
Mockito.when(spyServlet.doUseForwardHeaders()).thenReturn(true);
Mockito.when(request.getHeader(Mockito.eq("X-Forwarded-For"))).thenReturn("192.168.1.1");
when(spyServlet.proxyNets()).thenReturn(proxynet);
when(spyServlet.doUseForwardHeaders()).thenReturn(true);
when(request.getHeader(Mockito.eq("X-Forwarded-For"))).thenReturn("192.168.1.1");

Check warning on line 268 in server/src/test/java/com/cloud/api/ApiServletTest.java

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Remove this useless "eq(...)" invocation; pass the values directly. 


See more on https://sonarcloud.io/project/issues?id=apache_cloudstack&issues=AZ__u_NPDoP9WiI4fUg2&open=AZ__u_NPDoP9WiI4fUg2&pullRequest=13871
Assert.assertEquals(InetAddress.getByName("192.168.1.1"), spyServlet.getClientAddress(request));
}

@Test
public void getClientAddressWithHttpXForwardedFor() throws UnknownHostException {
String[] proxynet = {"127.0.0.0/8"};
Mockito.when(spyServlet.proxyNets()).thenReturn(proxynet);
Mockito.when(spyServlet.doUseForwardHeaders()).thenReturn(true);
Mockito.when(request.getHeader(Mockito.eq("HTTP_X_FORWARDED_FOR"))).thenReturn("192.168.1.1");
when(spyServlet.proxyNets()).thenReturn(proxynet);
when(spyServlet.doUseForwardHeaders()).thenReturn(true);
when(request.getHeader(Mockito.eq("HTTP_X_FORWARDED_FOR"))).thenReturn("192.168.1.1");

Check warning on line 277 in server/src/test/java/com/cloud/api/ApiServletTest.java

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Remove this useless "eq(...)" invocation; pass the values directly. 


See more on https://sonarcloud.io/project/issues?id=apache_cloudstack&issues=AZ__u_NPDoP9WiI4fUg3&open=AZ__u_NPDoP9WiI4fUg3&pullRequest=13871
Assert.assertEquals(InetAddress.getByName("192.168.1.1"), spyServlet.getClientAddress(request));
}

@Test
public void getClientAddressWithRemoteAddr() throws UnknownHostException {
String[] proxynet = {"127.0.0.0/8"};
Mockito.when(spyServlet.proxyNets()).thenReturn(proxynet);
Mockito.when(spyServlet.doUseForwardHeaders()).thenReturn(true);
when(spyServlet.proxyNets()).thenReturn(proxynet);
when(spyServlet.doUseForwardHeaders()).thenReturn(true);
Assert.assertEquals(InetAddress.getByName("127.0.0.1"), spyServlet.getClientAddress(request));
}

@Test
public void getClientAddressWithHttpClientIp() throws UnknownHostException {
String[] proxynet = {"127.0.0.0/8"};
Mockito.when(spyServlet.proxyNets()).thenReturn(proxynet);
Mockito.when(spyServlet.doUseForwardHeaders()).thenReturn(true);
Mockito.when(request.getHeader(Mockito.eq("HTTP_CLIENT_IP"))).thenReturn("192.168.1.1");
when(spyServlet.proxyNets()).thenReturn(proxynet);
when(spyServlet.doUseForwardHeaders()).thenReturn(true);
when(request.getHeader(Mockito.eq("HTTP_CLIENT_IP"))).thenReturn("192.168.1.1");

Check warning on line 294 in server/src/test/java/com/cloud/api/ApiServletTest.java

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Remove this useless "eq(...)" invocation; pass the values directly. 


See more on https://sonarcloud.io/project/issues?id=apache_cloudstack&issues=AZ__u_NPDoP9WiI4fUg4&open=AZ__u_NPDoP9WiI4fUg4&pullRequest=13871
Assert.assertEquals(InetAddress.getByName("192.168.1.1"), spyServlet.getClientAddress(request));
}

@Test
public void getClientAddressDefault() throws UnknownHostException {
Mockito.when(request.getRemoteAddr()).thenReturn("127.0.0.1");
when(request.getRemoteAddr()).thenReturn("127.0.0.1");
Assert.assertEquals(InetAddress.getByName("127.0.0.1"), spyServlet.getClientAddress(request));
}

Expand Down Expand Up @@ -326,8 +327,25 @@

@Test
public void testSkip2FAcheckForUserWhenAlreadyVerified() {
Mockito.when(session.getAttribute("userid")).thenReturn(1L);
Mockito.when(session.getAttribute(ApiConstants.IS_2FA_VERIFIED)).thenReturn(true);
when(session.getAttribute("userid")).thenReturn(1L);
when(session.getAttribute(ApiConstants.IS_2FA_VERIFIED)).thenReturn(true);

boolean result = servlet.skip2FAcheckForUser(session);
Assert.assertEquals(true, result);
}

@Test
public void testSkip2FAcheckForUserWhenVerifiedAttributeIsAbsent() {
servlet.accountMgr = accountMgr;
when(session.getAttribute("userid")).thenReturn(1L);
when(session.getAttribute(ApiConstants.IS_2FA_VERIFIED)).thenReturn(null);
when(accountMgr.getUserAccountById(1L)).thenReturn(userAccount);
when(userAccount.getDomainId()).thenReturn(1L);
when(userAccount.isUser2faEnabled()).thenReturn(false);

ConfigKey<Boolean> enableUserTwoFactorAuthentication = Mockito.mock(ConfigKey.class);

Check warning on line 346 in server/src/test/java/com/cloud/api/ApiServletTest.java

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Use a static import for "mock". 


See more on https://sonarcloud.io/project/issues?id=apache_cloudstack&issues=AZ_6-d1yyvrdxWit8HOp&open=AZ_6-d1yyvrdxWit8HOp&pullRequest=13871
AccountManagerImpl.enableUserTwoFactorAuthentication = enableUserTwoFactorAuthentication;
when(enableUserTwoFactorAuthentication.valueIn(1L)).thenReturn(false);

boolean result = servlet.skip2FAcheckForUser(session);
Assert.assertEquals(true, result);
Comment on lines +346 to 351
Expand All @@ -337,10 +355,10 @@
public void testDoNotSkip2FAcheckForUserWhen2FAEnabled() {
servlet.accountMgr = accountMgr;
HttpSession cuurentSession = Mockito.mock(HttpSession.class);
Mockito.when(cuurentSession.getAttribute("userid")).thenReturn(1L);
Mockito.when(cuurentSession.getAttribute(ApiConstants.IS_2FA_VERIFIED)).thenReturn(false);
Mockito.when(accountMgr.getUserAccountById(1L)).thenReturn(userAccount);
Mockito.when(userAccount.isUser2faEnabled()).thenReturn(true);
when(cuurentSession.getAttribute("userid")).thenReturn(1L);
when(cuurentSession.getAttribute(ApiConstants.IS_2FA_VERIFIED)).thenReturn(false);
when(accountMgr.getUserAccountById(1L)).thenReturn(userAccount);
when(userAccount.isUser2faEnabled()).thenReturn(true);

boolean result = servlet.skip2FAcheckForUser(cuurentSession);
Assert.assertEquals(false, result);
Expand All @@ -350,16 +368,16 @@
public void testDoNotSkip2FAcheckForUserWhen2FAMandated() {
servlet.accountMgr = accountMgr;
HttpSession cuurentSession = Mockito.mock(HttpSession.class);
Mockito.when(cuurentSession.getAttribute("userid")).thenReturn(1L);
Mockito.when(cuurentSession.getAttribute(ApiConstants.IS_2FA_VERIFIED)).thenReturn(false);
when(cuurentSession.getAttribute("userid")).thenReturn(1L);
when(cuurentSession.getAttribute(ApiConstants.IS_2FA_VERIFIED)).thenReturn(false);

Mockito.when(accountMgr.getUserAccountById(1L)).thenReturn(userAccount);
Mockito.when(userAccount.getDomainId()).thenReturn(1L);
Mockito.when(userAccount.isUser2faEnabled()).thenReturn(false);
when(accountMgr.getUserAccountById(1L)).thenReturn(userAccount);
when(userAccount.getDomainId()).thenReturn(1L);
when(userAccount.isUser2faEnabled()).thenReturn(false);

ConfigKey<Boolean> mandateUserTwoFactorAuthentication = Mockito.mock(ConfigKey.class);
AccountManagerImpl.mandateUserTwoFactorAuthentication = mandateUserTwoFactorAuthentication;
Mockito.when(mandateUserTwoFactorAuthentication.valueIn(1L)).thenReturn(false);
when(mandateUserTwoFactorAuthentication.valueIn(1L)).thenReturn(false);

boolean result = servlet.skip2FAcheckForUser(cuurentSession);
Assert.assertEquals(true, result);
Expand All @@ -369,20 +387,20 @@
public void testSkip2FAcheckForUserWhen2FAisNotEnabledAndNotMandated() {
servlet.accountMgr = accountMgr;
HttpSession cuurentSession = Mockito.mock(HttpSession.class);
Mockito.when(cuurentSession.getAttribute("userid")).thenReturn(1L);
Mockito.when(cuurentSession.getAttribute(ApiConstants.IS_2FA_VERIFIED)).thenReturn(false);
when(cuurentSession.getAttribute("userid")).thenReturn(1L);
when(cuurentSession.getAttribute(ApiConstants.IS_2FA_VERIFIED)).thenReturn(false);

Mockito.when(accountMgr.getUserAccountById(1L)).thenReturn(userAccount);
Mockito.when(userAccount.getDomainId()).thenReturn(1L);
Mockito.when(userAccount.isUser2faEnabled()).thenReturn(false);
when(accountMgr.getUserAccountById(1L)).thenReturn(userAccount);
when(userAccount.getDomainId()).thenReturn(1L);
when(userAccount.isUser2faEnabled()).thenReturn(false);

ConfigKey<Boolean> enableUserTwoFactorAuthentication = Mockito.mock(ConfigKey.class);
AccountManagerImpl.enableUserTwoFactorAuthentication = enableUserTwoFactorAuthentication;
Mockito.when(enableUserTwoFactorAuthentication.valueIn(1L)).thenReturn(true);
when(enableUserTwoFactorAuthentication.valueIn(1L)).thenReturn(true);

ConfigKey<Boolean> mandateUserTwoFactorAuthentication = Mockito.mock(ConfigKey.class);
AccountManagerImpl.mandateUserTwoFactorAuthentication = mandateUserTwoFactorAuthentication;
Mockito.when(mandateUserTwoFactorAuthentication.valueIn(1L)).thenReturn(true);
when(mandateUserTwoFactorAuthentication.valueIn(1L)).thenReturn(true);

boolean result = servlet.skip2FAcheckForUser(cuurentSession);
Assert.assertEquals(false, result);
Expand All @@ -406,7 +424,7 @@
@Test
public void testVerify2FAWhenAuthenticatorNotFound() throws UnknownHostException {
String command = ValidateUserTwoFactorAuthenticationCodeCmd.APINAME;
Mockito.when(authManager.getAPIAuthenticator(command)).thenReturn(null);
when(authManager.getAPIAuthenticator(command)).thenReturn(null);
StringBuilder auditTrailSb = new StringBuilder();
Map<String, Object[]> params = new HashMap<String, Object[]>();
String responseType = HttpUtils.RESPONSE_TYPE_XML;
Expand All @@ -420,9 +438,9 @@
public void testVerify2FAWhenExpectedCommandIsNotCalled() throws UnknownHostException {
servlet.accountMgr = accountMgr;
String command = "listZones";
Mockito.when(session.getAttribute("userid")).thenReturn(1L);
Mockito.when(accountMgr.getUserAccountById(1L)).thenReturn(userAccount);
Mockito.when(userAccount.isUser2faEnabled()).thenReturn(true);
when(session.getAttribute("userid")).thenReturn(1L);
when(accountMgr.getUserAccountById(1L)).thenReturn(userAccount);
when(userAccount.isUser2faEnabled()).thenReturn(true);

StringBuilder auditTrailSb = new StringBuilder();
Map<String, Object[]> params = new HashMap<String, Object[]>();
Expand Down
Loading