Community
Participate
Working Groups
Some parts of the password management lifecicle might or might not make sense for a particular password provider. In particular: - For OS integration modules "change password" functionality has little meaning and might be better replaced with "setup password recovery" - For Windows module in particular "Logout/Clear password cache" is meaningless as it re-aquires password automatically as long as user logged in. To provide more meaningful UI we could add hints for providers to specify in the plugin.xml. On the extension point schema side (org.eclipse.equinox.security.secureStorage) this will translate into an additional optional attribute to specify an open-ended set of hints. An open question would be what exactly hints we might need. For now it seems like a single "AutomaticPasswordGeneration" would do. (On the implemenation side we can make xml reader to quetly ignore unknow hints.)
Do you have a patch in hand for this? If so, please attach to the bug.
The PMC has been informed about this API change request. We need to decide if this should go into 3.4 or not. Marking for 3.4 RC1.
Oleg, will you be providing a patch for the PMC to review? Even if its not approved we'll have the patch for 3.4.1 or 3.5.
On a second thought, there are scenarios for OS integration password providers where password change might be necessary. For Windows: - If Windows is a standalone workstation and it was re-installed without the creation of a "Password reset disk" - Secure storage "master" password can be re-covered using "Password recovery" but that process would have to be repeated in every session - To return to the automatic silent behaviour password woudl have to be changed after it has been recovered For Mac the scenario would be similar but triggred by the "master" password deleted from the Mac OS keyring. Those would not be a typical everyday scenarios but they certainly can happen and in this situation changing password would allow issue to be easilfy fixed, even if the UI is not the most esthetic. The alternative would be to delete and re-create secure storage. As such the UI difference in having an "AutomaticPasswordGeneration" hint would be not to show a wizard but do all the work silently and only display a result message box. The button "Change password" should remain available.
(In reply to comment #3) > Oleg, will you be providing a patch for the PMC to review? Even if its not > approved we'll have the patch for 3.4.1 or 3.5. There is no patch ready or in the works at this moment - so you are certainly welcome to make one, it won't interfere with other changes.
Created attachment 99377 [details] Patch I - non API changes At the Equinox meeting we discussed this and decided to split change into two portions: - Part I: does everything but does not change the schema - Part II: change to the schema As PDE does not flag "extra" attributes as errors we can modify our OS integration providers to supply the "passwords are auto-generated" hint. But in the Part I we don't change the schema so no API change. A Part II will be a small separate patch and likely will go into a separate bug (together with the schema change for the bug 230234) for PMC review and might be included in 3.4 if approved or in 3.5.
Created attachment 99378 [details] Patch II - schema change
Adding Kim to CC as the change affects Mac integration module.
(In reply to comment #6) > Created an attachment (id=99377) [details] > Patch I - non API changes > > Just a small comment about this patch - line 167 of PasswordProviderSelector appears to have an unwanted comment.
Tom, could you review the Patch I (non-API)?
Chris, is the PDE behavior in comment 6 intended and can we depend on it?
(In reply to comment #11) > Chris, is the PDE behavior in comment 6 intended and can we depend on it? Without looking at this deeply, PDE should flag invalid children elements. For example, you'll see something like this from PDE... Description Resource Path Location Type Element 'hint' is not legal as a child of element 'page'. plugin.xml org.eclipse.pde.ui line 36 Plug-in Problem If it's not flagging it, it's probably a bug on our end, we try to make sure things are as valid as possible based on the schema. How about using the ':" data stuffing approach in someway? <provider class="org.eclipse.equinox.internal.security.win32.WinCrypto:hint=AutomaticPasswordGeneration" priority="5"> </provider>
Hmm then we can't rely on separating this change into API and non-API parts. I'll provide a merged patch for this bug and the patch for the bug 230234 as it affects about the same areas. Once that is done we'll have to see if PMCs will be willing to approve it for 3.4.
Created attachment 99482 [details] Combined patch This is a combined patch for this bug and the bug 230234 (Add "description" text to password providers). The change in code is straightforward. The API change is simple and backward compatible: - new optional attribute "description" is added to the password providers schema - new optional list of hints added to the password providers schema with one hint "AutomaticPasswordGeneration" available at this time In terms of future expandability, new hints can be added in future in a backward-compatible way to provide more streamlined UI integration.
(In reply to comment #12) > If it's not flagging it, it's probably a bug on our end, we try to make sure > things are as valid as possible based on the schema. It is flagged as a warning by default. When the schema is modified in the workspace you have to manually force a rebuild of the extension bundles to get the warning to show up. Changing the schema does not not seem to trigger a rebuild/revalidation of the plugin.xmls( Question: can we live with the warning in 3.4 for our fragments and get by with no API change in the schema?
From the patch... - public PasswordProviderDescription(String name, String id, int priority) { + public PasswordProviderDescription(String name, String id, int priority, String description, List hints) { ... I take it that this is internal, and we are confident that no one would be interested in calling a version that did not pass in the description and hints. I.e. it's ok to delete the old method (rather than leave it and have it call the new one with a null description and hint). Other than that, the patch looks reasonable assuming it's driving the security support properly, which I did not verify.
Yes, that class is from the package: org.eclipse.equinox.internal.security.storage.friends which is marked as x-friends:="org.eclipse.equinox.security.ui" in the manifest. This package is used to provide some functionality to the equinox.security.ui bundle that is not desirable to be exposed to the rest of the world as API. This is an internal class; I am not aware of anybody (aside from equinox.security.ui) using it at this time. Re comment 15: it is a possibility, but let's see if we can get an approval - it will be simpler in a long run.
(In reply to comment #15) > Question: can we live with the warning in 3.4 for our fragments and get by with > no API change in the schema? I have to ask: Are we just trying to avoid the PMC approval process here? Either we're confident that the schema extension is of the right form, or we are not. 1) Maybe we're not and we'd like to mark it the equivalent of provisional. In which case we should use Chris' data stuffing approach (I have to say, I love that name <g>), which from what I gather will not trigger a warning (right Chris?). It'll just be our little "in way" of providing the info until we're confident of the right schema form. 2) If we *are* confident its the right form, then it should be an added tag (as it is in the proposal), mod to the schema, and we go for API approval. Adding a tag and hoping the PDE builder doesn't flag doesn't make sense to me. Its all the work/risk of an API change without the benefit of it being a real API change (ie. a public well documented way of providing the info). I think we should just push forward with (2).
Question about the patch: Maybe I just don't understand exsd's, but in secureStorage.exsd, the <hint> element is marked "required". <element name="hint"> <complexType> <attribute name="value" use="required"> I assume this means you need the <hint> element (although it can be empty of hints). Shouldn't <hint> be optional? If there are no hints then we default to some reasonable behaviour. After all, they are "hints".
(In reply to comment #19) > Question about the patch: > Maybe I just don't understand exsd's, but in secureStorage.exsd, the <hint> > element is marked "required". > <element name="hint"> > <complexType> > <attribute name="value" use="required"> > I assume this means you need the <hint> element (although it can be empty of > hints). Shouldn't <hint> be optional? If there are no hints then we default to > some reasonable behaviour. After all, they are "hints". That notation means that "value" is a required attribute of a hint. Meaning, that if you did specify a hint, it should have a value. However, the number of hints can be zero: <element name="provider"> <complexType> <sequence minOccurs="0" maxOccurs="unbounded"> <element ref="hint"/> </sequence> The best way to see how it works is to apply a patch and use the PDE editor to create an extension for the org.eclipse.equinox.security.secureStorage. (By the way, if anybody knows a better way to describe it be sure to let me know.) As I said above, I'd prefer to have a complete solution (which involves schema changes) to be done for 3.4 as the changes are trivial. Doing it in parts only going to take more time and confusion in a long run.
Let's do this.
(In reply to comment #18) > (In reply to comment #15) > > Question: can we live with the warning in 3.4 for our fragments and get by with > > no API change in the schema? > > I have to ask: Are we just trying to avoid the PMC approval process here? > > Either we're confident that the schema extension is of the right form, or we > are not. > I was under the impression that we may not be confident in schema extension given the very late stage of the release. But given the amount of eyes looking at the fix perhaps this is not the case. > 1) Maybe we're not and we'd like to mark it the equivalent of provisional. In > which case we should use Chris' data stuffing approach (I have to say, I love > that name <g>), which from what I gather will not trigger a warning (right > Chris?). It'll just be our little "in way" of providing the info until we're > confident of the right schema form. > > 2) If we *are* confident its the right form, then it should be an added tag (as > it is in the proposal), mod to the schema, and we go for API approval. > > Adding a tag and hoping the PDE builder doesn't flag doesn't make sense to me. > Its all the work/risk of an API change without the benefit of it being a real > API change (ie. a public well documented way of providing the info). > > I think we should just push forward with (2). > I do agree with this, but wanted to make sure we are absolutely confident in this late API addition. Even simple API additions should not be taken lightly. But if we all agree that this is good API that we are willing to support for future releases then lets do it. (In reply to comment #20) > > That notation means that "value" is a required attribute of a hint. Meaning, > that if you did specify a hint, it should have a value. However, the number of > hints can be zero: > <element name="provider"> > <complexType> > <sequence minOccurs="0" maxOccurs="unbounded"> > <element ref="hint"/> > </sequence> > > The best way to see how it works is to apply a patch and use the PDE editor to > create an extension for the org.eclipse.equinox.security.secureStorage. > > (By the way, if anybody knows a better way to describe it be sure to let me > know.) > > As I said above, I'd prefer to have a complete solution (which involves schema > changes) to be done for 3.4 as the changes are trivial. Doing it in parts only > going to take more time and confusion in a long run. > From other examples Chris showed me I think we should use the following sequence for hint ... <sequence> <element ref="hint" minOccurs="0" maxOccurs="unbounded"/> </sequence> It is semantically the same. In DTD form the difference is (hint)* vs. (hint*) (In reply to comment #21) > Let's do this. > I agree, after the small tweak to the schema.
+1 with schema change. I've run the patch and it looks good. As am minor item I've logged bug #231354 regarding a small usability issue around the sequence when there is no wizard (= OS provider). It does not block this patch.
Created attachment 99541 [details] Combined patch with updated schema Updated combined patch to use multiplcity specification as suggested in the comment 22.
Patch applied to the CVS Head, thanks everybody! Please don't open a bug saying "Security preferences page has too many words now" :-).
*** Bug 230234 has been marked as a duplicate of this bug. ***
I looked at the new page in I20080510-2000. Nice! Yes, it is wordy, but as a user I rather like to understand where and how my passwords are stored. Security is a sensitive area. Thanks Oleg and Kevin for the work!
