Community
Participate
Working Groups
For now, CDO UI doesn't provide any mean to change / reset passwords when the security manager is activated. This feature is nevertheless required to operate a CDO from the client side. The following elements should be added: - in CDO Sessions view, add an item "Change password" in the menu when the user is logged into the repository. This action should open a dialog asking for the previous password and twice the new one to avoid mistakes. All entries should be masked. Provision for checking the strength of the password should exist. - in CDO editor, for the security model, remove the "New Child/Password" item of the User contextual menu and replace it by "Change Password". This action should open a dialog similar to the previous one, but without asking for the previous password, which is obviously unknown from the Administrators. In both cases, the new password shall be sent encrypted to the server.
Moving all outstanding enhancements to 4.3
As outlined in bug 417103 this is not just a UI issue, we'll also need core support (e.g. API, protocol, ...).
See also bug 417469.
I can work on a "Change Password" function.
Hi, Eike, I have created a new branch bugs/399306 with working implementations of: * Change Password: server-initiated protocol for changing the user's password. The InteractivePasswordCredentialsProvider implements new extension API for password change, using an extension of the CredentialsDialog (a CredentialsUpdateDialog) to ask the user for a new password. The server authenticates the user and, if successful, changes the password. This is implemented in commit c36d3d9. * Reset Password: an elaboration of the previously described change-password protocol, to distinguish between "change" and "reset" operations on the password. The difference is that a "reset" may only be done under an adminstrator's authentication because it does not require knowledge of the user's previous password. In this case, the user being authenticated and the user whose password is changed are different. However, it is still a *change*: the CredentialsResetDialog generates a new password to return (securely via DiffieHellman) to the server. This temporary password is then presented to the administrator to share with the user whose password was reset, to change it ASAP. A "Copy to Clipboard" button is provided to assist. This is implemented in commit f3be3bd. What has not been done yet is, and which I'm not sure that I intend to do, is: * indication of password "strength" in the Change Password dialog, as requested by the submitter of this enhancement. To do this, I expect I would look for a third-party library that will require IP IP approval by Eclipse Foundation. I would split this off as a new enhancement in Bugzilla to be treated separately * update the security model to let a UserPassword be flagged as temporary with an expiry timestamp. This would force users to change their passwords on login (could be done with an elaboration of the existing Authentication signal) I would be interested in your feed-back on this branch before proceeding any further with this. For your convenience, I have recorded a short demo of the new functionality in action: https://www.youtube.com/watch?v=8RhmKJs5CgE Thanks!
Hi Christian, I just wanted to review the changes in your branch and tried to rebase them on top of master. Many files had conflicts because you've changed the first line of the copyright headers. Please leave that line intact in all files and just add your name/company to the Constributors: section. Maybe you can also rebase the branch to spare me some time (it's so busy here these days). The same might be needed for bug 418452. Thanks ;-)
(In reply to Eike Stepper from comment #6) > Hi Christian, I just wanted to review the changes in your branch and tried > to rebase them on top of master. Many files had conflicts because you've > changed the first line of the copyright headers. Please leave that line > intact in all files and just add your name/company to the Constributors: > section. Maybe you can also rebase the branch to spare me some time (it's so > busy here these days). The same might be needed for bug 418452. Thanks ;-) Thanks for taking a look at this, Eike! It's much appreciated. I'll get that rebased today and will push a new branch with a single commit to make it simpler to review. I (try to remember to) change the the copyright statement to include all copyright holders when necessary because I thought that is what Eclipse's IP policy requires. Is it not? Or do you have some tool that updates all of the copyrights before a release is made? For example, at least the year should be kept up-to-date. In any case, I'll get this branch rebased and squashed (yes, there are a lot of files to change; adding a signal to a protocol has a considerable ripple effect!)
(In reply to Christian W. Damus from comment #7) > I (try to remember to) change the the copyright statement to include all > copyright holders when necessary because I thought that is what Eclipse's IP > policy requires. Is it not? It's my understanding that the copyrights are generally assoviated with the changes applied, whether or not mentioned in the first line or in the Contributors section. I'd absolutely not like it if all contributors would append their names, corporate names and addresses into the first copyright line. I think that's what the Contributors section with 1 line per contributor/change is for. I'm not even sure if it's really necessary to include all the detailed change infos there, but I don't care. > Or do you have some tool that updates all of > the copyrights before a release is made? For example, at least the year > should be kept up-to-date. Of course I have a self-made tool for that: http://thegordian.blogspot.de/2013/05/copyright-headers-from-git-history.html ;-) > In any case, I'll get this branch rebased and > squashed (yes, there are a lot of files to change; adding a signal to a > protocol has a considerable ripple effect!) You wanna write a wizard for that? :P
(In reply to Eike Stepper from comment #8) Hi, Eike, I have rebased my changes onto this morning's master (Eastern time), fixed up all of the copyright conflicts, squashed all of the commits into one, and pushed a new branch bugs/399306a (note the 'a' suffix) I think this should make it much easier to review the changes I described in comment #5. > It's my understanding that the copyrights are generally assoviated with the > changes applied, whether or not mentioned in the first line or in the > Contributors section. I'd absolutely not like it if all contributors would > append their names, corporate names and addresses into the first copyright > line. I think that's what the Contributors section with 1 line per > contributor/change is for. I'm not even sure if it's really necessary to > include all the detailed change infos there, but I don't care. Thanks, Eike. I shall stick to appending contributor comments, as indeed that should help to reduce merge headaches. I'll let your tool take care of the dates. > > In any case, I'll get this branch rebased and > > squashed (yes, there are a lot of files to change; adding a signal to a > > protocol has a considerable ripple effect!) > > You wanna write a wizard for that? :P Hah! Not really, no. That's rather outside of my budget (and skill!).
I've reviewed your changes in depth now. Impressive how well you've understood the principles of Net4j. I really like your implementation. I have refactored and reformatted here and there. Because of my limited time I'd prefer not to describe everything here in detail. Please review my changes extra carefully! The only no-go in your changes is that they leave a bunch of PDE compiler warnings in org.eclipse.emf.cdo.security.ui. At one point we should refactor all these old and poorly maintained objectContributions to the newer command framework. But you can also decide to disable the compiler checks until then. What I don't like so much is the API for the resetCredentials use case. Am I right that this always requires admin permissions? Ideally we'd have that API as an interface in net4j.util.security and implemented by UserImpl so that the generic CDO UI can show an action/command for it. The difficulty would be to delegate its invocation to the client session in order to trigger the protocol. BTW. one reason for my disliking is this new cdo.security.ui plugin with only one action delegate. But somehow it strikes me that your other branch with the simplified security editor might also make use of it. So for now I've moved the CDOSession.resetCredentials() method into InternalCDOSession. That gives us time to rethink ;-) Please feel free to merge your branch once you've removed the PDE warnings. Thanks for the great work!
(In reply to Eike Stepper from comment #10) Thanks very much, Eike! I really appreciate your taking the time to review such a large and wide-ranging change set. Especially in the run up to ECE! > I've reviewed your changes in depth now. Impressive how well you've > understood the principles of Net4j. I really like your implementation. I > have refactored and reformatted here and there. Because of my limited time > I'd prefer not to describe everything here in detail. Please review my > changes extra carefully! I certainly shall. That will help me to learn better the coding and design style and that I should try to fit into. > The only no-go in your changes is that they leave a bunch of PDE compiler > warnings in org.eclipse.emf.cdo.security.ui. At one point we should refactor > all these old and poorly maintained objectContributions to the newer command > framework. But you can also decide to disable the compiler checks until then. OK, yes. I did it this way because originally I had intended this action to be with the rest of the object contributions in oee.cdo.ui, but that would have needed a dependency on the security model that I didn't want to add. I am happy to change to the new menus way; it's very easy to do and aligns with how I did action contributions for bug 418452 :-) Warnings will be gone. > What I don't like so much is the API for the resetCredentials use case. Am I > right that this always requires admin permissions? Ideally we'd have that Correct. This requires admin authorization because resetting a user's password does not require knowing the user's current password, so we can't have just any old user doing it. > API as an interface in net4j.util.security and implemented by UserImpl so > that the generic CDO UI can show an action/command for it. The difficulty OK, but how would the client get access to the UserImpl to invoke this API on it? Regular users don't have access to the security model, and I don't think regular users should have access to this API at all because it should be an administrative thing. However, I like your suggestion of implementing this as API on the User in the security model. Would you mind if I re-work the implementation a bit to do that? > would be to delegate its invocation to the client session in order to > trigger the protocol. BTW. one reason for my disliking is this new > cdo.security.ui plugin with only one action delegate. But somehow it strikes You mean, like the oee.cdo.ui.shared plug-in that has only one class in it (that isn't an OM)? :-P > me that your other branch with the simplified security editor might also > make use of it. So for now I've moved the CDOSession.resetCredentials() Indeed, bug 418452 adds a whopping big editor into this very plug-in. :-) > method into InternalCDOSession. That gives us time to rethink ;-) Right. As I said, I could try the refactoring that you suggested to move this into the net4j.util.security package. If I manage to make that work, would you like to review again? > Please feel free to merge your branch once you've removed the PDE warnings. > Thanks for the great work! Thanks! It was fun, actually.
Eike: one follow-up question. These changes (as you know) introduce a new plug-in. Should it have version 1.0.0 or 4.2.0?
(In reply to comment #12) > Eike: one follow-up question. These changes (as you know) introduce a new > plug-in. Should it have version 1.0.0 or 4.2.0? I can answer this myself. The new expressions model plug-ins were introduced with version 4.3 (for Luna), so I shall do the same with the new oee.cdo.security.ui plug-in. I have pushed a new commit to the bugs/339306a branch. It updates the Reset Password... action to use the new menu contribution mechanism, supporting user-defined key bindings. This includes new reusable command Handler analogues of the SafeActionDelegate and LongRunningActionDelegate in the Net4J UI Util.
Fixed on CDO master (Luna, 4.3) Commits: 98dd1927de57f31ef26e6f265a2879eec3ba350c Initial implementation of password-change and password-reset protocol and UI. 1851055a45b9e47e92949ff4a4dd6d99c831fdfb Refactorings and other tweaks in code review by Eike. a380c91f5c2cb63269b93336796d2bcb559bffde Addresses additional code review comments, especially: - use UI Menus extension instead of deprecated popup-menus (enables user-customizable keybindings, which I have tested with the new Reset Password... action). This includes new reusable handler implementations in oe.net4j.util.ui analogous to the SafeActionDelegate and LongRunningActionDelegate - fix a resource-leak warning in the CredentialsChallengeIndication (I noticed this in a Gerrit test build) - set version of new oee.cdo.security.ui plug-in at 4.3.0 instead of 1.0.0 I have raised additional enhancements to address follow-up items not critical to the resolution of this enhancement: Bug 420407 [Security] Change password dialog should show indication of password "strength" Bug 420408 [Security] Implement temporary password expiry on password reset Bug 420410 [Security] Move password reset API from session to security model
Yes, 1.0.0 or 4.3.0 are both okay. I see that you've merged the branch to master. That's okay, too ;-)
(In reply to Eike Stepper from comment #15) > Yes, 1.0.0 or 4.3.0 are both okay. > > I see that you've merged the branch to master. That's okay, too ;-) Thanks, Eike. You did ask me to merge this to master, right?
> You did ask me to merge this to master, right? Yeah! I switch between too many tasks these days :P
Hi Christian, I thought you could make use of the session's userID property to show/hide the "Change Password..." menu item. Below is example markup for an objectContribution and you can list the subclasses of org.eclipse.net4j.util.properties.Properties to get an overview of the available properties. <objectContribution adaptable="true" id="org.eclipse.emf.cdo.ui.CDOResourceNodeContributionsWritableContainer" objectClass="org.eclipse.emf.cdo.eresource.CDOResourceNode"> <action class="org.eclipse.emf.cdo.internal.ui.actions.RemoveResourceActionDelegate" icon="icons/full/elcl16/delete_edit.gif" id="org.eclipse.emf.cdo.ui.RemoveResource" label="%action.label.2" tooltip="%action.tooltip.1"> </action> <enablement> <test property="org.eclipse.emf.cdo.object.writableContainer" value="true"/> </enablement> </objectContribution>
Feel also free to add new properties to ease the writing of the plugin.xml markup. Properties are invisible in the Properties view if the category is not specified. org.eclipse.emf.internal.cdo.object.ObjectProperties has examples. Don't forget to update the plugin.xml (each Properties class has a main method that outputs the needed markup) ;-)
(In reply to Eike Stepper from comment #19) Thanks for the tips, Eike. I have squirrelled away a like for future reference. However, in this case, I don't see what you're suggesting vis-a-vis the user ID property. Are you suggesting that the availability of this action should depend on what a user's ID is? I would expect all users to be able to change their passwords ...
No, I mean that the userID of the session is (typically) only set to non-null if some kind of authentication has happened. So we could use userID!=null to make the menu item visible. A new (uncategorized) boolean property userAuthenticated would probably be nice.
(In reply to Eike Stepper from comment #21) Ah! Right, I have been working with a security manager so long, now, that I had forgotten that it is optional. commit 82637f6 It was as simple as checking for a user ID in the CDOItemProvider (which is where the action is contributed). There is no impact on plugin.xml nor on properties.
Closing.
