Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 3 commits into from

Conversation

bohwaz
Copy link
Contributor

@bohwaz bohwaz commented Oct 8, 2019

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
Copy link
Contributor Author

bohwaz commented Oct 8, 2019

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
Copy link
Contributor Author

bohwaz commented Oct 11, 2019

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

@cmb69
Copy link
Member

cmb69 commented Oct 17, 2019

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
Copy link
Contributor Author

bohwaz commented Oct 17, 2019

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
Copy link
Member

@cmb69 cmb69 left a comment

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

ext/sqlite3/sqlite3.c Show resolved Hide resolved
ext/sqlite3/sqlite3.c Outdated Show resolved Hide resolved
ext/sqlite3/sqlite3.c Outdated Show resolved Hide resolved
ext/sqlite3/sqlite3.c Outdated Show resolved Hide resolved
ext/sqlite3/sqlite3.c Show resolved Hide resolved
ext/sqlite3/sqlite3.stub.php Outdated Show resolved Hide resolved
ext/sqlite3/tests/sqlite3_40_setauthorizer.phpt Outdated Show resolved Hide resolved
@bohwaz
Copy link
Contributor Author

bohwaz commented Nov 19, 2019

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

@bohwaz
Copy link
Contributor Author

bohwaz commented Dec 10, 2019

@cmb69 everything should be fixed, thank you.

Copy link
Member

@cmb69 cmb69 left a comment

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
Copy link
Member

cmb69 commented Dec 20, 2019

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