Community
Participate
Working Groups
Add API to set flags for saving passwords.
Created attachment 81217 [details] mylyn/context/zip
Created attachment 81219 [details] patch The patch adds support in TaskRepository to store passwords without saving to the key ring. It also adds checkboxes to the repository settings page to select if passwords should be saved. The implementation adds a generic method to TaskRepository that allows to set and retrieve credentials for different types of authentication (repository (default), http, proxy). The authentication type is a string. I have cleaned up some of the code duplication in that class but it should be reviewed for 3.0. Rob, please review and apply. The repository settings page is currently lacking a warning that storing passwords is potentially dangerous. This could be displayed in the dialog header when Save Password is selected to avoid cluttering the UI further.
Created attachment 81220 [details] mylyn/context/zip
Isn't 3 separate settings for saving passwords is too much or I am missing something? I think it complicates UI even more then per-repository timeouts that been considered too complicated (and in my humble opinion timeouts are more critical then this).
I agree that having three check boxes is not optimal. The reason I added all three is that there is scenario where a I have less valuable passwords for proxies or http authentication than for the repository. I would want to save those but enter the one for the repository once per session. At some point the check boxes could be replaced by a separate UI in the preferences for managing passwords. I suggest to go with the current solution and improve when have more resources.
The TaskRepository refactoring and unit tests look great. Patch applied. Lets iterate on the repository settings page ui. I agree with Eugene's observation regarding number of save password options. We should also consider switching the presentation order to: Save Password [X] (we may still have space for a short warning label right after that as well).
Few more random comments on the changes: - @deprecated should have some description what to use instead - values for standard properties (including should "save password") should came from single place in IRepositoryConstants and not from constants in TaskRepository class - Why the following method are public api? I don't see them used anywhere outside TaskRepository getUserName(String authType) getPassword(String authType) setCredentials(String authType, String username, String password)
(In reply to comment #7) > - @deprecated should have some description what to use instead IMHO, the deprecated fields shouldn't have been API in the first place. The alternative is to not use them. > - values for standard properties (including should "save password") should came > from single place in IRepositoryConstants and not from constants in > TaskRepository class Sounds like a good idea. Please file a bug report for that. > - Why the following method are public api? I don't see them used anywhere > outside TaskRepository > getUserName(String authType) > getPassword(String authType) > setCredentials(String authType, String username, String password) The patch is part of a larger set of patches that I split up to ease the review process. Please see the sibling bugs and parent bug of this report for details.
(In reply to comment #8) > > - @deprecated should have some description what to use instead > IMHO, the deprecated fields shouldn't have been API in the first place. The > alternative is to not use them. So, that is what you should put into the deprecation text > > - values for standard properties (including should "save password") should > came > > from single place in IRepositoryConstants and not from constants in > > TaskRepository class > Sounds like a good idea. Please file a bug report for that. I am really surprised with such response. My comments are related to the api changes in a scope of the currently *open* bug. > > - Why the following method are public api? I don't see them used anywhere > > outside TaskRepository > > getUserName(String authType) > > getPassword(String authType) > > setCredentials(String authType, String username, String password) > The patch is part of a larger set of patches that I split up to ease the review > process. Please see the sibling bugs and parent bug of this report for details. I don't know about other patches, but for example there are 3 shortcuts to the getUserName() method. Are you planning to expose non standard authTypes or what?
(In reply to comment #9) > (In reply to comment #8) > > > - @deprecated should have some description what to use instead > > IMHO, the deprecated fields shouldn't have been API in the first place. The > > alternative is to not use them. > > So, that is what you should put into the deprecation text Okay. > I am really surprised with such response. My comments are related to the api > changes in a scope of the currently *open* bug. Sorry, I didn't realize IRepositoryConstants exists already. I thought you were suggesting a larger refactoring to extract all default values from TaskRepository. Currently the only default value defined in IRepositoryConstants seems to be the kind whereas other defaults such as time zone or charset are defined in TaskRepository which is the example I followed in the patch. > > > - Why the following method are public api? I don't see them used anywhere > > > outside TaskRepository > > > getUserName(String authType) > > > getPassword(String authType) > > > setCredentials(String authType, String username, String password) > > The patch is part of a larger set of patches that I split up to ease the > review > > process. Please see the sibling bugs and parent bug of this report for > details. > > I don't know about other patches, but for example there are 3 shortcuts to the > getUserName() method. Are you planning to expose non standard authTypes or what? Not sure what you mean. I want to avoid code duplication that is why I added these methods. I would like to remove other methods to retrieve credentials for 3.0.
(In reply to comment #10) > Not sure what you mean. I want to avoid code duplication that is why I added > these methods. I would like to remove other methods to retrieve credentials for > 3.0. -1. It is basically issue about arbitrary attributes vs. strongly typed access. While I am usually in a favor of arbitrary attributes, in this case I prefer strongly typed methods, because this stuff is stored into the keyring and it is better to control what user can store in there.
Not sure if you understand the new API. It only exposes the key prefix, nothing else.
(In reply to comment #12) > Not sure if you understand the new API. It only exposes the key prefix, nothing > else. Exactly. It should be exposing Enum and not String-based prefixes, or leave the current methods alone. I'd suggest to keep the current methods, because this change don't bring anything besides pure aestetic, but it will affect integrators.
(In reply to comment #13) > Exactly. It should be exposing Enum and not String-based prefixes, or leave the > current methods alone. I'd suggest to keep the current methods, because this > change don't bring anything besides pure aestetic, but it will affect > integrators. The reason I did not choose an enum type is that it can not be extended by integrators and it will keep us from adding new authentication types (e.g. for socks proxies) without breaking backwards compatibility. The API is used by TaskRepositoryLocationUi for storing and retrieving credentials using the string prefix which is provided by the caller (connector). If we know for certain that no additional authentication types will be added in the future and that connectors will never require more than the three provided types DEFAULT, HTTP and PROXY I'll consider making the authentication type an enum.
(In reply to comment #14) > The reason I did not choose an enum type is that it can not be extended by > integrators and it will keep us from adding new authentication types (e.g. for > socks proxies) without breaking backwards compatibility. Is there a use case or driver for extending that? socks proxy is a bad example because I don't think that someone would have both socks and http proxy, so that setting is meant to be saved under proxy password for any proxy type. > The API is used by TaskRepositoryLocationUi for storing and retrieving > credentials using the string prefix which is provided by the caller (connector). > If we know for certain that no additional authentication types will be added in > the future and that connectors will never require more than the three provided > types DEFAULT, HTTP and PROXY I'll consider making the authentication type an > enum. See my previous comment about changing api without new use cases.
Another point is that the TaskRepository API is String based, allows access to arbitrary attributes and the new methods are consistent with that. Rob, Mik, other opinions about restricing the authentication types to the ones currently supported by using an enum type?
(In reply to comment #16) > Another point is that the TaskRepository API is String based, allows access to > arbitrary attributes and the new methods are consistent with that. That api is ok because it is not storing stuff to the keyring.
Created attachment 81542 [details] use enum for make authentication
Created attachment 81543 [details] mylyn/context/zip
(In reply to comment #18) > Created an attachment (id=81542) > use enum for make authentication Steffen, once again, do you have some use case for which existing api is not sufficient? What is the reason to change it besides pure aestetics?
(In reply to comment #20) > (In reply to comment #18) > > Created an attachment (id=81542) [details] > > use enum for make authentication > > Steffen, once again, do you have some use case for which existing api is not > sufficient? What is the reason to change it besides pure aestetics? The current API is inconsistent, duplicates code, difficult to extend, has race conditions and not easy to test. My patches reduces the number of methods on TaskRepository significantly, keeps the functionality and addresses the issues mentioned in the previous sentence. See bug 200634 for a use case.
(In reply to comment #21) > The current API is inconsistent, duplicates code, difficult to extend, I still don't hear about need to extend. However I do see that your changes are about to break support for non-web connectors (including mail and rmi-based connectors) and I think that web credentials should not be part of the TaskRepository api. > has race conditions and not easy to test would you please elaborate about the race conditions? also I don't see how changing methods that store stuff into the same place is easier to test. can you please explain? > My patches reduces the number of methods on TaskRepository significantly, > keeps the functionality and addresses the issues mentioned in the previous sentence. strangely it feels like number of methods just doubled and I still don't see how racing conditions are addressed > See bug 200634 for a use case. Unless I missed something, that bug don't add new auth types, so old methods should be sufficient.
(In reply to comment #22) > > The current API is inconsistent, duplicates code, difficult to extend, > > I still don't hear about need to extend. The API was extended to set a flag wether the password should be stored. > However I do see that your changes are > about to break support for non-web connectors (including mail and rmi-based > connectors) and I think that web credentials should not be part of the > TaskRepository api. Credential handling is part of the TaskRepository API. The naming is not perfect but consistent with other classes in the web core plugin. Sharing the credentials classes between the web core plug-in and tasks framework > > has race conditions and not easy to test > > would you please elaborate about the race conditions? Credentials can not be retrieved as a consistent pair due to getUserName() and getPassword() being two separate methods. > also I don't see how > changing methods that store stuff into the same place is easier to test. can you > please explain? Instead of duplicating test cases three times for exactly the same logic (storing a username/password) pair the test logic can now be implemented once. > strangely it feels like number of methods just doubled and I still don't see how > racing conditions are addressed I haven't addressed all race conditions in TaskRepository, yet, but that should be done at some point. I added a comment to the methods that I plan to deprecate and then remove. Eugene, what is the point of this discussion?
(In reply to comment #23) > > However I do see that your changes are > > about to break support for non-web connectors (including mail and rmi-based > > connectors) and I think that web credentials should not be part of the > > TaskRepository api. > > Credential handling is part of the TaskRepository API. The naming is not perfect > but consistent with other classes in the web core plugin. Sharing the > credentials classes between the web core plug-in and tasks framework It shouldn't be, because it is specific only to http stuff. So, I'd say turn it other way around - create factory the would make credential instance from the TaskRepository, but don't couple those two together. > Instead of duplicating test cases three times for exactly the same logic > (storing a username/password) pair the test logic can now be implemented once. Can't you just write a selector method in your test case that will encapsulate logic for calling the right metods? > Eugene, what is the point of this discussion? I still don't getting the point in these api changes. A smaller api stuff been kicked out in a past... Also I do believe that getUserName() is less obscure then getAuthSomething(someEnumType), besides, it is a valid use case to allow to set user an password independent from each other (i.e. set the user name in repo settings and only enter password upon connect).
I deem that the API changes proposed in the patches are sensible for reasons outlined in previous comments. I am open to discuss better approaches but I will wait for other opinions before continuing this discussion. (In reply to comment #24) > It shouldn't be, because it is specific only to http stuff. So, I'd say turn it > other way around - create factory the would make credential instance from the > TaskRepository, but don't couple those two together. That sounds like a more generic approach that could be worth investigating. So far I have not seen a need for more than username/password credentials but there could certainly be other use-cases. Please file a bug report and provide more details about the proposed API so we can consider this for 3.0. > > Eugene, what is the point of this discussion? > > I still don't getting the point in these api changes. A smaller api stuff been > kicked out in a past... Thank you for clarifying. I understand your frustration. What I don't understand is how these endless arguments help the project or you.
Created attachment 81553 [details] use enum for authentication type The patch uses an enum to determine the authentication type. My assumption that additional enum constants can not be added without breaking binary compatibility was not correct. This can be extended on the framework level in the future but now restricts integrators to the pre-defined authentication types.
Created attachment 81554 [details] mylyn/context/zip
(In reply to comment #25) > That sounds like a more generic approach that could be worth investigating. So > far I have not seen a need for more than username/password credentials but there > could certainly be other use-cases. Please file a bug report and provide more > details about the proposed API so we can consider this for 3.0. The api is already in. Please don't break the use cases that api support.
(In reply to comment #24) > I still don't getting the point in these api changes. A smaller api stuff been kicked out in a past... I understand your frustration with bug 199818, but we have dedicated a large amount of discussion time to figuring that out and its simply not a straightforward change. I think this one was quite straightforward even though it turned out involved. Frankly I'm not sure if this feature was important enough but we took it seriously because it is security related. If there are other places where there were straightforward API changes that should have been applied before this one please email me with them so that I can review our prioritization process.
Mik, my objection was not to the feature but to the way it been implemented. From the patch I've seen that api changes wasn't needed and functionality could have been implmented on the old api. Also please pay some attention about my comment on breaking support of the non http-based connectors.
(In reply to comment #30) > Mik, my objection was not to the feature but to the way it been implemented. > From the patch I've seen that api changes wasn't needed and functionality could > have been implmented on the old api. Implementing new API early and deprecating old API will give integrators more time to migrate. > Also please pay some attention about my comment on breaking support of the non > http-based connectors. Could you please post a test case that demonstrates how the new API breaks non-http based connectors?
Committed slightly modified patch to CVS.
Guys, why no one responded to my concerns about coupling TaskRepository class with web-specific stuff? Namely the WebCredentials class, which is imply something related to "web". I haven't looked at the code in deep details, and at least this credential class is purely named (should have no "web" in it). Also, while you are on those changes, and actually changing the public api, it would make sense to not stop half way and make auth extendable, for example, take into the account use cases when authorization not need any user name/password, but require a private key to be specified.
I chose the name WebCredentials to avoid a name clash with the Credentials class that is part of HttpClient and often used in the same classes. Are there any other suggestions for naming that class? Supporting certificate authentication has certainly come up as a use-case. It is out of scope of this bug report to provide an extensive authentication API and not a priority for 2.2. Still I want to make sure that the changes made to the authentication API are stable and enable extendability for future releases. Eugene, it would be very helpful if you opened a new bug report and listed your requirements so we can ensure that the current API meet those requirements before it is frozen for 2.2.
Let's discuss the remaining issues on bug 208935.
