mailinabox icon indicating copy to clipboard operation
mailinabox copied to clipboard

Jammyjellyfish2204 nextcloud24

Open stvhay opened this issue 2 years ago β€’ 18 comments

This is my best attempt to get MIAB up to NC 24 for the Jammy release.

stvhay avatar Sep 12 '22 05:09 stvhay

  • Nginx is configured to respond to webfinger and nodeinfo requests.
  • config.php has to be adjusted during the update to allow the update to complete. This is due to maybe a bug fix on the strictness of the read_only config setting.
  • an imagemagick library is added to get NC to stop complaining about svg support.
  • logic is added to set a telephone locale, which NC needs to guess country codes.
  • fixed overwrite.cli.url. which NC now wants the full URL.

Notes: There are quite a few apps that could be disabled in NC. Not sure how much it is worth it to pick through them.

stvhay avatar Sep 12 '22 05:09 stvhay

I keep finding things to improve. Sorry--I'm done. :)

stvhay avatar Sep 12 '22 16:09 stvhay

So it turns out occ provides a way to change config parameters and I think I should patch the implementation to use it before we merge. Much cleaner.

stvhay avatar Sep 12 '22 22:09 stvhay

Great work on this!

We're going to need this in there (iI've run it manually):

sudo -u www-data php$PHP_VER /usr/local/lib/owncloud/occ db:add-missing-primary-keys

This is the diff for the config update:

