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

Make CORS domains configurable in UI #13174

Merged
merged 12 commits into from Jul 25, 2018
Merged

Make CORS domains configurable in UI #13174

merged 12 commits into from Jul 25, 2018

Conversation

sgiehl
Copy link
Member

@sgiehl sgiehl commented Jul 16, 2018

The new tuple input fields kind of did the same as the configuration for trusted host.
But to be able to provide the current value in the format the JS needs it, I added a new callback to the field config to convert the value. It's currently called prepareValue, but I'm not very happy with that name, but couldn't think of a better one. Maybe someone has an idea...

Note: By moving the trusted host setting to the SystemSetting class there is a change in UI. Before the was a confirmation box shown before changing the trusted hosts. Imho it's not really needed, but we could also implement an optional confirmation for all plugin settings (that can be activated in each SystemSettings class)

fixes #13156

@sgiehl sgiehl added not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. Needs Review PRs that need a code review labels Jul 16, 2018
@sgiehl sgiehl added this to the 3.6.0 milestone Jul 16, 2018
return $this->storage->getValue($this->name, $this->defaultValue, $this->type);
$value = $this->storage->getValue($this->name, $this->defaultValue, $this->type);

$config = $this->configureField();
Copy link
Member

Choose a reason for hiding this comment

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

A while ago we refactored the entire settings API so we basically wouldn't need to configureField() when getting the value as it is a performance issue. Neither should we validateValue ideally. Any chance we can tweak the UI component to return the value in an alternative format through a flag or something?
We'd really need to make sure to leave the getValue as it is... I probably should have added a comment about this...

$corsDomains[] = $value['domain'];
}
}
return $corsDomains;
Copy link
Member

Choose a reason for hiding this comment

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

I think this could be shortened to return array_column($values, 'domain');

Copy link
Member

Choose a reason for hiding this comment

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

Same for $hostnames below

*
* @var null|\Closure
*/
public $prepareValue = null;
Copy link
Member

Choose a reason for hiding this comment

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

Not sure if it's better or worse (you can be the judge), but what about naming this variable $decodeValue?

@sgiehl
Copy link
Member Author

sgiehl commented Jul 17, 2018

For now I've done @diosmosis suggestions.
@tsteur I'm not sure what the best solution would be to improve it. Should we add something like a field array field type, that does the same as multi tuple but with only one field and thus returns the data as a simple array? Or do you have a better suggestion?

@tsteur
Copy link
Member

tsteur commented Jul 17, 2018

multi tuple but with only one field and thus returns the data as a simple array

sounds good to me 👍 be also reusable then etc.

@sgiehl
Copy link
Member Author

sgiehl commented Jul 18, 2018

@tsteur I've now implemented a new field field array to handle that. that avoids the problem with transforming values for the config...

@tsteur
Copy link
Member

tsteur commented Jul 19, 2018

👍 works
Can we add an example to the UI demo page for that field? And maybe we can also add a new changelog entry for this new field similar to in 3.5.0?

Here I would maybe only write "hostname" instead of "valid matomo hostname"?
image

In the other title we can maybe say "Cross-Origin Resource Sharing (CORS)" instead of the lower case writing?
image

In the config it also saves an empty value for each entry like: trusted_hosts[] = ""
Maybe in the settings transform field config we could automatically remove the last empty entry? Not sure if this can be otherwise done in the JS widget.

@sgiehl
Copy link
Member Author

sgiehl commented Jul 19, 2018

Here I would maybe only write "hostname" instead of "valid matomo hostname"?

That was this way before. Could change it, but that needs new translations then...

Everthing else should be updated now

@tsteur
Copy link
Member

tsteur commented Jul 19, 2018

Added a comment re the demo page otherwise 👍from my side. Tests will need to be fixed though (eg system setting is now returned in API)

@sgiehl
Copy link
Member Author

sgiehl commented Jul 22, 2018

@tsteur I've reorganized the form demos a bit. Let me know what you think...

@tsteur
Copy link
Member

tsteur commented Jul 22, 2018

Looks great 👍 for some reason I couldn't see the failing UI test in https://builds-artifacts.matomo.org/matomo-org/matomo/corssettings/28884/ ... the tests would still need to be fixed I reckon (unless I missed it in the changes files in the PR)

@sgiehl
Copy link
Member Author

sgiehl commented Jul 22, 2018

tests should now be updated. Not sure why the UI tests didn't fail before...

@tsteur
Copy link
Member

tsteur commented Jul 23, 2018

👍

@sgiehl
Copy link
Member Author

sgiehl commented Jul 23, 2018

So guess we are good to merge? Could anybody confirm that it's fine the trusted hosts confirmation will be removed?

@tsteur
Copy link
Member

tsteur commented Jul 23, 2018

For me it would be fine to merge and that the confirmation is removed. Not really needed IMO

@diosmosis
Copy link
Member

Noticed something odd in the config file diagnostic page: https://builds-artifacts.matomo.org/matomo-org/matomo/corssettings/28915/UIIntegrationTest_admin_diagnostics_configfile.png

The cors_domain/trusted_hosts settings are now in the CoreAdminHome section as well as General. Is that expected?

@mattab
Copy link
Member

mattab commented Jul 23, 2018

Could anybody confirm that it's fine the trusted hosts confirmation will be removed?

If possible it's slightly better to keep it, but it's also not a blocker/required. How much effort would it be to keep it?

@sgiehl
Copy link
Member Author

sgiehl commented Jul 24, 2018

If possible it's slightly better to keep it, but it's also not a blocker/required. How much effort would it be to keep it?

we would need to extend the default behavior for system settings provided by plugins to implement something like that

@sgiehl
Copy link
Member Author

sgiehl commented Jul 24, 2018

The cors_domain/trusted_hosts settings are now in the CoreAdminHome section as well as General. Is that expected?

Need to have a look at that. Imho it doesn't make sense to list system settings there that are of type ManagedInConfigOnly

@sgiehl
Copy link
Member Author

sgiehl commented Jul 24, 2018

@diosmosis I've adjusted the way settings stored in config are handled...

@diosmosis
Copy link
Member

👍 looks good, will merge

@diosmosis
Copy link
Member

diosmosis commented Jul 24, 2018

@sgiehl Actually spoke too soon, there is another failing test in ConfigReaderTest. Tried to fix myself but wasn't able to say whether the changes to addConfigValuesFromSystemSettings() were correct or not. Maybe the !empty($configValues[$configSection][$name]) check should also make sure the config section isn't a plugin? So it would skip if General or database_tests is used, but not if ExamplePluginSettings is used.

EDIT: that's probably not the correct solution because then the UI_CONTROL_PASSWORD if won't be invoked in some cases.

@sgiehl
Copy link
Member Author

sgiehl commented Jul 25, 2018

@diosmosis should be fixed now

@diosmosis diosmosis merged commit ef9c48c into 3.x-dev Jul 25, 2018
@diosmosis diosmosis deleted the corssettings branch July 25, 2018 19:11
@diosmosis
Copy link
Member

👍 merged

InfinityVoid pushed a commit to InfinityVoid/matomo that referenced this pull request Oct 11, 2018
* Make CORS domain configureable in UI

* Move trusted host settings to SystemSettings class

* Use unique id for pluginSettings

* Improve styling

* Improve help text

* improve code & naming

* Implements new UI field array type

* review adjustments

* reorganize form demo

* update UI files

* update system test files

* Improve handling of Config Settings
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Review PRs that need a code review not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Let super users configure CORS hostnames in the UI
4 participants