Add new settings to set the contact email address #2279

Pierre-Assemat · 2017-08-10T21:30:21Z

danlamanna

I see this is WIP but I decided to leave a couple of comments.

danlamanna · 2017-08-10T21:41:39Z

girder/models/setting.py

+    @setting_utilities.validator(SettingKey.CONTACT_EMAIL_ADDRESS)
+    def validateCoreContactEmailAddress(doc):
+        if not doc['value']:
+            raise ValidationException('Email from address must not be blank.', 'value')


There is a configuration option for specifying a regex email addresses must conform to, that can probably be used here as well.

Per the discussion here we probably want to support more than a simple email address regex (especially since the default EMAIL_FROM_ADDRESS default already uses a proper name component).

The space and URL encoding described by the RFC also should be done, but probably not in the validator (as that will make the value hard to edit). The best place is probably in the Pug template.

I skimmed through RFC6068, and it appears that some degree of escaping is definitely necessary. I'm not yet clear on whether the syntax with a proper name is legal, though I expect most clients would tolerate it.

We still need to address this.

danlamanna · 2017-08-10T21:46:07Z

girder/utility/webroot.py

+        events.bind('model.setting.save.after', CoreEventHandler.WEBROOT_SETTING_CHANGE,
+                    self._onSettingSave)
+
+    def _onSettingSave(self, event):


This sort of thing (which exists in other places in Girder thanks to yours truly) will not work in a distributed installation.

For the purposes of this PR this might be fine, but I think that #2274 might try to remove these patterns. For more detail about it there's a thread on girder-developers with the subject "Signalling between multiple Girder servers".

Indeed, as part of fully implementing #2274, the WebrootBase.indexHtml should be switched to use a proper cache. I see nothing more that can be done on this PR.

brianhelba · 2017-08-10T22:44:58Z

clients/web/src/templates/layout/layoutFooter.pug

@@ -1,6 +1,6 @@
 .g-footer-links
  a#g-about-link(href="http://girder.readthedocs.org/en/latest/user-guide.html") About
-  a#g-contact-link(href="mailto:kitware@kitware.com") Contact
+  a#g-contact-link(href=href) Contact


Can this be something like contactHref?

brianhelba · 2017-08-10T22:46:00Z

clients/web/src/views/App.js

