vyommani commented on code in PR #1200:
URL: https://github.com/apache/ranger/pull/1200#discussion_r3964481092
##########
security-admin/src/test/java/org/apache/ranger/biz/TestSessionMgr.java:
##########
@@ -730,4 +737,372 @@ public void
testSetUserRoles_ConfigSuperUserGrantsKeyAdminForSysAdmin() {
PropertiesUtil.getPropertiesMap().remove(RangerConstants.RANGER_ADMIN_SUPER_USERS);
}
+
+ @Test
+ public void testProcessSuccessLogin_LimitZeroDoesNotExpireOldestSession() {
+ RangerContextHolder.setSecurityContext(null);
+
PropertiesUtil.getPropertiesMap().put(SessionMgr.PROP_SESSION_LIMIT_CONCURRENCY,
"0");
+
+ setupAuthentication("limitUser");
+
+ XXPortalUser portalUser = portalUser("limitUser", 70L);
+ stubPortalUserLookup(portalUser);
+ stubRolesAndPermissions(portalUser);
+ stubAuthSessionCreate(200L);
+
when(httpUtil.getDeviceType(anyString())).thenReturn(RangerCommonEnums.DEVICE_UNKNOWN);
+
+ HttpSession currentSession = mock(HttpSession.class);
+ when(currentSession.getAttribute("auditLoginId")).thenReturn(null);
+
+ HttpServletRequest request = mock(HttpServletRequest.class);
+ when(request.getSession()).thenReturn(currentSession);
+ when(request.getRequestURI()).thenReturn("/index.html");
+ when(request.getAttribute("spnegoEnabled")).thenReturn(null);
+
+ UserSessionBase ret =
sessionMgr.processSuccessLogin(XXAuthSession.AUTH_TYPE_PASSWORD, "Mozilla/5.0",
request);
+
+ assertNotNull(ret);
+ assertEquals(70L, ret.getUserId());
+ }
+
+ @Test
+ public void
testProcessSuccessLogin_LimitOneExpiresOldestFormLoginSession() {
+ RangerContextHolder.setSecurityContext(null);
+
PropertiesUtil.getPropertiesMap().put(SessionMgr.PROP_SESSION_LIMIT_CONCURRENCY,
"1");
+
+ setupAuthentication("limitUser");
+
+ XXPortalUser portalUser = portalUser("limitUser", 71L);
+ stubPortalUserLookup(portalUser);
+ stubRolesAndPermissions(portalUser);
+ stubAuthSessionCreate(201L);
+
when(httpUtil.getDeviceType(anyString())).thenReturn(RangerCommonEnums.DEVICE_UNKNOWN);
+
+ HttpSession currentSession = mock(HttpSession.class);
+ when(currentSession.getAttribute("auditLoginId")).thenReturn(null);
+
+ HttpSession oldestSession = mockUiSession("limitUser", 71L, false, 1L);
+
+ HttpServletRequest request = mock(HttpServletRequest.class);
+ when(request.getSession()).thenReturn(currentSession);
+ when(request.getRequestURI()).thenReturn("/index.html");
+ when(request.getAttribute("spnegoEnabled")).thenReturn(null);
+
+ try (MockedStatic<RangerHttpSessionListener> mocked =
Mockito.mockStatic(RangerHttpSessionListener.class)) {
+ CopyOnWriteArrayList<HttpSession> sessions = new
CopyOnWriteArrayList<>();
+ sessions.add(oldestSession);
+
mocked.when(RangerHttpSessionListener::getActiveSessionOnServer).thenReturn(sessions);
+
+ UserSessionBase ret =
sessionMgr.processSuccessLogin(XXAuthSession.AUTH_TYPE_PASSWORD, "Mozilla/5.0",
request);
+
+ assertNotNull(ret);
+
verify(oldestSession).setAttribute(SessionMgr.SESSION_ATTR_CONCURRENT_EXPIRED,
Boolean.TRUE);
+
verify(oldestSession).setAttribute(SessionMgr.SESSION_ATTR_CONCURRENT_EXPIRED_SSO,
false);
+ verify(oldestSession).invalidate();
+ }
+ }
+
+ @Test
+ public void
testProcessSuccessLogin_LimitOneMarksOldestSsoSessionExpiredWithoutInvalidate()
{
+ RangerContextHolder.setSecurityContext(null);
+
PropertiesUtil.getPropertiesMap().put(SessionMgr.PROP_SESSION_LIMIT_CONCURRENCY,
"1");
+
+ setupAuthentication("ssoUser");
+
+ XXPortalUser portalUser = portalUser("ssoUser", 72L);
+ stubPortalUserLookup(portalUser);
+ stubRolesAndPermissions(portalUser);
+ stubAuthSessionCreate(202L);
+
when(httpUtil.getDeviceType(anyString())).thenReturn(RangerCommonEnums.DEVICE_UNKNOWN);
+
+ HttpSession currentSession = mock(HttpSession.class);
+ when(currentSession.getAttribute("auditLoginId")).thenReturn(null);
+
+ HttpSession oldestSession = mockUiSession("ssoUser", 72L, true, 1L);
+
+ HttpServletRequest request = mock(HttpServletRequest.class);
+ when(request.getSession()).thenReturn(currentSession);
+ when(request.getRequestURI()).thenReturn("/index.html");
+ when(request.getAttribute("spnegoEnabled")).thenReturn(Boolean.TRUE);
+
+ try (MockedStatic<RangerHttpSessionListener> mocked =
Mockito.mockStatic(RangerHttpSessionListener.class)) {
+ CopyOnWriteArrayList<HttpSession> sessions = new
CopyOnWriteArrayList<>();
+ sessions.add(oldestSession);
+
mocked.when(RangerHttpSessionListener::getActiveSessionOnServer).thenReturn(sessions);
+
+ UserSessionBase ret =
sessionMgr.processSuccessLogin(XXAuthSession.AUTH_TYPE_TRUSTED_PROXY,
"Mozilla/5.0", request);
+
+ assertNotNull(ret);
+ assertTrue(ret.isSSOEnabled());
+
verify(oldestSession).setAttribute(SessionMgr.SESSION_ATTR_CONCURRENT_EXPIRED,
Boolean.TRUE);
+
verify(oldestSession).setAttribute(SessionMgr.SESSION_ATTR_CONCURRENT_EXPIRED_SSO,
true);
+ verify(oldestSession, never()).invalidate();
+ }
+ }
+
+ @Test
+ public void
testProcessSuccessLogin_DownloadRequestDoesNotConsumeSessionQuota() {
+ RangerContextHolder.setSecurityContext(null);
+
PropertiesUtil.getPropertiesMap().put(SessionMgr.PROP_SESSION_LIMIT_CONCURRENCY,
"1");
+
+ setupAuthentication("limitUser");
+
+ XXPortalUser portalUser = portalUser("limitUser", 73L);
+ stubPortalUserLookup(portalUser);
+ stubRolesAndPermissions(portalUser);
+
when(httpUtil.getDeviceType(anyString())).thenReturn(RangerCommonEnums.DEVICE_UNKNOWN);
+
+ HttpSession currentSession = mock(HttpSession.class);
+ when(currentSession.getAttribute("auditLoginId")).thenReturn(null);
+
+ HttpServletRequest request = mock(HttpServletRequest.class);
+ when(request.getSession()).thenReturn(currentSession);
+
when(request.getRequestURI()).thenReturn("/service/plugins/policies/download/hadoopdev");
+ when(request.getAttribute("spnegoEnabled")).thenReturn(null);
+
+ UserSessionBase ret =
sessionMgr.processSuccessLogin(XXAuthSession.AUTH_TYPE_PASSWORD, "Mozilla/5.0",
request);
+
+ assertNotNull(ret);
+
verify(currentSession).setAttribute(SessionMgr.SESSION_ATTR_DOWNLOAD_ONLY,
Boolean.TRUE);
+ }
+
+ @Test
+ public void testProcessSuccessLogin_ApiRequestDoesNotConsumeSessionQuota()
{
+ RangerContextHolder.setSecurityContext(null);
+
PropertiesUtil.getPropertiesMap().put(SessionMgr.PROP_SESSION_LIMIT_CONCURRENCY,
"1");
+
+ setupAuthentication("limitUser");
+
+ XXPortalUser portalUser = portalUser("limitUser", 74L);
+ stubPortalUserLookup(portalUser);
+ stubRolesAndPermissions(portalUser);
+ stubAuthSessionCreate(204L);
+
when(httpUtil.getDeviceType(anyString())).thenReturn(RangerCommonEnums.DEVICE_UNKNOWN);
+
+ HttpSession currentSession = mock(HttpSession.class);
+ when(currentSession.getAttribute("auditLoginId")).thenReturn(null);
+
+ HttpServletRequest request = mock(HttpServletRequest.class);
+ when(request.getSession()).thenReturn(currentSession);
+
when(request.getRequestURI()).thenReturn("/service/public/v2/api/policies");
+ when(request.getAttribute("spnegoEnabled")).thenReturn(null);
+
+ UserSessionBase ret =
sessionMgr.processSuccessLogin(XXAuthSession.AUTH_TYPE_PASSWORD, "curl/8.0",
request);
+
+ assertNotNull(ret);
+ verify(currentSession).setAttribute(SessionMgr.SESSION_ATTR_NON_UI,
Boolean.TRUE);
+ verify(currentSession,
never()).setAttribute(eq(SessionMgr.SESSION_ATTR_CONCURRENT_EXPIRED), any());
+ }
+
+ @Test
+ public void
testProcessSuccessLogin_LimitTwoExpiresOnlyOldestOfTwoExistingSessions() {
+ RangerContextHolder.setSecurityContext(null);
+
PropertiesUtil.getPropertiesMap().put(SessionMgr.PROP_SESSION_LIMIT_CONCURRENCY,
"2");
+
+ setupAuthentication("limitUser");
+
+ XXPortalUser portalUser = portalUser("limitUser", 75L);
+ stubPortalUserLookup(portalUser);
+ stubRolesAndPermissions(portalUser);
+ stubAuthSessionCreate(205L);
+
when(httpUtil.getDeviceType(anyString())).thenReturn(RangerCommonEnums.DEVICE_UNKNOWN);
+
+ HttpSession currentSession = mock(HttpSession.class);
+ when(currentSession.getAttribute("auditLoginId")).thenReturn(null);
+
+ HttpSession oldestSession = mockUiSession("limitUser", 75L, false, 1L);
+ HttpSession newerSession = mockUiSession("limitUser", 75L, false, 2L);
+
+ HttpServletRequest request = mock(HttpServletRequest.class);
+ when(request.getSession()).thenReturn(currentSession);
+ when(request.getRequestURI()).thenReturn("/index.html");
+ when(request.getAttribute("spnegoEnabled")).thenReturn(null);
+
+ try (MockedStatic<RangerHttpSessionListener> mocked =
Mockito.mockStatic(RangerHttpSessionListener.class)) {
+ CopyOnWriteArrayList<HttpSession> sessions = new
CopyOnWriteArrayList<>();
+ sessions.add(newerSession);
+ sessions.add(oldestSession);
+
mocked.when(RangerHttpSessionListener::getActiveSessionOnServer).thenReturn(sessions);
+
+ UserSessionBase ret =
sessionMgr.processSuccessLogin(XXAuthSession.AUTH_TYPE_PASSWORD, "Mozilla/5.0",
request);
+
+ assertNotNull(ret);
+
verify(oldestSession).setAttribute(SessionMgr.SESSION_ATTR_CONCURRENT_EXPIRED,
Boolean.TRUE);
+ verify(oldestSession).invalidate();
+ verify(newerSession,
never()).setAttribute(eq(SessionMgr.SESSION_ATTR_CONCURRENT_EXPIRED), any());
+ verify(newerSession, never()).invalidate();
+ }
+ }
+
+ @Test
+ public void testProcessSuccessLogin_DoesNotExpireOtherUsersSessions() {
+ RangerContextHolder.setSecurityContext(null);
+
PropertiesUtil.getPropertiesMap().put(SessionMgr.PROP_SESSION_LIMIT_CONCURRENCY,
"1");
+
+ setupAuthentication("userB");
+
+ XXPortalUser portalUser = portalUser("userB", 76L);
+ stubPortalUserLookup(portalUser);
+ stubRolesAndPermissions(portalUser);
+ stubAuthSessionCreate(206L);
+
when(httpUtil.getDeviceType(anyString())).thenReturn(RangerCommonEnums.DEVICE_UNKNOWN);
+
+ HttpSession currentSession = mock(HttpSession.class);
+ when(currentSession.getAttribute("auditLoginId")).thenReturn(null);
+
+ HttpSession otherUserSession = mockUiSession("userA", 77L, false, 1L);
+
+ HttpServletRequest request = mock(HttpServletRequest.class);
+ when(request.getSession()).thenReturn(currentSession);
+ when(request.getRequestURI()).thenReturn("/index.html");
+ when(request.getAttribute("spnegoEnabled")).thenReturn(null);
+
+ try (MockedStatic<RangerHttpSessionListener> mocked =
Mockito.mockStatic(RangerHttpSessionListener.class)) {
+ CopyOnWriteArrayList<HttpSession> sessions = new
CopyOnWriteArrayList<>();
+ sessions.add(otherUserSession);
+
mocked.when(RangerHttpSessionListener::getActiveSessionOnServer).thenReturn(sessions);
+
+ UserSessionBase ret =
sessionMgr.processSuccessLogin(XXAuthSession.AUTH_TYPE_PASSWORD, "Mozilla/5.0",
request);
+
+ assertNotNull(ret);
+ assertEquals(76L, ret.getUserId());
+ verify(otherUserSession,
never()).setAttribute(eq(SessionMgr.SESSION_ATTR_CONCURRENT_EXPIRED), any());
+ verify(otherUserSession, never()).invalidate();
+ }
+ }
+
+ @Test
+ public void
testEnforceConcurrentSessionLimit_ConcurrentSameUserExpiresOldest() throws
Exception {
Review Comment:
Nice to see a real multi-threaded test here. One issue with what it actually
proves: `mockUiSession()` (line 1090) stubs `getAttribute(anyString())` to
always return null except for `AKA_SC_SESSION_KEY` — it doesn't reflect state
written by `setAttribute()`. So when thread A calls
`expireConcurrentSession(oldestSession)` and sets
`SESSION_ATTR_CONCURRENT_EXPIRED`, thread B's subsequent
`findActiveUiSessionsForUser()` call still sees
`getAttribute(SESSION_ATTR_CONCURRENT_EXPIRED)` as null/false and will *also*
treat `oldestSession` as eligible and expire it again. That's why the assertion
is `atLeastOnce()` rather than `times(1)` — and it means this test would pass
identically even if the `synchronized (lock)` block around this method were
removed, since nothing here actually distinguishes "properly serialized, thread
B found nothing left to expire" from "unsynchronized, thread B redundantly
expired the same session again."
Could this be tightened to actually demonstrate the lock matters — e.g. two
*distinct* candidate sessions (`oldestSession`, `secondOldestSession`) with
`limit=1`, and asserting that only one of the two ends up expired in total
across both threads (rather than both, which is what you'd see without the
lock)? A stateful fake/spy that tracks `setAttribute` calls instead of the
current stub would also let `otherSessions.size() < limit` actually
short-circuit correctly on the second thread.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
