Community
Participate
Working Groups
When connecting to an http site it is possible to get prompted for a login and a pwd before being able to proceed. We need to hook into ECF to receive the proper callbacks and ask the information to the user. One thing that is particularly interesting here is the user interaction model since we don't want the user to be prompted when the download occurs in the background (for example we could just move on). It may also be interesting to store the login/pwd locally.
Created attachment 91766 [details] Authentication v01 Patch changes: +Added a service for calling a UI dialog to get login details. +Changed ECFMetadataTransport to throw a ProtocolException if there is an authentication error. +SimpleMetadataRepositoryFactory uses the service to prompt for login details. It reprompts if the login details failed and cancels the validateAndLoad() operation if the login dialog was canceled. The patch still needs some work to ensure the prompt is shown if the user tries to reconnect after canceling the login prompt, the artifact repository doesn't have a login prompt, and the prompt may occur during a silent install.
Created attachment 92038 [details] Authentication v02 Patch changes: +Added automatic saving of the login details using a secure preference store. On Windows, the store is saved to the user's home directory (I believe it's the home directory on Linux and Mac as well). TODO: +Test a silent install. +Convert ECFTransport so that artifact repository has a login prompt.
Created attachment 92927 [details] Authentication v03 Patch changes: +User is prompted whenever necessary, even during silent install. +Added checkbox to the prompt to allow the user to specify whether the login details should be saved or not. The details are still saved in a secure preference store in the user's home directory.
Created attachment 94556 [details] Updated patch Review still in progress. This patch has some minor tweaks on previous patch.
When I click cancel on the login dialog, I get errors like the following logged. The transport classes should instead convert UserCancelledException into the runtime OperationCanceledException. This will then be caught by the UI and handled as a cancelation (which is different from an error, and doesn't need logging): org.eclipse.equinox.internal.provisional.p2.core.ProvisionException: The repository could not be read: http://arthorne.com. at org.eclipse.equinox.internal.provisional.spi.p2.artifact.repository.SimpleArtifactRepositoryFactory.load(SimpleArtifactRepositoryFactory.java:87) at org.eclipse.equinox.internal.p2.artifact.repository.ArtifactRepositoryManager.loadRepository(ArtifactRepositoryManager.java:357) at org.eclipse.equinox.internal.p2.artifact.repository.ArtifactRepositoryManager.loadRepository(ArtifactRepositoryManager.java:338) at org.eclipse.equinox.internal.provisional.p2.ui.operations.ProvisioningUtil.loadArtifactRepository(ProvisioningUtil.java:107) at org.eclipse.equinox.internal.provisional.p2.ui.model.ArtifactRepositoryElement.getRepository(ArtifactRepositoryElement.java:79) at org.eclipse.equinox.internal.provisional.p2.ui.model.ArtifactRepositoryElement.fetchChildren(ArtifactRepositoryElement.java:57) at org.eclipse.equinox.internal.provisional.p2.ui.model.ArtifactRepositoryElement.fetchDeferredChildren(ArtifactRepositoryElement.java:95) at org.eclipse.ui.progress.DeferredTreeContentManager$1.run(DeferredTreeContentManager.java:234) at org.eclipse.core.internal.jobs.Worker.run(Worker.java:55) Caused by: org.eclipse.ecf.filetransfer.UserCancelledException: at org.eclipse.equinox.internal.p2.artifact.repository.ECFTransport.setLogin(ECFTransport.java:200) at org.eclipse.equinox.internal.provisional.spi.p2.artifact.repository.SimpleArtifactRepositoryFactory.transfer(SimpleArtifactRepositoryFactory.java:146) at org.eclipse.equinox.internal.provisional.spi.p2.artifact.repository.SimpleArtifactRepositoryFactory.getLocalFile(SimpleArtifactRepositoryFactory.java:108) at org.eclipse.equinox.internal.provisional.spi.p2.artifact.repository.SimpleArtifactRepositoryFactory.load(SimpleArtifactRepositoryFactory.java:49)
Reviewing the code some more, I don't like the fact that everyone calling Transport class methods have to write the while loop that catches the authentication error and retries. I would like to see this pushed entirely into the transport class, so callers don't have to deal with this complexity and we localize the logic for handling authentication in one place. I.e., the caller would just invoke Transport#download and the transport class would handle authentication, retries on password prompt, etc, and return the eventual result (success, fail, or cancel). Ideally we will eventually merge the two transport classes and we will be left with one place handling this code. Right now, if we wanted to switch to a limited number of retries on the password prompt, we have to change all the callers to deal with this. Other issues might come up around this code and it would be code to only have to make fixes in one place.
FYI, I'm currently hacking on this patch. I'll attach an update soon.
Created attachment 94776 [details] Updated patch Folded the rety logic into the transport classes. The net result is almost no code change in the repository classes (just one change to pass in location as URL rather than string).
The only outstanding issue I have with this patch is that the prompt UI only seems to be enabled for the admin UI. We really need this for the end-user UI. Susan can correct me if I'm wrong, but I think the UI bits should go in org.eclipse.equinox.p2.ui so they are available in both admin UI and end-user UI. It would also be good to do a little more testing with my latest patch on an authenticated repository.
>Susan can correct me if I'm wrong, but I think the UI bits should go in >org.eclipse.equinox.p2.ui so they are available in both admin UI and end-user >UI. It would also be good to do a little more testing with my latest patch on >an authenticated repository. Yes, I think you want the UserValidationDialog to live in org.eclipse.equinox.internal.provisional.p2.ui.dialogs. The IServiceUI implementation (currently called AdminServiceUI) should probably be renamed to ValidationDialogServiceUI and moved to the same package. If we wish this dialog to be used in both UI's (which I think we do), then both ProvAdminUIActivator and ProvSDKUIActivator should register an instance of ValidationDialogServiceUI.
Tim, can you make this remaining fix and I'll release.
Created attachment 95208 [details] Authentication Patch changes: +Moved all the code to p2.ui from p2.ui.admin +Registered the service with ProvUIActivator Does the ProvAdminUIActivator have to register the service as well? If it requires p2.ui it should have the service registered by ProvUIActivator. I tested it and it works either way. The patch includes the service registration in ProvAdminUIActivator as well.
>Does the ProvAdminUIActivator have to register the service as well? If it >requires p2.ui it should have the service registered by ProvUIActivator. I think ProvAdminUIActivator and Prov*SDK*UIActivator should register those services. I think the app should decide how to present the authentication. If ProvUIActivator does it, then it's made the decision for all apps using it....
Created attachment 95395 [details] Authentication Patch changes: +Renamed the AdminServiceUI to ValidationDialogServiceUI This patch is ready for review.
Created attachment 95397 [details] Authentication Gargh, I attached the old patch by accident.
Thanks for all the work on this Tim. Patch released.
>I think ProvAdminUIActivator and Prov*SDK*UIActivator should register those >services. I think the app should decide how to present the authentication. If >ProvUIActivator does it, then it's made the decision for all apps using it.... Was there a conscious decision not to do this (we want to ensure the service is always registered by any UI) or just oversight?
(In reply to comment #17) > >I think ProvAdminUIActivator and Prov*SDK*UIActivator should register those > >services. I think the app should decide how to present the authentication. If > >ProvUIActivator does it, then it's made the decision for all apps using it.... > > Was there a conscious decision not to do this (we want to ensure the service is > always registered by any UI) or just oversight? > Oh yes, that was an oversight. Unless there's some reason for not registering the service there, I'll provide a patch for it.
Reopening to address remaining issue raised by Susan.
Created attachment 95563 [details] Authentication Patch changes: +Registered the ValidationDialogServiceUI with ProvSDKUIActivator. +Renamed the IServiceUI field names in ProvAdminUIActivator and ProvUIActivator to not use Admin.
Thanks, patch released. I actually removed the fields entirely from the two activator classes because they are unused.
Okay, I am not explaining myself very well in this bug. My issue was that ProvUIActivator should *not* register the service because it is more of a "class library" plug-in and doesn't make assumptions as to what UI should be used (even though it provides an implementation). Therefore ProvSDKUIActivator and ProvAdminUIActivator *should* register the service. Another way to put it - ProvUIActivator could do it for everyone (and no one else need register) or else it should not do it at all (and then all apps must do it). But we don't need all three to do it. I removed the registration from ProvUIActivator.
My fault - I reviewed what was in the patch, but didn't review what wasn't in the patch (removing registration from UI bundle). Your point was clear, I was rushing through applying patches and should have looked more carefully.
