yiisoft / yii2

Yii 2: The Fast, Secure and Professional PHP Framework
http://www.yiiframework.com
BSD 3-Clause "New" or "Revised" License
14.23k stars 6.91k forks source link

PostgreSQL RBAC: assignmentTable with integer column type for user_id #5435

Closed kamovich closed 9 years ago

kamovich commented 9 years ago

I have PostgreSQL and assignmentTable with integer column type for user_id and foreign key to user table. So, if user is not authorized, calling checkAccess will thrown database exception with text "invalid input syntax for integer".

samdark commented 9 years ago

User ID could be and is string in many cases so I'm not sure it's a good idea to force integers.

cebe commented 9 years ago

maybe just do not force it to be anything? related: #4489 Not sure if this a fix to this will break things, set for GA for now.

qiangxue commented 9 years ago

I agree we should not cast user ID to strings in the DB queries.

nineinchnick commented 9 years ago

There is a workaround for this to define a view for the assignment table which casts the integer column to a varchar and use it in AuthManager instead.

samdark commented 9 years ago

There is an issue then. We're using varchar(64) to store IDs by default. In case of non-casting to any type when doing

$manager->assign($reader, 42);

PostgreSQL will throw an exception about types but MySQL will silently cast 42 to characters and save it. Then when doing

$manager->getAssignments(42);

PostgreSQL will again throw an exception but MySQL will attempt to search and will find nothing because 42 != '42'.

That's what is fixed with these casts to (string).

samdark commented 9 years ago

I'm not sure how to fix both cases.

qiangxue commented 9 years ago

With the current string typecasting, it is impossible for the assignment table to use integer IDs, which I think is incorrect behavior.

In practice, the type of the user ID column in the assignment table should normally be adjusted according to the id column type used in the user table. And normally there should be a FK constraint to ensure consistency. If this is true, there should be no type conversion issue.

samdark commented 9 years ago

By default our migrations and dumps are using varchar and are not constrainted to actual user ID: https://github.com/yiisoft/yii2/blob/master/framework/rbac/migrations/schema-mysql.sql#L52 One can store users, for example, in Redis or PHP array w/o any problem this way.

samdark commented 9 years ago

Removing casting and using integers will give you incorrect results when using MySQL which is very common case.

kamovich commented 9 years ago

@samdark with your fix (casts to (string)) PostrgreSQL not throwing exception if user_id is not empty (not null, empty string etc.) even column type is integer.

But if user_id is empty it casts to empty string. So query became like this:

SELECT * FROM "auth_assignment" WHERE "user_id"=''
qiangxue commented 9 years ago

with your fix (casts to (string)) PostrgreSQL not throwing exception if user_id is not empty (not null, empty string etc.) even column type is integer.

@kamovich Isn't this contradicting to what you have reported in this issue that PostgreSQL would throw "invalid input syntax for integer"?

kamovich commented 9 years ago

I wrote that PostrgreSQL throws exception when user not authorized. In other cases works correctly.

I will try to clarify. PostrgreSQL throws exception just for this query:

SELECT * FROM "auth_assignment" WHERE "user_id"=''

For this query work fine:

SELECT * FROM "auth_assignment" WHERE "user_id"='42'
samdark commented 9 years ago

Well, then it's easier. I could add empty id checks since it doesn't even make any sense to issue queries in this case.

kamovich commented 9 years ago

@samdark in my app I did exactly same and its fix this problem

jbcrouigneau commented 9 years ago

qiangxue wrote :

With the current string typecasting, it is impossible for the assignment table to use integer IDs, which I think is incorrect behavior.

In practice, the type of the user ID column in the assignment table should normally be adjusted according to the id column type used in the user table. And normally there should be a FK constraint to ensure consistency. If this is true, there should be no type conversion issue.

Very true ! user_id as VARCHAR is not consistent with User table in advanced template (where user id is INT). This is an issue to me (can't define a FK). Would be great to solve it.

cebe commented 9 years ago

you can write your own migration to create the table with an integer id.