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.

Sign up for GitHub

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

Jump to bottom

making NEXTCLOUD_TRUSTED_DOMAINS after installation #2084

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

making NEXTCLOUD_TRUSTED_DOMAINS after installation #2084

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a16b4f7
making NEXTCLOUD_TRUSTED_DOMAINS after install
Oct 13, 2023
d13714b
backwards compatibility
Oct 13, 2023
5a8fe00
Moving Config file to right dir
suterdev Oct 16, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions .config/trusteddomains.config.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
<?php

if (getenv('NEXTCLOUD_TRUSTED_DOMAINS')) {
// Get originaly configured values
include(__DIR__.'/config.php');
Copy link
Contributor

Choose a reason for hiding this comment

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

That's evil, don't do that.

Copy link
Author

Choose a reason for hiding this comment

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

The $CONFIG is overwritten so that shouldn't be an issue. But if anyone does interesting things in config.php it could break things, you are right. I did this to make it backwards compatible, so people can still use their values manually configured in config.php (or with occ). Any ideas how to make this happen?

Copy link
Author

Choose a reason for hiding this comment

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

@kesselb any ideas?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, no ideas. But it still looks like a bad idea to include the actual configuration here again.

Copy link

Choose a reason for hiding this comment

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

The environment variable should override trusted_domains in config.php. But since this will be a change in behavior from earlier, when it was only set during the installation, I can see the issue of breaking a lot of existing setups. My suggestion is to create a new variable for this. Something like just $TRUSTED_DOMAINS, which will also be more in line with other env variables like $TRUSTED_PROXIES which do not use the NEXTCLOUD_ prefix. Then you could just do this:

$trustedDomains = getenv('TRUSTED_DOMAINS');
if ($trustedDomains) {
  $CONFIG['trusted_domains'] = array_filter(array_map('trim', explode(' ', $trustedDomains)));
}

and nothing should break unless the user explicitly sets the new variable. The old $NEXTCLOUD_TRUSTED_DOMAINS should then perhaps be deprecated.


// Add environment variables
$CONFIG = array(
'trusted_domains' => array_merge($CONFIG['trusted_domains'],array_map(function($domain){return trim($domain);},explode(",",getenv('NEXTCLOUD_TRUSTED_DOMAINS'))))
Copy link

Choose a reason for hiding this comment

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

@melvin-suter I think the 'NEXTCLOUD_TRUSTED_DOMAINS' is separated as mentioned in the docker doc.

);
}