Merge pull request #743 from weseek/imprv/saml-settings-validation

Imprv/saml settings validation

resource/locales/en-US/translation.json

@@ -363,6 +363,7 @@
     "Treat email matching as identical_warn": "WARNING: Be aware of security because the system treats the same user as a match of <code>%s</code>.",
     "Use env var if empty": "Use env var <code>%s</code> if empty",
     "Use default if both are empty": "If both ​​are empty, the default value <code>%s</code> is used.",
+    "missing mandatory configs": "The following mandatory items are not set in either database nor environment variables.",
     "ldap": {
       "server_url_detail": "The LDAP URL of the directory service in the format <code>ldap://host:port/DN</code> or <code>ldaps://host:port/DN</code>.",
       "bind_mode": "Binding Mode",
@@ -405,7 +406,7 @@
       "cert_detail1": "PEM-encoded X.509 signing certificate to validate the response from IdP",
       "cert_detail2": "If both are empty, no validation is processed.",
       "Use env var if empty": "If the value in the database is empty, the value of the environment variable <code>%s</code> is used.",
-      "note for the only env option": "Currently, the setting item that enables or disables the SAML authentication and the highlighted setting items use only the value of environment variables.<br>To change this setting, please change to false or delete the value of the environment variable <code>%s</code> ."
+      "note for the only env option": "The setting item that enables or disables the SAML authentication and the highlighted setting items use only the value of environment variables.<br>To change this setting, please change to false or delete the value of the environment variable <code>%s</code> ."
     },
     "OAuth": {
       "register": "Register for %s",

resource/locales/ja/translation.json

@@ -379,6 +379,7 @@
     "Treat email matching as identical_warn": "警告: <code>%s</code> の一致を以て同一ユーザーであるとみなすので、セキュリティに注意してください",
     "Use env var if empty": "空の場合、環境変数 <code>%s</code> を利用します",
     "Use default if both are empty": "どちらの値も空の場合、デフォルト値 <code>%s</code> を利用します",
+    "missing mandatory configs": "以下の必須項目の値がデータベースと環境変数のどちらにも設定されていません",
     "ldap": {
       "server_url_detail": "LDAP URLを <code>ldap://host:port/DN</code> または <code>ldaps://host:port/DN</code> の形式で入力してください。",
       "bind_mode": "Bind モード",

src/server/form/admin/securityPassportSaml.js

@@ -4,12 +4,12 @@ const form = require('express-form');
 const field = form.field;
 
 module.exports = form(
-  field('settingForm[security:passport-saml:isEnabled]').trim().toBooleanStrict().required(),
-  field('settingForm[security:passport-saml:entryPoint]').trim().required().isUrl(),
-  field('settingForm[security:passport-saml:issuer]').trim().required(),
-  field('settingForm[security:passport-saml:attrMapId]').trim().required(),
-  field('settingForm[security:passport-saml:attrMapUsername]').trim().required(),
-  field('settingForm[security:passport-saml:attrMapMail]').trim().required(),
+  field('settingForm[security:passport-saml:isEnabled]').trim().toBooleanStrict(),
+  field('settingForm[security:passport-saml:entryPoint]').trim().isUrl(),
+  field('settingForm[security:passport-saml:issuer]').trim(),
+  field('settingForm[security:passport-saml:attrMapId]').trim(),
+  field('settingForm[security:passport-saml:attrMapUsername]').trim(),
+  field('settingForm[security:passport-saml:attrMapMail]').trim(),
   field('settingForm[security:passport-saml:attrMapFirstName]').trim(),
   field('settingForm[security:passport-saml:attrMapLastName]').trim(),
   field('settingForm[security:passport-saml:cert]').trim(),

src/server/routes/admin.js

@@ -1100,6 +1100,8 @@ module.exports = function(crowi, app) {
   actions.api.securityPassportSamlSetting = async(req, res) => {
     const form = req.form.settingForm;
 
+    validateSamlSettingForm(req.form);
+
     if (!req.form.isValid) {
       return res.json({status: false, message: req.form.errors.join('\n')});
     }
@@ -1476,6 +1478,21 @@ module.exports = function(crowi, app) {
     }, callback);
   }
 
+  /**
+   * validate setting form values for SAML
+   *
+   * This validation checks, for the value of each mandatory items,
+   * whether it from the environment variables is empty and form value to update it is empty.
+   */
+  function validateSamlSettingForm(form) {
+    for (const key of crowi.passportService.mandatoryConfigKeysForSaml) {
+      const formValue = form.settingForm[key];
+      if (crowi.configManager.getConfigFromEnvVars('crowi', key) === null && formValue === '') {
+        form.errors.push(`${key} is required`);
+      }
+    }
+  }
+
   return actions;
 };
 

src/server/service/passport.js

@@ -53,6 +53,18 @@ class PassportService {
      * the flag whether serializer/deserializer are set up successfully
      */
     this.isSerializerSetup = false;
+
+    /**
+     * the keys of mandatory configs for SAML
+     */
+    this.mandatoryConfigKeysForSaml = [
+      'security:passport-saml:isEnabled',
+      'security:passport-saml:entryPoint',
+      'security:passport-saml:issuer',
+      'security:passport-saml:attrMapId',
+      'security:passport-saml:attrMapUsername',
+      'security:passport-saml:attrMapMail'
+    ];
   }
 
   /**
@@ -467,6 +479,19 @@ class PassportService {
     this.isSamlStrategySetup = false;
   }
 
+  /**
+   * return the keys of the configs mandatory for SAML whose value are empty.
+   */
+  getSamlMissingMandatoryConfigKeys() {
+    const missingRequireds = [];
+    for (const key of this.mandatoryConfigKeysForSaml) {
+      if (this.crowi.configManager.getConfig('crowi', key) === null) {
+        missingRequireds.push(key);
+      }
+    }
+    return missingRequireds;
+  }
+
   /**
    * setup serializer and deserializer
    *

src/server/util/swigFunctions.js

@@ -114,6 +114,10 @@ module.exports = function(crowi, app, req, locals) {
     return locals.isEnabledPassport() && config.crowi['security:passport-saml:isEnabled'];
   };
 
+  locals.getSamlMissingMandatoryConfigKeys = function() {
+    return crowi.passportService.getSamlMissingMandatoryConfigKeys();
+  };
+
   locals.googleLoginEnabled = function() {
     // return false if Passport is enabled
     // because official crowi mechanism is not used.

src/server/views/admin/widget/passport/saml.html

@@ -21,14 +21,14 @@
                  value="true"
                  type="radio"
                  {% if true === isSamlEnabled %}checked{% endif %}
-                 {% if useOnlyEnvVars %}disabled{% endif %}> ON
+                 {% if useOnlyEnvVars %}readonly{% endif %}> ON
         </label>
         <label class="btn btn-default btn-rounded btn-outline {% if !isSamlEnabled %}active{% endif %}" data-active-class="default">
           <input name="{{nameForIsSamlEnabled}}"
                  value="false"
                  type="radio"
                  {% if !isSamlEnabled %}checked{% endif %}
-                 {% if useOnlyEnvVars %}disabled{% endif %}> OFF
+                 {% if useOnlyEnvVars %}readonly{% endif %}> OFF
         </label>
       </div>
     </div>
@@ -52,6 +52,18 @@
 
   <fieldset id="passport-saml-hide-when-disabled" {%if !isSamlEnabled %}style="display: none;"{% endif %}>
 
+    {% set missingMandatoryConfigKeys = getSamlMissingMandatoryConfigKeys() %}
+    {% if isSamlEnabled && missingMandatoryConfigKeys.length !== 0 %}
+    <div class="alert alert-danger">
+      {{ t("security_setting.missing mandatory configs") }}
+      <ul>
+        {% for missingMandatoryConfigKey in missingMandatoryConfigKeys %}
+        <li>{{ missingMandatoryConfigKey }}</li>
+        {% endfor %}
+      </ul>
+    </div>
+    {% endif %}
+
     <h4>Basic Settings</h4>
     <table class="table authentication-settings-table {% if useOnlyEnvVars %}use-only-env-vars{% endif %}">
       <colgroup>
@@ -70,7 +82,7 @@
                    type="text"
                    name="settingForm[security:passport-saml:entryPoint]"
                    value="{{ getConfigFromDB('crowi', 'security:passport-saml:entryPoint') || '' }}"
-                   {% if useOnlyEnvVars %}disabled{% endif %}>
+                   {% if useOnlyEnvVars %}readonly{% endif %}>
           </td>
           <td>
             <input class="form-control"
@@ -91,7 +103,7 @@
                    type="text"
                    name="settingForm[security:passport-saml:issuer]"
                    value="{{ getConfigFromDB('crowi', 'security:passport-saml:issuer') || '' }}"
-                   {% if useOnlyEnvVars %}disabled{% endif %}>
+                   {% if useOnlyEnvVars %}readonly{% endif %}>
           </td>
           <td>
             <input class="form-control"
@@ -127,7 +139,7 @@
                  type="text"
                  name="settingForm[security:passport-saml:attrMapId]"
                  value="{{ getConfigFromDB('crowi', 'security:passport-saml:attrMapId') || '' }}"
-                 {% if useOnlyEnvVars %}disabled{% endif %}>
+                 {% if useOnlyEnvVars %}readonly{% endif %}>
           <p class="help-block">
             <small>
               {{ t("security_setting.SAML.id_detail") }}
@@ -153,7 +165,7 @@
                  type="text"
                  name="settingForm[security:passport-saml:attrMapUsername]"
                  value="{{ getConfigFromDB('crowi', 'security:passport-saml:attrMapUsername') || '' }}"
-                 {% if useOnlyEnvVars %}disabled{% endif %}>
+                 {% if useOnlyEnvVars %}readonly{% endif %}>
           <p class="help-block">
             <small>
               {{ t("security_setting.SAML.username_detail") }}
@@ -179,7 +191,7 @@
                  type="text"
                  name="settingForm[security:passport-saml:attrMapMail]"
                  value="{{ getConfigFromDB('crowi', 'security:passport-saml:attrMapMail') || '' }}"
-                 {% if useOnlyEnvVars %}disabled{% endif %}>
+                 {% if useOnlyEnvVars %}readonly{% endif %}>
           <p class="help-block">
             <small>
               {{ t("security_setting.SAML.mapping_detail", t("Email")) }}
@@ -204,7 +216,7 @@
                  type="text"
                  name="settingForm[security:passport-saml:attrMapFirstName]"
                  value="{{ getConfigFromDB('crowi', 'security:passport-saml:attrMapFirstName') || '' }}"
-                 {% if useOnlyEnvVars %}disabled{% endif %}>
+                 {% if useOnlyEnvVars %}readonly{% endif %}>
           <p class="help-block">
             <small>
               {{ t("security_setting.SAML.mapping_detail", t("security_setting.SAML.First Name")) }}
@@ -231,7 +243,7 @@
                  type="text"
                  name="settingForm[security:passport-saml:attrMapLastName]"
                  value="{{　getConfigFromDB('crowi', 'security:passport-saml:attrMapLastName') || '' }}"
-                 {% if useOnlyEnvVars %}disabled{% endif %}>
+                 {% if useOnlyEnvVars %}readonly{% endif %}>
           <p class="help-block">
             <small>
               {{ t("security_setting.SAML.mapping_detail", t("security_setting.SAML.Last Name")) }}
@@ -315,7 +327,7 @@
                       type="text"
                       rows="5"
                       name="settingForm[security:passport-saml:cert]"
-                      {% if useOnlyEnvVars %}disabled{% endif %}
+                      {% if useOnlyEnvVars %}readonly{% endif %}
             >{{ getConfigFromDB('crowi', 'security:passport-saml:cert') || '' }}</textarea>
             <p class="help-block">
               <small>