search

YunoHost-Apps / cryptpad_ynh

CryptPad package for YunoHost
https://cryptpad.fr
GNU General Public License v3.0
27 stars 21 forks source link

2024.9.1 #217

Open ericgaspar opened 5 months ago

ericgaspar commented 5 months ago

Problem

Solution

PR Status

Automatic tests

Automatic tests can be triggered on https://ci-apps-dev.yunohost.org/ after creating the PR, by commenting "!testme", "!gogogadgetoci" or "By the power of systemd, I invoke The Great App CI to test this Pull Request!". (N.B. : for this to work you need to be a member of the Yunohost-Apps organization)

ericgaspar commented 5 months ago

!testme

yunohost-bot commented 5 months ago

Alrighty! Test Badge

yunohost-bot commented 5 months ago

:books: :bug: Test Badge

ericgaspar commented 5 months ago

This admin e-mail setting no longer appears to be included upstream. Is this still valid? https://github.com/YunoHost-Apps/cryptpad_ynh/blob/6f0391dc77ac82dbc7fe90f3d8cdb985d1c4ee86/conf/config.js#L141

mathilde-cryptpad commented 5 months ago

This admin e-mail setting no longer appears to be included upstream. Is this still valid?

https://github.com/YunoHost-Apps/cryptpad_ynh/blob/6f0391dc77ac82dbc7fe90f3d8cdb985d1c4ee86/conf/config.js#L141

No, it's not needed any more. This setting is specified from the admin panel since a few releases.

ericgaspar commented 5 months ago

Something is wrong with the NGINX config. The app can't access to the sandbox domain. when pressing text/drive I get sframe-boot.js must only be loaded in a nested context...

Ddataa commented 5 months ago

!testme

yunohost-bot commented 5 months ago

:carousel_horse: Test Badge

yunohost-bot commented 5 months ago

:books: :bug: Test Badge

ericgaspar commented 4 months ago

!testme

yunohost-bot commented 4 months ago

Living in the future, are we? Test Badge

yunohost-bot commented 4 months ago

:rocket: Test Badge

tituspijean commented 3 months ago

!testme

yunohost-bot commented 3 months ago

:worm: Test Badge

yunohost-bot commented 3 months ago

:carousel_horse: Test Badge

tituspijean commented 3 months ago

Tested and working.

It looks like the main page loads well, but I cannot access the tools, cryptpad cannot access the sandbox domain. I had no time investigating further.

mathilde-cryptpad commented 3 months ago

~Tested and working.~

It looks like the main page loads well, but I cannot access the tools, cryptpad cannot access the sandbox domain. I had no time investigating further.

I can't say for sure this is the source of the issue but see the domain section of our Administrator Guide. Both domains (main and sandbox ones), must be generated together.

Here is how we usually do it:

acme.sh --issue -d your-main-domain.com -d your-sandbox-domain.com -w /var/www/le_root/

If the TLS certificate isn't valid for both domains, then of course the main one isn't authorized to access the sandbox one.

tituspijean commented 3 months ago

It cannot be done with YunoHost in its current development state. :(

mathilde-cryptpad commented 3 months ago

It cannot be done with YunoHost in its current development state. :(

Sad. We plan to maybe implement some other iframe functionality that would allow to run CryptPad on a single domain while still benefiting from sandboxing.

Ddataa commented 2 months ago

is this a new feature regarding sandbox domain that is blocking this PR ?

because the thing about two domains with correct certs is actually working in current version so it can be done in current development state then :) ?

mathilde-cryptpad commented 2 months ago

is this a new feature regarding sandbox domain that is blocking this PR ?

because the thing about two domains with correct certs is actually working in current version so it can be done in current development state then :) ?

Well, if it's supported by YunoHost, I don't see any blocker for making it work! :)

Josue-T commented 1 month ago

is this a new feature regarding sandbox domain that is blocking this PR ?

because the thing about two domains with correct certs is actually working in current version so it can be done in current development state then :) ?

We use this feature on metronome app here: https://github.com/YunoHost-Apps/metronome_ynh/blob/master/hooks/cert_alternate_names

I think for this app we just should reuse this solution. (ideally we should have a generic solution for this, but it might still take some time to implement this).

ericgaspar commented 1 month ago

!testme

yunohost-bot commented 1 month ago

:book: :worm: Test Badge

yunohost-bot commented 1 month ago

:stuck_out_tongue_winking_eye: Test Badge

ericgaspar commented 1 week ago

!testme

yunohost-bot commented 1 week ago

Living in the past, are we? :cow::eye: Test Badge

yunohost-bot commented 1 week ago

Test it, book it, worm it, technologic. :robot: :notes: Test Badge

ericgaspar commented 1 week ago

