[
https://issues.apache.org/jira/browse/KNOX-2790?focusedWorklogId=803460&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-803460
]
ASF GitHub Bot logged work on KNOX-2790:
----------------------------------------
Author: ASF GitHub Bot
Created on: 25/Aug/22 06:49
Start Date: 25/Aug/22 06:49
Worklog Time Spent: 10m
Work Description: smolnar82 commented on code in PR #624:
URL: https://github.com/apache/knox/pull/624#discussion_r954564130
##########
gateway-service-knoxsso/src/main/java/org/apache/knox/gateway/service/knoxsso/WebSSOResource.java:
##########
@@ -264,9 +264,14 @@ private Response getAuthenticationToken(int statusCode) {
original = originalUrlCookies.get(0).getValue();
}
+ Principal p = request.getUserPrincipal();
+ ConcurrentSessionVerifier verifier =
services.getService(ServiceType.CONCURRENT_SESSION_VERIFIER);
+ if (!verifier.verifySessionForUser(p.getName())) {
Review Comment:
Now we have this same 3-line code block in two places; I'd create a new
private void method that throws the web app exception if needed (to avoid code
duplication).
##########
gateway-server/src/main/java/org/apache/knox/gateway/session/control/InMemoryConcurrentSessionVerifier.java:
##########
@@ -51,10 +51,16 @@ public class InMemoryConcurrentSessionVerifier implements
ConcurrentSessionVerif
private long cleaningPeriod;
private final ScheduledExecutorService expiredTokenRemover =
Executors.newSingleThreadScheduledExecutor();
+ /**
+ * This function is used after the verifySessionForUser function.
+ * It checks the session limits even though verifySessionForUser already
checked them,
+ * because in high stress situations there might be some threads which get
verified even though they are over the limit in
+ * verifySessionForUser function, so we catch them here.
+ */
@Override
- public boolean verifySessionForUser(String username, JWT jwtToken) {
+ public boolean registerToken(String username, JWT jwtToken) {
Review Comment:
This could be simplified like that:
```
if (verifySessionForUser(userName)) {
//lock
concurrentSessionCounter.putIfAbsent(username, new HashSet<>());
//unlock
return true;
}
return false;
```
Issue Time Tracking
-------------------
Worklog Id: (was: 803460)
Time Spent: 0.5h (was: 20m)
> Split ConcurrentSessionVerifier.verifySessionForUser
> ----------------------------------------------------
>
> Key: KNOX-2790
> URL: https://issues.apache.org/jira/browse/KNOX-2790
> Project: Apache Knox
> Issue Type: Sub-task
> Components: Server
> Affects Versions: 2.0.0
> Reporter: Sandor Molnar
> Assignee: Balazs Marton
> Priority: Critical
> Fix For: 2.0.0
>
> Time Spent: 0.5h
> Remaining Estimate: 0h
>
> Currently, the ConcurrentSessionVerifier.verifySessionForUser does 2 things:
> * verifies the user if he/she is allowed to have another session
> * registers the given token into the concurrentSessionCounter map
> These 2 functionalities should be split:
> * boolean verifySessionForUser(String userName);
> * void registerToken(String userName, JWT token);
> With this split, in WebSSOResource, the session verification can be done
> before the token is actually created and token registration can be done
> after. It's important because it might be a security leak to generate tokens
> in advance that will not be used at all but, in case of token management is
> enabled, may fill up the disk/memory with unused tokens.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
