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

making NEXTCLOUD_TRUSTED_DOMAINS after installation #2084

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

Conversation

melvin-suter
Copy link

ALL configurations should be made with environment variables instead of manually editing configurations.
The ENV variable is already there but ignored after installation. This change will allow trusted domains to be configured over ENV variables, while still considering the configured values inside config.php.

Melvin mngmt-02 added 2 commits October 13, 2023 16:37
Signed-off-by: Melvin mngmt-02 <[email protected]>
Signed-off-by: Melvin Suter <[email protected]>
Signed-off-by: Melvin mngmt-02 <[email protected]>
Signed-off-by: Melvin Suter <[email protected]>
@kesselb
Copy link
Contributor

kesselb commented Oct 14, 2023

Hi,

@melvin-suter
Copy link
Author

Hi,

Changed it. Thanks.


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.


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

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants