|
Description
Nick Clemens (kidclamp)
2022-10-21 12:54:28 UTC
Created attachment 142361 [details] [review] Bug 31908: Only count local suggestions if there is a userenv Hmm..I though maybe we could just fix the front end, but it gets weird Apply patch Sign in as unauthorised user You are not allowed Sign in as authorised user Note username refers to unauthorised user Not moving to security - you must have a vlid users authentication to get in - and accessing any page immediately logs you out again, but this is weird and bad, upping severity - my $local_pendingsuggestions_count = $pendingsuggestions->search({ 'me.branchcode' => C4::Context->userenv()->{'branch'} })->count();
+ if( C4::Context->userenv() && C4::Context->userenv()->{branch} ){
In this case there should be a userenv. Why it isnt? Reported this too on bug 32075. Weird.
(In reply to Nick Clemens from comment #2) > Hmm..I though maybe we could just fix the front end, but it gets weird > > Apply patch > Sign in as unauthorised user > You are not allowed > Sign in as authorised user > Note username refers to unauthorised user Without your patch, this triggers: Can't use an undefined value as a HASH reference at /usr/share/koha/mainpage.pl line 88 This brings us closer to finding the cause ! *** Bug 32075 has been marked as a duplicate of this bug. *** Digging a bit further. With your test plan in mind I tend to not trust these lines in Auth:
else {
$info{'nopermission'} = 1;
C4::Context::_unset_userenv($sessionID);
}
Apparently not the above.
But this seems to work:
#if a user enters an id ne to the id in the current session, we need to log them in...
#first we need to clear the anonymous session...
$anon_search_history = $session->param('search_history');
$session->delete();
$session->flush;
$cookie = $cookie_mgr->clear_unless( $query->cookie, @$cookie );
C4::Context::_unset_userenv($sessionID);
$sessionID = undef;
undef $userid;
If we clear $userid here, it will go into the unless($userid) block and get a new session. Note that $userid and $q_userid are not equal here !
From 20.11.x:
#if a user enters an id ne to the id in the current session, we need to log them in...
#first we need to clear the anonymous session...
$debug and warn "query id = $q_userid but session id = $s_userid";
$anon_search_history = $session->param('search_history');
$session->delete();
$session->flush;
C4::Context->_unset_userenv($sessionID);
$sessionID = undef;
$userid = undef;
See former comment too.
Note that probably in the 2FA changes, this $userid line got "lost" somewhere.
So I propose to reinsert it here.
Created attachment 143392 [details] [review] Bug 31908: Resolve second login with another userid Somewhere the line undef $userid got removed. We need it to resolve the second login situation. Test plan: Login in staff with user missing privileges. On the login form login again with another staff user. Note that you do no longer crash. Run t/db../Auth.t Run t/db../Koha/Auth/TwoFactorAuth.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Comment on attachment 142361 [details] [review] Bug 31908: Only count local suggestions if there is a userenv This patch should no longer be needed You can recreate the problem too by logging into the OPAC with user A, and on another tab login with user B on staff. (In reply to Marcel de Rooy from comment #12) > You can recreate the problem too by logging into the OPAC with user A, and > on another tab login with user B on staff. And if you try to replicate with two OPAC tabs, the second tab will not login but it will logout the first one too. Nice. Resolved with the patch. (In reply to Nick Clemens from comment #3) > Not moving to security - you must have a vlid users authentication to get in > - and accessing any page immediately logs you out again, but this is weird > and bad, upping severity Decided to move it to Security now to be safe. Rmaints: This bug is present too in 21.11 and 22.05. Isn't it that cookies constrained by domain, and you are using localhost for both? Using localhost:8080 and localhost:8081 will be considered the same domain and thus the conflicting situation. Did you identify the commit that caused this? (In reply to Jonathan Druart from comment #17) > Did you identify the commit that caused this? Caused by commit 6545259bbf8a8683ade45c65aa57a00a8981fbae (HEAD) Bug 28785: Centralize cookie auth check in check_cookie_auth (In reply to Tomás Cohen Arazi from comment #16) > Isn't it that cookies constrained by domain, and you are using localhost for > both? Using localhost:8080 and localhost:8081 will be considered the same > domain and thus the conflicting situation. This is not relevant here. You can reproduce on opac only or on staff only too. Shouldn't we set $logout to reach the following block?
936 if ( $auth_state eq 'failed' || $logout ) {
937 $sessionID = undef;
938 $userid = undef;
939 }
(In reply to Jonathan Druart from comment #20) > Shouldn't we set $logout to reach the following block? > > 936 if ( $auth_state eq 'failed' || $logout ) { > 937 $sessionID = undef; > 938 $userid = undef; > 939 } No it is not a logout. We should do it as in the patch ;) (In reply to Marcel de Rooy from comment #18) > (In reply to Jonathan Druart from comment #17) > > Did you identify the commit that caused this? > > Caused by > commit 6545259bbf8a8683ade45c65aa57a00a8981fbae (HEAD) > > Bug 28785: Centralize cookie auth check in check_cookie_auth Yes, the removal. The code was correct at this point, because the test later was unless ( $loggedin ) My guess is that the bug has been introduced by commit 2c2c3662349446acefb7f850088cfd58500d0e67 Bug 28786: Improve readability in C4::Auth::checkauth I will try and have a look later this week, at least at providing some tests. My suggestion to set $logout may not be perfect, the idea was to have the 2 undef statements clearly put in a single place, and understand in which different cases we are reaching them. So maybe a separate flag? This sounds interesting. Tempted to take a peek as well... (In reply to Jonathan Druart from comment #22) > I will try and have a look later this week, at least at providing some tests. > My suggestion to set $logout may not be perfect, the idea was to have the 2 > undef statements clearly put in a single place, and understand in which > different cases we are reaching them. So maybe a separate flag? An additional test would be great. I think that we should restore this single line here and not make things more complicated. Clearing the old userid from the old cookie is logical since it is no longer valid with a new login. And this also makes us get in the unless block to get a new session to replace the old session. Adding yet another flag does not make things easier. And yes, the whole module needs a rewrite. But not here. Can we get this moving in the meantime? Ok. But it's missing tests anyway... Created attachment 143491 [details] [review] Bug 31908: Resolve second login with another userid Somewhere the line undef $userid got removed. We need it to resolve the second login situation. Test plan: Login in staff with user missing privileges. On the login form login again with another staff user. Note that you do no longer crash. Run t/db../Auth.t Run t/db../Koha/Auth/TwoFactorAuth.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> In my 21.11 version the backport has quite another face btw:
diff --git a/C4/Auth.pm b/C4/Auth.pm
index b2d49b3..d568e59 100644
--- a/C4/Auth.pm
+++ b/C4/Auth.pm
@@ -958,8 +958,9 @@ sub checkauth {
}
unless ( $userid ) {
- #we initiate a session prior to checking for a username to allow for anonymous sessions...
- $session ||= get_session("") or die "Auth ERROR: Cannot get_session()";
+ if( !$session or !$sessionID ) { # if we cleared sessionID, we need a new session
+ $session = get_session() or die "Auth ERROR: Cannot get_session()";
+ }
# Save anonymous search history in new session so it can be retrieved
# by get_template_and_user to store it in user's search history after
In 21.11 we need the new condition around get_session from master, and we dont need the undef $userid. Due to all changes in the meantime..
Looked at 22.05 now. The current patch should be fine for that branch. So only 21.11 needs an adjusted version as in previous comment. Created attachment 143786 [details] [review] Bug 31908: Add selenium tests I've failed to provide test to C4::Auth and so provide selenium tests. But it would be great of somebody else had a try. (In reply to Jonathan Druart from comment #31) > I've failed to provide test to C4::Auth and so provide selenium tests. But > it would be great of somebody else had a try. Will give it a try now Created attachment 143819 [details] [review] Bug 31908: Replace an exit by a safe_exit in Auth.pm L1314 No change in user experience. But since we can mock safe_exit, we can enhance test results. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 143820 [details] [review] Bug 31908: Add a test to show issue Test plan: Without next patch, run Auth.t. Should fail now before next patch resolves problem: not ok 2 - Login of patron2 approved ok 3 - Did not return previous session ID not ok 4 - New session ID not empty Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 143821 [details] [review] Bug 31908: Resolve second login with another userid Somewhere the line undef $userid got removed. We need it to resolve the second login situation. Test plan: Login in staff with user missing privileges. On the login form login again with another staff user. Note that you do no longer crash. Run t/db../Auth.t Run t/db../Koha/Auth/TwoFactorAuth.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 143822 [details] [review] Bug 31908: Add selenium tests OK Test included. QA team: Please go ahead Please In order to get the selenium test to work, I had to change my base URLs like so in koha-testing-docker: OPACBaseURL: http://koha_koha_1:8080 staffClientBaseURL: http://koha_koha_1:8081 -- Overall, this is a straightforward change. Created attachment 143884 [details] [review] Bug 31908: Replace an exit by a safe_exit in Auth.pm L1314 No change in user experience. But since we can mock safe_exit, we can enhance test results. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: David Cook <dcook@prosentient.com.au> Created attachment 143885 [details] [review] Bug 31908: Add a test to show issue Test plan: Without next patch, run Auth.t. Should fail now before next patch resolves problem: not ok 2 - Login of patron2 approved ok 3 - Did not return previous session ID not ok 4 - New session ID not empty Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: David Cook <dcook@prosentient.com.au> Created attachment 143886 [details] [review] Bug 31908: Resolve second login with another userid Somewhere the line undef $userid got removed. We need it to resolve the second login situation. Test plan: Login in staff with user missing privileges. On the login form login again with another staff user. Note that you do no longer crash. Run t/db../Auth.t Run t/db../Koha/Auth/TwoFactorAuth.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: David Cook <dcook@prosentient.com.au> Created attachment 143887 [details] [review] Bug 31908: Add selenium tests Signed-off-by: David Cook <dcook@prosentient.com.au> Passing QA as it is simple and it works.
--
Side note:
The block for clearing the session would actually be amenable to putting in a function.
In future, something like the following perhaps:
($anon_search_history,$session,$cookie,$sessionID,$userid) = clear_session({
session => $session,
cookie_mgr => $cookie_mgr,
cookie => $cookie,
req_cookie => $query->cookie,
});
Speaking of refactoring, this bug has inspired me to take a little look at bug 32203... Just saying, but it took 6 months for (trivial) 27342 to be pushed. It's not giving me much energy and confidence to continue C4::Auth cleaning. (In reply to Jonathan Druart from comment #46) > Just saying, but it took 6 months for (trivial) 27342 to be pushed. It's not > giving me much energy and confidence to continue C4::Auth cleaning. Indeed, it takes too long. But I think that you could agree that the need for C4::Auth refactoring/rewriting only has increased by now? Recent discussion on one of the other reports.. 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. Unfortunately the patch doesn't apply clean on 21.11.x branch... Trying to backport though. Well, although I manage to solve the conflicts it doesn't solve the issue. Tried different thing with no success so far... I can definitely reproduce on 21.11 though. The patch provided, or which I tried to backport from 22.05.x doesn't solve the issue on 21.11 (also conflicts when applying). Can you provide a backport for 21.11.x? (In reply to Arthur Suzuki from comment #54) > The patch provided, or which I tried to backport from 22.05.x doesn't solve > the issue on 21.11 (also conflicts when applying). > Can you provide a backport for 21.11.x? Did you see comment28 ? Created attachment 145092 [details] [review] Bug 31908: [21.11.x] Resolve second login with another userid As near as I can tell, the problem is that we don't get a new session because while we flish the session, we don't clear the variable I don't fully understand how this all works, but this seems to work Created attachment 145093 [details] [review] Bug 31908: [21.11.x] Add a test to show issue Test plan: Without next patch, run Auth.t. Should fail now before next patch resolves problem: not ok 2 - Login of patron2 approved ok 3 - Did not return previous session ID not ok 4 - New session ID not empty Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: David Cook <dcook@prosentient.com.au> I got the unit tests passing, but not selenium, though they were failing on code I don't think this patch touched Patch provided by Nick is fully working, thx! Applied to 21.11.x-security for next release (21.11.15 I think) (In reply to Marcel de Rooy from comment #55) > (In reply to Arthur Suzuki from comment #54) > > The patch provided, or which I tried to backport from 22.05.x doesn't solve > > the issue on 21.11 (also conflicts when applying). > > Can you provide a backport for 21.11.x? > > Did you see comment28 ? Arthur: Comment28 contained my version. Ignored since November 9 ? Hi, so this fails to compile on 21.05.x due to bug 29957 no being there to provide $cookie_mgr. It wasn't backported because it conflicted and it looks very hard to backport. 21.05.x also lacks $auth_state so it needs commit 610644ae89b97ee27864eaff8d0f16f351356ea9 from 21.11.x) Bug 32208: (follow-up) Fix compile issue This one will be simple to apply. Bug 28785 is also lacking for $return and $more_info. It looks very hard to backport. Due to the two hard (and maybe impossible) to backport dependencies it seems it's needed to have the changes of this bug be remade on top of 21.05.x. Something like having the 21.11.x version of the patch side by side with 21.05.x and copying what can be and reimplementing the rest. (In reply to Victor Grousset/tuxayo from comment #61) > Hi, so this fails to compile on 21.05.x due to bug 29957 no being there to > provide $cookie_mgr. It wasn't backported because it conflicted and it looks > very hard to backport. > > > 21.05.x also lacks $auth_state so it needs commit > 610644ae89b97ee27864eaff8d0f16f351356ea9 from 21.11.x) > Bug 32208: (follow-up) Fix compile issue > This one will be simple to apply. > > > Bug 28785 is also lacking for $return and $more_info. It looks very hard to > backport. > > > Due to the two hard (and maybe impossible) to backport dependencies it seems > it's needed to have the changes of this bug be remade on top of 21.05.x. > Something like having the 21.11.x version of the patch side by side with > 21.05.x and copying what can be and reimplementing the rest. See comment15. You do not need it in 21.05. 21.11 has a failing test 02:29:36 koha_1 | # Failed test 'New session ID not empty' 02:29:36 koha_1 | # at t/db_dependent/Auth.t line 151. 02:29:36 koha_1 | Use of uninitialized value in string eq at /kohadevbox/koha/C4/Context.pm line 804. 02:29:36 koha_1 | Use of uninitialized value $sessionID in string eq at /kohadevbox/koha/C4/Context.pm line 804. 02:29:36 koha_1 | # Looks like you failed 1 test of 6. 02:29:36 koha_1 | 02:29:36 koha_1 | # Failed test 'While still logged in, relogin with another user' 02:29:36 koha_1 | # at t/db_dependent/Auth.t line 175. 02:29:36 koha_1 | # Looks like you failed 1 test of 5. 02:29:36 koha_1 | 02:29:36 koha_1 | # Failed test 'checkauth() tests' 02:29:36 koha_1 | # at t/db_dependent/Auth.t line 179. Marcel, do you think you can have a look at this? Also, 21.05 is totally broken, the version of the following commit is wrong: commit 68cde441b325a8ceabe44355d616507e84d03783 Bug 31908: Resolve second login with another userid All jenkins builds are failing for 21.05 with a compilation error: Global symbol "$cookie_mgr" requires explicit package name (did you forget to declare "my $cookie_mgr"?) at /kohadevbox/koha/C4/Auth.pm line 953. Marcel said it's not needed for 21.05, so we need to revert bug 32208 and bug 31908 from this branch. (In reply to Marcel de Rooy from comment #62) > See comment15. You do not need it in 21.05. Thanks a lot! I had that feeling after my comment and tried to test yesterday but for some reason my 21.05 env doesn't start... (on an older revision of course). I emailed and pinged Wainui on the chat so they don't loose time checking for being affected as I suggested them yesterday. (In reply to Jonathan Druart from comment #63) > 21.11 has a failing test > > 02:29:36 koha_1 | # Failed test 'New session ID not empty' > 02:29:36 koha_1 | # at t/db_dependent/Auth.t line 151. > 02:29:36 koha_1 | Use of uninitialized value in string eq at > /kohadevbox/koha/C4/Context.pm line 804. > 02:29:36 koha_1 | Use of uninitialized value $sessionID in string eq > at /kohadevbox/koha/C4/Context.pm line 804. > 02:29:36 koha_1 | # Looks like you failed 1 test of 6. > 02:29:36 koha_1 | > 02:29:36 koha_1 | # Failed test 'While still logged in, relogin > with another user' > 02:29:36 koha_1 | # at t/db_dependent/Auth.t line 175. > 02:29:36 koha_1 | # Looks like you failed 1 test of 5. > 02:29:36 koha_1 | > 02:29:36 koha_1 | # Failed test 'checkauth() tests' > 02:29:36 koha_1 | # at t/db_dependent/Auth.t line 179. > > Marcel, do you think you can have a look at this? Will be looking now. I really do not understand what has happened on 21.11 while all info was here on the report. Furthermore I see that Nick submitted patches, but it seems they were not pushed to 21.11. It would be helpful to remove them here if you do so. And as explained before, these patches should not have been pushed to 21.11 in its current form. I responded to your question, Arthur, but you just ignored it. I am going to send a few patches here now. Hopefully you will take a look !! Comment on attachment 145092 [details] [review] Bug 31908: [21.11.x] Resolve second login with another userid Removing. Has not been pushed. Comment on attachment 145093 [details] [review] Bug 31908: [21.11.x] Add a test to show issue Removing. Not sure about status. It became a mess on 21.11. Created attachment 145250 [details] [review] Bug 31908: [21.11.x] Revert "Resolve second login with another userid" This reverts commit 0a89528d580596a694b05d3853a7627ac75dd5df. Created attachment 145251 [details] [review] Bug 31908: [21.11.x] Fix Auth.pm Created attachment 145252 [details] [review] Bug 31908: [21.11.x] Fix Auth.t Arthur: Please push the last three patches on top of 21.11.x.
Please test Auth.t yourself too.
Note that I had to add one line on another location (syncing with master) to address this test failure:
not ok 11
# Failed test at t/db_dependent/Auth.t line 617.
# got: '9a78ed8d04cdc9d206ed2f37ad5e53ee'
# expected: undef
With your added patches I got the tests to pass on my 21.11.x branch, thx. Just pushed the patches to the public repos |