|
Description
Martin Renvoize (ashimema)
2022-05-10 08:38:49 UTC
This was still a problem last time I looked I added a script for this on bug 40545. I also added a method for dealing with this action which puts an action log when this happens. So I propose: - We add a permission (ideas?) - We make this controller allow a patron with the right permissions to clear the 2FA on any patron, or themselves. Due to the security of this one, I'd say only superlibrarian should be able to clear/reset the MFA for another patron. If MFA is enabled, I think anyone should be able to change their own MFA. (One example is you've got a new phone and you need to change over to the authenticator app on your new phone. This would apply to both the staff interface and the OPAC.) (In reply to David Cook from comment #3) > Due to the security of this one, I'd say only superlibrarian should be able > to clear/reset the MFA for another patron. > > If MFA is enabled, I think anyone should be able to change their own MFA. > (One example is you've got a new phone and you need to change over to the > authenticator app on your new phone. This would apply to both the staff > interface and the OPAC.) I agree. I’ll send a patch tomorrow (In reply to Tomás Cohen Arazi (tcohen) from comment #4) > I agree. I’ll send a patch tomorrow Sounds good! If we did want more fine-grained permissions, I think it would be a good use case for bug 40546. Although that's also something we could pursue later. Created attachment 184938 [details] [review] Bug 30724: Allow superlibrarians to manage 2FA for other patrons This enhances the two-factor authentication management interface to allow superlibrarians to disable 2FA for other patrons when they lose access to their authenticator device. Controller changes (members/two_factor_auth.pl): * Accept borrowernumber parameter to select target patron * Implement authorization checks (self-service or superlibrarian) * Use proper HTTP status codes (403/404) for error conditions * Update all patron operations to use selected patron * Integrate with new reset_2fa() method for consistency * Pass another_user flag to template for conditional display Template changes (two_factor_auth.tt): * Use has_2fa_enabled() method instead of auth_method string comparison * Prevent superlibrarians from enabling 2FA for other users * Show explanatory message when 2FA setup is restricted * Maintain proper conditional display logic UI Integration changes: * Update members toolbar to show 2FA option for superlibrarians * Pass borrowernumber parameter in all 2FA-related URLs * Maintain context when canceling 2FA registration * Use consistent parameter naming (borrowernumber vs patron_id) To test: 1. Enable TwoFactorAuthentication system preference 2. Set up 2FA for a test patron 3. As superlibrarian, visit patron details page 4. Click 'Manage two-factor authentication' in toolbar => SUCCESS: 2FA management page loads for the selected patron 5. Disable 2FA for the patron => SUCCESS: 2FA is disabled for the target patron, not the superlibrarian 6. Verify 'Enable 2FA' button is hidden with explanatory text => SUCCESS: Shows message that users must enable 2FA themselves 7. Test authorization: try accessing as non-superlibrarian for different patron => FAIL: Returns 403 Forbidden error 8. Sign off :-D Created attachment 184939 [details] [review] Bug 30724: (follow-up) Improve user interface text and messaging This enhances the user experience with clearer, more contextual messaging throughout the two-factor authentication management interface. Template improvements: * Add contextual page titles showing patron name when managing others * Replace generic status messages with personalized, descriptive text * Provide clear explanation of why 2FA setup is restricted for admins * Add actionable guidance for superlibrarians on available options * Use appropriate alert styling (success/info/warning) for different states * Include patron names in messages for better context The new messaging explains the security rationale behind restrictions and provides clear guidance on what actions are available to different user types. To test: 1. As superlibrarian, manage 2FA for another patron => SUCCESS: Page title shows "Two-factor authentication for [Name]" => SUCCESS: Status messages are personalized with patron name => SUCCESS: Clear explanation of setup restrictions with actionable guidance 2. Manage your own 2FA => SUCCESS: Messages use "Your account" language appropriately 3. Sign off :-D I attempted to test, but wasn't able to enable 2Fa for the patron: 1. Applied the patches and restarted everything. 2. Enabled TwoFactorAuthentication system preference. 3. For Henry, set a password and added some permissions (didn't make superlibrarian). 4. Logged in as Henry 5. Went to [Henry's] My account > More > Manage two-factor authentication 6. Get "Error: You don't have permission to access this page." So, can't seem to enable 2FA for a staff user. Also having trouble setting 2FA for the koha user: 1. My account > More > Manage two-factor authentication 2. Scan code with app 3. Enter code from app 4. Get Error 404 page 5. Messages in logs 172.18.0.1 - - [01/Sep/2025:02:39:06 +0000] "POST /api/v1/app.pl/api/v1/auth/two-factor/registration/verification HTTP/1.1" 204 - "http://127.0.0.1:8081/cgi-bin/koha/members/two_factor_auth.pl?borrowernumber=51" "Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/139.0.0.0 Safari/537.36" 172.18.0.1 - - [01/Sep/2025:02:39:06 +0000] "GET /intranet/members/two_factor_auth.pl HTTP/1.1" 302 0 "http://127.0.0.1:8081/cgi-bin/koha/members/two_factor_auth.pl?borrowernumber=51" "Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/139.0.0.0 Safari/537.36" 172.18.0.1 - - [01/Sep/2025:02:39:07 +0000] "GET errors/404.pl HTTP/1.1" 404 36495 "http://127.0.0.1:8081/cgi-bin/koha/members/two_factor_auth.pl?borrowernumber=51" "Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/139.0.0.0 Safari/537.36" 6. 2FA is actually enabled - log in prompts for code, and log in works. So, possibly something I'm doing wrong. (In reply to David Nind from comment #8) > I attempted to test, but wasn't able to enable 2Fa for the patron: > > 1. Applied the patches and restarted everything. > 2. Enabled TwoFactorAuthentication system preference. > 3. For Henry, set a password and added some permissions (didn't make > superlibrarian). > 4. Logged in as Henry > 5. Went to [Henry's] My account > More > Manage two-factor authentication > 6. Get "Error: You don't have permission to access this page." > > So, can't seem to enable 2FA for a staff user. I think this is a general Koha bug: a staff user cannot see their own account details page unless they have `borrowers => [ 'edit_borrowers', 'list_borrowers' ]` permissions. Is out of the scope of this bug, so you can try heading to the page directly to try: http://kohadev-intra.localhost/cgi-bin/koha/members/two_factor_auth.pl?borrowernumber=19 Best regards Thanks!. Sorry, not having much luck testing this one: 1. For Step 5: As the koha user, after clicking the 'Disable two-factor authentication' for the patron (Henry in this case), I get a 404 page not found error and 2FA is not disabled for the patron (they are still prompted to enter the code when they log in). 2. After apply the patches, I did a yarn build and restart_all in case that was required. I also get the same result if Henry is a superlibrarian, or has a limited set of permissions including 'borrowers'. Created attachment 186235 [details] [review] Bug 30724: Allow superlibrarians to manage 2FA for other patrons This enhances the two-factor authentication management interface to allow superlibrarians to disable 2FA for other patrons when they lose access to their authenticator device. Controller changes (members/two_factor_auth.pl): * Accept borrowernumber parameter to select target patron * Implement authorization checks (self-service or superlibrarian) * Use proper HTTP status codes (403/404) for error conditions * Update all patron operations to use selected patron * Integrate with new reset_2fa() method for consistency * Pass another_user flag to template for conditional display Template changes (two_factor_auth.tt): * Use has_2fa_enabled() method instead of auth_method string comparison * Prevent superlibrarians from enabling 2FA for other users * Show explanatory message when 2FA setup is restricted * Maintain proper conditional display logic UI Integration changes: * Update members toolbar to show 2FA option for superlibrarians * Pass borrowernumber parameter in all 2FA-related URLs * Maintain context when canceling 2FA registration * Use consistent parameter naming (borrowernumber vs patron_id) To test: 1. Enable TwoFactorAuthentication system preference 2. Set up 2FA for a test patron 3. As superlibrarian, visit patron details page 4. Click 'Manage two-factor authentication' in toolbar => SUCCESS: 2FA management page loads for the selected patron 5. Disable 2FA for the patron => SUCCESS: 2FA is disabled for the target patron, not the superlibrarian 6. Verify 'Enable 2FA' button is hidden with explanatory text => SUCCESS: Shows message that users must enable 2FA themselves 7. Test authorization: try accessing as non-superlibrarian for different patron => FAIL: Returns 403 Forbidden error 8. Sign off :-D Sponsored-by: ByWater Solutions Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> Created attachment 186236 [details] [review] Bug 30724: (follow-up) Improve user interface text and messaging This enhances the user experience with clearer, more contextual messaging throughout the two-factor authentication management interface. Template improvements: * Add contextual page titles showing patron name when managing others * Replace generic status messages with personalized, descriptive text * Provide clear explanation of why 2FA setup is restricted for admins * Add actionable guidance for superlibrarians on available options * Use appropriate alert styling (success/info/warning) for different states * Include patron names in messages for better context The new messaging explains the security rationale behind restrictions and provides clear guidance on what actions are available to different user types. To test: 1. As superlibrarian, manage 2FA for another patron => SUCCESS: Page title shows "Two-factor authentication for [Name]" => SUCCESS: Status messages are personalized with patron name => SUCCESS: Clear explanation of setup restrictions with actionable guidance 2. Manage your own 2FA => SUCCESS: Messages use "Your account" language appropriately 3. Sign off :-D Sponsored-by: ByWater Solutions Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> Created attachment 186237 [details] [review] Bug 30724: (follow-up) Corrections for self-enabling 2FA I agree with David, there were bugs here.. I've added a follow-up which I believe resolves the issue.. but I'd love someone to go through the test plan again to make sure I'm not hallucinating. I'm also not really happy with the introduction of a TT variable in the script block.. I'm sure there's a better way but my brain hasn't got the processing power this late on a friday afternoon. De Morgan's law is correctly applied :-D
-unless ( !$another_user || $logged_in_user->is_superlibrarian() ) {
+if ( $another_user && !$logged_in_user->is_superlibrarian() ) {
I reckon I started with `unless $logged_in_as_user->is_superlibrarian()` and shouldn't rephrased as I added more conditions.
Created attachment 186251 [details] [review] Bug 30724: Allow superlibrarians to manage 2FA for other patrons This enhances the two-factor authentication management interface to allow superlibrarians to disable 2FA for other patrons when they lose access to their authenticator device. Controller changes (members/two_factor_auth.pl): * Accept borrowernumber parameter to select target patron * Implement authorization checks (self-service or superlibrarian) * Use proper HTTP status codes (403/404) for error conditions * Update all patron operations to use selected patron * Integrate with new reset_2fa() method for consistency * Pass another_user flag to template for conditional display Template changes (two_factor_auth.tt): * Use has_2fa_enabled() method instead of auth_method string comparison * Prevent superlibrarians from enabling 2FA for other users * Show explanatory message when 2FA setup is restricted * Maintain proper conditional display logic UI Integration changes: * Update members toolbar to show 2FA option for superlibrarians * Pass borrowernumber parameter in all 2FA-related URLs * Maintain context when canceling 2FA registration * Use consistent parameter naming (borrowernumber vs patron_id) To test: 1. Enable TwoFactorAuthentication system preference 2. Set up 2FA for a test patron 3. As superlibrarian, visit patron details page 4. Click 'Manage two-factor authentication' in toolbar => SUCCESS: 2FA management page loads for the selected patron 5. Disable 2FA for the patron => SUCCESS: 2FA is disabled for the target patron, not the superlibrarian 6. Verify 'Enable 2FA' button is hidden with explanatory text => SUCCESS: Shows message that users must enable 2FA themselves 7. Test authorization: try accessing as non-superlibrarian for different patron => FAIL: Returns 403 Forbidden error 8. Sign off :-D Sponsored-by: ByWater Solutions Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> Signed-off-by: David Nind <david@davidnind.com> Created attachment 186252 [details] [review] Bug 30724: (follow-up) Improve user interface text and messaging This enhances the user experience with clearer, more contextual messaging throughout the two-factor authentication management interface. Template improvements: * Add contextual page titles showing patron name when managing others * Replace generic status messages with personalized, descriptive text * Provide clear explanation of why 2FA setup is restricted for admins * Add actionable guidance for superlibrarians on available options * Use appropriate alert styling (success/info/warning) for different states * Include patron names in messages for better context The new messaging explains the security rationale behind restrictions and provides clear guidance on what actions are available to different user types. To test: 1. As superlibrarian, manage 2FA for another patron => SUCCESS: Page title shows "Two-factor authentication for [Name]" => SUCCESS: Status messages are personalized with patron name => SUCCESS: Clear explanation of setup restrictions with actionable guidance 2. Manage your own 2FA => SUCCESS: Messages use "Your account" language appropriately 3. Sign off :-D Sponsored-by: ByWater Solutions Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> Signed-off-by: David Nind <david@davidnind.com> Created attachment 186253 [details] [review] Bug 30724: (follow-up) Corrections for self-enabling 2FA Signed-off-by: David Nind <david@davidnind.com> Have added my sign-off as things now work as per the test plan. Nice work everyone! Pushed to main for 25.11 Tests are missing, and you broke the existing ones!
t/db_dependent/selenium/authentication_2fa.t ..
# Failed test 'Must be on the page with the pref on'
# at t/db_dependent/selenium/authentication_2fa.t line 73.
# 'Error 404 › Koha'
# doesn't match '(?^u:Two-factor authentication)'
Created attachment 187962 [details] [review] Bug 30724: Fix selenium tests - fix logic if no borrowernumber passed Then consider we want to edit the logged in user settings Created attachment 187963 [details] [review] Bug 30724: Fix selenium tests - wording Created attachment 187964 [details] [review] Bug 30724: Fix selenium tests - info => success class I don't think this is correct. We only want success after we enabled 2FA This third patch fixes the selenium tests but they are not correct IMO. We still need new tests to cover changes from this bug report. Created attachment 188037 [details] [review] Bug 30724: Fix selenium tests - fix logic if no borrowernumber passed Then consider we want to edit the logged in user settings Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 188038 [details] [review] Bug 30724: Fix selenium tests - wording Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 188039 [details] [review] Bug 30724: Fix selenium tests - info => success class I don't think this is correct. We only want success after we enabled 2FA Signed-off-by: Nick Clemens <nick@bywatersolutions.com> (In reply to Jonathan Druart from comment #25) > Created attachment 187964 [details] [review] [review] > Bug 30724: Fix selenium tests - info => success class > > I don't think this is correct. We only want success after we enabled 2FA I am not sure of the problem here - you check for alert-success - but only expect it not blank when 2FA enabled, unless I misread (In reply to Jonathan Druart from comment #26) > This third patch fixes the selenium tests but they are not correct IMO. > > We still need new tests to cover changes from this bug report. More tests are always good, opened bug 41038 follow-ups pushed to main (In reply to Nick Clemens (kidclamp) from comment #30) > (In reply to Jonathan Druart from comment #25) > > Created attachment 187964 [details] [review] [review] [review] > > Bug 30724: Fix selenium tests - info => success class > > > > I don't think this is correct. We only want success after we enabled 2FA > > I am not sure of the problem here - you check for alert-success - but only > expect it not blank when 2FA enabled, unless I misread We want the alert-success class only after the form is submitted. When you go to members/two_factor_auth.pl?borrowernumber=51 and the 2FA is enabled it should be alert-info. > (In reply to Jonathan Druart from comment #26) > > This third patch fixes the selenium tests but they are not correct IMO. > > > > We still need new tests to cover changes from this bug report. > > More tests are always good, opened bug 41038 It's not "always good", it's mandatory when it's auth and we have already tests covering the area ;) |