Recently we replaced every use of YAML::Syck or YAML by YAML::XS, and removed YAML::Syck and YAML from cpanfile. But we use CGI::Session::Serialize::yaml which does not work with YAML::XS and need either YAML::Syck or YAML So we shoud either: 1) Reintroduce one of YAML or YAML::Syck into cpanfile, or 2) Use a different serializer, or 3) Implement in Koha a serializer that use YAML::XS (should be easy, look at https://metacpan.org/release/CGI-Session-Serialize-yaml/source/lib/CGI/Session/Serialize/yaml.pm) Which one do you prefer ?
Why does it need to be listed in the cpanfile? Is it because it is needed so that it is included in the debian control file? If I run ./debian/list-deps it adds libyaml-libyaml-perl but is it by random chance (I don't know)?
(In reply to Joonas Kylmälä from comment #1) > Why does it need to be listed in the cpanfile? Is it because it is needed so > that it is included in the debian control file? If I run ./debian/list-deps > it adds libyaml-libyaml-perl but is it by random chance (I don't know)? just realized libyaml-libyaml-perl is the YAML::XS version... Let me run it a bit longer and see if any other packages appear (the list-deps script is slow).
Why don't we use CGI::Session::Serialize::default instead?
(In reply to Jonathan Druart from comment #3) > Why don't we use CGI::Session::Serialize::default instead? It was switched to yaml because of a bug in the default serializer: session management: use YAML::Syck for serialization (a858e8a8b895640f2). Could check if that diacritic bug is still valid.
I tried to use čollčá as the library name and the default serializer and the bug reported a decade a code doesn't happen anymore, so I think we could use the default serializer. Just have to come up with a upgrade plan. At least the sessions table need to be emptied but do we have update scripts for emptying memcached and temp files where the sesssions might also be stored?
I've tried as well and I didn't face any encoding issues.
(In reply to Joonas Kylmälä from comment #5) > I tried to use čollčá as the library name and the default serializer and the > bug reported a decade a code doesn't happen anymore, so I think we could use > the default serializer. Just have to come up with a upgrade plan. At least > the sessions table need to be emptied but do we have update scripts for > emptying memcached and temp files where the sesssions might also be stored? Why that?
(In reply to Jonathan Druart from comment #7) > (In reply to Joonas Kylmälä from comment #5) > > I tried to use čollčá as the library name and the default serializer and the > > bug reported a decade a code doesn't happen anymore, so I think we could use > > the default serializer. Just have to come up with a upgrade plan. At least > > the sessions table need to be emptied but do we have update scripts for > > emptying memcached and temp files where the sesssions might also be stored? > > Why that? Maybe it is not needed after all, I thought it would somehow try to parse all the sessions and possibly break, e.g. maybe if a malicous user tries to use the session id for the old YAML session could it break something or allow unauthorized access? Also now I found an encoding issue with the default serializer, if you use branchname and branchcode "ÄÄÄ~ÄãÃ" it breaks and shows question marks in intranet: [WARN] (in cleanup) CGI::Session::Driver::mysql::store(): DBI Exception: DBD::mysql::db do failed: Incorrect string value: '\xC4\xC4\xC4~\xC4\xE3...' for column `koha_kohadev`.`sessions`.`a_session` at row 1 [for Statement "INSERT INTO sessions (id, a_session) VALUES(?, ?) ON DUPLICATE KEY UPDATE a_session = ?"] at /usr/share/perl5/CGI/Session.pm line 251 This works with the YAML serializer.
Indeed, I only tried with the SessionStorage=tmp file I managed to fix this problem if I remove the decode in set_userenv, but need to find side-effects.
(In reply to Jonathan Druart from comment #9) > Indeed, I only tried with the SessionStorage=tmp file > > I managed to fix this problem if I remove the decode in set_userenv, but > need to find side-effects. I was also investigating if this happens because of some wrong DB connection initialization in Koha::Database.pm code. Nothing interesting found yet. I got the idea from this discussion: https://stackoverflow.com/questions/10957238/incorrect-string-value-when-trying-to-insert-utf-8-into-mysql-via-jdbc
Created attachment 120835 [details] [review] Bug 23817: Use the default CGI::Session serializer
Everything seems to work correctly with that patch. Tested all the 3 combinaisons of SessionStorage and the following branchcode/branchname: "CPL", "ÄÄÄ~ÄãÃ", "✔️ ❤️ ★"
Thanks for the code Joubu. I have not checked the solution yet but if you think it is right, could you please do similar changes for C4/InstallAuth.pm?
(In reply to Joonas Kylmälä from comment #13) > Thanks for the code Joubu. I have not checked the solution yet but if you > think it is right, could you please do similar changes for C4/InstallAuth.pm? and remove CGI::Session::Serializer::yaml from cpanfile ? :) The patch works great for me
Created attachment 120836 [details] [review] Bug 28317: Use the default CGI::Session serializer We remove YAML::Syck on bug 22824 and YAML on 27673, to use YAML::XS. However we need one of them for CGI::Session::Serialize::yaml It's preferable to change the serializer and use the default one instead of writing one based on YAML::XS (or patch the existing ::yaml that does not seem maintained). There was an encoding bug reported on the default serializer (see commit a858e8a8b895640f2) but we fail to recreate it. Test plan: Create 3 libraries with branchcode=branchname: "CPL", "ÄÄÄ~ÄãÃ" and "✔️ ❤️ ★" Use the 3 options of SessionStorage and switch from one logged in library to another. Confirm that everything is working correctly (ie. no ending issue in the library name at the top-right corner)
Created attachment 120837 [details] [review] Bug 28317: Use the default CGI::Session serializer We remove YAML::Syck on bug 22824 and YAML on 27673, to use YAML::XS. However we need one of them for CGI::Session::Serialize::yaml It's preferable to change the serializer and use the default one instead of writing one based on YAML::XS (or patch the existing ::yaml that does not seem maintained). There was an encoding bug reported on the default serializer (see commit a858e8a8b895640f2) but we fail to recreate it. Test plan: Create 3 libraries with branchcode=branchname: "CPL", "ÄÄÄ~ÄãÃ" and "✔️ ❤️ ★" Use the 3 options of SessionStorage and switch from one logged in library to another. Confirm that everything is working correctly (ie. no ending issue in the library name at the top-right corner)
Created attachment 120839 [details] [review] Bug 28317: Use the default CGI::Session serializer We remove YAML::Syck on bug 22824 and YAML on 27673, to use YAML::XS. However we need one of them for CGI::Session::Serialize::yaml It's preferable to change the serializer and use the default one instead of writing one based on YAML::XS (or patch the existing ::yaml that does not seem maintained). There was an encoding bug reported on the default serializer (see commit a858e8a8b895640f2) but we fail to recreate it. Test plan: Create 3 libraries with branchcode=branchname: "CPL", "ÄÄÄ~ÄãÃ" and "✔️ ❤️ ★" Use the 3 options of SessionStorage and switch from one logged in library to another. Confirm that everything is working correctly (ie. no ending issue in the library name at the top-right corner) Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 120840 [details] [review] Bug 28317: Use the default CGI::Session serializer We remove YAML::Syck on bug 22824 and YAML on 27673, to use YAML::XS. However we need one of them for CGI::Session::Serialize::yaml It's preferable to change the serializer and use the default one instead of writing one based on YAML::XS (or patch the existing ::yaml that does not seem maintained). There was an encoding bug reported on the default serializer (see commit a858e8a8b895640f2) but we fail to recreate it. Test plan: Create 3 libraries with branchcode=branchname: "CPL", "ÄÄÄ~ÄãÃ" and "✔️ ❤️ ★" Use the 3 options of SessionStorage and switch from one logged in library to another. Confirm that everything is working correctly (ie. no ending issue in the library name at the top-right corner) Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Followed the test plan + tested the install process after making sure I had no YAML or YAML::Syck installed. No problem detected
(In reply to Jonathan Druart from comment #3) > Why don't we use CGI::Session::Serialize::default instead? It's a shame that we're losing the YAML serialization. As noted on https://metacpan.org/pod/CGI::Session::Serialize::yaml, using YAML makes it easy to share session data across different languages. I have third-party apps that can use Koha session data by parsing the YAML. It looks like I'll have to think of something much more complicated to deal with the change in serialization.
(In reply to Julian Maurice from comment #0) > Recently we replaced every use of YAML::Syck or YAML by YAML::XS, and > removed YAML::Syck and YAML from cpanfile. > But we use CGI::Session::Serialize::yaml which does not work with YAML::XS > and need either YAML::Syck or YAML > > So we shoud either: > 1) Reintroduce one of YAML or YAML::Syck into cpanfile, or > 2) Use a different serializer, or > 3) Implement in Koha a serializer that use YAML::XS (should be easy, look at > https://metacpan.org/release/CGI-Session-Serialize-yaml/source/lib/CGI/ > Session/Serialize/yaml.pm) > > Which one do you prefer ? We could fork CGI::Session::Serialize::yaml and make a CGI::Session::Serialize::Koha. We could also contribute a patch or raise an enhancement request to CGI::Session::Serialize::yaml, so that it can use YAML::XS. It looks like the module got a new maintainer last year https://rt.cpan.org/Public/Bug/Display.html?id=131715
Interestingly, Tina Muller who has taken over CGI::Session::Serialize::yaml is also the maintainer of YAML::XS. Ahhh CGI::Session::Serialize::yaml also has an undocumented feature. If you look at the source code, you'll see that you can set $CGI::Session::Serialize::yaml::Flavour to whatever YAML module you want that can use Dump/Load methods. (Note that you'd have to preload it yourself with 'use' or 'require'.) So... it looks like CGI::Session::Serialize::yaml can work with YAML::XS. Personally, it would be awesome if we could retain the use of CGI::Session::Serialize::yaml. I'm going to move this to "In Discussion", but if no one else cares, feel free to move back to "Passed QA".
I've opened a couple issues at https://rt.cpan.org/Dist/Display.html?Name=CGI-Session-Serialize-yaml regarding documentation and module options.
(In reply to David Cook from comment #20) > I have third-party apps that can use Koha session data by parsing the YAML. > It looks like I'll have to think of something much more complicated to deal > with the change in serialization. It's not very complicated, you just have to use `CGI::Session::Serialize::default->thaw($session_data)` > We could fork CGI::Session::Serialize::yaml and make a > CGI::Session::Serialize::Koha. > > Ahhh CGI::Session::Serialize::yaml also has an undocumented feature. If you > look at the source code, you'll see that you can set > $CGI::Session::Serialize::yaml::Flavour to whatever YAML module you want > that can use Dump/Load methods. (Note that you'd have to preload it yourself > with 'use' or 'require'.) > > So... it looks like CGI::Session::Serialize::yaml can work with YAML::XS. > > Personally, it would be awesome if we could retain the use of > CGI::Session::Serialize::yaml. If we really want to stick with YAML, I'd prefer Koha to have its own implementation rather than relying on an undocumented "feature" (from my point of view that is not a feature, this is just messing with a module's internal variables)
David, thanks for the investigation. All what you suggest is more work than what we have now, and I won't have more time to dedicate to this, we are close to the release. We should open a new bug report to reintroduce yaml using YAML::XS, later. Unless someone else has time to do it before the end of the week.
(In reply to Julian Maurice from comment #24) > (In reply to David Cook from comment #20) > > I have third-party apps that can use Koha session data by parsing the YAML. > > It looks like I'll have to think of something much more complicated to deal > > with the change in serialization. > It's not very complicated, you just have to use > `CGI::Session::Serialize::default->thaw($session_data)` My third-party apps neither use Perl nor have the capacity to use Perl, so that's not an option :p. > > We could fork CGI::Session::Serialize::yaml and make a > > CGI::Session::Serialize::Koha. > > > > Ahhh CGI::Session::Serialize::yaml also has an undocumented feature. If you > > look at the source code, you'll see that you can set > > $CGI::Session::Serialize::yaml::Flavour to whatever YAML module you want > > that can use Dump/Load methods. (Note that you'd have to preload it yourself > > with 'use' or 'require'.) > > > > So... it looks like CGI::Session::Serialize::yaml can work with YAML::XS. > > > > Personally, it would be awesome if we could retain the use of > > CGI::Session::Serialize::yaml. > If we really want to stick with YAML, I'd prefer Koha to have its own > implementation rather than relying on an undocumented "feature" (from my > point of view that is not a feature, this is just messing with a module's > internal variables) It's not an internal variable. It's a package variable. That's why they have scoped it that way. Based on my conversations with the CGI::Session::Serialize::yaml maintainer, we'll probably add a class method for explicitly choosing the YAML implementation.
(In reply to Jonathan Druart from comment #25) > David, thanks for the investigation. > > All what you suggest is more work than what we have now, and I won't have > more time to dedicate to this, we are close to the release. > We should open a new bug report to reintroduce yaml using YAML::XS, later. > Unless someone else has time to do it before the end of the week. Surely the least amount of work would be to just keep the YAML::Syck dependency for CGI::Session::Serialize::yaml for the time being, and then change it to YAML::XS later? If we change it to the default serializer now, I think it would much harder to switch back to YAML later.
Then write an alternative patch and test it please.
(In reply to David Cook from comment #27) > (In reply to Jonathan Druart from comment #25) > > David, thanks for the investigation. > > > > All what you suggest is more work than what we have now, and I won't have > > more time to dedicate to this, we are close to the release. > > We should open a new bug report to reintroduce yaml using YAML::XS, later. > > Unless someone else has time to do it before the end of the week. > > Surely the least amount of work would be to just keep the YAML::Syck > dependency for CGI::Session::Serialize::yaml for the time being, and then > change it to YAML::XS later? > > If we change it to the default serializer now, I think it would much harder > to switch back to YAML later. Wouldn't it make more sense for your to add an API endpoint for outputting deserialized session info? That way you aren't messing with Koha's internals, which are subject to change.
(In reply to Kyle M Hall from comment #29) > (In reply to David Cook from comment #27) > > (In reply to Jonathan Druart from comment #25) > > > David, thanks for the investigation. > > > > > > All what you suggest is more work than what we have now, and I won't have > > > more time to dedicate to this, we are close to the release. > > > We should open a new bug report to reintroduce yaml using YAML::XS, later. > > > Unless someone else has time to do it before the end of the week. > > > > Surely the least amount of work would be to just keep the YAML::Syck > > dependency for CGI::Session::Serialize::yaml for the time being, and then > > change it to YAML::XS later? > > > > If we change it to the default serializer now, I think it would much harder > > to switch back to YAML later. > > Wouldn't it make more sense for your to add an API endpoint for outputting > deserialized session info? That way you aren't messing with Koha's > internals, which are subject to change. +1
(In reply to David Cook from comment #26) > My third-party apps neither use Perl nor have the capacity to use Perl, so > that's not an option :p. Would writing a small Perl proxy(is that's the right term) work?
(In reply to Kyle M Hall from comment #29) > Wouldn't it make more sense for your to add an API endpoint for outputting > deserialized session info? That way you aren't messing with Koha's > internals, which are subject to change. I shouldn't have said third-party apps, as they're not really third-party. They're just non-Koha apps served off the same domain. Regarding your question, yes and no. 1. No: Firstly, everything is subject to change. I've seen API integrations break when the API endpoint gets updated. In this case, I can see Koha changes coming, and the code can get updated. Plus, the sessions are fairly stable overall (when serialized as YAML at least). It's not a drama. Secondly, at the moment I can use the session without any local customizations to Koha. 2. Yes: If I were to use an API endpoint, it would require customization, and I probably wouldn't create an endpoint outputting session data. Instead, I'd probably make multiple API endpoints to get the specific data that I need (like effective user permissions). In this case, I have access to the CGISESSID cookie, so I have the session ID, and Koha is working kind of like a SSO system. If I were to use an API, I would need an endpoint that could convert a session ID to a borrowernumber. But then Koha doesn't have any authorizations/permissions that would really be appropriate for that kind of administrative process. And I would consider it an administrative process. As such, I wouldn't love the idea of it being part of Koha's public Internet-accessible API. I would want to lock down that API endpoint within a separate Koha Admin API which could be limited to specific IP addresses or a local network. Koha also doesn't have a concept of "system users" that are reserved just for API user (and which can't be deleted by library staff). So... while using an API might make sense... it would be a tonne of work. Much more work than just reading the session from the database. -- At the end of the day, I imagine we'll switch the serializer to default, and then I'll have to think up some kind of hacked together API solution. Or scrap the other apps. Koha really needs an administrator level above superlibrarian, or some other management interface. But it all takes considerable work.
(In reply to Julian Maurice from comment #24) > If we really want to stick with YAML, I'd prefer Koha to have its own > implementation rather than relying on an undocumented "feature" (from my > point of view that is not a feature, this is just messing with a module's > internal variables) (In reply to Jonathan Druart from comment #28) > Then write an alternative patch and test it please. Ok. I figured Julian had already disagreed with my proposal, but I can do it anyway.
(In reply to David Cook from comment #33) > (In reply to Julian Maurice from comment #24) > > If we really want to stick with YAML, I'd prefer Koha to have its own > > implementation rather than relying on an undocumented "feature" (from my > > point of view that is not a feature, this is just messing with a module's > > internal variables) > > (In reply to Jonathan Druart from comment #28) > > Then write an alternative patch and test it please. > > Ok. I figured Julian had already disagreed with my proposal, but I can do it > anyway. Ah, alas, due to the Debian packaging of https://packages.debian.org/buster/libcgi-session-serialize-yaml-perl, you cannot remove libyaml-perl and libyaml-syck-perl without removing libcgi-session-serialize-yaml-perl. I'm still not sure why we have to remove YAML or YAML::Syck. I understand that it would be annoying to have one of the modules around just for handling the session, but it would be nice to keep the session serialized as YAML.
Well, I just looked at CGI::Session::Serialize::default and did a little test using its freeze method. The data ends up looking like this: $D = {'firstname' => 'David','surname' => 'Cook','id' => '1'};;$D I could probably parse that...
Interestingly, CGI::Session::Serialize::default writes out a Perl data structure and then uses the equivalent of eval() to inject it back into Perl: https://metacpan.org/release/CGI-Session/source/lib/CGI/Session/Serialize/default.pm#L39.
If we're set on removing the YAML and YAML::Syck dependencies, then I say we move this back to "Passed QA". While in theory we could use YAML::XS with CGI::Session::Serialize::yaml, the Debian packaging of it means we would have to keep either YAML or YAML::Syck installed on the system, even if it was unused.
Pushed to master for 21.05, thanks to everybody involved!
Depends on Bug 22824 not in 20.11.x
On Koha-testing-docker I get randomly the following error when trying to select a new branch on intranet (/cgi-bin/koha/circ/set-library.pl): CGI::Session::Driver::mysql::store(): DBI Exception: DBD::mysql::db do failed: Incorrect string value: '\xE4iskap...' for column `koha_kohadev`.`sessions`.`a_session` at row 1 [for Statement "INSERT INTO sessions (id, a_session) VALUES(?, ?) ON DUPLICATE KEY UPDATE a_session = ?"] at /usr/share/perl5/CGI/Session.pm line 251 at /usr/share/perl5/DBIx/Class/Exception.pm line 77 The branch I try to select has the following name: "Alkuperäiskappaleet, ei lainata".
(In reply to Joonas Kylmälä from comment #41) > On Koha-testing-docker I get randomly the following error when trying to > select a new branch on intranet (/cgi-bin/koha/circ/set-library.pl): > > CGI::Session::Driver::mysql::store(): DBI Exception: DBD::mysql::db do > failed: Incorrect string value: '\xE4iskap...' for column > `koha_kohadev`.`sessions`.`a_session` at row 1 [for Statement "INSERT INTO > sessions (id, a_session) VALUES(?, ?) ON DUPLICATE KEY UPDATE a_session = > ?"] at /usr/share/perl5/CGI/Session.pm line 251 > at /usr/share/perl5/DBIx/Class/Exception.pm line 77 > > The branch I try to select has the following name: "Alkuperäiskappaleet, ei > lainata". Aha, I am able to reproduce the error now always: switch first to "Alkuperäiskappaleet, ei lainata" branch and then try it to set it again and you will get this error.
(In reply to Joonas Kylmälä from comment #41) > On Koha-testing-docker I get randomly the following error when trying to > select a new branch on intranet (/cgi-bin/koha/circ/set-library.pl): > > CGI::Session::Driver::mysql::store(): DBI Exception: DBD::mysql::db do > failed: Incorrect string value: '\xE4iskap...' for column > `koha_kohadev`.`sessions`.`a_session` at row 1 [for Statement "INSERT INTO > sessions (id, a_session) VALUES(?, ?) ON DUPLICATE KEY UPDATE a_session = > ?"] at /usr/share/perl5/CGI/Session.pm line 251 > at /usr/share/perl5/DBIx/Class/Exception.pm line 77 > > The branch I try to select has the following name: "Alkuperäiskappaleet, ei > lainata". I made another report now as bug 28489, I think these two are related. This doesn't happen when strict_sql_modes = 0, only with strict_sql_modes = 1