Closed Bug 954532 Opened 10 years ago Closed 10 years ago

Authorization loop if lastMessageId is empty for a Twitter account

Categories

(Chat Core :: Twitter, defect)

defect
Not set
minor

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: clokep, Assigned: florian)

Details

Attachments

(1 file, 1 obsolete file)

*** Original post on bio 1098 at 2011-10-21 00:28:00 UTC ***

I found this when testing stuff on my Twitter account: I cleared the lastMessageId (so it became an empty string). Our twitter code then throws an error and requests reauthorization.  If you try to authorize, it just asks again and loops.

I'm not sure if this could happen without users messing with their profile, but we should attempt to handle a bad value in there (if possible).
*** Original post on bio 1098 at 2011-10-21 00:32:09 UTC ***

The error is:
Error: 400 - {"error":"Error in 'since_id' parameter: invalid id.","request":"\/1\/statuses\/mentions.json?include_entities=1&count=200&since_id="}
Source File: file:///C:/Users/clokep/instantbird2/objdir-ib-release/mozilla/dist/bin/components/twitter.js
Line: 558
Source Code:
twitter
*** Original post on bio 1098 at 2011-11-09 00:46:55 UTC ***

As flo said on IRC:
well, from a quick read of the summary, it seems there are actually 2 different issues: 1. The bad handling of that preference value (probably trivial to fix) 2. The authorization loop; how come we detect a 401 - unauthorized error if there's a syntax error?
it's possible the OAuth code misbehaves for a parameter with an empty value, in which case we should fix it
it's also possible the twitter server sends us an inappropriate reply, in which case we can't do much

I'm working on this.
Assignee: nobody → clokep
Status: NEW → ASSIGNED
*** Original post on bio 1098 as attmnt 981 at 2011-11-09 00:55:00 UTC ***

This is the "easy" part of the fix, which simply verifies that the ID we've loaded is made up of digits.

While testing this, the lastMessageId preference didn't seem to update between restarting my Instantbird, perhaps fallout from flo's big rewrite that was just checked in?
Attachment #8352722 - Flags: review?(florian)
Attached patch Patch v2Splinter Review
*** Original post on bio 1098 as attmnt 1073 at 2011-12-11 23:04:00 UTC ***

Taking over, with an updated/version of attachment 8352722 [details] [diff] [review] (bio-attmnt 981):
- fixed the regexp
- added warning when we ignore the value of a pref
- fixed the broken detection of the 401 HTTP status.
Attachment #8352815 - Flags: review?(clokep)
Comment on attachment 8352722 [details] [diff] [review]
Verify the lastMessageId preference

*** Original change on bio 1098 attmnt 981 at 2011-12-11 23:04:13 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352722 - Attachment is obsolete: true
Attachment #8352722 - Flags: review?(florian)
Assignee: clokep → florian
*** Original post on bio 1098 at 2011-12-11 23:05:22 UTC ***

(In reply to comment #4)

> - fixed the broken detection of the 401 HTTP status.

Filed https://bugzilla.mozilla.org/show_bug.cgi?id=709622 to get a JS strict warning added for this trivial code error.
Comment on attachment 8352815 [details] [diff] [review]
Patch v2

*** Original change on bio 1098 attmnt 1073 at 2011-12-11 23:07:52 UTC ***

Looks good, thanks for investigating the second part of this (and fixing my regex!). :)
Attachment #8352815 - Flags: review?(clokep) → review+
*** Original post on bio 1098 at 2011-12-12 01:09:37 UTC ***

Checked in as http://hg.instantbird.org/instantbird/rev/5a9c9760fb59
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.2
You need to log in before you can comment on or make changes to this bug.