From a4dac2851e23533c64a23a056da0d82574d8e300 Mon Sep 17 00:00:00 2001 From: Mathieu Baudier Date: Mon, 19 Jul 2021 07:57:45 +0200 Subject: [PATCH] Make CMS sesison management more robust. --- .../src/org/argeo/cms/auth/CmsAuthUtils.java | 69 +++++++++++-------- .../cms/internal/auth/CmsSessionImpl.java | 15 +++- 2 files changed, 51 insertions(+), 33 deletions(-) diff --git a/org.argeo.cms/src/org/argeo/cms/auth/CmsAuthUtils.java b/org.argeo.cms/src/org/argeo/cms/auth/CmsAuthUtils.java index e9462c3ad..f5503d5c5 100644 --- a/org.argeo.cms/src/org/argeo/cms/auth/CmsAuthUtils.java +++ b/org.argeo.cms/src/org/argeo/cms/auth/CmsAuthUtils.java @@ -123,6 +123,7 @@ class CmsAuthUtils { // subject.getPrincipals().removeAll(subject.getPrincipals(AnonymousPrincipal.class)); } + @SuppressWarnings("unused") synchronized static void registerSessionAuthorization(HttpServletRequest request, Subject subject, Authorization authorization, Locale locale) { // synchronized in order to avoid multiple registrations @@ -131,46 +132,54 @@ class CmsAuthUtils { HttpSession httpSession = request.getSession(false); assert httpSession != null; String httpSessId = httpSession.getId(); - String remoteUser = authorization.getName() != null ? authorization.getName() - : NodeConstants.ROLE_ANONYMOUS; + boolean anonymous = authorization.getName() == null; + String remoteUser = !anonymous ? authorization.getName() : NodeConstants.ROLE_ANONYMOUS; request.setAttribute(HttpContext.REMOTE_USER, remoteUser); request.setAttribute(HttpContext.AUTHORIZATION, authorization); - CmsSessionImpl cmsSession = CmsSessionImpl.getByLocalId(httpSessId); - if (cmsSession != null) { - if (authorization.getName() != null) { - if (cmsSession.getAuthorization().getName() == null) { - cmsSession.close(); - cmsSession = null; - } else if (!authorization.getName().equals(cmsSession.getAuthorization().getName())) { + CmsSessionImpl cmsSession; + CmsSessionImpl currentLocalSession = CmsSessionImpl.getByLocalId(httpSessId); + if (currentLocalSession != null) { + boolean currentLocalSessionAnonymous = currentLocalSession.getAuthorization().getName() == null; + if (!anonymous) { + if (currentLocalSessionAnonymous) { + currentLocalSession.close(); + // new CMS session + cmsSession = new WebCmsSessionImpl(subject, authorization, locale, request); + } else if (!authorization.getName().equals(currentLocalSession.getAuthorization().getName())) { throw new IllegalStateException("Inconsistent user " + authorization.getName() - + " for existing CMS session " + cmsSession); - } - // keyring - if (cmsSession != null) + + " for existing CMS session " + currentLocalSession); + } else { + // keep current session + cmsSession = currentLocalSession; + // keyring subject.getPrivateCredentials().addAll(cmsSession.getSecretKeys()); + } } else {// anonymous - if (cmsSession.getAuthorization().getName() != null) { - cmsSession.close(); - // TODO rather throw an exception ? log a warning ? - cmsSession = null; + if (!currentLocalSessionAnonymous) { + currentLocalSession.close(); + throw new IllegalStateException( + "Existing CMS session " + currentLocalSession + " was not logged out properly."); } + // keep current session + cmsSession = currentLocalSession; } - } else if (cmsSession == null) { + } else { + // new CMS session cmsSession = new WebCmsSessionImpl(subject, authorization, locale, request); } - // request.setAttribute(CmsSession.class.getName(), cmsSession); - if (cmsSession != null) { - CmsSessionId nodeSessionId = new CmsSessionId(cmsSession.getUuid()); - if (subject.getPrivateCredentials(CmsSessionId.class).size() == 0) - subject.getPrivateCredentials().add(nodeSessionId); - else { - UUID storedSessionId = subject.getPrivateCredentials(CmsSessionId.class).iterator().next() - .getUuid(); - // if (storedSessionId.equals(httpSessionId.getValue())) - throw new IllegalStateException( - "Subject already logged with session " + storedSessionId + " (not " + nodeSessionId + ")"); - } + + if (cmsSession == null)// should be dead code (cf. SuppressWarning of the method) + throw new IllegalStateException("CMS session cannot be null"); + + CmsSessionId nodeSessionId = new CmsSessionId(cmsSession.getUuid()); + if (subject.getPrivateCredentials(CmsSessionId.class).size() == 0) { + subject.getPrivateCredentials().add(nodeSessionId); + } else { + UUID storedSessionId = subject.getPrivateCredentials(CmsSessionId.class).iterator().next().getUuid(); + // if (storedSessionId.equals(httpSessionId.getValue())) + throw new IllegalStateException( + "Subject already logged with session " + storedSessionId + " (not " + nodeSessionId + ")"); } } else { // TODO desktop, CLI diff --git a/org.argeo.cms/src/org/argeo/cms/internal/auth/CmsSessionImpl.java b/org.argeo.cms/src/org/argeo/cms/internal/auth/CmsSessionImpl.java index b6966765d..c18348385 100644 --- a/org.argeo.cms/src/org/argeo/cms/internal/auth/CmsSessionImpl.java +++ b/org.argeo.cms/src/org/argeo/cms/internal/auth/CmsSessionImpl.java @@ -10,7 +10,6 @@ import java.util.Collection; import java.util.HashMap; import java.util.HashSet; import java.util.Hashtable; -import java.util.LinkedHashSet; import java.util.Locale; import java.util.Map; import java.util.Set; @@ -60,7 +59,7 @@ public class CmsSessionImpl implements CmsSession { private Map dataSessions = new HashMap<>(); private Set dataSessionsInUse = new HashSet<>(); - private LinkedHashSet additionalDataSessions = new LinkedHashSet<>(); + private Set additionalDataSessions = new HashSet<>(); private Map views = new HashMap<>(); @@ -129,14 +128,17 @@ public class CmsSessionImpl implements CmsSession { } public Set getSecretKeys() { + checkValid(); return getSubject().getPrivateCredentials(SecretKey.class); } public Session newDataSession(String cn, String workspace, Repository repository) { + checkValid(); return login(repository, workspace); } public synchronized Session getDataSession(String cn, String workspace, Repository repository) { + checkValid(); // FIXME make it more robust if (workspace == null) workspace = NodeConstants.SYS_WORKSPACE; @@ -207,12 +209,18 @@ public class CmsSessionImpl implements CmsSession { return !isClosed(); } - protected boolean isClosed() { + private void checkValid() { + if (!isValid()) + throw new IllegalStateException("CMS session " + uuid + " is not valid since " + end); + } + + final protected boolean isClosed() { return getEnd() != null; } @Override public Authorization getAuthorization() { + checkValid(); return authorization; } @@ -258,6 +266,7 @@ public class CmsSessionImpl implements CmsSession { @Override public void registerView(String uid, Object view) { + checkValid(); if (views.containsKey(uid)) throw new IllegalArgumentException("View " + uid + " is already registered."); views.put(uid, view); -- 2.30.2