Skip to content

Add setAuthorizer method to SQLite3 (implement userland authorizer)#4797

Closed
bohwaz wants to merge 3 commits into
php:masterfrom
bohwaz:feature/sqlite-authorizer-merge
Closed

Add setAuthorizer method to SQLite3 (implement userland authorizer)#4797
bohwaz wants to merge 3 commits into
php:masterfrom
bohwaz:feature/sqlite-authorizer-merge

Conversation

@bohwaz

@bohwaz bohwaz commented Oct 8, 2019

Copy link
Copy Markdown
Contributor

Adds the possibility to define a userland callback that will be used to authorize or not an action on the database.

This allows to effectively implement access restrictions to a SQLite database.

See https://www.sqlite.org/c3ref/set_authorizer.html for details and https://www.sqlite.org/c3ref/c_alter_table.html for action codes and parameters passed to the callback.

The callback will receive 1 to 5 parameters:

  • int $action_code refers to one of the codes mentioned in the SQLite doc. All the codes are available as constants of the SQLite3 class.
  • nullable string arguments 1 to 4 contain various data passed on by SQLite, usually table name, column name, etc.

The callback must return either SQLite3::OK, SQLite3::IGNORE or SQLite3::DENY. Anything else will trigger an error and will be treated as DENY.

It is possible to disable a previously-set authorizer by passing NULL to the setAuthorizer method.

This patch is also available against the PHP 7.4 branch, if needed.

@bohwaz bohwaz force-pushed the feature/sqlite-authorizer-merge branch 2 times, most recently from 7fb63b7 to 3991d9e Compare October 8, 2019 14:08
@bohwaz bohwaz changed the title Add setAuthorizer method to SQLite3 (implement userland authorizer) WIP: Add setAuthorizer method to SQLite3 (implement userland authorizer) Oct 8, 2019
@bohwaz

bohwaz commented Oct 8, 2019

Copy link
Copy Markdown
Contributor Author

Seems that I have a memory leak somewhere, will try to understand where it comes from on tomorrow.

… userland callback that will be used to authorize or not an action on the database
@bohwaz bohwaz force-pushed the feature/sqlite-authorizer-merge branch from 3991d9e to 51b524b Compare October 11, 2019 11:13
@bohwaz

bohwaz commented Oct 11, 2019

Copy link
Copy Markdown
Contributor Author

OK I checked and added some stuff, but I can't find where the leak is coming from :( anyone can help me?

@cmb69

cmb69 commented Oct 17, 2019

Copy link
Copy Markdown
Member

Thanks for the PR! Looks like very useful functionality; I'll try to find some time for a thorough review. :)

Regarding the memory leak: it seems to me that you have to release db_obj->authorizer_fci.function_name in php_sqlite3_object_free_storage().

@bohwaz

bohwaz commented Oct 17, 2019

Copy link
Copy Markdown
Contributor Author

Regarding the memory leak: it seems to me that you have to release db_obj->authorizer_fci.function_name in php_sqlite3_object_free_storage().

Ah yes of course, I didn't think about that, I was sure it was coming from something I missed in using the params inside the fci! Thanks!

@bohwaz bohwaz changed the title WIP: Add setAuthorizer method to SQLite3 (implement userland authorizer) Add setAuthorizer method to SQLite3 (implement userland authorizer) Oct 22, 2019

@cmb69 cmb69 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot! I'm generally fine with this, but I think some minor issues should be addressed (see below).

Comment thread ext/sqlite3/sqlite3.c
Comment thread ext/sqlite3/sqlite3.c Outdated
Comment thread ext/sqlite3/sqlite3.c Outdated
Comment thread ext/sqlite3/sqlite3.c Outdated
Comment thread ext/sqlite3/sqlite3.c
Comment thread ext/sqlite3/sqlite3.stub.php Outdated
Comment thread ext/sqlite3/tests/sqlite3_40_setauthorizer.phpt Outdated
@bohwaz

bohwaz commented Nov 19, 2019

Copy link
Copy Markdown
Contributor Author

Thanks for the review @cmb69 all very good and valid points, I'll fix that ASAP :)

@bohwaz

bohwaz commented Dec 10, 2019

Copy link
Copy Markdown
Contributor Author

@cmb69 everything should be fixed, thank you.

@cmb69 cmb69 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @bohwaz! LGTM now.

If there are no objections, I'll merge this in a week.

@cmb69

cmb69 commented Dec 20, 2019

Copy link
Copy Markdown
Member

Applied as 3958592. Thanks again @bohwaz – useful new feature!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants