Bugzilla – Full Text Bug Listing
| Summary: | Prompting for username/password should show if info is already on record and allow editing | ||
|---|---|---|---|
| Product: | [Eclipse Project] Equinox | Reporter: | Henrik Lindberg <henrik.lindberg> |
| Component: | p2 | Assignee: | Henrik Lindberg <henrik.lindberg> |
| Status: | RESOLVED FIXED | QA Contact: | |
| Severity: | enhancement | ||
| Priority: | P3 | CC: | john.arthorne, matthew, pascal, susan |
| Version: | 3.5 | ||
| Target Milestone: | 3.5 | ||
| Hardware: | All | ||
| OS: | All | ||
| Whiteboard: | |||
|
Description
Henrik Lindberg
Wouldn't this mean p2 is introducing API for retrieving password for a given location? I.e., couldn't someone then hook this method to record/copy people's passwords? (In reply to comment #1) > Wouldn't this mean p2 is introducing API for retrieving password for a given > location? I.e., couldn't someone then hook this method to record/copy people's > passwords? > Yes, you are right, since it does not know where the service is coming from. When I have written similar things before, the UI was integrated. When using a service, it would be a bad idea to give it non encrypted password. I still think it is a good idea to show the user that there was something on file that was tried and did not work. It could just send a string like "original-password" and see if it got back the same string after user has edited. (In reply to comment #1) > Wouldn't this mean p2 is introducing API for retrieving password for a given > location? I.e., couldn't someone then hook this method to record/copy people's > passwords? > I thought more about this, and it is not more dangerous to pass the password on file to the UI than it is to rely on a service to get the password in the first place. A malicious plugin could provide a UI that sends of the passwords as they are entered. The UI service that prompts the user must be trusted not to reveal authentication information. *** Bug 273319 has been marked as a duplicate of this bug. *** I think IServiceUI.getUsernamePassword, should take extra parameters to make it possible for the UI to explain why the user is asked to authenticate. I think location and both username/password should be passed, but if there are strong feelings about the password, it can be omitted. A code should also be passed that indicates one of: - failed login without username/password (nothing was saved) - failed login using stored username/password - failed login using username/password just entered by the user How about just passing in an optional String, which would be the human-readable description of why the authentication failed (incorrectly entered password, incorrect stored password, etc). That keeps the API and UI simple while allowing us to tweak the level of detail shown in the message. I still think revealing the entered/stored password in this message would be bad - I don't think I've ever seen an authentication UI that showed me a password (valid or otherwise) in plain-text. (In reply to comment #6) > How about just passing in an optional String, which would be the human-readable > description of why the authentication failed (incorrectly entered password, > incorrect stored password, etc). That keeps the API and UI simple while > allowing us to tweak the level of detail shown in the message. > Sure, that works too - just thought that you want the UI/human concern in the UI. It is more difficult to write a test for different cases for instance if having to parse a message. How about a code and a message :) > I still think revealing the entered/stored password in this message would be > bad - I don't think I've ever seen an authentication UI that showed me a > password (valid or otherwise) in plain-text. > Authentication UIs typically shows stars, but you get a sense that there is a password and how long it is. (The mac has a very nice keystore where it is possible to reveal the password if you know the keystore's password). Anyway, the password is not the most important - the URI and the reason are. I think a fake password can be passed instead of the real - it is then possible to see if the user did change the password. > How about a code and a message :)
Sure. A good way to make this evolvable without breaking API again is to pass in an object (AuthenticationRequest or some such). That way we can add extra fields to it in the future and never have to break the API again.
Henrick, as discussed on the p2 call, if you provide the new API on the IServiceUI side I can add the string to the dialog and also clean up the appearance/layout, etc. One more thing...I'm assuming that for 3.5 we are just going to show whatever string was passed in. A code would be useful for the future if we decide to provide links to alternate tasks (go to the secure store prefs, delete this repo, or whatever...) (And sorry for misspelling your name above, Henrik!) Fixed - released to HEAD. Different messages are shown depending on if the name/password was stored, if it comes from the cache, or if being the first time it is requested. (Hint, try it with the testserver: http://localhost:8080/proxy/never, as this will prompt for ever). I decided to actually pass the password to the service - it is not shown in clear text. My reasoning is that it is the same service that would be used for entering passwords in the first place - so all passwords will have been available to this service. If a malicious user can replace this service they can still get all passwords entered by the user by stealing them as they are (re)-entered. I think this implementation is much more user friendly, even if you can't see the actual password you see if you made a stupid mistake (password is too long or short) - and it is clear that the system knew about some password. See Bug 274007 for future work (3.6) i this area. |