We don't want the secret to be available from the reports admin and so we must encrypt them.
Created attachment 124772 [details] [review] Bug 28998: Store encrypted secret
Created attachment 124773 [details] [review] Bug 28998: Add encryption_key to the config file
Created attachment 124774 [details] [review] Bug 28998: Add Crypt::CBC dependency
How do we generate the new config entry on upgrade?
This is looking great.. thanks for taking the time on it.. I'll swing back round to this once I've completed working through the 2FA bug.
242722e99df77be9ce0e4c6d52ff402e0a77d604 might help with generating the random key at koha-create.. need to dig out how we did it for upgrades
This is what we do with the API secret using Crypt::Eksblowfish::Bcrypt : $self->set( { secret => Koha::AuthUtils::hash_password( $self->{_plain_text_secret} ), client_id => $self->_generate_unused_uuid('client_id'), } Just reading a bit, it seems that AES is preferred over Blowfish. But this is theoretical and about larger texts to encrypt. Since we are only crypting a short secret here, I wonder if we should not stick to Blowfish here and use what we already have instead of yet another module?
Question: shouldn't we entirely move the 'secret' column to a separate table and add the table to a deny-list?
Looking here. Might propose some changes?
(In reply to Marcel de Rooy from comment #7) > Since we are only crypting a short secret here, I wonder if we should not > stick to Blowfish here and use what we already have instead of yet another > module? Using Crypt::Eksblowfish is a bit of a pain (you need to chunk yourself, add padding etc.) So just scratch that.
Created attachment 133291 [details] [review] Bug 28998: Introduce Koha::Encryption Test plan: Run t/db_dependent/Koha/Encryption.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 133317 [details] [review] Bug 28998: Introduce Koha::Encryption Test plan: Run t/db_dependent/Koha/Encryption.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> [AMENDED] Added copyright line to module.
Created attachment 133318 [details] [review] Bug 28998: (follow-up) Add Patron->encode_secret and ->decoded_secret Test plan: Run t/db_dependent/Koha/Patron.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 133319 [details] [review] Bug 28998: (follow-up) Apply changes to TwoFactorAuth module and script Test plan: Run t/db_dependent/Koha/Auth/TwoFactorAuth.t Walk thru cycle register - logout/login - deregister - logout/login. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 133320 [details] [review] Bug 28998: Adjust Selenium test Test plan: Run t/db_dependent/selenium/authentication_2fa.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 133321 [details] [review] Bug 28998: [TO_BE_SQUASHED] Adjust secret in Selenium test This patch can be squashed with the previous one, if all is ok. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 133322 [details] [review] Bug 28998: Add encryption_key to the config file Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 133323 [details] [review] Bug 28998: Add Crypt::CBC dependency Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
(In reply to Marcel de Rooy from comment #17) > Created attachment 133322 [details] [review] [review] > Bug 28998: Add encryption_key to the config file > > Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Recently a QAer insisted that I should adjust koha-create when adding koha-conf.xml entries :)
Reworked this a bit and tested on top of 28786 - 29873 - 29894. Main thing is splitting the first patch and moving some code to Koha::Patron. Taking the liberty to set it to Signed Off with one request: Please run the Selenium test. And if you agree, squash patch 4 and patch 5. If you would not agree, feel free to change status and tell me about it :)
Created attachment 133523 [details] [review] Bug 28998: Introduce Koha::Encryption Test plan: Run t/db_dependent/Koha/Encryption.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> [AMENDED] Added copyright line to module.
Created attachment 133524 [details] [review] Bug 28998: (follow-up) Add Patron->encode_secret and ->decoded_secret Test plan: Run t/db_dependent/Koha/Patron.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 133525 [details] [review] Bug 28998: (follow-up) Apply changes to TwoFactorAuth module and script Test plan: Run t/db_dependent/Koha/Auth/TwoFactorAuth.t Walk thru cycle register - logout/login - deregister - logout/login. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 133526 [details] [review] Bug 28998: Adjust Selenium test Test plan: Run t/db_dependent/selenium/authentication_2fa.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 133527 [details] [review] Bug 28998: [TO_BE_SQUASHED] Adjust secret in Selenium test This patch can be squashed with the previous one, if all is ok. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 133528 [details] [review] Bug 28998: Add encryption_key to the config file Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 133529 [details] [review] Bug 28998: Add Crypt::CBC dependency Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Trivial rebase on Patron
Created attachment 133580 [details] [review] Bug 28998: Introduce Koha::Encryption Test plan: Run t/db_dependent/Koha/Encryption.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> [AMENDED] Added copyright line to module. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 133581 [details] [review] Bug 28998: (follow-up) Add Patron->encode_secret and ->decoded_secret Test plan: Run t/db_dependent/Koha/Patron.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 133582 [details] [review] Bug 28998: (follow-up) Apply changes to TwoFactorAuth module and script Test plan: Run t/db_dependent/Koha/Auth/TwoFactorAuth.t Walk thru cycle register - logout/login - deregister - logout/login. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 133583 [details] [review] Bug 28998: Adjust Selenium test Test plan: Run t/db_dependent/selenium/authentication_2fa.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 133584 [details] [review] Bug 28998: [TO_BE_SQUASHED] Adjust secret in Selenium test This patch can be squashed with the previous one, if all is ok. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 133585 [details] [review] Bug 28998: Add encryption_key to the config file Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 133586 [details] [review] Bug 28998: Add Crypt::CBC dependency Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
This all makes a lot of sense to me.. I can see a case in the future for a distinct table, but I'm not aware of an allow-list on reports yet so that can come later in my opinion. I wonder slightly about the accessor names in Koha::Patron.. we could just silently encrypt/descrypt and continue to rely on 'secret' as our accessor name.. but that's not a big issue. I also wonder if the field in general should be clearer.. this secret us specific to this form of on time password for 2FA.. calling is 'secret' is very generic.. but again.. that's from the earlier bug. As is it.. PQA
Created attachment 133588 [details] [review] Bug 28998: (QA follow-up) POD Fix This POD fix keeps the QA script happy Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Still working here.. I just found it not working on my checkout and I'm not sure why.. setting back to give me time to work backwards.
OK, I spotted it.. I had dropped the config between first and second test. So.. I think this highlights an issue.. we should probably try/catch to give a better error than the 500 we currently throw on lack of configuration.. or fallback to unencrypted secrets when the key is missing.. but that re-introduces the security issue. Might even be worth a script to quickly encrypt un-encrypted secrets at upgrade if we don't get this pushed prior to release
Looking here
Created attachment 133619 [details] [review] Bug 28998: Introduce Koha::Encryption Test plan: Run t/db_dependent/Koha/Encryption.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> [AMENDED] Added copyright line to module. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 133620 [details] [review] Bug 28998: (follow-up) Add Patron->encode_secret and ->decoded_secret Test plan: Run t/db_dependent/Koha/Patron.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 133621 [details] [review] Bug 28998: (follow-up) Apply changes to TwoFactorAuth module and script Test plan: Run t/db_dependent/Koha/Auth/TwoFactorAuth.t Walk thru cycle register - logout/login - deregister - logout/login. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 133622 [details] [review] Bug 28998: Adjust Selenium test Test plan: Run t/db_dependent/selenium/authentication_2fa.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 133623 [details] [review] Bug 28998: [TO_BE_SQUASHED] Adjust secret in Selenium test This patch can be squashed with the previous one, if all is ok. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 133624 [details] [review] Bug 28998: Add encryption_key to the config file Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 133625 [details] [review] Bug 28998: Add Crypt::CBC dependency Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 133626 [details] [review] Bug 28998: (QA follow-up) POD Fix This POD fix keeps the QA script happy Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 133627 [details] [review] Bug 28998: (follow-up) Add warning on about for missing key Test plan: Make sure that you have no encryption_key in koha-conf. Check about page, system information tab. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 133628 [details] [review] Bug 28998: Check missing encryption key in script and module Script prints a warning. Module raises an exception. Unit test added. Test plan: Run unit test. Remove entry and check script. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 133629 [details] [review] Bug 28998: (follow-up) Check missing encryption key in script and module Script prints a warning. Module raises an exception. Unit test added. Test plan: Run unit test. Remove entry and check script. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Added two follow-ups to address your concerns.
(In reply to Martin Renvoize from comment #36) > I wonder slightly about the accessor names in Koha::Patron.. we could just > silently encrypt/descrypt and continue to rely on 'secret' as our accessor > name.. but that's not a big issue. I also wonder if the field in general > should be clearer.. this secret us specific to this form of on time password > for 2FA.. calling is 'secret' is very generic.. but again.. that's from the > earlier bug. Actually, I did that at first and decided to go for another accessor name. This is more transparent. We could address that (if needed) when we move it out of borrowers.
Created attachment 133631 [details] [review] Bug 28998: Introduce Koha::Encryption Test plan: Run t/db_dependent/Koha/Encryption.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> [AMENDED] Added copyright line to module. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 133632 [details] [review] Bug 28998: (follow-up) Add Patron->encode_secret and ->decoded_secret Test plan: Run t/db_dependent/Koha/Patron.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 133633 [details] [review] Bug 28998: (follow-up) Apply changes to TwoFactorAuth module and script Test plan: Run t/db_dependent/Koha/Auth/TwoFactorAuth.t Walk thru cycle register - logout/login - deregister - logout/login. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 133634 [details] [review] Bug 28998: Adjust Selenium test Test plan: Run t/db_dependent/selenium/authentication_2fa.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 133635 [details] [review] Bug 28998: [TO_BE_SQUASHED] Adjust secret in Selenium test This patch can be squashed with the previous one, if all is ok. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 133636 [details] [review] Bug 28998: Add encryption_key to the config file Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 133637 [details] [review] Bug 28998: Add Crypt::CBC dependency Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 133638 [details] [review] Bug 28998: (QA follow-up) POD Fix This POD fix keeps the QA script happy Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 133639 [details] [review] Bug 28998: (follow-up) Add warning on about for missing key Test plan: Make sure that you have no encryption_key in koha-conf. Check about page, system information tab. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 133640 [details] [review] Bug 28998: (follow-up) Check missing encryption key in script and module Script prints a warning. Module raises an exception. Unit test added. Test plan: Run unit test. Remove entry and check script. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 133641 [details] [review] Bug 28998: (follow-up) Check missing encryption key in script and module Script prints a warning. Module raises an exception. Unit test added. Test plan: Run t/db_dependent/Koha/Encryption.t Run t/db_dependent/Koha/Auth/TwoFactorAuth.t Remove entry and check script. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 133642 [details] [review] Bug 28998: (follow-up) Check missing encryption key in script and module Script prints a warning. Module raises an exception. Unit test added. Test plan: Run t/db_dependent/Koha/Encryption.t Run t/db_dependent/Koha/Auth/TwoFactorAuth.t Remove entry and check script. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 133645 [details] [review] Bug 28998: Introduce Koha::Encryption Test plan: Run t/db_dependent/Koha/Encryption.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> [AMENDED] Added copyright line to module. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 133646 [details] [review] Bug 28998: (follow-up) Add Patron->encode_secret and ->decoded_secret Test plan: Run t/db_dependent/Koha/Patron.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 133647 [details] [review] Bug 28998: (follow-up) Apply changes to TwoFactorAuth module and script Test plan: Run t/db_dependent/Koha/Auth/TwoFactorAuth.t Walk thru cycle register - logout/login - deregister - logout/login. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 133648 [details] [review] Bug 28998: Adjust Selenium test Test plan: Run t/db_dependent/selenium/authentication_2fa.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 133649 [details] [review] Bug 28998: [TO_BE_SQUASHED] Adjust secret in Selenium test This patch can be squashed with the previous one, if all is ok. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 133650 [details] [review] Bug 28998: Add encryption_key to the config file Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 133651 [details] [review] Bug 28998: Add Crypt::CBC dependency Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 133652 [details] [review] Bug 28998: (QA follow-up) POD Fix This POD fix keeps the QA script happy Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 133653 [details] [review] Bug 28998: (follow-up) Add warning on about for missing key Test plan: Make sure that you have no encryption_key in koha-conf. Check about page, system information tab. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 133654 [details] [review] Bug 28998: (follow-up) Check missing encryption key in script and module Script prints a warning. Module raises an exception. Unit test added. Test plan: Run t/db_dependent/Koha/Encryption.t Run t/db_dependent/Koha/Auth/TwoFactorAuth.t Remove entry and check script. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 133655 [details] [review] Bug 28998: (QA follow-up) Improve output on error This patch updates the output_and_exit call to instead pass an error parameter in the template and use it to display the warning instead of the rest of the page content in the 'Manage two-factor authentication' page. This allows for translation and makes the page adhere to normal practices. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Right, I'm happy with this all now.. PQA.. (except it'll be blocked until the dependency bugs are all PQA too)
Selenium tests failing further down the tree so blocked until those have been fixed and PQA'd
Hmm, OK.. the selenium failures are now introduced by this bug.. Is there any way to mock the config (encrpytion_key) for a selenium test?
OK, so the tests do pass.. However, you need to have the encryption_key config variable set. We can't easily set this using our Mock libs, as it won't work with the selenium tests.. so I think the solution is to just ensure k-t-d has that variable always set.
(In reply to Martin Renvoize from comment #80) > OK, so the tests do pass.. However, you need to have the encryption_key > config variable set. > > We can't easily set this using our Mock libs, as it won't work with the > selenium tests.. so I think the solution is to just ensure k-t-d has that > variable always set. Create a pull request on KTD ? Then I push master right ?
(In reply to Fridolin Somers from comment #81) > (In reply to Martin Renvoize from comment #80) > > OK, so the tests do pass.. However, you need to have the encryption_key > > config variable set. > > > > We can't easily set this using our Mock libs, as it won't work with the > > selenium tests.. so I think the solution is to just ensure k-t-d has that > > variable always set. > > Create a pull request on KTD ? > Then I push master right ? I do not follow this. As soon as we push this patch, and packaging has been updated accordingly, a new build should be fine imo. Why do we change the order here and wait for ktd instead??
I think Marcel is correct, once pushed k-t-d should get new packages magically and we should get the config option filled. So, go for it.. :)
Personally, I would not find it an improvement if we should(!) wait for ktd updates on a local koha-conf copy while master would be fine. Btw the implicit assumption that every Koha dev uses ktd is just not true ;)
ktd has its own koha-conf. If the entry is mandatory then it needs to be added there. ktd is used by Jenkins and so patches have to not break the CI. I am afraid that at least the 2FA selenium tests will fail. Note that we are talking about a one-line change patch... If we need it I can write a patch for that.
(In reply to Jonathan Druart from comment #85) > ktd has its own koha-conf. If the entry is mandatory then it needs to be > added there. > > ktd is used by Jenkins and so patches have to not break the CI. > > I am afraid that at least the 2FA selenium tests will fail. > > Note that we are talking about a one-line change patch... If we need it I > can write a patch for that. My problem is not that one line. It is the principle behind it. ktd should be smarter than this (imo).
https://gitlab.com/koha-community/koha-testing-docker/-/merge_requests/317
(In reply to Marcel de Rooy from comment #86) > (In reply to Jonathan Druart from comment #85) > > ktd has its own koha-conf. If the entry is mandatory then it needs to be > > added there. > > > > ktd is used by Jenkins and so patches have to not break the CI. > > > > I am afraid that at least the 2FA selenium tests will fail. > > > > Note that we are talking about a one-line change patch... If we need it I > > can write a patch for that. > > My problem is not that one line. It is the principle behind it. ktd should > be smarter than this (imo). Yes, it's a known problem, see https://gitlab.com/koha-community/koha-testing-docker/-/issues/281
(In reply to Jonathan Druart from comment #88) > (In reply to Marcel de Rooy from comment #86) > > (In reply to Jonathan Druart from comment #85) > > > ktd has its own koha-conf. If the entry is mandatory then it needs to be > > > added there. > > > > > > ktd is used by Jenkins and so patches have to not break the CI. > > > > > > I am afraid that at least the 2FA selenium tests will fail. > > > > > > Note that we are talking about a one-line change patch... If we need it I > > > can write a patch for that. > > > > My problem is not that one line. It is the principle behind it. ktd should > > be smarter than this (imo). > > Yes, it's a known problem, see > https://gitlab.com/koha-community/koha-testing-docker/-/issues/281 Ha, now we're talking :)
Thanks a lot, I push. Sorry for not knowing much from our process.
I've squashed : Bug 28998: [TO_BE_SQUASHED] Adjust secret in Selenium test With Bug 28998: Adjust Selenium test
Needs to add Crypt::CBC dependency
Dependency is actually OK... we've already got it listed in the cpanfile on this bug and the version we're pinned to is the latest version that all our debians ship. There's another bug requesting a newer version which would allow us to tighten up security a little more.. but we don't need anything else doing here and now :)
Pushed to master for 22.05, thanks to everybody involved [U+1F984]
Created attachment 134636 [details] [review] Bug 28998: Fix Auth.
Created attachment 134637 [details] [review] Bug 28998: (QA follow-up) Silence some useless warnings This patch fixes the tests and also silences some useless warnings about REMOTE_ADDRESS not being defined. A test is added for a legit warning that is thrown. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
@RM: Follow-ups pushed to fix Jenkins.
Comment on attachment 134637 [details] [review] Bug 28998: (QA follow-up) Silence some useless warnings Review of attachment 134637 [details] [review]: ----------------------------------------------------------------- ::: t/db_dependent/Auth.t @@ +151,4 @@ > } > > t::lib::Mocks::mock_preference( 'TwoFactorAuthentication', 0 ); > + $patron->encode_secret('one_secret'); It feels weird here to require a secret set when TwoFactorAuthentication is disabled?
(In reply to Martin Renvoize from comment #98) > Comment on attachment 134637 [details] [review] [review] > Bug 28998: (QA follow-up) Silence some useless warnings > > Review of attachment 134637 [details] [review] [review]: > ----------------------------------------------------------------- > > ::: t/db_dependent/Auth.t > @@ +151,4 @@ > > } > > > > t::lib::Mocks::mock_preference( 'TwoFactorAuthentication', 0 ); > > + $patron->encode_secret('one_secret'); > > It feels weird here to require a secret set when TwoFactorAuthentication is > disabled? Thats true. I will move it a bit lower. Submitting a follow-up.
Created attachment 134671 [details] [review] Bug 28998: (follow-up) Adjust the Auth.t fix Move the encode_secret call where we need it (when pref is enabled and patron switches to 2FA). Mock the koha-conf encryption_key. Disable 2FA when exiting subtest. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Martin: Please have a look again. @RM: Push last patch only.
Looking good, tests passing perfectly still. Add my SO line and pushed to master.
Hi, on U22 I found this warnings: 14:36:09 koha_1 | WARNING: The key derivation method "opensslv1" is deprecated. Using -pbkdf=>'pbkdf2' would be better. 14:36:09 koha_1 | Pass -nodeprecate=>1 to inhibit this message. 14:36:09 koha_1 | at /kohadevbox/koha/Koha/Patron.pm line 2167. 14:36:09 koha_1 | WARNING: The key derivation method "opensslv1" is deprecated. Using -pbkdf=>'pbkdf2' would be better. 14:36:09 koha_1 | Pass -nodeprecate=>1 to inhibit this message. 14:36:09 koha_1 | at /kohadevbox/koha/Koha/Patron.pm line 2167. 14:36:09 koha_1 | WARNING: The key derivation method "opensslv1" is deprecated. Using -pbkdf=>'pbkdf2' would be better. 14:36:09 koha_1 | Pass -nodeprecate=>1 to inhibit this message. 14:36:09 koha_1 | at /kohadevbox/koha/Koha/Patron.pm line 2167.
See bug 30594 ;)
(In reply to Jonathan Druart from comment #4) > How do we generate the new config entry on upgrade? I don't think this was ever done... which has been driving me a little crazy lately [U+1F605]