@@ -40,6 +40,7 @@ var App = View.extend({
        this._started = false;
        settings = settings || {};
        if (settings.start === undefined || settings.start) {
+            this.contactEmail = settings.contactEmail;


We should always store this setting in an instance variable, even if we're not starting the app.

brianhelba · 2017-08-10T22:46:57Z

clients/web/src/views/layout/FooterView.js

    render: function () {
        this.$el.html(LayoutFooterTemplate({
-            apiRoot: getApiRoot()
+            apiRoot: getApiRoot(),
+            href: this.emailHref


Can this be something like contactHref?

brianhelba · 2017-08-10T22:47:35Z

clients/web/src/views/layout/FooterView.js

@@ -9,9 +9,16 @@ import 'girder/stylesheets/layout/footer.styl';
 * This view shows the footer in the layout.
 */
 var LayoutFooterView = View.extend({
+
+    initialize: function (settings) {
+        var mail = settings.contactEmail; // Has to change


This should have a fallback value, in case settings.contactEmail is not provided.

Also, use let or const.

the fallback value can be the default email ?

I'm not sure, but maybe not. If a default is needed for this widget, it means that it's being used in another application that didn't pass enough parameters at all.

Maybe if no user is passed at all, then don't use an href? What do you think?

I used a blank ('') instead, but do you mean test if settings.contactEmail is provided and if not don't use href and hide the contact link ?

I think that's a good idea.

This case will only apply when a some external code imports and reuses this view, but it should be handled gracefully.

This commit take both the client and the server side in adding new settings

This test verify the emailAddress setting in the webroot.mako

brianhelba · 2017-08-14T16:35:18Z

clients/web/src/views/layout/FooterView.js

@@ -9,9 +9,18 @@ import 'girder/stylesheets/layout/footer.styl';
 * This view shows the footer in the layout.
 */
 var LayoutFooterView = View.extend({
+
+    initialize: function (settings) {
+        const mail = settings.contactEmail || '';


In initialize, I think we should always set this.contactEmail to be equal to settings.contactEmail, with a default of null if it's not provided.

Then, in render, we should conditionally apply the concatenation with mailto: (encoding it if necessary), then pass that result (or null) into the template.

This will also LayoutFooterView to be subclassed such that it takes the same parameters, but renders them in a different way.

brianhelba · 2017-08-14T16:35:30Z

clients/web/src/views/App.js

@@ -39,6 +39,7 @@ var App = View.extend({
    initialize: function (settings) {
        this._started = false;
        settings = settings || {};
+        this.contactEmail = settings.contactEmail || '';


Let's use null for the value when no setting is provided, since that will never be confused with a case where a server sends a setting value that's purposely set to empty string.

brianhelba · 2017-08-14T16:40:36Z

tests/cases/webroot_test.py

@@ -42,6 +42,9 @@ def testAccessWebRoot(self):
        self.assertTrue('girder_app.min.js' in body)
        self.assertTrue('girder_lib.min.js' in body)

+        emailAddress = self.model('setting').get(SettingKey.CONTACT_EMAIL_ADDRESS)


This is just going to get the default.

To ensure that changes to this setting are immediately respected, we should next set this setting to something totally different, then make another request to get the webroot, and ensure that the value is updated.

brianhelba · 2017-08-14T16:43:08Z

clients/web/src/templates/layout/layoutFooter.pug

@@ -1,6 +1,7 @@
 .g-footer-links
  a#g-about-link(href="http://girder.readthedocs.org/en/latest/user-guide.html") About
-  a#g-contact-link(href="mailto:kitware@kitware.com") Contact
+  if (contactHref !== '')


Pug conditionals don't need parentheses.

I think you can check whether this is truthy at all (if contactHref), which will fail if it's null (intentionally not set) or an empty string (intentionally set, but empty).

brianhelba · 2017-08-14T16:45:06Z

clients/web/src/templates/body/systemConfiguration.pug

+      input#g-core-contact-email-address.form-control.input-sm(
+          type="text", value=settings['core.contact_email_address'] || '',
+          placeholder=`Default: ${defaults['core.contact_email_address'] || 'none'}`,
+          title="The contact email address for support.")


How about title="The contact email address for users to obtain support with this instance."? I'm open to even better phrasing, but this is a little more descriptive.

brianhelba · 2017-08-14T16:46:53Z

girder/models/setting.py

+    @setting_utilities.validator(SettingKey.CONTACT_EMAIL_ADDRESS)
+    def validateCoreContactEmailAddress(doc):
+        if not doc['value']:
+            raise ValidationException('Email from address must not be blank.', 'value')


We still need to address this.

brianhelba · 2017-08-14T16:52:33Z

clients/web/src/views/layout/FooterView.js

+    initialize: function (settings) {
+        const mail = settings.contactEmail || '';
+        if (mail !== '') {
+            this.emailHref = 'mailto:' + mail;


Use an ES6 template literal for this concatenation.

brianhelba · 2017-08-14T23:26:51Z

After reading more of RFC6068 myself, it's clear that we cannot use a name-addr (like 'Kitware Support <kitware@kitware.com>' or 'Foo Bar <foo@bar.com>'.

Although that spec also requires us to percent-encode many other characters that may appear in a valid email address (e.g. +), I think we can add the percent-encoding as part of a later enhancement. It will need to be done in the Javascript code, rather than the Python validation, as it's specific to the mailto: encoding, not the value of the email address itself. Some basic validation of the email address in the setting would also be nice, but apparently that's not trivial either.

So, for now, I think you need to just replace the default and test email addresses with just the core addr-spec components (e.g. 'kitware@kitware.com' or 'foo@bar.com').

Pierre-Assemat · 2017-08-15T12:34:47Z

Do I need to test if it's a good email address with some regular expression too ?

And do I need to change the Email from address too ?

brianhelba · 2017-08-15T15:39:41Z

EMAIL_FROM_ADDRESS is not used in a mailto: link, and so does not need to follow the stricter rules of RFC6068, rather, it can probably use RFC5322.

Because:

EMAIL_FROM_ADDRESS already does not validate using an email regex
There is the potential for duplicate functionality with the email_regex setting from Girder's config file
A robust regex for email-validation looks to be quite complicated

I think it's justified to merge this without extensive validation, then fix things more globally in a separate PR. I don't want to derail this entire PR for a corner case, as long as we eventually fix the corner case. I'll file an issue for it now.

brianhelba · 2017-08-15T17:13:25Z

#2282

brianhelba

A minor issue inline.

More importantly, I realized that this now contains an injection vulnerability. Try setting the contact email address to exactly

foo@bar.com'});});</script><script>$(function(){girder.app.render(); alert('Pwned');});</script><script>$(function(){new girder.views.App({parentView: null,dummy: '

via the settings page.

While we could attempt to sanitize some of this via a more sophisticated validator, the simplest and most reliable way is to probably encode the string to JSON programatically, which will escape any quotes for us. Any evil script tags, etc. would remain unescaped inside the string, but they would be escaped when Pug renders the footer link (since Pug escapes everything by default).

So, rather than include quotes in the webroot.mako template, use Python's to convert the string value to JSON, and inject that result into webroot.mako, and set it (without explicit quotes) as the value of contactEmail: .

brianhelba · 2017-08-15T22:39:04Z

clients/web/src/views/layout/FooterView.js

+    initialize: function (settings) {
+        const mail = settings.contactEmail || null;
+        if (mail !== null) {
+            this.emailHref = `mailto: ${mail}`;


RFC6068 says that there's no space between mailto: and the email address,

Also, in my opinion, it'd be cleaner overall to name this local variable const contactEmail, and the instance variable this.contactHref.

When you propose to convert into JSON you mean in the webroot.py ? Just like that :
contactEmail = json.dumps(ModelImporter.model('setting').get(SettingKey.CONTACT_EMAIL_ADDRESS))

And I just insert this into the template webroot.mako ?

mgrauer · 2017-08-16T19:56:47Z

Two in one day! 🥂

danlamanna reviewed Aug 10, 2017

View reviewed changes

brianhelba requested changes Aug 10, 2017

View reviewed changes

brianhelba mentioned this pull request Aug 10, 2017

Serve a Content Security Policy via Girder server #2281

Open

brianhelba added domain: server domain: web-client status: WIP labels Aug 11, 2017

Pierre Assemat added 2 commits August 11, 2017 12:44

Add new settings to set the contact email address

886d36b

This commit take both the client and the server side in adding new settings

Add tests for settings in the server and client side

9e39cb4

This test verify the emailAddress setting in the webroot.mako

Pierre-Assemat force-pushed the set-email-contact-link branch from 832ef92 to 5682b4c Compare August 11, 2017 17:03

Hide the contact link when the setting is missing

ff63f22

Pierre-Assemat force-pushed the set-email-contact-link branch 2 times, most recently from e48b126 to a0b2c2c Compare August 14, 2017 16:07

Pierre-Assemat changed the title ~~WIP : Add new settings to set the contact email address~~ Add new settings to set the contact email address Aug 14, 2017

Pierre-Assemat removed the status: WIP label Aug 14, 2017

brianhelba requested changes Aug 14, 2017

View reviewed changes

Pierre-Assemat force-pushed the set-email-contact-link branch 2 times, most recently from d75c5d3 to f74043a Compare August 14, 2017 18:26

correct syntax and complete test

e834649

Pierre-Assemat force-pushed the set-email-contact-link branch from f74043a to e834649 Compare August 15, 2017 17:26

brianhelba requested changes Aug 15, 2017

View reviewed changes

brianhelba previously approved these changes Aug 16, 2017

View reviewed changes

Prevent injection into the contact email setting

de4ec3b

Pierre-Assemat dismissed brianhelba’s stale review via de4ec3b August 16, 2017 19:37

Pierre-Assemat force-pushed the set-email-contact-link branch from 307f0f3 to de4ec3b Compare August 16, 2017 19:37

brianhelba approved these changes Aug 16, 2017

View reviewed changes

Pierre-Assemat merged commit 26341b3 into master Aug 16, 2017

Pierre-Assemat deleted the set-email-contact-link branch August 16, 2017 19:54

brianhelba mentioned this pull request Sep 4, 2017

Fixes #2322. Do not HTML-escape values inlined into javascript #2325

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add new settings to set the contact email address #2279

Add new settings to set the contact email address #2279

Pierre-Assemat commented Aug 10, 2017

danlamanna left a comment

danlamanna Aug 10, 2017

brianhelba Aug 10, 2017

brianhelba Aug 10, 2017

brianhelba Aug 14, 2017

danlamanna Aug 10, 2017

brianhelba Aug 10, 2017

brianhelba Aug 10, 2017

brianhelba Aug 10, 2017

brianhelba Aug 10, 2017

brianhelba Aug 10, 2017

brianhelba Aug 10, 2017

Pierre-Assemat Aug 11, 2017

brianhelba Aug 11, 2017

Pierre-Assemat Aug 11, 2017

brianhelba Aug 11, 2017

brianhelba Aug 14, 2017

brianhelba Aug 14, 2017

brianhelba Aug 14, 2017

brianhelba Aug 14, 2017

brianhelba Aug 14, 2017

brianhelba Aug 14, 2017

brianhelba Aug 14, 2017

brianhelba commented Aug 14, 2017

Pierre-Assemat commented Aug 15, 2017 •

edited

Loading

brianhelba commented Aug 15, 2017

brianhelba commented Aug 15, 2017

brianhelba left a comment

brianhelba Aug 15, 2017

Pierre-Assemat Aug 16, 2017 •

edited

Loading

mgrauer commented Aug 16, 2017

Add new settings to set the contact email address #2279

Add new settings to set the contact email address #2279

Conversation

Pierre-Assemat commented Aug 10, 2017

danlamanna left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brianhelba commented Aug 14, 2017

Pierre-Assemat commented Aug 15, 2017 • edited Loading

brianhelba commented Aug 15, 2017

brianhelba commented Aug 15, 2017

brianhelba left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Pierre-Assemat Aug 16, 2017 • edited Loading

Choose a reason for hiding this comment

mgrauer commented Aug 16, 2017

Pierre-Assemat commented Aug 15, 2017 •

edited

Loading

Pierre-Assemat Aug 16, 2017 •

edited

Loading