Task #3201 (closed)
BUG: User cannot change password
Reported by: | atarkowska | Owned by: | jamoore |
---|---|---|---|
Priority: | minor | Milestone: | OMERO-Beta4.2.1 |
Component: | Services | Version: | n.a. |
Keywords: | n.a. | Cc: | jburel, cneves |
Resources: | n.a. | Referenced By: | n.a. |
References: | n.a. | Remaining Time: | 0.0d |
Sprint: | 2010-11-11 (19) |
Description (last modified by jmoore)
Request Method: POST Request URL: http://localhost:8000/webadmin/myaccount/save/ Exception Type: SecurityViolation Exception Value: exception ::omero::SecurityViolation { serverStackTrace = ome.conditions.SecurityViolation: Bad authentication credentials for this action at ome.security.basic.BasicMethodSecurity.checkMethod(BasicMethodSecurity.java:130) at ome.security.basic.BasicSecurityWiring.invoke(BasicSecurityWiring.java:78) at org.springframework.aop.framework.ReflectiveMethodInvocation.proceed(ReflectiveMethodInvocation.java:172) at ome.services.blitz.fire.AopContextInitializer.invoke(AopContextInitializer.java:40) at org.springframework.aop.framework.ReflectiveMethodInvocation.proceed(ReflectiveMethodInvocation.java:172) at org.springframework.aop.framework.JdkDynamicAopProxy.invoke(JdkDynamicAopProxy.java:202) at $Proxy66.changePassword(Unknown Source) at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39) at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25) at java.lang.reflect.Method.invoke(Method.java:592) at ome.services.blitz.util.IceMethodInvoker.invoke(IceMethodInvoker.java:179) at ome.services.throttling.Callback.run(Callback.java:56) at ome.services.throttling.InThreadThrottlingStrategy.callInvokerOnRawArgs(InThreadThrottlingStrategy.java:56) at ome.services.blitz.impl.AbstractAmdServant.callInvokerOnRawArgs(AbstractAmdServant.java:136) at ome.services.blitz.impl.AdminI.changePassword_async(AdminI.java:133) at omero.api._IAdminTie.changePassword_async(_IAdminTie.java:106) at omero.api._IAdminDisp.___changePassword(_IAdminDisp.java:1245) at omero.api._IAdminDisp.__dispatch(_IAdminDisp.java:1467) at IceInternal.Incoming.invoke(Incoming.java:159) at Ice.ConnectionI.invokeAll(ConnectionI.java:2037) at Ice.ConnectionI.message(ConnectionI.java:972) at IceInternal.ThreadPool.run(ThreadPool.java:577) at IceInternal.ThreadPool.access$100(ThreadPool.java:12) at IceInternal.ThreadPool$EventHandlerThread.run(ThreadPool.java:971) serverExceptionClass = ome.conditions.SecurityViolation message = Bad authentication credentials for this action } Exception Location: /Users/ola/Dev/omero/dist/lib/python/omero_api_IAdmin_ice.py in changePassword, line 350
Change History (20)
comment:1 Changed 13 years ago by jmoore
comment:2 Changed 13 years ago by jmoore
- Cc jburel added
- Description modified (diff)
Ola, sorry, the issue with the second omero.client is another problem you might run into (see the first comment of #2099). #911 says that a user can only call changePassword if the session that they got the IAdmin instance from was created using a real password and not joinSession(). Otherwise an attacker could:
- sniff a session uuid from the wire (which will eventually timeout)
- login with the session uuid
- call changePassword
- start creating new sessions with the new password
I realize that in the web scenario N-1 of the workers will have been authenticated with a session uuid, so if you receive a SecurityViolation you will need to re-authenticate, or create a temporary SSL-based omero.client with the real password.
There are currently no TestingScenarios dealing with passwords, so I added #3202.
comment:3 Changed 13 years ago by jmoore
- Cc cneves-x added
comment:4 Changed 13 years ago by jmoore
- Status changed from new to assigned
comment:5 Changed 13 years ago by jmoore
- Remaining Time set to 0
- Resolution set to fixed
- Status changed from assigned to closed
comment:6 Changed 13 years ago by jmoore
In discussion with Ola, IAdmin.changePassword should perhaps be considered deprecated for the reasons I put in the javadoc in r8479, namely that it's hard to know if your session is a truly authenticated one or whether it used joinSession (though we could add a method to find out or perhaps a field in EventContext). Instead, it's best to always pass the previous password via changePasswordWithOldPassword (should we change the name?)
comment:7 follow-up: ↓ 9 Changed 13 years ago by jburel
I have been thinking about the new method.
I can currently in the Web admin create a user w/o password so the value will be null.
This implies that the user won't be able to change it.
I don't think we should allow to create a user w/o password b/c the client requires user name and password to log in so not very logic.
comment:8 Changed 13 years ago by jmoore
comment:9 in reply to: ↑ 7 Changed 13 years ago by jmoore
J-M, take a look at the test in the previous commit (r8489) and see if we need to discuss. The value for the password when not set should be empty not null, so the logic should still continue to work. The only time a user can set the password to null is by calling changePassword(null), in which case they become locked out. That may be what we need to change.
As for allowing users without passwords (i.e. with empty passwords), I'm hesitant to make that change. It's both how much of our testing functionality works as well as how the "guest" user functions. We may need to review comment:7:ticket:2609.
comment:10 Changed 13 years ago by atarkowska
Shall we also change or deprecate changeUserPassword? It can be also called by user.
comment:11 Changed 13 years ago by atarkowska
Basically changeUserPassword shouldn't be restricted to admin only?
comment:12 Changed 13 years ago by jmoore
changeUserPassword is restricted to admin or the owner of a group in which the given user is a member.
comment:13 Changed 13 years ago by atarkowska
I didn't know that @RolesAllowed?("user") means admin or owner restriction
comment:14 Changed 13 years ago by jmoore
comment:15 Changed 13 years ago by atarkowska
comment:16 Changed 13 years ago by jmoore
comment:17 Changed 13 years ago by atarkowska
comment:18 Changed 13 years ago by jmoore
comment:19 Changed 13 years ago by atarkowska
According to Brian's comment about stealing session uuid, joining session and changing password, I definitely agree the way changePassword works now is not secure. Currently we server tree methods:
@RolesAllowed({"user", "HasPassword"}) public void changePassword(String newPassword); @RolesAllowed({"user"}) public void changePasswordWithOldPassword(String oldPassword, String newPassword); @RolesAllowed("user") public void changeUserPassword(final String user, final String newPassword);
We could possibly fix it by keeping changePasswordWithOldPassword and adding changeUserPassword(final String user, final String newPassword, final String MY_PASSWORD).
There should be always a authentication step (even for admins) in changing password.
comment:20 Changed 13 years ago by jmoore
Ola, which comment from Brian are you referring to? Would you mind opening a new ticket possibly with tests of what you'd like to have implemented? Would it suffice to have HasPassword on changeUserPassword?
Ola, this is because you are not using a secure connection to pass the password. See #911 for the background and we can discuss when you're ready. In Insight, a second omero.client is kept for making these calls.