Summary: | [telnet] Telnet should handle invalid user id or password more gracefully | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | [Tools] Target Management | Reporter: | Martin Oberhuber <mober.at+eclipse> | ||||||||
Component: | RSE | Assignee: | Sheldon <sheldond> | ||||||||
Status: | RESOLVED FIXED | QA Contact: | Martin Oberhuber <mober.at+eclipse> | ||||||||
Severity: | normal | ||||||||||
Priority: | P2 | Keywords: | contributed, helpwanted | ||||||||
Version: | 2.0 | ||||||||||
Target Milestone: | 2.0 | ||||||||||
Hardware: | All | ||||||||||
OS: | All | ||||||||||
Whiteboard: | |||||||||||
Bug Depends on: | 178201, 186569 | ||||||||||
Bug Blocks: | 194473 | ||||||||||
Attachments: |
|
Description
Martin Oberhuber
2007-05-11 10:45:13 EDT
Created attachment 67256 [details] patch for bug 186570 The contains the fix to handle incorrect logins more gracefully. After applying the patch, copy the icons folder from SSH plugin, because the telnet plugin is using a copy of the UserValidationDialog for prompting the user to enter login information if it fails. This dialog uses images form the icons folder, which may cause null pointer exceptions if it does not exist. When the login to telnet session times out due to multiple login attempts a SystemMessageException is thrown which currently does not pop up a dialog. The exception is written to console. Need to have a look at this Created attachment 67384 [details]
Updated patch against TM HEAD, Formatting changed only
Created attachment 67386 [details]
Updated patch without UserValidationDialog
Thanks for the patch, I reviewed it throughly.
I do not like the idea of a UserValidationDialog for the following reasons:
* A password entered in the UserValidationDialog will not be saved, so it is
not available the next time. It's better to inform the user that login
failed, so he can just try again and enter correct credentials in RSE's
dialog rather than a special local dialog. In general, Credentials should
be set and managed by the RSE framework only (if possible).
Note that SSH is slightly different here, because it has the concept of a Passphrase and the concept of Keyboard-Interactive authentication. These work with a challenge-response kind of model, where it is not possible to know in advance what kind of information is needed, so in this special case it is necessary to do a special dialog other than RSE. But for telnet, I'd like to keep the RSE dialog only.
I also slightly optimized the readUntil() method to not do indexOf() so often; and changed the String status of your connectThread to the int status you also use for readUntil().
These changes make the patch a lot simpler (95 lines of code) which I can apply without another IP Review. I'm applying the modified patch as attached.
On taking a second look, I also removed the "Telnet Only" systemType from your patch, since it is already defined in org.eclipse.rse.subsystems.shells.telnet This leaves the current patch with two modifications: - Connect done in a separate thread (that's good so it can be interrupted) - readUntil returns FAIL when pattern "closed" or "incorrect" is found. I also added the following additional modification - Use ProgressMonitor to check for cancel - Use a timeout for connect in case remote waits forever - Improve error reporting (see also bug #187218) But from the original bug report, still many issues are open. The items below reference the numbered items from the original description: 1. The text printed by the remote side during the login sequence is not yet logged or printed. For Reference, a Console could be used like FTP does, or the messages could be recorded as part of the SystemMessageException. Without knowing what the prompts from the remote side were, users for whom the login sequence fails have almost no chance getting the properties in the PropertySet right. 2., 3. are kind of addressed 4. Is addressed though the timeout is not configurable yet 5., 6. The default prompt is not fault tolerant enough yet. Many people will have different login prompts, so they will never get the "connected" status. At the very least, a regular expression like [#>%$] should be used to check for the prompt. Or, don't use a prompt by default at all and consider the connection connected already. Actually, this might be a solution for (1) as well -- show the entire login sequence in the shell being launched, and being very tolerant by allowing the "connected" status almost unconditionally. I'm keeping the bug open but moving severity down, since the connect now always happens on a background thread, and it is cancelable. This means that at the worst, the Telnet feature itself does not work, but the rest of Eclipse and RSE will be healthy. I'd still like to see this improved for 2.0 though. More patches will be appreciated, please contribute patches against CVS HEAD. Closing this bug since the patch was applied. Remaining issues are tracked for post 2.0 in bug #194473. [target cleanup] 2.0 RC1 was the original target milestone for this bug Comment on attachment 67256 [details] patch for bug 186570 patch ended up being used |