| Summary: | Re-login without enough permissions needs attention | ||
|---|---|---|---|
| Product: | Koha | Reporter: | Marcel de Rooy <m.de.rooy> |
| Component: | Architecture, internals, and plumbing | Assignee: | Marcel de Rooy <m.de.rooy> |
| Status: | RESOLVED FIXED | QA Contact: | Testopia <testopia> |
| Severity: | major | ||
| Priority: | P5 - low | CC: | 1joynelson, arthur.suzuki, dcook, didier.gautheron, fridolin.somers, jonathan.druart, laurent.ducos, lucas, m.de.rooy, martin.renvoize, nick, tomascohen, wainuiwitikapark |
| Version: | unspecified | ||
| Hardware: | All | ||
| OS: | All | ||
| Change sponsored?: | --- | Patch complexity: | Small patch |
| Documentation contact: | Documentation submission: | ||
| Text to go in the release notes: | Version(s) released in: |
22.11.01,22.05.08
|
|
| Circulation function: | |||
| Bug Depends on: | 31908 | ||
| Bug Blocks: | |||
| Attachments: |
Bug 32208: Extend Auth.t
Bug 32208: Adjust Auth.pm for relogin without perms Bug 32208: Extend Auth.t Bug 32208: Adjust Auth.pm for relogin without perms Bug 32208: Extend Auth.t Bug 32208: Adjust Auth.pm for relogin without perms |
||
|
Description
Marcel de Rooy
2022-11-15 13:54:34 UTC
Created attachment 143914 [details] [review] Bug 32208: Extend Auth.t Test plan: Without follow-up patch, Auth.t should fail: not ok 5 - No permission response not ok 6 - checkauth returned failure Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 143915 [details] [review] Bug 32208: Adjust Auth.pm for relogin without perms If a second login on top of a current session fails on permissions, we should not grant access without context. Test plan: [1] Run t/db../Auth.t, it should pass now. [2] Test interface with/without this patch: Pick two users: A has perms, B has not. Put two staff login forms in two tabs. Login as A in tab1. Login as B in tab2. Without this patch, B gets in and crashes. With this patch, B does not get in ('no perms'). Bonus: Go to opac if on same domain. You are still logged in as B. NOTE: I added a FIXME here, since you could argue about filling the session info or otoh deleting the session. We present an authorization failure; people may not realize that they are still logged in (see test plan - bonus). Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> This is just like 31908 a very small fix for a kind of edge case in regular Auth patterns. But it makes me wonder if similar things like here might be possible with all the Auth variations like CAS, Shibboleth, 2FA ? Created attachment 143946 [details] [review] Bug 32208: Extend Auth.t Test plan: Without follow-up patch, Auth.t should fail: not ok 5 - No permission response not ok 6 - checkauth returned failure Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 143947 [details] [review] Bug 32208: Adjust Auth.pm for relogin without perms If a second login on top of a current session fails on permissions, we should not grant access without context. Test plan: [1] Run t/db../Auth.t, it should pass now. [2] Test interface with/without this patch: Pick two users: A has perms, B has not. Put two staff login forms in two tabs. Login as A in tab1. Login as B in tab2. Without this patch, B gets in and crashes. With this patch, B does not get in ('no perms'). Bonus: Go to opac if on same domain. You are still logged in as B. NOTE: I added a FIXME here, since you could argue about filling the session info or otoh deleting the session. We present an authorization failure; people may not realize that they are still logged in (see test plan - bonus). Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> (In reply to Nick Clemens from comment #5) > Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Thanks! This patch can be applied on current 22.05.x branch too. This patch is not relevant for 21.11.x (in contrast with 31908 btw). We are still checking $logged_in and not the new $auth_state. Created attachment 143970 [details] [review] Bug 32208: Extend Auth.t Test plan: Without follow-up patch, Auth.t should fail: not ok 5 - No permission response not ok 6 - checkauth returned failure Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz> Created attachment 143971 [details] [review] Bug 32208: Adjust Auth.pm for relogin without perms If a second login on top of a current session fails on permissions, we should not grant access without context. Test plan: [1] Run t/db../Auth.t, it should pass now. [2] Test interface with/without this patch: Pick two users: A has perms, B has not. Put two staff login forms in two tabs. Login as A in tab1. Login as B in tab2. Without this patch, B gets in and crashes. With this patch, B does not get in ('no perms'). Bonus: Go to opac if on same domain. You are still logged in as B. NOTE: I added a FIXME here, since you could argue about filling the session info or otoh deleting the session. We present an authorization failure; people may not realize that they are still logged in (see test plan - bonus). Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz> (In reply to Marcel de Rooy from comment #3) > This is just like 31908 a very small fix for a kind of edge case in regular > Auth patterns. But it makes me wonder if similar things like here might be > possible with all the Auth variations like CAS, Shibboleth, 2FA ? All the more reason to make a group project of streamlining our AuthN/AuthZ code I reckon. (In reply to Chris Cormack from comment #10) > Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz> Thanks! (In reply to David Cook from comment #11) > (In reply to Marcel de Rooy from comment #3) > > This is just like 31908 a very small fix for a kind of edge case in regular > > Auth patterns. But it makes me wonder if similar things like here might be > > possible with all the Auth variations like CAS, Shibboleth, 2FA ? > > All the more reason to make a group project of streamlining our AuthN/AuthZ > code I reckon. Sure. Our community needs to improve on that skill. This would be a nice opportunity :) What is the status of this patch? Does this need to be backported to 21.05.x for security? Pushed to 22.11.x-security on the security repo for 22.11.01 release. Pushed to 21.05.x-security on the security repo for 21.05.22 release. This patch depends on 31908, which I didn't succeed to backport on 21.11.x. Will retry after 31908 is available on 21.11. (In reply to Arthur Suzuki from comment #18) > This patch depends on 31908, which I didn't succeed to backport on 21.11.x. > Will retry after 31908 is available on 21.11. I have 31908 working OK, but this patch relies on some Auth refactoring that has taken place in 22.05 and beyond.. The new test fails for 21.11.x but the fix doesn't work. Tomas and I are working through it now. (In reply to wainuiwitikapark from comment #17) > Pushed to 21.05.x-security on the security repo for 21.05.22 release. This patch is not meant for 21.05 and 21.11. See comment8. reverted on 21.11.x |