index df6be86..f22467a 100755
--- a/setup/nextcloud.sh
+++ b/setup/nextcloud.sh
@@ -322,18 +322,16 @@ cat > $CONFIG_TEMP <<-EOF
        "system": {
                "config_is_read_only": true,
                "trusted_domains": ["$PRIMARY_HOSTNAME"],
-         "memcache.local": "\\\OC\\\Memcache\\\APCu",
-         "mail_from_address": "administrator",
-         "logtimezone": "$TIMEZONE",
-         "logdateformat": "Y-m-d H:i:s",
-         "mail_domain": "$PRIMARY_HOSTNAME",
-         "default_phone_region": "$PHONE_REGION",
-               "user_backends": [
-                       {
-                               "class": "\\\OCA\\\UserExternal\\\IMAP",
-                               "arguments": [ "127.0.0.1", 143, null, null, false, false ]
-                       }
-               ]
+               "memcache.local": "\\\\OC\\\\Memcache\\\\APCu",
+               "overwrite.cli.url": "https://${PRIMARY_HOSTNAME}/cloud",
+               "mail_from_address": "administrator",
+               "logtimezone": "$TIMEZONE",
+               "logdateformat": "Y-m-d H:i:s",
+               "mail_domain": "$PRIMARY_HOSTNAME",
+               "user_backends": [{
+                       "class": "\\\\OCA\\\\UserExternal\\\\IMAP",
+                       "arguments": ["127.0.0.1", 143, null, null, false, false]
+               }]
        }
 }
 EOF```

Please check the initial config setup too. Those memcache and class values are not correct either. I don't think they will work during a clean install. I haven't tested this. To make it easier to debug you can grap the generated temp file and paste it in a web validator. 

When all of this is done the only thing remaining on my upgraded install is the SQLite warning and that's fine.

yodax avatar Sep 14 '22 13:09 yodax

@yodax Thank you for taking the time to review and provide comments to my PR. I have pushed a revision to the code which I believe addresses all of your concerns. Let me know if you see anything else.

Oh. One thing---I've run this from a fresh Ubuntu install and it worked fine. But one thing I think we could do once we get it merged is test the different upgrade paths someone might take or what we are going to support. Right now I've started from a fresh 22.04 install, so I've not looked at dist-upgrading to 22.04 and going from there.

stvhay avatar Sep 14 '22 16:09 stvhay

Not sure if you missed it, this needs to be added to the config update too:

+ "overwrite.cli.url": "https://${PRIMARY_HOSTNAME}/cloud",

Now it's only in the initial config setup.

yodax avatar Sep 14 '22 16:09 yodax

dist-upgrading to 22.04

This is an unsupported upgrade path. So no worries about that one. I've tested a fresh install but with a restored backup of /home/user-data.

Nice work!

yodax avatar Sep 14 '22 16:09 yodax

  • "overwrite.cli.url": "https://${PRIMARY_HOSTNAME}/cloud",

Pushed.

stvhay avatar Sep 14 '22 16:09 stvhay

Awesome. I'll pull tomorrow and double check. But this looks good to me, I'll report back how it went!

yodax avatar Sep 14 '22 16:09 yodax

Awesome. I'll pull tomorrow and double check. But this looks good to me, I'll report back how it went!

Thanks! If you are interested, I have a branch on my repo with all of my pending PRs and a roundcube update from @kiekerjan. This includes #2153 #2156 #2157 #2158 and #2159. I've been running it for a while and it seems good. But I understand if you want to be conservative since you are running this on your actual production machine.

Anyways, here is the branch: https://github.com/stvhay/mailinabox/tree/jammyjellyfish2204-everything

stvhay avatar Sep 14 '22 16:09 stvhay

This worked for me. The telephone field is empty, which is fine.

I just noticed an extra error in the log:

Error: stepping, UNIQUE constraint failed: oc_users_external.uid, oc_users_external.backend (19)

sqlite> select * from oc_users_external;
[email protected]|{localhost:993/imap/ssl/novalidate-cert}|Michael Kroes
[email protected]|127.0.0.1|
*** other users ommited **

I think it's due to this line in the original Jammy PR by @JoshData : https://github.com/mail-in-a-box/mailinabox/pull/2083/files#diff-82e1be7bfd6ea28b0bc7b8ee6ae4a78dcaea281159e0853b3039b97fc231cbceR360

I trashed the 127.* records and updated the other records to have 127.0.0.1 as the backend. Then it runs without errors.

Could you check your table and see if it migrated correctly? If this is a problem we can solve it separately, it has nothing to do with the PR itself.

yodax avatar Sep 15 '22 06:09 yodax

Could you check your table and see if it migrated correctly? If this is a problem we can solve it separately, it has nothing to do with the PR itself.

sqlite> select * from oc_users_external ;
127.0.0.1|[email protected]|
sqlite> 

Mine looks fine, but it is probably because I'm coming from a fresh install.

stvhay avatar Sep 15 '22 08:09 stvhay

Thanks for checking. For me this PR is good πŸ‘

@JoshData did you see any duplicates in that table when you upgraded your instance? I'm happy to blame this on something that happened on my machine alone. If more users report it, we could write a script that fixes it.

yodax avatar Sep 15 '22 08:09 yodax

Yeah I had duplicates.

JoshData avatar Sep 15 '22 10:09 JoshData

I have a patch developed to update all the wget_verify downloads to SHA256 hashes. We want to NSA-proof our email after all :). Should I include it here or wait until its merged and do a separate PR? The scope includes zpush, roundcube, and management as well.

stvhay avatar Sep 15 '22 19:09 stvhay

I have a patch developed to update all the wget_verify downloads to SHA256 hashes. We want to NSA-proof our email after all :). Should I include it here or wait until its merged and do a separate PR?

Separate PR. Please don't make this harder for me. :|

JoshData avatar Sep 17 '22 12:09 JoshData

Merged in jammyjellyfish2204 and cleaned merge conflicts.

Potentially conflicts with #2162 due to:

  • It contains updated SHA256 hashes, whereas this PR contains SHA-1 hashes.
  • It holds SHA256 values for NC versions only up to 23.

stvhay avatar Sep 17 '22 16:09 stvhay

I've tried to simplify this PR for ease of review and inclusion into the project:

  • Taken out the changes to manipulate the config via the Nextcloud API.
  • Removed the stripping down and disabling of additional NC functionality to just calendar and contacts plugins.

What I've kept:

  • Added nginx rewrites to handle webfinger and nodeinfo requests to eliminate NC warnings
  • Added libmagickcore-6.q16-6-extra package to eliminate NC SVG warnings
  • Added db:add-missing-primary-keys to the db migration step
  • The config parameter overwrite.cli.url is populated with the full URL, per NC warning
  • The config parameter default_phone_region value is surmised from the locale, or left blank if the locale is not defined
  • Added log rotataion for nextcloud.log. Rotates when log reaches 10M.

What I've added:

  • Added a cron job for sending calendar updates, per recommendations in the NC manual.
  • Updated the .well-known URLs for CardDAV and CalDAV, including to the domain TLD for better service discovery.
  • Added a TXT record to provide path to CalDAV and CardDAV. This goes with the SRV record that is already there.

What I'm not doing, but could do if there is an appetite for it:

  • Update the nginx proxy section for nginx to correspond more closely to the most recent example in the docs.

stvhay avatar Sep 17 '22 22:09 stvhay

Saw this during setup:

Installing Nextcloud (contacts/calendar)...─────────────────────Hostname────────────────────────────────────────┐
Nextcloud is already latest version needs a name, called a 'hostname'. The name will form a part of the box's   β”‚  
Error: stepping, UNIQUE constraint failed: oc_users_external.uid, oc_users_external.backend

louwers avatar Sep 29 '22 07:09 louwers

I am still seeing this error

Error: stepping, UNIQUE constraint failed: oc_users_external.uid, oc_users_external.backend (19)

What to do? Ran latest script on latest Ubuntu.

binarykitchen avatar Oct 13 '22 21:10 binarykitchen

@yodax yes I would write such a script since it's happening to other users including me.

binarykitchen avatar Oct 13 '22 21:10 binarykitchen