|
Description
Nick Clemens (kidclamp)
2023-08-10 11:53:30 UTC
Further testing - it seems if you simply enter an invalid username when you get the permissions needed log in, you get in every time (In reply to Nick Clemens from comment #1) > Further testing - it seems if you simply enter an invalid username when you > get the permissions needed log in, you get in every time Confirmed using bug 34449 and bug 34513 test plans together. If I understand the flow... 1. Log in as "catalogue" permissioned user 2. Go to http://localhost:8081/cgi-bin/koha/admin/background_jobs.pl 3. Do a syspref search 4. Note the auth screen "Error: You do not have permission to access this page" 5. Log in as "blah" with password "blah" 6. Note that the syspref screen loads (but if you go to any other pages you've actually been logged out) -- So a shorter version: 1. Log in as "catalogue" permissioned user 2. Go to http://localhost:8081/cgi-bin/koha/admin/preferences.pl?tab=&op=search&searchfield=baseurl 3. Note the auth screen "Error: You do not have permission to access this page" 4. Log in as "blah" with password "blah" 5. Note that the syspref screen loads (but if you go to any other pages you've actually been logged out) -- It's worth noting that if you go to http://localhost:8081/cgi-bin/koha/admin/preferences.pl?tab=&op=search&searchfield=baseurl as an anonymous user and try to type in blah/blah, it won't let you in Confirming that it works with any page (with varying degrees of impact) such as http://localhost:8081/cgi-bin/koha/tools/additional-contents.pl?category=html_customizations So it looks like a problem with the core auth code... [U+1F973][U+1F389][U+1F38A][U+1FA85][U+1FAA9] Ok, so it turns out you don't need any permissions - you just need an authenticated user. So it's looking like an AuthZ problem. 1. Go to http://localhost:8081/cgi-bin/koha/admin/preferences.pl?tab=&op=search&searchfield=baseurl 2. Log in as an OPAC user with 0 permissions 3. Note the auth screen "Error: You do not have permission to access this page" 4. Type "blah" into the username and password fields and click "Log in" 5. Read the system preference search results Raising the importance on this one because it's very not good. You also don't need to supply any garbage username/password. You can just click "Log in" 1. Go to http://localhost:8081/cgi-bin/koha/admin/preferences.pl?tab=&op=search&searchfield=baseurl 2. Log in as an OPAC user with 0 permissions 3. Note the auth screen "Error: You do not have permission to access this page" 4. Read the system preference search results I've got a fix for this. I'll be posting it in a couple hours. Have to run to a meeting. Created attachment 154357 [details] [review] Bug 34513: Set auth state correctly when changing auth sessions This patch sets the $auth_state to failed when changing auth sessions, so that the new login attempt gets processed correctly (instead of skipping the authorization step). Test plan: 0. Apply the patch 1. koha-plack --reload kohadev 2. Go to http://localhost:8081/cgi-bin/koha/admin/preferences.pl?tab=&op=search&searchfield=baseurl 3. Log in as an OPAC user with 0 permissions 4. Note the auth screen "Error: You do not have permission to access this page" 5. Click "Log in" 6. Note that you're still shown a login screen (and that you've been logged out of your previous authenticated session) After that, Maybe we should create for access denied a different page than auth page. Simply an alert text, 'previous page' button and a link to auth page (Log in as a different user). (In reply to Fridolin Somers from comment #10) > After that, > > Maybe we should create for access denied a different page than auth page. > Simply an alert text, 'previous page' button and a link to auth page (Log in > as a different user). That's a good point. I've had people say to me "Koha logged me out" and I've had to say to them "No, it says you're not authorized to view that page". So it probably would be a good idea to clarify the not authorized screen. I know everyone's travelling for Kohacon, but this would be a really good one to get tested and pushed! It's a 1 line fix which closes up a massive hole. We need tests... (In reply to Jonathan Druart from comment #13) > We need tests... In general or for this change? I think that a lot of our AuthN and AuthZ code is needlessly complex. A lot of our code could be condensed down into two simple AuthN and AuthZ steps like I've previously done here: https://bugs.koha-community.org/bugzilla3/page.cgi?id=splinter.html&bug=31380&attachment=144969 First we authenticate and then we authorize. If we can't authenticate, then we can't authorize (unless the authorization step is explicitly that no authorization is required). (In reply to David Cook from comment #14) > (In reply to Jonathan Druart from comment #13) > > We need tests... > > In general or for this change? We need tests for this change. I have quickly written that, thinking it would fail on master but it passes.
subtest "foo" => sub {
plan tests => 3;
my $patron = $builder->build_object( { class => 'Koha::Patrons', value => { flags => 4 } } );
my $password = 'password';
t::lib::Mocks::mock_preference( 'RequireStrongPassword', 0 );
$patron->set_password( { password => $password } );
my $cgi_mock = Test::MockModule->new('CGI');
$cgi_mock->mock( 'request_method', sub { return 'POST' } );
my $cgi = CGI->new;
my $auth = Test::MockModule->new( 'C4::Auth' );
$auth->mock( 'safe_exit', sub { return } );
$cgi->param( 'userid', $patron->userid );
$cgi->param( 'password', $password );
# Logged in!
my ( $userid, $cookie, $sessionID, $flags ) = C4::Auth::checkauth( $cgi, 0, { catalogue => 1 } );
is( $userid, $patron->userid );
my $first_sessionID = $sessionID;
( $userid, $cookie, $sessionID, $flags ) = C4::Auth::checkauth( $cgi, 0, { borrowers => 1 } );
is( $userid, undef);
isnt( $sessionID, $first_sessionID);
};
We have quite good testing in t/db_dependent/selenium/authentication.t if you prefer to prove end-to-end tests. But ideally we should have both. The test is wrong. I did not set the sessionid in HTTP_COOKIE env var. (In reply to Jonathan Druart from comment #17) > We have quite good testing in t/db_dependent/selenium/authentication.t if > you prefer to prove end-to-end tests. But ideally we should have both. I've never managed to get the kohadev-intra.mydnsname.org stuff to work with k-t-d. Maybe I should have another crack at that... (In reply to Jonathan Druart from comment #17) > We have quite good testing in t/db_dependent/selenium/authentication.t if > you prefer to prove end-to-end tests. But ideally we should have both. Ok I've got those running now. I'll look at providing a test for that... Created attachment 154439 [details] [review] Bug 34513: Add end-to-end test for authorization check after first failed authorization Created attachment 154440 [details] [review] Bug 34513: Add checkauth unit test for resetting auth state when changing users Both those tests will fail on master but succeed with this patch. I get a fail on the tests with the patch applied
# Failed test 'Trying to change to a non-existent user should fail login'
# at /kohadevbox/koha/t/db_dependent/selenium/authentication.t line 61.
# 'Error: Can't use an undefined value as a HASH reference at /kohadevbox/koha/mainpage.pl line 93'
# doesn't match '(?^u:Invalid username or password)'
# Looks like you failed 1 test of 7.
(In reply to Chris Cormack from comment #24) > I get a fail on the tests with the patch applied > > # Failed test 'Trying to change to a non-existent user should fail login' > # at /kohadevbox/koha/t/db_dependent/selenium/authentication.t line 61. > # 'Error: Can't use an undefined value as a HASH > reference at /kohadevbox/koha/mainpage.pl line 93' > # doesn't match '(?^u:Invalid username or password)' > # Looks like you failed 1 test of 7. Did you restart your Starman after applying the patch? That's the error you should see for an unpatched Koha. Looking back, it looks like I forgot to provide a test plan. If I recall correctly, it goes something like this: Test plan: 0. Apply patch 1. koha-plack --restart kohadev 2. perl /kohadevbox/koha/t/db_dependent/selenium/authentication.t Hoping that some folk can look at this once they're back from Kohacon. Created attachment 154743 [details] [review] Bug 34513: Set auth state correctly when changing auth sessions This patch sets the $auth_state to failed when changing auth sessions, so that the new login attempt gets processed correctly (instead of skipping the authorization step). Test plan: 0. Apply the patch 1. koha-plack --reload kohadev 2. Go to http://localhost:8081/cgi-bin/koha/admin/preferences.pl?tab=&op=search&searchfield=baseurl 3. Log in as an OPAC user with 0 permissions 4. Note the auth screen "Error: You do not have permission to access this page" 5. Click "Log in" 6. Note that you're still shown a login screen (and that you've been logged out of your previous authenticated session) Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 154744 [details] [review] Bug 34513: Add end-to-end test for authorization check after first failed authorization Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 154745 [details] [review] Bug 34513: Add checkauth unit test for resetting auth state when changing users Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 154746 [details] [review] Bug 34513: (QA follow-up) Tidy QA: Looking here now Created attachment 154781 [details] [review] Bug 34513: Set auth state correctly when changing auth sessions This patch sets the $auth_state to failed when changing auth sessions, so that the new login attempt gets processed correctly (instead of skipping the authorization step). Test plan: 0. Apply the patch 1. koha-plack --reload kohadev 2. Go to http://localhost:8081/cgi-bin/koha/admin/preferences.pl?tab=&op=search&searchfield=baseurl 3. Log in as an OPAC user with 0 permissions 4. Note the auth screen "Error: You do not have permission to access this page" 5. Click "Log in" 6. Note that you're still shown a login screen (and that you've been logged out of your previous authenticated session) Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 154782 [details] [review] Bug 34513: Add end-to-end test for authorization check after first failed authorization Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 154783 [details] [review] Bug 34513: Add checkauth unit test for resetting auth state when changing users Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 154784 [details] [review] Bug 34513: (QA follow-up) Tidy Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Did not test Selenium. But could reproduce undesired behavior without patch and could no longer with patch. OK C4/Auth.pm OK critic OK forbidden patterns OK git manipulation OK pod OK pod coverage SKIP spelling OK tidiness OK valid OK t/db_dependent/Auth.t OK critic OK forbidden patterns OK git manipulation OK pod SKIP spelling OK tidiness OK valid OK t/db_dependent/selenium/authentication.t OK critic OK forbidden patterns OK git manipulation OK pod SKIP spelling OK tidiness OK valid Joubu: no word about spelling :) Much appreciated folks :) Pushed to master for 23.11. Nice work everyone, thanks! |