MM-918: Close all user sessions after password change and on logout #171

jnsereko · 2021-08-12T03:06:14Z

Hello Everyone, I have tried to improve session management by keeping track of sessions opened by every user before.
When a user changes his password or logs out, all sessions must be invalidated so that an attacker is trapped out.

As @ibacher had advised me, we had to even implement this feature in core but this was almost impossible because the classes I created weren't accessible in the adminui-module.
Thank you
cc @isears @sherrif10 @ibacher

session.management.mp4

jnsereko · 2021-08-12T04:03:56Z

This PR depends on openmrs/openmrs-module-appui#38 and openmrs/openmrs-module-adminui#64

omod/src/main/java/org/openmrs/web/SessionListener.java

HerbertYiga · 2021-08-13T07:14:58Z

well it looks like we should have things written to core so that any other module that uses the changes simply picks them from core than depending on legacy ui module

jnsereko · 2021-08-13T07:22:04Z

well it looks like we should have things written to core so that any other module that uses the changes simply picks them from core than depending on legacy ui module

Yeah, I agree with you @HerbertYiga. That was my first desire but I ran into issues while trying to access my changes implemented in core. This caused the need to change the OpenmrsVersion used in all modules that need these features. and hence the need to almost change everything in these modules

jnsereko · 2021-08-19T07:30:30Z

Hello @isears @sherrif10 @HerbertYiga @ibacher @dkayiwa everyone is welcome,

I have made some changes to embrace better concurrent functionalities and implement suggestions from Sarah E. Elder
I have tried to dodge changing the interface side of OMRS after a password change. you can view the video below.

Uploading f1f35151-cef0-459f-8af9-b717033ed352.mp4…

How to test this out:

open chrome and login as any user, admin, clerk, nurse ...
open firefox window and/or firefox private browser, and login as the same user.

Below are the test cases i thought about.

On password change, all sessions are invalidated and the list of user sessions becomes 0.
logging out from one session doesn't affect other existing sessions.

jnsereko · 2021-08-19T07:32:15Z

For this to work fine, you must watch openmrs/openmrs-module-adminui#64

ibacher

@jnsereko Thanks for this. It's looking like a good outline; just a couple of more nit-picky types of things to clean up and this should be ready-to-go.

ibacher · 2021-08-19T18:33:42Z

omod/src/main/java/org/openmrs/web/servlet/LogoutServlet.java

 import javax.servlet.http.HttpSession;

 import org.openmrs.api.context.Context;
+import org.openmrs.web.user.CurrentUsers;


This import seems unnecessary.

ibacher · 2021-08-19T18:46:58Z

omod/src/main/java/org/openmrs/web/user/CurrentUsers.java

 		List<String> userNames = new ArrayList<String>();
 		synchronized (currentUsers) {
-			for (String value : currentUsers.values()) {
+			for (String value : currentUsers.keySet()) {
 				userNames.add(value);
 			}
 		}
 		Collections.sort(userNames);
 		return userNames;


All of this seems like unnecessary ceremony.

List<String> userNames = currentUsers.keySet().stream().collect(Collectors.toList()); Collections.sort(userNames); return userNames;

omod/src/main/java/org/openmrs/web/user/CurrentUsers.java

ibacher · 2021-08-19T18:50:39Z

omod/src/main/java/org/openmrs/web/user/CurrentUsers.java

+				}
+			}
+			currentUsers.remove(currentUserName);
+			log.info("Found {} sessions for the user: {}", sessions.size(), currentUserName);


These would be better as debug messages

ibacher · 2021-08-19T18:50:55Z

omod/src/main/java/org/openmrs/web/user/CurrentUsers.java

