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

Add ability to disable URL overwriting #5168

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

Conversation

docjyJ
Copy link
Collaborator

@docjyJ docjyJ commented Aug 25, 2024

Hey

I need to have multiple domains.
My cloud is shared with a friend who has his own domain.
I know I'm not the only one bothering you with this feature. #2598

This PR allows disabling the configuration of OVERWRITEHOST and OVERWRITEPROTOCOL.

Of course, AIO does not support multiple domains, even with redirection disabled.

Take care

Signed-off-by: Jean-Yves <7360784+docjyJ@users.noreply.github.com>
@docjyJ docjyJ added 3. to review Waiting for reviews enhancement New feature or request labels Aug 25, 2024
@docjyJ docjyJ added this to the next milestone Aug 25, 2024
@docjyJ docjyJ requested a review from szaimen August 25, 2024 15:32
@docjyJ docjyJ self-assigned this Aug 25, 2024
@szaimen
Copy link
Collaborator

szaimen commented Aug 29, 2024

Hi @docjyJ, this is a though one. I need to think about it...

@szaimen
Copy link
Collaborator

szaimen commented Aug 29, 2024

My cloud is shared with a friend who has his own domain.

So you don't see any way you both could use one domain together? Also, running 2 AIO instances is not an option?

@docjyJ
Copy link
Collaborator Author

docjyJ commented Aug 29, 2024

According to the documentation:

  • overwritehost The automatic hostname detection of Nextcloud can fail in certain reverse proxy and CLI/cron situations.
  • overwrite.cli.url Use this configuration parameter to specify the base URL for any URLs which are generated within Nextcloud using any kind of command line tools (cron or occ).

It seems that nextcloud can detect the url behind a Caddy proxy. In addition, my PR does not modify the overwrite.cli.url parameter, this should avoid problematic cases in CLI/cron.

The url defined in the instance is the default url for CLI/cron tasks, with a Caddy proxy revese, there should be no error.

I think it's a good idea for small instances. Many users ask for it.

We can add the mention "at your own risk".

@szaimen
Copy link
Collaborator

szaimen commented Aug 30, 2024

I see. Thanks for an explanation of what this does.

However the problem is rather of a conceptual nature:

  1. Many of the other components are not going to work with multiple domains out of the box (like talk and collabora, maybe more)
  2. overwrite.cli.url only supports 1 domain
  3. In my opinion, we already have too many settings. Adding more options does not make it easier to configure and rather more complex to maintain (from the maintainers side), so I would like to only add settings that really make sense

I hope this clarifies things a bit why I did not want to add this in the past...

@docjyJ
Copy link
Collaborator Author

docjyJ commented Aug 30, 2024

I understand.

  1. Many of the other components are not going to work with multiple domains out of the box (like talk and collabora, maybe more)

Currently I have a way I have to disable this feature manually by commenting out the proxy configuration.

I collaborated to install, I will test how it works with several domains.

  1. overwrite.cli.url only supports 1 domain

Yes, I am looking like several people to add domain aliases. There is always the default domain $NC_DOMAIN.

  1. In my opinion, we already have too many settings. Adding more options does not make it easier to configure and rather more complex to maintain (from the maintainers side), so I would like to only add settings that really make sense

We agree I am not asking for a new feature. I would like not to have to reconfigure the second domain with each update... The trusted_proxies parameters are persisted but not overwritehost...

If it's really too restrictive I can create my own image. What do you think?

@docjyJ
Copy link
Collaborator Author

docjyJ commented Aug 30, 2024

Hi, I've been looking into Collabora. They officially support multiple domains :

https://sdk.collaboraonline.com/docs/installation/Configuration.html#multihost-configuration

The solution of being able to configure multiple instances does not interest me because I have a low-performance configuration...

@szaimen szaimen modified the milestones: v9.5.0, next Sep 4, 2024
@szaimen
Copy link
Collaborator

szaimen commented Sep 5, 2024

Sorry for my late reply. I was on vacation...

Hi, I've been looking into Collabora. They officially support multiple domains :

https://sdk.collaboraonline.com/docs/installation/Configuration.html#multihost-configuration

The solution of being able to configure multiple instances does not interest me because I have a low-performance configuration...

Yes, I know that it is technically possible to make it work but currently it is not implemented on the AIO side.

So as I said above, if we merge this, quite a few of AIOs built-in features will not work.


Another reason is that I don't want to maintain this feature.

However I would be fine with merging this if you @docjyJ would step in as the maintainer of this feature. That means if bug reports or any questions regarding this feature come in, I would ask you for help on the topic. If that is fine for you, I would continue with the review.

@docjyJ
Copy link
Collaborator Author

docjyJ commented Sep 5, 2024

Why not, I can write a documentation with :

  • How to disable automatic redirection.
  • Works only with an external reverse proxy.
  • Link to NC doc about proxy
  • Only for small instances (for a large number of users it's better to have 2 aio)
  • Then how to configure collabora and nextcloud with others dommain.
  • You need a main domain for the CLI...
  • Experimental features

I use collabora, but not talk eventually I can install it to write documentation

I understand that you have a lot of work to do. Thank you very much!

I hope to meet you next weekend !

@szaimen szaimen modified the milestones: v9.5.1, next Sep 10, 2024
@szaimen
Copy link
Collaborator

szaimen commented Sep 12, 2024

Why not, I can write a documentation with :

  • How to disable automatic redirection.
  • Works only with an external reverse proxy.
  • Link to NC doc about proxy
  • Only for small instances (for a large number of users it's better to have 2 aio)
  • Then how to configure collabora and nextcloud with others dommain.
  • You need a main domain for the CLI...
  • Experimental features

Sounds good 👍

I understand that you have a lot of work to do. Thank you very much!

Thank you! :)

I hope to meet you next weekend !

Yes, that will be great! Cool that you are coming to the conf! Looking forward seeing you! :)

@szaimen szaimen added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Sep 15, 2024
@szaimen szaimen modified the milestones: v9.6.0, next Sep 18, 2024
@docjyJ docjyJ marked this pull request as draft September 28, 2024 18:25
@docjyJ docjyJ marked this pull request as ready for review September 28, 2024 18:26
@docjyJ docjyJ marked this pull request as draft September 28, 2024 18:26
@docjyJ docjyJ removed this from the next milestone Sep 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2. developing Work in progress enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants