… I have noticed that org.openmrs.api.db.UserDAO#getLoginCredential(org.openmrs.User) is only accessed up to the UserDAO layer and not at the service layer. I needed to use this method since user.getSecretQuestion() is deprecated and returns null even when the secret question is saved! For now I am planning to call into the dao from the controller which is a sub standard way of doing things, so i thought of ringing this thought in the devs minds:
Can we expose or write a getLoginCredential(org.openmrs.User) method to be accessed at the service layer?
For now, what would be your advise to return such credentials like saved question into the controller?
My understanding is that the AdminUI sprint is simply redoing the already existing functionality in the new reference application UI. So why not just use the exact same service layer API which is currently used by the legacy UI? Or are you trying to add new UI functionality that was not in the legacy UI?
We are doing similar stuff but adding on; for-example for the reference application; we are going to display the secret question instead of a blank field which was initially used in the legacy ui and may imply that the user has none defined
O, Right, i had looked up to a user with none. Actually i called into the UserDAO class to get it since user.getSecretQuestion() is deprecated, like; getComponent(UserDAO.class).getLoginCredential(Context.getAuthenticatedUser()).getSecretQuestion()
I hope this does us no harm @dkayiwa!
The short answer is “No.” he LoginCredential javadoc` specifically states: “This should never be used by anything except for UserDAO and UserService methods that change passwords.”
It sounds like user authentication is poorly designed.
I can understand why, for security, we would not want to populate every User instance with its secret question.
I recall pushing to have login credentials separated from the API (in hopes that TRUNK-381 might get done some day).
Our single secret question + secret answer approach to self-service security is pretty lame and should be replaced (e.g., with temporary password to email). While we may not solve that here, we should at least not dig ourselves deeper into the hole while solving the immediate problem.
Certainly, having to bypass the service layer and go through the DAO is a red flag that something is wrong. Given the above points, I would think a UserService.getSecretQuestionForUser(User) would be a temporary fix (it could use the LoginCredential without exposing it outside the API). Unfortunately, this needs to be accessible to anonymous users, since its use case is during password recovery… which means anyone can anonymously obtain the secret question for any user for whom they can guess their username (part of the reason this approach is lame). At a mimimum, we should log any time it is used (where exposed in the web layer – e.g., a REST endpoint – we’d want to log the IP address of the requestor) and we should double-check to make sure that an adminstrator can disable this feature system-wide for security (requiring an admin to reset a user’s password when forgotten). When we can implement a less embarassing self-service password reset feature (e.g., email temporary password), we can finally get rid of this user-generated question/answer pair approach altogether.
I would just add that I recommend that whatever we do as a temporary solution, we should do this in the adminui module. (I.e. we should not make these temporary changes in openmrs-core.)
@Darius the Burke’s proposed solutions aren’t going to be implemented in core in anytime soon, in the short run we are going to use the deprecated property accessors for question and answer on the user object.
FYI on the account form am working on, i currently don’t support setting the secret question and answer for this same reason.
I just saw @k_joseph’s pull request, he has a work around in the adminui module where he fetches the userDAO bean from the Context and uses that to get the login credentials and it seems like a fine work around to me for now.
Currently, that method doesn’t exist and getting the LoginCredential object into the controller means exposing all it’s other properties such as; salt, secret answer, hashed password into the controller though i had only used secret question property which i return onto the ui and nothing else
How can you restrict access to something needed for password recovery (a feature usually only seen when you are not yet authenticated)? Except for managing their secret question, the user would not be authenticated when trying to reset their password.
What I had meant to say is that if we’re directly accessing the DAO bean from the My Account page, it should be hardcoded in such a way that it only shows the secret question of the authenticated user (e.g. doesn’t respect any ?username=… in the request).
Since, as Burke points out, anyone can learn anyone else’s secret question just by knowing their username and doing the “forgot password” workflow, I guess any security weakness is already there and easy to access. So, you can ignore this message of mine, and the previous one.
