gcode-mirror / oauth-php

Automatically exported from code.google.com/p/oauth-php
MIT License
0 stars 0 forks source link

Server/Client library must support UUID as user ID´s #82

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago
Most distributed database systems (e.g. hBase, Cassandra) "only" support UUIDs 
by concept

http://en.wikipedia.org/wiki/Universally_unique_identifier

But the PHP OAuth Consumer and Server library only supports integer as user IDs.
That´s a bad choice, i think this should be changed. This won´t be a breaking 
change.

greetings André

Original issue reported on code.google.com by fiedler....@gmail.com on 16 Nov 2010 at 11:03

GoogleCodeExporter commented 9 years ago

Original comment by fiedler....@gmail.com on 16 Nov 2010 at 11:04

GoogleCodeExporter commented 9 years ago
Any suggestions of a patch that won't break backwards compatibility? I can't 
see it. Converting ocr_usa_id_ref or ocr_id to char/varchar doesn't seem to be 
a good idea.

You know you can always create a table such as

CREATE TABLE oauth_to_uuid (
    ocr_id int(11),
    uuid char(16)
);

Original comment by brunobg%...@gtempaccount.com on 17 Nov 2010 at 5:17

GoogleCodeExporter commented 9 years ago
Such a table won´t work with an custom database wrapper made for a distributed 
database (that´s what i´m trying now). In an distributed database you cant 
have auto increment id´s. Except you implement an stand alone id-server.

I think it should work, if  i change all queries in OAuthStroeSQL to use 
something like oct_usa_id_ref = \'%s\' (instead of %d) and validate every 
$user_id at the beginning of an method. Like:

$user_id = preg_replace('/[^a-z0-9\-]/i', '', $user_id);

+ changing the database from int(11) to varchar(36)

That wont break older db´s and still work with integers.
What do you think?

Original comment by fiedler....@gmail.com on 17 Nov 2010 at 5:39

GoogleCodeExporter commented 9 years ago
Btw, there are some "bugs" in OAuthStroeSQL... at some places there´s noted 

oct_usa_id_ref = %s

That´s a bit risky. ;o)

Original comment by fiedler....@gmail.com on 17 Nov 2010 at 5:41

GoogleCodeExporter commented 9 years ago
[deleted comment]
GoogleCodeExporter commented 9 years ago
e.g. 
http://code.google.com/p/oauth-php/source/browse/trunk/library/store/OAuthStoreS
QL.php#213

$user_id can be a sql injection... you don´t know. Should be %d ... or 
rewritten for use of UUIDs ;o)

Original comment by fiedler....@gmail.com on 17 Nov 2010 at 5:59

GoogleCodeExporter commented 9 years ago
I just fixed the missing quotes. I don't know how I could have missed that... 
It's not a catastrophe because any sane application wouldn't get the user id 
from a GET/POST, and the query is passed through a string escaper but it was 
bad anyway -- I think it would lead to a bad query that wouldn't run, but not 
an injection. Thanks a lot André :)

Now, about your actual request. Your proposal would work, but I'm a little 
worried about backward compatibility. People would have to update their 
database schemas and there's a penalty for the int->string conversion in joins. 

If you can't have auto_increments, then don't you have a larger problem with 
all the primary keys in the oauth_* tables? Notice that my oauth_to_uuid does 
not have an auto increment id, so perhaps the ugly wrapper could work...

Original comment by brunobg%...@gtempaccount.com on 17 Nov 2010 at 8:27

GoogleCodeExporter commented 9 years ago
Hm, all id´s should work as UUID... let me think about it a bit more. My 
actual thoughts are using MySQL only for OAuth handling (not much work), all 
other data is stored in Cassandra on more than one node. So at the moment UUID 
user_ids are enough. But if someone has 500.000 users, he may want to switch 
the whole OAuth thing to a distributed database. He will definitively have a 
problem.

Let me think about this a bit more...

greetings André

Original comment by fiedler....@gmail.com on 17 Nov 2010 at 9:04

GoogleCodeExporter commented 9 years ago
If OAuth is moved to a distributed database, you won't use the MySQL storage 
system. So you could easily create a new storage system with a schema that uses 
UUIDs instead. The migration would not be difficult if you have the (int id) 
<-> UUID table... right?

Original comment by brunobg%...@gtempaccount.com on 23 Nov 2010 at 5:29

GoogleCodeExporter commented 9 years ago
Yes, my problem is, that i temporary mix mysql and cassandra. ;o) It works by 
now and you´re right. If i write a storage system for use with cassandra it 
will work fine. Thats what i want to do... so no problem... except a few small. 
;o)

I don´t know why, but at some places in the library id´s (user id, osr id 
aso.) get parsed to int. But not every time. :/ I think this should be removed. 
If necessary, it should be parsed in storage system. But i think this isn´t 
really necessary.

Original comment by fiedler....@gmail.com on 23 Nov 2010 at 5:48

GoogleCodeExporter commented 9 years ago
Hey Andre! 

I couldn't find these places ids are parsed to int... But I agree, if they 
exist, they should be removed and the storage system deal with it. Can you tell 
me where they happen or, better, send a patch? ;)

Original comment by brunobg%...@gtempaccount.com on 24 Nov 2010 at 7:58

GoogleCodeExporter commented 9 years ago
Hi Bruno,

i´ve fetcht r175 and checked again. Seems someone from my team inserted these 
intvar() calls in the library. He added xAuth support. :/ So the only thing 
that should be changed are the docu-comments, like this:

* @param object usr_id

And the initializers, like this:

function doRequest ( $usr_id = null, ...)

i will close this, think it works fine. If time is ready, i will release a 
cassandra wrapper based on https://github.com/thobbs/phpcassa but thats another 
topic.

thx for your help, André :o)

Original comment by fiedler....@gmail.com on 24 Nov 2010 at 9:44