Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Seafile: 10.0.1 -> 11.0.12 #318727

Merged
merged 5 commits into from
Sep 30, 2024
Merged

Seafile: 10.0.1 -> 11.0.12 #318727

merged 5 commits into from
Sep 30, 2024

Conversation

melvyn2
Copy link
Contributor

@melvyn2 melvyn2 commented Jun 10, 2024

Description of changes

Updates seafile-server and seahub to 11.0.9. This requires adding the python package djangosaml2. Finally, cleans up the seafile module a little to ensure the proper, matching packages are used.

This required adding a migration script as the sqlite backend was deprecated in favor of MySQL as of version 11. To make this change easier, the persistent user setup of #290104 was merged in.

Finally, a change to the CSRF setup means some users might need to manually whitelist their domains, like so:

  services.seafile = {
    ...

   seahubExtraConf = ''
     CSRF_TRUSTED_ORIGINS = ["https://example.com"]
   '';
  };

This closes #258719

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.11 Release Notes (or backporting 23.11 and 24.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@github-actions github-actions bot added 6.topic: python 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` labels Jun 10, 2024
@melvyn2
Copy link
Contributor Author

melvyn2 commented Jun 10, 2024

Tests for seafile are failing on master but worked on nixos-24.05; I'll take a look when I can.

Also, the update to 11.0 requires changing a reverse proxy setting that nixos doesn't allow. From https://cloud.seatable.io/dtable/external-links/7b976c85f504491cbe8e/?tid=0000&vid=0000&row-id=BQhH-2HSQs68Nq2EW91DBA, the new django CSRF protection requires the proxy server to pass the Host header untouched, but this is blocked by https://github.com/yandex/gixy/blob/master/docs/en/plugins/hostspoofing.md. This means that either nginx config validation has to be entirely disabled, or CSRF protection has to be disabled.
Neither are ideal solutions; maybe the validation should be a little more configurable, so that skip flags can be passed to gixy.

@melvyn2 melvyn2 force-pushed the update-seafile branch 2 times, most recently from f85db95 to 28352f8 Compare June 11, 2024 21:24
@melvyn2
Copy link
Contributor Author

melvyn2 commented Jun 11, 2024

Migration from 10.0 to 11.0 seem to be working properly now, and tests are passing both on 24.05 and unstable. The reverse proxy issue remains, but this is technically not caused by this module, so I'll undraft. A release note line may be necessary.

@melvyn2 melvyn2 marked this pull request as ready for review June 11, 2024 22:19
@melvyn2 melvyn2 requested a review from greizgh June 11, 2024 23:02
@melvyn2 melvyn2 force-pushed the update-seafile branch 2 times, most recently from c5ed206 to 0ff1f13 Compare June 12, 2024 02:13
@ofborg ofborg bot added the 8.has: package (new) This PR adds a new package label Jun 12, 2024
@ofborg ofborg bot requested a review from schmittlauch June 12, 2024 02:44
@greizgh
Copy link
Contributor

greizgh commented Jun 17, 2024

Thank you @melvyn2 for looking into this. I've been put off by the deprecation of sqlite (see upstream changelog). It seems to work, but I wouldn't assume this will be the case for long. Are you aware of a decision change?
I see you setup seahub DB from the sql scripts, I've relied on django migrations because it felt weird to have both mechanisms in place. But I guess this is closer to what upstream does.
Regarding CSRF protection, disabling it is not an option (at least to me), but the CSRF_TRUSTED_ORIGINS approach in conf is quite common (and recommended by upstream). Maybe we should expose this through the module (alternatively, this can already be set through extraConfig)?

@melvyn2
Copy link
Contributor Author

melvyn2 commented Jun 17, 2024

I've been put off by the deprecation of sqlite (see upstream changelog). It seems to work, but I wouldn't assume this will be the case for long. Are you aware of a decision change?

No, I just missed that in the changelog somehow 🤦
I'll re-draft this and see if I can get mysql working in a decent amount of time.

I see you setup seahub DB from the sql scripts, I've relied on django migrations because it felt weird to have both mechanisms in place. But I guess this is closer to what upstream does.

I actually undid this, I originally thought the SQL script was required because during my first attempted upgrade the migration silently failed. The django migrations do work normally and are simpler to use so I've reverted to only using them.

Regarding CSRF protection, disabling it is not an option (at least to me), but the CSRF_TRUSTED_ORIGINS approach in conf is quite common (and recommended by upstream). Maybe we should expose this through the module (alternatively, this can already be set through extraConfig)?

By "disabling" CSRF I meant simply adding the domain to CSRF_TRUSTED_ORIGINS, because I saw this as "bypassing" the protection for that specific origin - it was just bad phrasing from me. Exposing the setting in the module seems like a good choice considering how often this will be required for users to set.

@melvyn2 melvyn2 marked this pull request as draft June 17, 2024 20:32
@melvyn2
Copy link
Contributor Author

melvyn2 commented Jun 19, 2024

The MariaDB-based seafile module is passing tests, and migrations from sqlite/10.0 are almost working. Last error is https://manual.seafile.com/deploy/migrate_from_sqlite_to_mysql/#encountered-errno-150-foreign-key-constraint-is-incorrectly-formed,
which seems to require manually reordering the output of the sqlite dump...
At worst I can solve this with a nasty find-and-replace but I'm looking for a better solution in the mean time.

@melvyn2
Copy link
Contributor Author

melvyn2 commented Jun 19, 2024

Of course as soon as I post that, I find https://mariadb.com/kb/en/server-system-variables/#foreign_key_checks ...
Will try it next time

@melvyn2 melvyn2 force-pushed the update-seafile branch 2 times, most recently from ba8afc7 to cbc574c Compare July 4, 2024 05:26
@melvyn2
Copy link
Contributor Author

melvyn2 commented Jul 4, 2024

MariaDB migration is now working in a test environment, and using the ccnet service URL as a default URL for the CSRF whitelist seems like a decent solution for the CSRF issue.
I'll try the migration on my own setup now and see how it goes!

@melvyn2 melvyn2 marked this pull request as ready for review July 4, 2024 06:20
@melvyn2 melvyn2 requested a review from natsukium as a code owner July 4, 2024 06:20
@melvyn2
Copy link
Contributor Author

melvyn2 commented Jul 4, 2024

Works on my end, after these little changes!

@ofborg ofborg bot requested a review from greizgh July 27, 2024 22:07
@wegank wegank added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Aug 10, 2024
@wegank wegank added the 2.status: merge conflict This PR has merge conflicts with the target branch label Sep 10, 2024
@melvyn2 melvyn2 changed the title Seafile: 10.0.1 -> 11.0.9 Seafile: 10.0.1 -> 11.0.12 Sep 28, 2024
@melvyn2
Copy link
Contributor Author

melvyn2 commented Sep 28, 2024

I haven't been able to work on this for a decent period of time, but I think with this rebase and modifications (all passing tests and running on my server), it is ready to be merged.

I've removed the remote-mysql additions because they are incomplete and will take non-negligible effort to get into a working state. This was "extra" functionality anyways that should really be in a separate PR; the old sqlite database only supported running on the same host anyways.
I've attached the last state of the WIP commit of this additions in case someone else feels an urge to add this functionality.
mysql_configurable.patch

@melvyn2
Copy link
Contributor Author

melvyn2 commented Sep 28, 2024

Seems like the failed PR is an OfBorg/GitHub API issue and not something to do with this PR...

@ofborg ofborg bot removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Sep 28, 2024
@ofborg ofborg bot requested a review from schmittlauch September 28, 2024 01:49
nixos/doc/manual/release-notes/rl-2411.section.md Outdated Show resolved Hide resolved
nixos/modules/services/networking/seafile.nix Outdated Show resolved Hide resolved
nixos/modules/services/networking/seafile.nix Outdated Show resolved Hide resolved
nixos/modules/services/networking/seafile.nix Outdated Show resolved Hide resolved
nixos/modules/services/networking/seafile.nix Outdated Show resolved Hide resolved
nixos/modules/services/networking/seafile.nix Outdated Show resolved Hide resolved
nixos/modules/services/networking/seafile.nix Outdated Show resolved Hide resolved
nixos/modules/services/networking/seafile.nix Outdated Show resolved Hide resolved
@ofborg ofborg bot added the 11.by: package-maintainer This PR was created by the maintainer of the package it changes label Sep 29, 2024
Copy link
Contributor

@greizgh greizgh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The migration went fine. Let's merge this since seafile is broken on master.

@melvyn2 melvyn2 added the 12.approved-by: package-maintainer This PR was reviewed and approved by a maintainer listed in the package label Sep 30, 2024
@h7x4
Copy link
Member

h7x4 commented Sep 30, 2024

I'll merge this as is since its better than having it broken on master, but I don't think enabling mysql on the local host is ideal. Would be great if you could open an issue to track progress so others can find it and see that it's not intended as a long term solution. That being said, thank you for the great effort spanning several months!

@h7x4 h7x4 merged commit 636185e into NixOS:master Sep 30, 2024
29 checks passed
@melvyn2 melvyn2 deleted the update-seafile branch September 30, 2024 21:54
checkPhase = ''
runHook preCheck

python tests/run_tests.py
Copy link
Member

@dotlambda dotlambda Oct 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
python tests/run_tests.py
${python.interpreter} tests/run_tests.py

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 6.topic: python 8.has: changelog 8.has: documentation This PR adds or changes documentation 8.has: module (update) This PR changes an existing module in `nixos/` 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 11.by: package-maintainer This PR was created by the maintainer of the package it changes 12.approvals: 1 This PR was reviewed and approved by one reputable person 12.approved-by: package-maintainer This PR was reviewed and approved by a maintainer listed in the package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update request: seahub 9.0.10 → 11.0.0
9 participants