-	public static Map<String, String> init(ServletContext servletContext) {
-		Map<String, String> currentUserMap = Collections.synchronizedMap(new TreeMap<String, String>());
+	public static Map<String, CopyOnWriteArrayList<HttpSession>> init(ServletContext servletContext) {
+		Map<String, CopyOnWriteArrayList<HttpSession>> currentUserMap = Collections


A few things here:

It's probably better to use a plain HashMap unless we specifically need a sorted map.

It seems a little excessive to have both a synchronized map and a CopyOnWriteArrayList. If we're tracking multiple sessions, a simple ArrayList in a synchronized map is probably fine. Even better would be to just use a plain HashMap and just use a simple lock.

The main reason why I resorted to CopyOnWriteArryList was that I ran into java.util.ConcurrentModificationException as I was trying to remove a session from the sessions-list while iterating trough all other existing sessions.
This is how it all happened.

private static void invalidateAllUserSessions(String currentUserName, ........ for (HttpSession session : sessions) { if (session != null) { session.invalidate(); } }

which calls the method removeSessionFromList through the SessionListener class run on every session invalidation.

@ibacher, Basing on what I have explained above, will option 2 bring out concurrency very well?

That's likely to happen with almost any Java list implementation. You just can't change a list directly while iterating over it. Usually, the way around it it something like this:

Iterator<HttpSession> iterator = sessions.iterator(); while (iterator.hasNext()) { HttpSession session = iterator.next(); session.invalidate(); iterator.remove(); }

The for (HttpSession session : sessions) bit is actually just syntactic sugar for the first 3 lines of the above example.

I used the iterator too before I changed to the CopyOnWriteArryList

Iterator<HttpSession> iterator = sessions.iterator(); while (iterator.hasNext()) { HttpSession session = iterator.next(); session.invalidate(); iterator.remove(); }

if we use this, the SessionListener will also try to remove the element on session invalidation. In my thinking, I expect some errors here :)

Sometimes sessions are invalidated even when the user hasn't clicked logout. or password change for example when the instance(system) is left unused for like 20 minutes .
My main goal is to use the listener to update the sessions list each time the session is invalidated to prevent having null objects in the list. But this takes place from a different method on a different state of the system.

I don't know if this is clear! But that's how I had understood this.

So, the SessionListener will never remove any item from the list. This is because it doesn't know about the list and has no way of removing it. So the list should never contain any null objects.

What it might make sense to do is to instead have a pruneExpiredSessions() method that does something like this:

private static void pruneExpiredSessions(List<HttpSession> sessions) { long currentTimestamp = System.currentTimeMillis(); Iterator<HttpSession> iterator = sessions.iterator(); while (iterator.hasNext()) { HttpSession session = iterator.next(); if (session.getLastAccessTime() - currentTimestamp > session.getMaxInactiveInterval()) { iterator.remove(); } } }

Then after running it:

if (sessions.size() == 0) { currentUsers.remove(currentUserName); }

And, yeah, if you're having an issue with an infinite loop of invalidations, then thing to do is to track which session your actually already invalidating, e.g.,

private static void invalidateAllUserSessions(HttpSession httpSession, String currentUserName, Map<String, CopyOnWriteArrayList<HttpSession>> currentUsers) { log.info("Finding sessions for the user: {}", currentUserName); CopyOnWriteArrayList<HttpSession> sessions = currentUsers.get(currentUserName); if (sessions != null) { for (HttpSession session : sessions) { if (!httpSession.equals(session)) { session.invalidate(); } } currentUsers.remove(currentUserName); log.debug("Found {} sessions for the user: {}", sessions.size(), currentUserName); } else { log.debug("No sessions found for this user: {}", currentUserName); } }

And, yeah, if you're having an issue with an infinite loop of invalidations, then thing to do is to track which session your actually already invalidating, e.g.,

Thank you @ibacher for really taking some time to look at this. I am going to polish this and then push the code.

omod/src/main/java/org/openmrs/web/user/CurrentUsers.java

ibacher · 2021-08-19T18:55:21Z

omod/src/main/java/org/openmrs/web/user/CurrentUsers.java

-		if (log.isDebugEnabled()) {
-			log.debug("Removing user from the current users. session: " + sessionId + " user: "
-			        + currentUsers.get(sessionId));
+		User currentUser = Context.getAuthenticatedUser();


Shouldn't we be getting the user name from the httpSession?

Is it a must that every session must have the session-attribute like username set? I honestly don't even know the exact attribute I will be looking for.

Definitely not! I was thinking of looking for a particular session attribute: I was thinking of getting the user by the particular session, e.g.,

public static List<HttpSession> getSessionsForSessionUser(HttpSession httpSession, Map<String, CopyOnWriteArrayList<HttpSession>> currentUsers) { for (Map.Entry<String, CopyOnWriteArrayList<HttpSession>> entry : currentUsers.entrySet()) { List<HttpSession> sessions = entry.getValue(); if (sessions != null && sessions.contains(httpSession)) { return sessions; } } return Collections.emptyList(); }

ibacher · 2021-08-19T19:00:00Z

omod/src/main/java/org/openmrs/web/user/CurrentUsers.java

+		if (currentUser != null) {
+			String userName = getValidUserName(currentUser);
+			Map<String, CopyOnWriteArrayList<HttpSession>> currentUsers = getCurrentUsers(httpSession);
+			invalidateAllUserSessions(userName, currentUsers);


Do we actually want to invalidate all of the user's sessions when the log out of a single session? I would suggest instead that we simply remove the user from the currentUsers map when the number of sessions remaining is 0.

I aimed at invalidating all user sessions on password change. I created this method so that I use it in the adminui-module, and invalidate all user sessions after a password change.

The on logout, the request is always invalidated which calls the listener and the list of sessions is updated

So, I totally buy that we need a method that invalidates all user sessions and that makes a lot of sense. What doesn't make sense to me is killing all of a user's sessions every time one of their sessions expires (this is different from "what happens when I change my password" where it makes sense to kill all of the users sessions). That is to say that the password reset code probably needs to have a special case to invalidate all of the sessions.

That is to say that the password reset code probably needs to have a special case to invalidate all of the sessions.

yeah, sure! @ibacher

In simple terms each time the method removeUser(httpSession) is used, it will invalidate all sessions for the requested user.
Each time a request is invalidated like https://github.com/openmrs/openmrs-module-legacyui/blob/master/omod/src/main/java/org/openmrs/web/servlet/LogoutServlet.java#L46 the Listener is executed and that session is removed from the sessions list of a specified user to prevent references to null objects in the list.

jnsereko

I have added some changes basing on the comments and discussion i hard with @ibacher
I kindly request for review.

cc @ibacher @dkayiwa @sherrif10 @ibacher

ibacher · 2021-09-01T19:20:34Z

omod/src/main/java/org/openmrs/web/user/CurrentUsers.java

+			CopyOnWriteArrayList<HttpSession> sessions = currentUsers.get(username);
+			sessions.remove(httpSession);
+			log.debug("Removed session: {}. Remaining sessions: {}", httpSession, sessions.size());
+			System.out.println("Removed session " + httpSession + "  Total sessions " + sessions.size());


Before we merge this PR, could we get rid of these System.out.println() calls?

Hello @ibacher
Thank you so much for looking at this. I use System functions to test some cases. Removed it.
Thank you

MM-918: Improved session management logiic Update omod/src/main/java/org/openmrs/web/user/CurrentUsers.java Co-authored-by: Ian <[email protected]> Update omod/src/main/java/org/openmrs/web/user/CurrentUsers.java Co-authored-by: Ian <[email protected]> Update omod/src/main/java/org/openmrs/web/user/CurrentUsers.java Co-authored-by: Ian <[email protected]> Solved travis, by using getUserName() method MMM-918: improved session implementation logic MM-918: Removed System.out functions

jnsereko · 2021-09-11T18:33:13Z

Hello @ibacher @isears @sherrif10
Is this ready for merge?

If yes, may you also take a look at openmrs/openmrs-module-adminui#64

ibacher · 2021-09-15T13:03:25Z

@jnsereko Thanks for the ping on this. It clearly dropped off my radar. Is there a ticket for this?

This was referenced Aug 12, 2021

MM-918: Close all user sessions on log out openmrs/openmrs-module-appui#38

Closed

MM-918 [WIP]: Invalidate all user sessions after password change openmrs/openmrs-module-adminui#64

Open

HerbertYiga reviewed Aug 12, 2021

View reviewed changes

omod/src/main/java/org/openmrs/web/SessionListener.java Show resolved Hide resolved

ibacher requested changes Aug 19, 2021

View reviewed changes

jnsereko force-pushed the MM-918 branch from 0bb6c02 to db8d667 Compare August 31, 2021 08:11

jnsereko commented Aug 31, 2021

View reviewed changes

ibacher reviewed Sep 1, 2021

View reviewed changes

ibacher approved these changes Sep 1, 2021

View reviewed changes

jnsereko force-pushed the MM-918 branch from 7b30ae2 to b77fec0 Compare September 2, 2021 04:04

ibacher merged commit 79cb4ce into openmrs:master Sep 15, 2021

MM-918: Close all user sessions after password change and on logout #171

MM-918: Close all user sessions after password change and on logout #171

Uh oh!

Conversation

jnsereko commented Aug 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jnsereko commented Aug 12, 2021

Uh oh!

Uh oh!

HerbertYiga commented Aug 13, 2021

Uh oh!

jnsereko commented Aug 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jnsereko commented Aug 19, 2021

Uh oh!

jnsereko commented Aug 19, 2021

Uh oh!

ibacher left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnsereko Aug 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ibacher Aug 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnsereko Aug 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ibacher Aug 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnsereko Aug 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnsereko Aug 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnsereko left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnsereko Sep 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnsereko commented Aug 12, 2021 •

edited

Loading

jnsereko commented Aug 13, 2021 •

edited

Loading

jnsereko Aug 19, 2021 •

edited

Loading

ibacher Aug 19, 2021 •

edited

Loading

jnsereko Aug 19, 2021 •

edited

Loading

ibacher Aug 19, 2021 •

edited

Loading

jnsereko Aug 19, 2021 •

edited

Loading

jnsereko Aug 19, 2021 •

edited

Loading

jnsereko Sep 2, 2021 •

edited

Loading