Bugzilla – Full Text Bug Listing
| Summary: | [repository] Cannot update from update site with authentication | ||||||
|---|---|---|---|---|---|---|---|
| Product: | [Eclipse Project] Equinox | Reporter: | Cédric Chabanois <cchabanois> | ||||
| Component: | p2 | Assignee: | P2 Inbox <equinox.p2-inbox> | ||||
| Status: | RESOLVED FIXED | QA Contact: | |||||
| Severity: | normal | ||||||
| Priority: | P3 | CC: | carlos, eckart.langhuth, henrik.lindberg, john.arthorne, kim.moir, pascal, t-oberlies | ||||
| Version: | 3.7.1 | ||||||
| Target Milestone: | 3.7.2 | ||||||
| Hardware: | All | ||||||
| OS: | All | ||||||
| Whiteboard: | |||||||
| Bug Depends on: | |||||||
| Bug Blocks: | 385894 | ||||||
| Attachments: |
|
||||||
|
Description
Cédric Chabanois
Move all 3.8 bugs to Juno. (In reply to comment #1) > Move all 3.8 bugs to Juno. As this bug severely effects the usage of any p2 repository requiring authorization you may consider to provide a fix with Indigo SR 2 release. I have also noticed that it depends on which of PreLoadingRepositoryHandler$2 or DeferredTreeContentManager$1 starts first and calls getUsernamePassword(). If you are *quick* the DeferredTreeContentManager$1 job calls getUsernamePassword() first and it works (since it doesn't have SUPPRESS_AUTHENTICATION_JOB_MARKER set). If you delay after opening the combo box then the background PreLoadingRepositoryHandler$2 job runs, calls getUsernamePassword() and as the submitter says returns null and p2 assumes this means "User clicked cancel" and the update from a site with authentication is then broken. We need a fix for this ASAP. I can reproduce this on Linux and Windows (it has nothing to do with the platform). @Matthiew: You are the assignee of this issue. Does this mean you are working on this issue? IMHO the root of the problem is that ValidationDialogServiceUI cannot fulfill the contract of UIServices.getUsernamePassword. The method does *not* allow the option of not showing the authentication dialog: "Opens a UI prompt for authentication details". The implementation in ValidationDialogServiceUI returns null in this case, which is the same as if the dialog was shown, and the user hit cancel. The Credentials class calling UIServices.getUsernamePassword relies on the semantic null<=>"user hit cancel" and this leads to the behaviour described in this bug. I see two options to fix this problem: 1) "Refine" (=break) the definition of UIServices.getUsernamePassword to define null as either "user canceled" or "dialog not shown", and add a boolean method canPrompt in UIServices to distinguish the cases. 2) Bite the bullet and introduce an new API, e.g. UIServices2, which defines a method that supports the prompt suppressed use case. Any opinions/preferences? I would like to get the fix into SR2, which really only leaves option 2. I vote for Option2, it makes the most sense and gives us a way forward. I have prepared two commits for the second option, and since this is introducing new API, I would like another p2 committer to review them. The first commit [1] fixes the problem by introducing an API extension of UIServices (called UIServices2) with contains a boolean promptEnabled() method. Credentials makes use of the API extension (if available), and ValidationDialogServiceUI implements UIServices2. I would like to also down-port this commit to 3.7.2. The second commit [2] fixes what lead to the problem: unclear JavaDoc in UIServices and an ValidationDialogServiceUI which doesn't stick to the documented API contracts. This changes the behaviour of ValidationDialogServiceUI (whereas the first commit doesn't), and therefore I would do this change in 3.8 M5. [1] https://github.com/oberlies/rt.equinox.p2/commit/9006d8807d5fab62fbb91a24ee10ec307b2ebe96 [2] https://github.com/oberlies/rt.equinox.p2/commit/d5c7c5317ec3725e2de580149df03a2382c9a124 Sorry, I must have tested the wrong thing. I have uploaded two fixed commits to my github fork. The new first commit is https://github.com/oberlies/rt.equinox.p2/commit/717ce34b8a076f6da83a61f3aee06f466959f9e9 The new second commit is https://github.com/oberlies/rt.equinox.p2/commit/533c39a5f9676bf4400cf8d233e287a47c9f39ee Created attachment 208917 [details] New bugfix proposal in format for @git am@ This patch makes use of the fact that the return value of UIServices#getUsernamePassword is underspecified, and introduces a new constant that _may_ be returned by implementations to explicitly indicate that the user cancelled the dialog. The main implementation (ValidationDialogServiceUI) obviously does that, and the only non-test caller (Credentials) only remembers cancel if the explicit constant for cancel is returned. I still would appreciate a review, but if I don't get any feedback, I'll integrate the change into 3.8 and 3.7.x early next week. The proposed patch is also available here: https://github.com/oberlies/rt.equinox.p2/tree/bug348366_alt3 (In reply to comment #8) > I still would appreciate a review, but if I don't get any feedback, I'll > integrate the change into 3.8 and 3.7.x early next week. > > The proposed patch is also available here: > https://github.com/oberlies/rt.equinox.p2/tree/bug348366_alt3 I looked at the diff, and the change looks reasonable given that is how it communicates a cancel for the UI. (Did not try applying it and running). Fixed in master with c56d308 [1]. Back-port to Indigo SR2 is still pending. [1] http://git.eclipse.org/c/equinox/rt.equinox.p2.git/commit/?id=c56d3086f43989513d7e3d2a149a0b6b1f82e9a7 The final fix looks great. I hope nobody has an https configuration wtih username="" and password="" :-) Please keep us updated on the backport. (In reply to comment #11) > I hope nobody has an https configuration wtih username="" and password="" :-) Are you referring to this line in Credentials.java? @if (loginDetails == UIServices.AUTHENTICATION_PROMPT_CANCELED) {@ The check is for identity and not equality, so this should work as intended. @Kim: I hope you are the right person to ask about this. I want to port the fix for this bug from master [1] back to the 3.7.2. This implies, that I cannot increment the minor version number of org.eclipse.equinox.p2.core. However this leads to API tool errors (see [2] for details), because I created a new public constant in the API class UIServices. Now my question is: Is it possible to define an API tool exception for this addition of the constant? What would be the correct @since annotation in this case? If this is not possible, I could also work around the API change, but this would mean developing a different fix for the R37x branch. [1] http://git.eclipse.org/c/equinox/rt.equinox.p2.git/commit/?id=c56d3086f43989513d7e3d2a149a0b6b1f82e9a7 [2] https://github.com/oberlies/rt.equinox.p2/commit/783fe951cf054fd3fd681add05e711194cac9d50 I don't know if I'm the right person to ask, someone on the pde api tools team might be better. The fix from master has been back-ported to R3_7_maintenance as commit b25110d [1]. I have worked around the API tools error through an additional commit only in R3_7_maintenance [2]. @p2 committers: I am not proud of these changes. If you have a better fix, feel free to apply it. In particular you may want to check if you find it acceptable how the changes "bend" the underspecified API of UIServices. Unfortunately, I'll be offline until SR2/M5, so in case of a problem please take care of it (and my apologies for any hassle.) [1] http://git.eclipse.org/c/equinox/rt.equinox.p2.git/commit/?id=b25110d66c3fee4132d6357c19e9858d0dd59790 [2] http://git.eclipse.org/c/equinox/rt.equinox.p2.git/commit/?h=R3_7_maintenance&id=78e1fa6669af8e6b9dff994d4670856830d91899 |