@tituspijean did you test this PR? (I have included yours: https://github.com/YunoHost-Apps/cryptpad_ynh/pull/228). I am unable to test right now

tituspijean commented 1 week ago

@tituspijean did you test this PR? (I have included yours: #228). I am unable to test right now

With this, the Drive now loads correctly. However apps (code, sheets, ...) do not load. Firefox states that "sandbox-domain will not allow Firefox to load the page is it integrated in another domain" and points to that page: https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/X-Frame-Options

I'm guessing YunoHost's standard CSP interfere here.

tituspijean commented 1 week ago

@mathilde-cryptpad you might be able to help us out. I'm trying to fit https://github.com/cryptpad/cryptpad/blob/main/docs/example-advanced.nginx.conf into the app's NGINX conf files.

I understand this is a all-in-one file that covers both the main domain and the sandbox one, so I duplicate it accordingly to fit YunoHost's files.

While the standard file (https://github.com/cryptpad/cryptpad/blob/main/docs/example.nginx.conf) works but triggers the error displayed above, the advanced one actually does not contain the equivalent of location / { proxy_pass ... ... and my browser is ending up downloading the contents of /var/www/cryptpad/customize/www/index.html instead of displaying it.

Josue-T commented 1 day ago

!testme

Tested on my side and now with the last fixes it work well.

Ideally we should test on some instance to be sure that the migration work well.

yunohost-bot commented 1 day ago

:book: :worm: Test Badge

yunohost-bot commented 1 day ago

Living in the past, are we? :cow::eye: Test Badge

tituspijean commented 1 day ago

On my instance, the sandbox domain is sandbox-pad.domain.example. The recent commits put it at sandbox.pad.domain.example without updating the domains list or notifying the user.

The command yunohost domain add has been removed. I would suggest to use it and add the appropriate permission to avoid clashes and allow Let's Encrypt certificate generation.

Edit: after manually configuring the domain and its certificate, the drive does not load (I've deleted my browser cache and forgotten the domain in Firefox):

Content-Security-Policy : Les paramètres de la page ont empêché l’exécution d’un script intégré (script-src-elem) car il enfreint la directive suivante : « script-src 'self' resource: https://pad.domain.example/ »

La ressource à l’adresse « https://sandbox.pad.domain.example/drive/inner.html?[removed details] » a été bloquée en raison de son en-tête Cross-Origin-Resource-Policy (ou de son absence). Consultez https://developer.mozilla.org/docs/Web/HTTP/Cross-Origin_Resource_Policy_(CORP)#

Josue-T commented 1 day ago

Hello,

Thanks for the feedback.

On my instance, the sandbox domain is sandbox-pad.domain.example. The recent commits put it at sandbox.pad.domain.example without updating the domains list or notifying the user.

Yes I agree that a change was made on this side, but it was needed to ensure that we use the same certificat than the main cryptpad domain. Note I added a warning about this here: https://github.com/YunoHost-Apps/cryptpad_ynh/pull/217/files#diff-57aeb84da86cb7420dfedd8e49bc644fb799d5413d01927a0417bde753e8922fR15, but it could be improved.

The command yunohost domain add has been removed. I would suggest to use it and add the appropriate permission to avoid clashes and allow Let's Encrypt certificate generation.

The issue with this is that we by this way currently there are not way to specify to use the same certificat than for the main domain, so it's why I changed this.

To be just sure can you do theses following step:

tituspijean commented 1 hour ago

Yes I agree that a change was made on this side, but it was needed to ensure that we use the same certificat than the main cryptpad domain. Note I added a warning about this here: #217 (files), but it could be improved.

I completely agree that simplifying the domain(s) is good. :) I am not sure the warning will be noticed. I would suggest to create a doc/POST_UPGRADE.d/<version>.md file requesting the admins to force-renew their certificate. The cert_alternate_names hook should do the rest.

Side note, opting for a subdomain will exclude people relying on .local domains and mDNS. I am unsure that's a huge population. 😶‍🌫️

Did you add a Yunohost domain for the sandbox subdomain ?

Yes, since the migration from sandbox-pad.domain.example to sandbox.pad.domain.example did not happen the way I wanted 😅. I have just realized doing this overwrote the custom configuration file for the sandbox domain. I think it should be handled by a hook, since admins would be a yunohost tools regen-conf away from breaking stuff.

Since it was this which broke the app, I have just reapplied the custom configuration, and now the app works. 👍

Can you go on crpytpaddomain.tld/checkup share me what is wrong ?

Now the checkup is way less angry: https://paste.yunohost.org/epiyefoger.rust

Edit: after re-reading our conversation, I now understand why you "hid" the sandbox domain by not integrating it in the webadmin with the standard way, and why you added a CNAME record suggestion. If we opt for that modus operandi, I think we need to clearly document what users need to do. (I actually do not know where/when that record suggestion is displayed)