vyommani commented on code in PR #1200:
URL: https://github.com/apache/ranger/pull/1200#discussion_r3902563861
##########
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/")
Review Comment:
There are now two different "is this a plugin/download request" checks in
this class: the existing audit-skip logic a few lines up only tests
`/secure/policies/download/` and `/secure/download/`, while this new method
(used for the session-quota bypass) tests seven different path substrings. I
checked all seven against the actual `@Path` mappings (`GdsREST`, `RoleREST`,
`ServiceREST`, `XUserREST`, `TagREST`/`TagRESTConstants`) and they do match
real endpoints, so this method itself looks correct — but having two
separately-maintained definitions of the same concept in one class is a drift
risk going forward.
Was it intentional that the audit-skip check and the quota-skip check cover
different URL sets? If they're meant to represent the same "plugin/download
traffic" concept, it'd be worth centralizing into one helper and using it in
both places. If they're deliberately different scopes (audit logging vs. quota
accounting), a short comment explaining why would help the next person who
touches this.
--
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]
