Bugzilla – Full Text Bug Listing
| Summary: | [transport] cancelling authentication during Help>Check for Updates... cancels the update check | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [Eclipse Project] Equinox | Reporter: | Susan McCourt <susan> | ||||||||
| Component: | p2 | Assignee: | Henrik Lindberg <henrik.lindberg> | ||||||||
| Status: | RESOLVED FIXED | QA Contact: | |||||||||
| Severity: | normal | ||||||||||
| Priority: | P3 | CC: | henrik.lindberg, john.arthorne, pascal | ||||||||
| Version: | 3.5 | Flags: | susan:
review+
|
||||||||
| Target Milestone: | 3.5 RC1 | ||||||||||
| Hardware: | PC | ||||||||||
| OS: | Windows XP | ||||||||||
| Whiteboard: | |||||||||||
| Attachments: |
|
||||||||||
|
Description
Susan McCourt
marking RC1 for investigation The workflows are different in 3.4, but I verified that checking for updates from the installed software page will abort if the user cancels an authentication prompt. I think I'm mostly concerned that we fixed this in the M6/M7 timeframe and reintroduced it. I tried an experimental patch whereby the QueryableRepositoryManager catches OperationCanceledException and ignores it, and keeps loading other repos. This allows the update check to continue, but then there are three more authentication prompts before the final "nothing to update" or update wizard comes up. Is this any better? John, Pascal, Henrik - do you recall any finding/fixing of this bug recently or am I hallucinating? I could have sworn we found this when Matt was testing authentication but I was not able to find a bug about it. Talked to Henrik about this. He does not think this is a regression. He proposes that a canceled login not throw OperationCanceledException. Instead, a canceled login would fail in the same way as if the user had done the three failed attempts. This way, the client code would continue. We would need to make sure that a future authentication request would indicate a previous cancel rather than a failed attempt. here is the IRC chat: helindbe> well, the cancel of the Auth dialog would then behave as a failure to authenticate <helindbe> just like if user tried logging in 3 times and faild <helindbe> so cancel means "cancel my three attempts of logging in" <helindbe> no one would see that as a user of say "load repo" <helindbe> the only side effect is that if you expect the cancel of login to cancel the entire operation - because that is how it worked before... <helindbe> and now it doesn't, and you don't know how to stop the job.... <helindbe> so - fix is not difficult - if the altered behaviour is wanted... ..... <susanmccourt_> helindbe: don't you have a login prompt that says something to effect that previous authentication attempts wer enot correct? <susanmccourt_> in the case where all the user did was cancel, we wouldn't want to indicate that previous ateempts had failed <susanmccourt_> the string in the login prompt <helindbe> ah - that place <helindbe> no, that is not possible - since the last attempt is not remembered <helindbe> it only knows about the difference between "a saved password did not work" and "don't know the password" This sounds like a reasonable approach, Henrik is going to prepare a patch and we can assess the risk/reward. Created attachment 134716 [details]
patch that fixes the problem
The patch fixes the problem by throwing a different exception. The previous choice of "UserCancelledException" was perhaps not a good choice, it is an ECF exception, and really means that the communication operation was canceled. Instead, a new Exception Credentials.LoginCanceledException is thrown. This exception is caught by RepositoryTransport and it treats this the same way as if the user maxed out on number of login attempts. In the case of a file download, the message in the final status is different if user canceled.
The other two cases "getLastModified" and "stream" (used when getting the mirrors list) just throw the same exception as when number of attempts has been exceeded.
A small note, this patch also includes a few lines of change regarding the handling of CoreException that comes from the change in ECF. - will create a new patch when a decision has been reached.
*** Bug 275227 has been marked as a duplicate of this bug. *** *** Bug 273664 has been marked as a duplicate of this bug. *** Created attachment 134717 [details]
updated patch - removed already released lines
clean patch.
I wanted to try the patch, but it's incomplete. I'm getting compile errors in RepositoryTransport since it doesn't catch LoginCanceledException. There must be some corresponding code in that class that is missing from the patch? Created attachment 134763 [details]
updated path - this time with both classes, and messages
Sorry about the incomplete patch. This is not the first time I got bitten by 'Team > Create Patch' when having selected more than one file - it only marks the first of them to be included when patching... (Should probably report this in a bz - I think it sucks)...
Anyway - I attached a new patch.
Should a review be requested here? Patch looks good. I checked the code paths that catch the authentication failure, and in those cases, there will be "authentication failure" notices added to the log, even in the case where the user cancels. Of course, this is the intention of the fix...some error must be logged that allows the original operation to continue. In the long run, I think we want to differentiate between "login attempts failed" and "user canceled login" even in those code paths, but for this point in the release I think it's a good compromise. I verified that the original problem reported here is fixed (the update check continues), and that the problem reported in bug 275227 is fixed (cancelling a login while resolving in the wizard will let the wizard keep going.) Note: Because we are not remembering and suppressing failed authentication (see discussion in bug 274447), the update check sequence will now prompt the user twice for each repo. The first time is when it's checking for updates, and the second time is when it is resolving the updates. We can do better here, obviously, but that will be revisited in terms of bug 274447. I opened bug 275690 to track a UI-side fix, since there are places where we know we could prevent multiple prompts. released to HEAD >20090511. |