Closed Bug 955520 Opened 10 years ago Closed 10 years ago

Account created with username@yahoo.* instead of username is handled badly

Categories

(Chat Core :: Yahoo! Messenger, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: aleth, Assigned: qheaden)

Details

Attachments

(2 files, 5 obsolete files)

*** Original post on bio 2083 at 2013-08-01 19:18:00 UTC ***

STR 
Create an account specifying the username email address instead of just the username (e.g. username@yahoo.it)

Result:
Account manager gets stuck on "Connecting...", ultimately fails with "Account locked due to too many login attempts".

Expected behaviour:
1) Strip the domain name before creating the account and handle this gracefully.
2) If there is a problem with the username, "Account locked" is probably not the best error message to receive, as a user.
*** Original post on bio 2083 at 2013-08-01 19:21:34 UTC ***

I didn't realize there were other possible domains outside of @yahoo.com and @yahoo.co.jp. Looks like we have to strip anything with @yahoo in it.
*** Original post on bio 2083 at 2013-08-01 19:23:13 UTC ***

Forgot to add that this actually DOES lock the username account, i.e. if I have another account with the correct username set, it won't connect anymore.
*** Original post on bio 2083 at 2013-08-01 19:23:36 UTC ***

(In reply to comment #1)
> I didn't realize there were other possible domains outside of @yahoo.com and
> @yahoo.co.jp. Looks like we have to strip anything with @yahoo in it.

There are plenty (e.g. yahoo.co.uk)
*** Original post on bio 2083 at 2013-08-01 19:26:36 UTC ***

(In reply to comment #1)
> I didn't realize there were other possible domains outside of @yahoo.com and
> @yahoo.co.jp. Looks like we have to strip anything with @yahoo in it.

Also, if e.g. @ is not an allowed character in Yahoo usernames, can we check for that too?
*** Original post on bio 2083 at 2013-08-01 19:41:12 UTC ***

(In reply to comment #4)
> (In reply to comment #1)
> > I didn't realize there were other possible domains outside of @yahoo.com and
> > @yahoo.co.jp. Looks like we have to strip anything with @yahoo in it.
> 
> Also, if e.g. @ is not an allowed character in Yahoo usernames, can we check
> for that too?

The @ character is allowed if the username contains any domain other than yahoo.*. That's according to the unofficial online docs anyway.

I'm wondering why the server didn't come back with error code 1013 for invalid username? And if it did give that error, why didn't the plug-in prevent automatic reconnect?
*** Original post on bio 2083 at 2013-08-01 19:45:44 UTC ***

(In reply to comment #5)
> I'm wondering why the server didn't come back with error code 1013 for invalid
> username? And if it did give that error, why didn't the plug-in prevent
> automatic reconnect?

It took quite a while on "connecting..." before it finally failed. And as per comment 2, the server seems to associate the connection attempt with the right account.
*** Original post on bio 2083 at 2013-08-01 20:01:47 UTC ***

It came to me why it didn't fail with an error. Because we strip the @yahoo.com and @yahoo.co.jp domains, Patrick and I thought it would be impossible to receive that error from the servers. So we removed the code that handles that error specifically. Unfortunately, we didn't take into account any @yahoo.* domain.

I'm working on a fix to strip all @yahoo.* domains.
Assignee: nobody → qheaden
Status: NEW → ASSIGNED
*** Original post on bio 2083 at 2013-08-01 20:07:09 UTC ***

Maybe it wasn't a good idea to remove handling for that error altogether. Expect the unexpected and all that ;)
Attached patch Patch 1 (obsolete) — Splinter Review
*** Original post on bio 2083 as attmnt 2667 at 2013-08-01 20:11:00 UTC ***

This fix extends domain stripping to all @yahoo.* domains, and it does it in the account constructor so that all of the checking and stripping won't have to be done per access of YahooAccount.cleanUsername.
Attachment #8354436 - Flags: review?(clokep)
Comment on attachment 8354436 [details] [diff] [review]
Patch 1

*** Original change on bio 2083 attmnt 2667 at 2013-08-01 20:36:28 UTC ***

>diff --git a/chat/protocols/yahoo/yahoo.js b/chat/protocols/yahoo/yahoo.js
>+  // Strip any @yahoo.* domains from username. Other email domains
>+  // can be left alone.
>+  let yahooDomainIndex = this.name.indexOf("@yahoo");
>+  if (yahooDomainIndex > 0)
>+    this.cleanUsername = this.name.slice(0, yahooDomainIndex);
>+  else
>+    this.cleanUsername = this.name;

Why not...this.cleanUsername = this.name.replace(/@yahoo\..+$/, "")?

Please add tests for this.
Attachment #8354436 - Flags: review?(clokep) → review-
*** Original post on bio 2083 at 2013-08-01 20:37:49 UTC ***

I wonder what the reviewer thought about comment 8?
*** Original post on bio 2083 at 2013-08-01 20:45:35 UTC ***

(In reply to comment #11)
> I wonder what the reviewer thought about comment 8?

I think this doesn't make sense because we don't want to show this error to the user. So removing this is still the right thing to do. We should possibly put a meaningful error in the error console in this situation, however.

I hope this isn't too blunt. :) Thanks for the comment.
*** Original post on bio 2083 at 2013-08-01 20:55:54 UTC ***

And just for reference, here are all of the possible Yahoo! email domains posted on Wikipedia: http://en.wikipedia.org/wiki/Yahoo!_Mail#Email_domains.
Attached patch Patch 2 (obsolete) — Splinter Review
*** Original post on bio 2083 as attmnt 2668 at 2013-08-01 21:06:00 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354437 - Flags: review?(clokep)
Comment on attachment 8354436 [details] [diff] [review]
Patch 1

*** Original change on bio 2083 attmnt 2667 at 2013-08-01 21:06:23 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354436 - Attachment is obsolete: true
Attached patch Tests (obsolete) — Splinter Review
*** Original post on bio 2083 as attmnt 2669 at 2013-08-01 21:07:00 UTC ***

Tests the regex used in the main patch.
Attachment #8354438 - Flags: review?(clokep)
Comment on attachment 8354437 [details] [diff] [review]
Patch 2

*** Original change on bio 2083 attmnt 2668 at 2013-08-01 21:12:20 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354437 - Flags: review?(clokep) → review+
Comment on attachment 8354438 [details] [diff] [review]
Tests

*** Original change on bio 2083 attmnt 2669 at 2013-08-01 21:13:52 UTC ***

Please use the public domain license as other tests.

This doesn't test the actual code it all. If we change how this is done in the account, this won't be affected.
Attachment #8354438 - Flags: review?(clokep) → review-
*** Original post on bio 2083 at 2013-08-01 21:17:30 UTC ***

(In reply to comment #16)
> Comment on attachment 8354438 [details] [diff] [review] (bio-attmnt 2669) [details]
> Tests
> 
> Please use the public domain license as other tests.
> 
> This doesn't test the actual code it all. If we change how this is done in the
> account, this won't be affected.

I agree with that, but how am I able to create a YahooAccount object with a custom name? Don't I have to create YahooProtocol objects as well? For testing purposes, should we perhaps split the domain stripping code into a class method?
*** Original post on bio 2083 at 2013-08-01 21:31:23 UTC ***

Why do you need a YahooProtocol object? You're not going to connect or anything, you just need it to be good enough to run the constructor.
Attached patch Tests v2 (obsolete) — Splinter Review
*** Original post on bio 2083 as attmnt 2670 at 2013-08-01 21:56:00 UTC ***

I made the mistake of thinking _init() in the YahooAccount constructor needed a more detailed object. I hope this is a better test.
Attachment #8354439 - Flags: review?(clokep)
Comment on attachment 8354438 [details] [diff] [review]
Tests

*** Original change on bio 2083 attmnt 2669 at 2013-08-01 21:56:33 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354438 - Attachment is obsolete: true
Comment on attachment 8354439 [details] [diff] [review]
Tests v2

*** Original change on bio 2083 attmnt 2670 at 2013-08-01 22:06:04 UTC ***

>diff --git a/chat/protocols/yahoo/test/test_yahooAccount.js b/chat/protocols/yahoo/test/test_yahooAccount.js
>+function run_test()
>+{
We could just make this the test, I don't have much opinion on that.

>+  // These are just a few of the many possible domains.
>+  let domains = ["yahoo.com.ar", "yahoo.com.au", "yahoo.com", "yahoo.co.jp",
>+                 "yahoo.it", "yahoo.com.cn/yahoo.cn", "yahoo.co.in/yahoo.in"];
The yahoo.com.xx/yahoo.xx ones in the list are meant to be two different addresses, I'm pretty sure.

>diff --git a/chat/protocols/yahoo/test/xpcshell.ini b/chat/protocols/yahoo/test/xpcshell.ini
> [test_yahoopacket.js]
> [test_yahooLoginHelper.js]
>+[test_yahooAccount.js]
Alphabetical order, please.
Attachment #8354439 - Flags: review?(clokep) → review-
*** Original post on bio 2083 at 2013-08-01 22:07:55 UTC ***

>   // Strip @yahoo.com or @yahoo.co.jp from username. Other email domains
>   // can be left alone.
>-  get cleanUsername() this.name.replace(this._protocol.emailSuffix, ""),

I just glanced at this out of curiosity and found this - shouldn't the comment be removed as well?
Attached patch Patch v3Splinter Review
*** Original post on bio 2083 as attmnt 2674 at 2013-08-02 04:16:00 UTC ***

Same as version 2 of the patch, but removes the old comment spotted by Nihanth. Good eye on that one! ;)
Attachment #8354443 - Flags: review?(clokep)
Comment on attachment 8354437 [details] [diff] [review]
Patch 2

*** Original change on bio 2083 attmnt 2668 at 2013-08-02 04:16:30 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354437 - Attachment is obsolete: true
Attached file Tests v3 (obsolete) —
*** Original post on bio 2083 as attmnt 2675 at 2013-08-02 04:17:00 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354444 - Flags: review?(clokep)
Comment on attachment 8354439 [details] [diff] [review]
Tests v2

*** Original change on bio 2083 attmnt 2670 at 2013-08-02 04:17:10 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354439 - Attachment is obsolete: true
Attached patch Tests v3 (patch)Splinter Review
*** Original post on bio 2083 as attmnt 2676 at 2013-08-02 04:18:00 UTC ***

Sorry. Didn't check the last upload as a patch type.
Attachment #8354445 - Flags: review?(clokep)
Comment on attachment 8354444 [details]
Tests v3

*** Original change on bio 2083 attmnt 2675 at 2013-08-02 04:18:16 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354444 - Attachment is obsolete: true
Attachment #8354444 - Flags: review?(clokep)
Comment on attachment 8354443 [details] [diff] [review]
Patch v3

*** Original change on bio 2083 attmnt 2674 at 2013-08-03 01:24:54 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354443 - Flags: review?(clokep) → review+
Comment on attachment 8354445 [details] [diff] [review]
Tests v3 (patch)

*** Original change on bio 2083 attmnt 2676 at 2013-08-03 01:25:14 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354445 - Flags: review?(clokep) → review+
*** Original post on bio 2083 at 2013-08-03 11:34:26 UTC ***

http://hg.instantbird.org/instantbird/rev/05965ac4df2e
http://hg.instantbird.org/instantbird/rev/6f33ac73b9ee
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.5
You need to log in before you can comment on or make changes to this bug.