Conversation
demiankatz
left a comment
There was a problem hiding this comment.
@LuomaJuha, I only had a few minutes to start looking at this today, so I haven't done anything resembling a full review, but see below for a few initial thoughts and suggestions! I'll try to look at more of the code as time permits.
|
@demiankatz thanks for suggestions! I think i will add the ApiKeyController to avoid that. And this is still in progress at the moment as there is conversion to doctrine going and need tests + some small adjustments listed in the description :) |
230f6e9 to
800ab92
Compare
…omponent, link-button component
…API key, added possibility to change header field for API key and log requests with API keys.
800ab92 to
78d900b
Compare
|
I'm opening this now for reviewing even though the doctrine which this uses is not yet merged. |
demiankatz
left a comment
There was a problem hiding this comment.
@LuomaJuha, I haven't had time to read through everything yet, but see below for some initial comments based on my first partial review.
themes/bootstrap5/templates/_ui/components/confirm-button.phtml
Outdated
Show resolved
Hide resolved
module/VuFindApi/src/VuFindApi/Controller/SearchApiController.php
Outdated
Show resolved
Hide resolved
module/VuFindApi/src/VuFindApi/Controller/SearchApiController.php
Outdated
Show resolved
Hide resolved
module/VuFindApi/src/VuFindApi/Controller/SearchApiController.php
Outdated
Show resolved
Hide resolved
demiankatz
left a comment
There was a problem hiding this comment.
Thanks, @LuomaJuha! I finished my review just now; see below for a few more comments.
Note that I haven't looked closely at the AccessTokenService yet, since it's full of warnings related to Doctrine not being merged yet. I'll review it more carefully after Doctrine is in place (and when I can actually start doing some hands-on experimentation).
Co-authored-by: Demian Katz <[email protected]>
Co-authored-by: Demian Katz <[email protected]>
Co-authored-by: Demian Katz <[email protected]>
Co-authored-by: Demian Katz <[email protected]>
Co-authored-by: Demian Katz <[email protected]>
Co-authored-by: Demian Katz <[email protected]>
…/NDL-VuFind2 into api-key-implementation
demiankatz
left a comment
There was a problem hiding this comment.
Thanks for the progress here, @LuomaJuha! Now that #2233 has been merged, you should be able to get this PR into a runnable state. Once you've done that, please request a new review from me and I'll start doing some hands-on testing. Also, see below for a very trivial formatting suggestion.
|
Here's the test failure -- once again, something to do with Doctrine entity annotations being mismatched with the actual database: |
…archApiControllerTest.php Co-authored-by: Demian Katz <[email protected]>
…/NDL-VuFind2 into api-key-implementation
|
@demiankatz Seems like i forgot that orm requires indexeses in the class also. Added it now. |
|
Thanks for fixing the test. I'm just about out of time for today but will try to give this another look when I get online tomorrow. In the meantime, if you have a chance, please merge the dev branch into this PR -- since I recently added PHP 8.4 CI support, we need to merge that in to get the new set of required CI tasks to pass. |
demiankatz
left a comment
There was a problem hiding this comment.
I noticed a few problems during review, but they're all simple and can be fixed by applying the suggestions below.
module/VuFind/sql/migrations/pgsql/11.0/012-add-api-key-table.sql
Outdated
Show resolved
Hide resolved
Co-authored-by: Demian Katz <[email protected]>
Co-authored-by: Demian Katz <[email protected]>
Co-authored-by: Demian Katz <[email protected]>
Co-authored-by: Demian Katz <[email protected]>
demiankatz
left a comment
There was a problem hiding this comment.
My hands-on testing revealed two minor things:
1.) I removed the confirm_delete translation string because it was unused elsewhere in VuFind... but it is needed here, so I have restored it.
2.) When I set this up, I forgot to generate salt and encountered an unintuitive error (had to look in logs to understand what was happening). I'm adding a "REQUIRED" label to the setting in config.ini to hopefully make it harder to overlook.
I'm going to apply my proposal and then approve and merge this. No further action required.
Thanks, @LuomaJuha!
|
One more last-minute addition: the new permission was not included in the summary of permissions in permissions.ini; I've inserted it. |
Dismissing with Ere's permission.
Still a work in progress, as the database entities needs to be converted into doctrine versions. Easier to open this for now as a draft pull request and finish this way.
api_keylast_usedcolumn. The update interval is once every hour to avoid extra calls to the database.010-add-api-key-table.sqlfeature.Developerwhich contains optiondeniedTemplateBehaviorto adjust the template rendered if user does not have proper permissions.default.Developer.Default settings require the user must be logged in and have a verified Email in their account.assertion[]options. These assertions are then checked with each permission check if set.API_Keys, which contains the next options:mode: If set to'disable'API keys are not used, default value. If set to'enabled'API keys can be used and created but rest api requests do not require them. If set to'enforced'using an API key with rest api is mandatory.token_salt: A random string of salt to add when generating API key tokens. This setting is required.header_field: In which header field should the API key token be provided in the request. This defaults to'X-API-KEY'.key_limit: Controls how many API keys can a user have at once. Default value is5.Still needs to be implemented:
Requires: #2233