Warning: Can't synchronize with repository "(default)" (/home/git/ome.git does not appear to be a Git repository.). Look in the Trac log for more information.
Notice: In order to edit this ticket you need to be either: a Product Owner, The owner or the reporter of the ticket, or, in case of a Task not yet assigned, a team_member"

Task #3201 (closed)

Opened 13 years ago

Closed 13 years ago

Last modified 13 years ago

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

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.

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

(In [8479]) Adding IAdmin.changePasswordWithOldPassword (See #911, Fix #3201)

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: 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

(In [8489]) Added test for blank passwords (See #3201)

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

See #1781 r6061. Several methods in IAdmin now make their decisions internally based on whether or not the current user is group owner.

comment:15 Changed 13 years ago by atarkowska

(In [8492]) this fixes change password issue, see #3201 and few minor issues with shares

comment:16 Changed 13 years ago by jmoore

(In [8493]) Modifying changePasswordWithOldPassword to take two RStrings (See #3201)

comment:17 Changed 13 years ago by atarkowska

(In [8495]) this make final changes password issue, see #3201

comment:18 Changed 13 years ago by jmoore

(In [8514]) Minor test fix (See #3201)

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?

Note: See TracTickets for help on using tickets. You may also have a look at Agilo extensions to the ticket.

1.3.13-PRO © 2008-2011 Agilo Software all rights reserved (this page was served in: 0.161779 sec.)

We're Hiring!