Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jnsereko
Copy link
Contributor

@jnsereko jnsereko commented Aug 6, 2021

This issue is still in development :
Every time a user changes his/her password, so long as he/she has not clicked the logout link, he/she can still login using his/her previous password.

cc @isears @sherrif10 @ibacher @dkayiwa

Comment on lines 114 to 115
Context.refreshAuthenticatedUser();
request.getSession().invalidate();
Copy link
Member

@ibacher ibacher Aug 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this forcibly log the user out? (I don't think that's desirable behaviour)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this forcibly log the user out?

Yes @ibacher The user is forcefully logged out. Thank you

Copy link
Contributor Author

@jnsereko jnsereko Aug 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe i should remove line 115 and add the code below to logout the user using the context so that he is redirected to the login screen. What is the better option?

 Context.logout();
 request.getSession().invalidate();
 request.getSession().setAttribute("manual-logout", "true");

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean, the real ideal would be if there's a way for a user to change their password and have the session continue uninterrupted, but, yeah, if we're going to log the user out, maybe just do the whole thing. We could even add a redirect to the login page here?

@jnsereko
Copy link
Contributor Author

jnsereko commented Aug 6, 2021

I came across this when i was trying to find out why different sessions exist after a password change.

Before:

06.08.2021_17.56.19_REC.mp4

After:

06.08.2021_17.45.47_REC.mp4

@ibacher
Copy link
Member

ibacher commented Aug 6, 2021

@jnsereko Thanks for the videos. So it looks like we're logging the user out anyways, I suppose we might as well continue with that behaviour.

@jnsereko
Copy link
Contributor Author

jnsereko commented Aug 6, 2021

So it looks like we're logging the user out anyways,

@ibacher I think, it would be better this way. Thank you so much
@isears

Copy link
Member

@isears isears left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @jnsereko

I also think it would be a good idea to redirect to login after a password change, if that doesn't already happen automatically.

Also, let's try to take this one step further and invalidate all the user's active sessions (e.g. on different browsers, or on different devices) on a password change. Do you think you can incorporate any of the ideas from this PR: openmrs/openmrs-module-webservices.rest#486 ?

@jnsereko
Copy link
Contributor Author

jnsereko commented Aug 7, 2021

Hey @isears @ibacher. Thanks for the review

I also think it would be a good idea to redirect to login after a password change,

My own worry is that after password change, I wanted to display a successful password change message. If the user is redirected to the login, will it be possible for us to display that successful password message? I tried it but it failed.

Do you think you can incorporate any of the ideas from this PR: openmrs/openmrs-module-webservices.rest#486 ?

This is really helpful @isears, thank you

@jnsereko jnsereko closed this Aug 7, 2021
@jnsereko jnsereko reopened this Aug 7, 2021
@jnsereko jnsereko force-pushed the MM-918 branch 3 times, most recently from 756764b to 954e4ee Compare August 12, 2021 03:39
@jnsereko
Copy link
Contributor Author

jnsereko commented Aug 12, 2021

I have added some changes but this PR depends on openmrs/openmrs-module-legacyui#171

cc @ibacher @sherrif10 @isears

@ibacher
Copy link
Member

ibacher commented Sep 15, 2021

@jnsereko If this depends on something in the legacyui, that's an argument to move it into core.

@jnsereko jnsereko changed the title MM-918 [WIP]: Authentication bug after password change MM-918 [WIP]: Invalidate all user sessions after password change Apr 12, 2022
@jnsereko
Copy link
Contributor Author

jnsereko commented Apr 12, 2022

so this fix corrects two bugs:

  • Invalidate all user sessions on password change: If user is logged in from different devices or browsers, all his sessions will be invalidated
  • Authentication bug on password change: A user could login with previous password after password change. This was because an old session existed in the system that allowed login with old credentials.
session.invalidation.mp4

cc @ibacher @dkayiwa

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants