I bet this will not immediately go through QA. Please don't hesitate to nitpick. :) Just as a disclaimer, I didn't want any flags, just appropriate positional arguments for the two contexts. That's why it might look unconventional.
Created attachment 167397 [details] [review] Bug 37025: Add CLI tool to generate/delete api keys for a given patron on a Koha instance To test: 1) Apply the patch. 2) Suggestion: `cp debian/scripts/koha-api-keys /usr/sbin/`. 3) Familiarize yourself with the tool by just running `koha-api-keys`. 4) Test that you can generate an api key with the generate op. 5) Test that you can delete an api key with the delete op. 6) Run `xsltproc /usr/share/xml/docbook/stylesheet/docbook-xsl-ns/manpages/docbook.xsl debian/docs/koha-api-keys.xml`. 7) Use man to verify that the manpage renders as expected (install man if not present, e.g. koha-testing-docker). 8) Run the syntax test for the docbook files `prove -v xt/verify-debian-docbook.t`. 9) Sign off. Or leave a comment on what you thinks needs to be improved.
This is cool. I was able to create and delete API keys for a user. At step #6 I tried running this command `xsltproc /usr/share/xml/docbook/stylesheet/docbook-xsl-ns/manpages/docbook.xsl debian/docs/koha-api-keys.xml` I got this error: warning: failed to load external entity "debian/docs/koha-api-keys.xml" unable to parse debian/docs/koha-api-keys.xml I see xml files in debian/docs/ but none for koha-api-keys I think it might be nice if the delete command would optionally take the client id as the first argument instead of prompting for it after. Then the usage for delete could be one command with three arguments similar to create koha-api-keys delete [ client_id ] [ borrowernumber ] [ instancename ]
Hi Brendan, thanks for testing this. By mistake I thought I had included that docbook file because I saw `koha-api-keys` in the diff. It is missing, though. You're absolutely correct. The delete op doesn't need a borrowernumber, that's why I read the client id from input. In general many scripts are split up into enable|disable pairs to keep their interfaces simple. Might be also an idea for this one. I mainly want the generation part because that I really need.
Created attachment 167524 [details] [review] Bug 37025: (follow-up) Add missing docbook xml for koha-api-keys
BTW Brendan, reading from input also implies that you can just read the client id from STDIN like this: `echo <client_id> | koha-api-keys delete <instance>` So there's not really a need for another argument.
Here's another form I just learned about, just because I haven't seen it until a couple minutes ago: `koha-api-keys delete kohadev <<< <client_id>`
I tested this again, but I got stuck on verifying the manpage this time. I'm using ktd, installed man with `sudo apt-get install man` Step 6 seemed to work now with the xml file that you added. `xsltproc /usr/share/xml/docbook/stylesheet/docbook-xsl-ns/manpages/docbook.xsl debian/docs/koha-api-keys.xml` Note: Writing koha-api-keys.8 I tried the command `man koha-api-keys` but got No manual entry for koha-api-keys Step 8 tests passed `prove -v xt/verify-debian-docbook.t` All tests successful.
I'm intrigued. What kind of use cases would you have for this?
Mainly for remote testing. I have an additional API that requires client credentials for bearer token generation and if I want to quickly reproduce a bug, I always need to generate a pair via the UI. That's why I wanted a solution which I can use to generate my client credentials, run my tests and invalidate the bearer token as well as the credentials all in one pipe.
Afraid I can't even get past the first hurdle: kohadev-koha@kohadevbox:koha(bug_37025_so)$ sudo koha-api-keys generate default 51 kohadev Unknown borrowernumber: '51' Don't know if I'm being a complete fool, do feel free to correct me if that is so! Jake.
Wow, that's weird!
Ah wait, don't use sudo :D
Ah, I see. In production, it would have to be run with sudo for proper access to sbin, so I don't think we can sign this off yet? Jake.
Ah, you're totally right! Also I think this should be wrapped in koha-shell when giving it a second look.
Created attachment 171014 [details] [review] Bug 37025: (follow-up) Wrap calls to koha modules in koha-shell This change should - allow seamless usage when run as root (implied by /usr/sbin). - give explicit control to the user instead of relying on the environment
Quoting on the shell is so annoying -_-
(In reply to Paul Derscheid from comment #16) > Quoting on the shell is so annoying -_- 1000%
(In reply to Paul Derscheid from comment #16) > Quoting on the shell is so annoying -_- My hot tip is don't quote on the shell. Even though it's just a tiny bit of code, you're better off putting it in its own script file and calling that via koha-shell. I've had this same nightmare heaps of times with local devs, and it just wasn't worth trying to escape every scenario. I seem to recall koha-shell does some truly awful things with quoting...
I am wondering if we should deviate here from the normal option handling in the other koha scripts. You would expect --generate and --delete here, etc. Might be confusing. Tested koha-api-keys generate 'test' 51 myclone Client ID: c3e0bc35-dbb7-4608-a452-5f2bb850e26b Client Secret: b6999c74-2713-4781-83e2-114fb0525b1b Tested debian/scripts/koha-api-keys delete myclone Client ID to delete: c3e0bc35-dbb7-4608-a452-5f2bb850e26b => How to delete is not clear from the usage statement Usage: debian/scripts/koha-api-keys generate|delete [ description ] [ borrowernumber ] [ instancename ] Also confusing: The bracket pairs seem to suggest that the last 3 parameters are optional. But they are not. Please add separate lines for the generate and delete. Just a question: Should we keep this in shell or move to perl instead? See for instance koha-shell.
Hi, Marcel, consistency is better. Will provide updated patches shortly. Thanks for testing.
Created attachment 185537 [details] [review] Bug 37025: (follow-up) Use consistent option handling and convert to Perl - Convert from bash to Perl for less headaches regarding quoting - Replace positional arguments with --generate and --delete flags - Add --description, --borrowernumber, and --client-id options - Add POD following Koha standards - Update DocBook XML documentation with new flag-based interface - Add audit logging for all API key operations via C4::Log - Document proper usage via koha-shell wrapper for administrative access Test plan: NOTE: As this tool requires Koha environment access and is installed to /usr/sbin/, it must be run via koha-shell when using sudo: 1. Test help output: sudo koha-shell -c "koha-api-keys --help" instancename 2. Test API key generation: sudo koha-shell -c "koha-api-keys --generate --description 'Test key' --borrowernumber 51" instancename => Should output Client ID and Client Secret in tab-separated format 3. Test interactive deletion (use echo to provide input): echo "CLIENT_ID_FROM_STEP_2" | sudo koha-shell -c "koha-api-keys --delete" instancename => Should delete successfully 4. Test scripted deletion: sudo koha-shell -c "koha-api-keys --delete --client-id CLIENT_ID_FROM_STEP_2" instancename => Should delete without prompting 5. Test error handling: sudo koha-shell -c "koha-api-keys --generate --description test" instancename => Should show error about missing --borrowernumber 6. Test invalid data: sudo koha-shell -c "koha-api-keys --generate --description test --borrowernumber 99999" instancename => Should show error about unknown borrowernumber 7. Verify audit logging: After steps 2 and 4, check action_logs table for APIKEY entries: sudo koha-mysql instancename -e "SELECT timestamp, module, action, object, info FROM action_logs WHERE module='APIKEY' ORDER BY timestamp DESC LIMIT 5;" => Should show logged generation and deletion operations 8. Verify DocBook XML validation: prove -v xt/verify-debian-docbook.t 9. Generate and verify man page: xsltproc /usr/share/xml/docbook/stylesheet/docbook-xsl-ns/manpages/docbook.xsl debian/docs/koha-api-keys.xml => Should generate koha-api-keys.8 without errors
Trying my best here to get this moving :) 1) QA checks - pass! 2) Code review a) Spelling Some slipped through, most are perfect: + <varlistentry> + <term><option>koha-api-keys</option></term> + <listitem> + <para>Create an api key for a given patron on a koha instance or delete one based on its client id.</para> + </listitem> + </varlistentry> API, Koha, ID :) +# koha-api-keys: generate or delete api keys for a given patron and +# koha instance. API 3) Testing a) Usage Having a koha- command that requires koha-shell at the same time seems a bit unusual. A rule of thumb we use at work is that if a script starts with koha- it should be run with sudo as root, if not use koha-shell of the instance. This breaks the rule :) When you run this script just with sudo, you get a nasty error message: sudo koha-api-keys Can't locate Koha/Script.pm in @INC (you may need to install the Koha::Script module) (@INC contains: /etc/perl /usr/local/lib/x86_64-linux-gnu/perl/5.36.0 /usr/local/share/perl/5.36.0 /usr/lib/x86_64-linux-gnu/perl5/5.36 /usr/share/perl5 /usr/lib/x86_64-linux-gnu/perl-base /usr/lib/x86_64-linux-gnu/perl/5.36 /usr/share/perl/5.36 /usr/local/lib/site_perl) at /usr/sbin/koha-api-keys line 24. BEGIN failed--compilation aborted at /usr/sbin/koha-api-keys line 24. Could we display a useful error instead hinting on the different usage? b) Command not found (note for other testers) sudo koha-shell -c "koha-api-keys --help" kohadev /bin/bash: line 1: koha-api-keys: command not found I have run koha_reset in hope it would copy some file into the right spot, but that didn't do the trick here. The first test plan in comment#1 included copying files, but it's not part of the current test plan. Still: it worked cp debian/scripts/koha-api-keys /usr/sbin/ sudo koha-shell -c "koha-api-keys --help" kohadev Help shows. c) Create API key (quotes problem) sudo koha-shell -c "koha-api-keys --generate --description 'Test key' --borrowernumber 51" kohadev Error: --generate requires --borrowernumber. Strangely, this works: sudo koha-shell -c "koha-api-keys --generate --description Test --borrowernumber 51" kohadev Problem appears to be the quotes. Fix the example in the test plan? Add a note to the help? c) Action log entries cannot be viewed in log viewer This adds a new module to the action_logs table, but we have no way to display these entries yet. So they don't show in the patron's modification log in the staff interface. Maybe for a follow-up bug?
Created attachment 191344 [details] [review] Bug 37025: (QA follow-up) Fix spelling in documentation - Change "api" to "API" - Change "koha" to "Koha" - Change "id" to "ID"
Created attachment 191345 [details] [review] Bug 37025: (QA follow-up) Use standard koha- script invocation pattern - Add Koha::Perl::Instance module for shared environment setup logic - Script now takes instance name as last argument - Re-exec with proper environment setup (PERL5LIB, KOHA_CONF) - Pass arguments directly via exec() to avoid shell quoting issues - Support both package and git installs - Use Koha::Script to set CLI interface for proper audit logging - Add APIKEY module to log viewer - Add translation for GENERATE action - Usage: sudo koha-api-keys [options] instancename - Update POD and DocBook XML documentation The new Koha::Perl::Instance module provides reusable functions for other perl-based debian scripts that need to run in a Koha environment: - in_koha_env(): Check if already running in Koha environment - get_instance_perl5lib(): Get PERL5LIB for an instance - reexec_in_koha_env(): Re-execute script with proper environment Test plan: NOTE: For testing, copy files first: cp debian/scripts/koha-api-keys /usr/sbin/ mkdir -p /usr/share/koha/bin/Koha/Perl cp debian/scripts/Koha/Perl/Instance.pm /usr/share/koha/bin/Koha/Perl/ 1. Test help output: sudo koha-api-keys --help 2. Test API key generation (with spaces in description): sudo koha-api-keys --generate --description 'Test key' --borrowernumber 51 instancename => Should output Client ID and Client Secret 3. Test interactive deletion: sudo koha-api-keys --delete instancename => Should prompt for Client ID, then delete 4. Test scripted deletion: sudo koha-api-keys --delete --client-id CLIENT_ID instancename => Should delete without prompting 5. Test error handling: sudo koha-api-keys --generate --description test instancename => Should show error about missing --borrowernumber 6. Test invalid instance: sudo koha-api-keys --generate --description test --borrowernumber 51 nonexistent => Should show error about instance not existing 7. Verify audit logging in log viewer: - Go to Tools > Log viewer - Select "API keys" module - Should see GENERATE and DELETE actions with interface "Command-line" 8. Verify DocBook XML validation: prove -v xt/verify-debian-docbook.t
Hi Katrin, thanks so much for reviewing this. I tried to address everything you found. Hope I didn't introduce new issues.