vyommani commented on code in PR #1200:
URL: https://github.com/apache/ranger/pull/1200#discussion_r3901458274


##########
security-admin/src/main/java/org/apache/ranger/biz/SessionMgr.java:
##########
@@ -498,6 +516,142 @@ public Date getLastSuccessLoginAuthTimeByUserId(String 
loginId) {
         return null;
     }
 
+    public static boolean isConcurrentSessionExpired(HttpSession session) {
+        if (session == null) {
+            return false;
+        }
+
+        try {
+            return 
Boolean.TRUE.equals(session.getAttribute(SESSION_ATTR_CONCURRENT_EXPIRED));
+        } catch (IllegalStateException e) {
+            return false;
+        }
+    }
+
+    public static boolean isConcurrentSessionExpiredSso(HttpSession session) {
+        if (session == null) {
+            return false;
+        }
+
+        try {
+            return 
Boolean.TRUE.equals(session.getAttribute(SESSION_ATTR_CONCURRENT_EXPIRED_SSO));
+        } catch (IllegalStateException e) {
+            return false;
+        }
+    }
+
+    /**
+     * When {@code ranger.session.limit.concurrency} is exceeded, expire the 
oldest UI sessions
+     * so the new login succeeds. SSO sessions are marked expired for Knox 
logout redirect.
+     */
+    protected void enforceConcurrentSessionLimit(String loginId, HttpSession 
currentSession) {

Review Comment:
   This read-then-expire sequence isn't atomic per `loginId`. Two 
near-simultaneous logins for the same user (e.g. two browser tabs, or a script 
racing the UI) can both read `findActiveUiSessionsForUser()` before either's 
new session is reflected, and both could independently decide nothing needs 
expiring  momentarily letting the user exceed `limit` by one. 
`CopyOnWriteArrayList` only makes the *iteration* thread-safe, not this 
check-then-act sequence.
   
   Given Admin login rate is low, a simple per-`loginId` lock (e.g. a striped 
lock, or `ConcurrentHashMap<String,Object>.computeIfAbsent` used as a lock 
table) around the find+expire sequence would close this without much cost. Not 
blocking, but worth a follow-up if not fixed here.



-- 
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]

Reply via email to