laravel / passport

Laravel Passport provides OAuth2 server support to Laravel.
https://laravel.com/docs/passport
MIT License
3.28k stars 776 forks source link

Fix 'revoked' datatype consistency #1678

Closed marech closed 1 year ago

marech commented 1 year ago

Summary

Column revoked within database is created with datatype boolean.

When using JWT library together with pgbouncer which is running in transaction pooling mode and db(postgres) configuration within laravel with options:

            'options' => [
                PDO::ATTR_CASE => PDO::CASE_NATURAL,
                PDO::ATTR_ERRMODE => PDO::ERRMODE_EXCEPTION,
                PDO::ATTR_ORACLE_NULLS => PDO::NULL_NATURAL,
                PDO::ATTR_STRINGIFY_FETCHES => false,
                PDO::ATTR_EMULATE_PREPARES => true,
                PDO::ATTR_PERSISTENT => true
            ]

throws error:

{"message":"[2023-07-12T10:06:14.957012+00:00] production.ERROR: SQLSTATE[42883]: 
Undefined function: 7 ERROR:  operator does not exist: boolean = integer LINE 1: ... 
not null and \"user_id\" = 123 and \"revoked\" = 0 and \"e...
^ HINT:  No operator matches the given name and argument types. 
You might need to add explicit type casts. 
(SQL: select * from \"oauth_access_tokens\" where \"oauth_access_tokens\".\"client_id\" = 4 and \"oauth_access_tokens\".\"client_id\" is not null and 
\"user_id\" = 123 and \"revoked\" = 0 and \"expires_at\" > 2023-07-12 10:06:14 order by \"expires_at\" desc limit 1) 
{\"userId\":123,\"exception\":\"[object] 
(Illuminate\\\\Database\\\\QueryException(code: 42883): SQLSTATE[42883]: Undefined function: 7 ERROR:  operator does not exist: boolean = integer\\nLINE 1: ... 
not null and \\\"user_id\\\" = 123 and \\\"revoked\\\" = 0 and \\\"e...

Problem:

In the current code, there are some calls where the column 'revoked' is addressed using integers, while in other places, it is addressed using booleans.

Solution:

This fix ensures consistency within the PHP code by ensuring that all calls using the revoked column utilize the boolean data type, thereby maintaining a uniform coding style and preventing the switching between different data types.

taylorotwell commented 1 year ago

1 and 0 are listed as valid values for Postgres boolean types. In addition, Laravel would still cast them to (int) behind the scenes even with this change:

CleanShot 2023-07-12 at 14 00 20@2x

marech commented 1 year ago

If in future people stumble upon this pull request in context of laravel + passport + postgres + pgbouncer in transaction mode, here are details what I went through and solution to it.

So by default just switching from session to transaction mode in pgbouncer throws error about pdo statement: ERROR: prepared statement \"pdo_stmt_00000001\" does not exist

To fix that issue prepared statements needs to be emulated on application side aka specifying: PDO::ATTR_EMULATE_PREPARES => true

Which means that application side will be responsible for preparing and binding correct values for query.

And this is where next issues occurs about data mismatch: ERROR: SQLSTATE[42804]: Datatype mismatch: 7 ERROR: column \"revoked\" is of type boolean but expression is of type integer LINE 1: update

From my understanding because when PDO::ATTR_EMULATE_PREPARES => true application is generating that query it specifically generates boolean as integer.

Yes from laravel code it always transforms boolean to integer and it works if database side prepares query/binds values.

But as in pgbouncer transaction mode prepared statements are not supported and we use POD option PDO::ATTR_EMULATE_PREPARES => true application generates that query and integer is never converted to boolean and postrges starts throwing error about datatype mismatch.

Postgres side test with boolean/integer

https://www.postgresql.org/docs/current/datatype-boolean.html

postgres=# CREATE TABLE test1 (a boolean, b text);
CREATE TABLE

postgres=# INSERT INTO test1 VALUES (true);
INSERT 0 1
postgres=# INSERT INTO test1 VALUES (false);
INSERT 0 1

postgres=# INSERT INTO test1 VALUES (1);
ERROR:  column "a" is of type boolean but expression is of type integer
LINE 1: INSERT INTO test1 VALUES (1);                                  ^
HINT:  You will need to rewrite or cast the expression.

postgres=# INSERT INTO test1 VALUES (0);
ERROR:  column "a" is of type boolean but expression is of type integer
LINE 1: INSERT INTO test1 VALUES (0);                                 ^
HINT:  You will need to rewrite or cast the expression.

postgres=# select * from test1 where a = 1;
ERROR:  operator does not exist: boolean = integer
LINE 1: select * from test1 where a = 1;                                  ^
HINT:  No operator matches the given name and argument types. You might need to add explicit type casts.

postgres=# select * from test1 where a = 0;
ERROR:  operator does not exist: boolean = integer
LINE 1: select * from test1 where a = 0;                                  ^
HINT:  No operator matches the given name and argument types. You might need to add explicit type casts.

postgres=# select * from test1 where a = '1';
 a | b
---+---
 t |
 t |
 t |
(3 rows)

postgres=# select * from test1 where a = '0';
 a | b
---+---
 f |
 f |
(2 rows)

postgres=# select * from test1 where a is true;
 a | b
---+---
 t |
 t |
 t |
(3 rows)

postgres=# select * from test1 where a is false;
 a | b
---+---
 f |
 f |
(2 rows)

But if application generates query with correct datatype as in using 'true' instead of int 1 that problem dissapears.

The solution to it is extending laravels db conenction and override bindValues function for example how library https://github.com/umbrellio/laravel-pg-extensions does it in https://github.com/umbrellio/laravel-pg-extensions/blob/master/src/PostgresConnection.php

So long story short to make pgbouncer work in transaction mode: composer require umbrellio/laravel-pg-extensions config/database.php add under postgres connection PDO::ATTR_EMULATE_PREPARES => true

Never the less keeping consistent style across library/application would be appreciated.