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).

Files: 
CommentFileSizeAuthor
#5 oauth_token_key_length_1002482.patch68.53 KBvoxpelli

Comments

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.

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...

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.

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 :/

Status:Active» Needs review
StatusFileSize
new68.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

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...

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.

Status:Needs review» Needs work
Issue tags:+Needs Documentation

I went ahead and committed this patch with a few adjustments to CVS - still need to actually write what has changed.

Status:Needs work» Fixed
Issue tags:-Needs Documentation

Documented in the release notes for OAuth 6.x-3.0-beta4: http://drupal.org/node/1118300

Status:Fixed» Closed (fixed)
Issue tags:-OAuth 3.x Stable

Automatically closed -- issue fixed for 2 weeks with no activity.