token_key column not large enough [#1002482]

I just spent hours debugging this - but when trying to pair up with the Yahoo OAuth for their API, then OAuth Token returned is MUCH larger than the 255 characters allowed in oauth_common_token.

I had to drop the tables and tweak the schema to change token_key to a text column and also drop the database indexes on it. Unfortunately, this will impact performance. I'm not really sure if there is a better approach to this.

The key I got back was 814 characters (not sure if they vary).

Comment	File	Size	Author
#5	oauth_token_key_length_1002482.patch	68.53 KB	voxpelli
#5

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

voxpelli CreditAttribution: voxpelli commented 20 December 2010 at 12:19

Assigned:

Unassigned

» voxpelli

Wow - that's an insane length of a token, but since they use it and the OAuth specification doesn't specify a max length I guess we need to tweak the module to handle this use case. Unfortunately changing a table column from a varchar to a text in that column not being very good to use as a key any longer - so I think we need to do some extra tweaking to accommodate for that.

Comment #2

nicholasThompson

English

CreditAttribution: nicholasThompson commented 20 December 2010 at 12:43

I agree - it's excessive. I also agree with the DB indexing issue.

The closest I have come to a conclusion is some kind of "hashed" index (token & consumer key Hashes as SHA1 or would MD5 suffice?!) But that has 2 issues:
1) It doesn't solve the "lookup on token key" issue
2) It *could* have key-collision.

The other option is to have a truncated-token column which is a 255 varchar indexed column. Whenever we SELECT on that col, we just pass in a truncated token. The risk there is if the first 255 characters of my 814 character token is the same as yours. The odds are astronomical - but it is possible. I guess in THAT case, a manual comparison of the remaining results wouldn't be TOO expensive...

Comment #3

voxpelli CreditAttribution: voxpelli commented 25 December 2010 at 22:42

Issue tags:

+OAuth 3.x Stable

A third alternative would be to increase the length column as MySQL 5.0+ supports up to 64kb characters (and PostgreSQL apparently 1gb) - setting a limit at the maximum would ensure that all but the craziest of services would be supported and would avoid using any text columns - which is good since most sites won't need them.

While doing this we should also move from relying on string keys to link consumers, tokens and nonces to instead use a serial ids to do so.

Should fix this prior to a stable release.

Comment #4

voxpelli CreditAttribution: voxpelli commented 30 December 2010 at 20:51

The move from relying on string keys for linking consumers, tokens, nonces etc to instead rely on numeric ids seems like a major refactoring - this can take a while - hopefully won't break too much compatibility :/

Comment #5

voxpelli CreditAttribution: voxpelli commented 10 January 2011 at 20:40

Status:

Active

» Needs review

File	Size
oauth_token_key_length_1002482.patch	68.53 KB

Here's a first version of the patch - please review and come with feedback.

What I've done:

I've moved the token and consumers table to rely on id-numbers for relations between tables
I've moved all keys and similar to be of type 'text' - as such the length of them won't cause any trouble
I've separated out provider specific data into their own tables so that clients don't need to care about a bunch of empty columns on every load
I've removed the consumer ui module as it became complicated to upgrade and consumers preferably should provide their own ui:s

Comment #6

voxpelli CreditAttribution: voxpelli commented 10 January 2011 at 20:44

Is dependent on a coding cleanup I will post asap - changes can be found here as well: https://github.com/voxpelli/drupal-oauth/commit/6788f41f7e18972178ecc9a1...

Comment #7

voxpelli CreditAttribution: voxpelli commented 10 January 2011 at 22:24

Coding style fixes committed to CVS - will probably commit the token patch tomorrow and then follow up with a few more coding style related patches and then continuing with a few other small patches. Then release a new beta and then perhaps start looking at D7.