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


##########
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) {
+        int limit = 
PropertiesUtil.getIntProperty(PROP_SESSION_LIMIT_CONCURRENCY, 0);
+
+        if (limit <= 0 || StringUtils.isBlank(loginId) || currentSession == 
null) {
+            return;
+        }
+
+        List<HttpSession> otherSessions = findActiveUiSessionsForUser(loginId, 
currentSession);
+
+        if (otherSessions.size() < limit) {
+            return;
+        }
+
+        otherSessions.sort(Comparator.comparingLong(session -> {
+            try {
+                return session.getCreationTime();
+            } catch (IllegalStateException e) {
+                return 0L;
+            }
+        }));
+
+        int toExpire = otherSessions.size() - limit + 1;
+
+        logger.info("Concurrent session limit {} exceeded for user {}; 
expiring {} older session(s)", limit, loginId, toExpire);
+
+        for (int i = 0; i < toExpire; i++) {
+            expireConcurrentSession(otherSessions.get(i));
+        }
+    }
+
+    static boolean isPluginOrSecureDownloadRequest(String uri) {
+        if (StringUtils.isEmpty(uri)) {
+            return false;
+        }
+
+        return uri.contains("/secure/policies/download/")
+                || uri.contains("/secure/download/")
+                || uri.contains("/plugins/policies/download/")
+                || uri.contains("/tags/download/")
+                || uri.contains("/roles/download/")
+                || uri.contains("/xusers/download/")
+                || uri.contains("/gds/download/");
+    }
+
+    private List<HttpSession> findActiveUiSessionsForUser(String loginId, 
HttpSession currentSession) {

Review Comment:
   `getActiveSessionOnServer()` returns a single-JVM static list, so this only 
sees sessions handled by the node that's processing the current login. In an 
HA/load-balanced Ranger Admin deployment (multiple nodes), a user can hold up 
to `limit` sessions *per node* rather than `limit` sessions cluster-wide — the 
concurrency limit isn't actually enforced globally.
   
   Could you either document this explicitly in the 
`ranger.session.limit.concurrency` property description (so operators aren't 
surprised in HA setups), or consider a DB-backed check against 
`XXAuthSession`/a shared store if cluster-wide enforcement is the intended 
guarantee?



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