From 938176be3e93c1fb4bb3818ba335a97c6393235c Mon Sep 17 00:00:00 2001
From: Scott Bishel <scott.bishel@mattermost.com>
Date: Mon, 22 Jun 2020 09:36:08 -0600
Subject: [PATCH] MM-24467- Use new ServiceProviderIdentifier in AuthnRequest
 (#14725)

* add ServiceProviderIdentifier to config

* Update config, add unit test

* fix unit test, update i18n

* add english translation for error

Co-authored-by: mattermod <mattermod@users.noreply.github.com>
---
 api4/user_test.go    |  1 +
 i18n/en.json         |  4 ++++
 model/config.go      | 13 +++++++++++++
 model/config_test.go | 23 +++++++++++++++++++++++
 4 files changed, 41 insertions(+)

diff --git a/api4/user_test.go b/api4/user_test.go
index 4c65ae8ce0..5862720f60 100644
--- a/api4/user_test.go
+++ b/api4/user_test.go
@@ -4599,6 +4599,7 @@ func TestLoginErrorMessage(t *testing.T) {
 		*cfg.SamlSettings.IdpUrl = "https://localhost/adfs/ls"
 		*cfg.SamlSettings.IdpDescriptorUrl = "https://localhost/adfs/services/trust"
 		*cfg.SamlSettings.IdpMetadataUrl = "https://localhost/adfs/metadata"
+		*cfg.SamlSettings.ServiceProviderIdentifier = "https://localhost/login/sso/saml"
 		*cfg.SamlSettings.AssertionConsumerServiceURL = "https://localhost/login/sso/saml"
 		*cfg.SamlSettings.IdpCertificateFile = app.SamlIdpCertificateName
 		*cfg.SamlSettings.PrivateKeyFile = app.SamlPrivateKeyName
diff --git a/i18n/en.json b/i18n/en.json
index f03837f0f1..cd90415d27 100644
--- a/i18n/en.json
+++ b/i18n/en.json
@@ -5350,6 +5350,10 @@
     "id": "model.config.is_valid.saml_signature_algorithm.app_error",
     "translation": "Invalid Signature Algorithm."
   },
+  {
+    "id": "model.config.is_valid.saml_spidentifier_attribute.app_error",
+    "translation": "Service Provider Identifier is required"
+  },
   {
     "id": "model.config.is_valid.saml_username_attribute.app_error",
     "translation": "Invalid Username attribute. Must be set."
diff --git a/model/config.go b/model/config.go
index f57cb8c0a7..4063ec35ca 100644
--- a/model/config.go
+++ b/model/config.go
@@ -2135,6 +2135,7 @@ type SamlSettings struct {
 	IdpUrl                      *string
 	IdpDescriptorUrl            *string
 	IdpMetadataUrl              *string
+	ServiceProviderIdentifier   *string
 	AssertionConsumerServiceURL *string
 
 	SignatureAlgorithm *string
@@ -2212,6 +2213,14 @@ func (s *SamlSettings) SetDefaults() {
 		s.IdpDescriptorUrl = NewString("")
 	}
 
+	if s.ServiceProviderIdentifier == nil {
+		if s.IdpDescriptorUrl != nil {
+			s.ServiceProviderIdentifier = NewString(*s.IdpDescriptorUrl)
+		} else {
+			s.ServiceProviderIdentifier = NewString("")
+		}
+	}
+
 	if s.IdpMetadataUrl == nil {
 		s.IdpMetadataUrl = NewString("")
 	}
@@ -3126,6 +3135,10 @@ func (s *SamlSettings) isValid() *AppError {
 			return NewAppError("Config.IsValid", "model.config.is_valid.saml_username_attribute.app_error", nil, "", http.StatusBadRequest)
 		}
 
+		if len(*s.ServiceProviderIdentifier) == 0 {
+			return NewAppError("Config.IsValid", "model.config.is_valid.saml_spidentifier_attribute.app_error", nil, "", http.StatusBadRequest)
+		}
+
 		if *s.Verify {
 			if len(*s.AssertionConsumerServiceURL) == 0 || !IsValidHttpUrl(*s.AssertionConsumerServiceURL) {
 				return NewAppError("Config.IsValid", "model.config.is_valid.saml_assertion_consumer_service_url.app_error", nil, "", http.StatusBadRequest)
diff --git a/model/config_test.go b/model/config_test.go
index 78a098dfd1..92bd32e46c 100644
--- a/model/config_test.go
+++ b/model/config_test.go
@@ -146,6 +146,7 @@ func TestConfigIsValidDefaultAlgorithms(t *testing.T) {
 	*c1.SamlSettings.IdpUrl = "http://test.url.com"
 	*c1.SamlSettings.IdpDescriptorUrl = "http://test.url.com"
 	*c1.SamlSettings.IdpCertificateFile = "certificatefile"
+	*c1.SamlSettings.ServiceProviderIdentifier = "http://test.url.com"
 	*c1.SamlSettings.EmailAttribute = "Email"
 	*c1.SamlSettings.UsernameAttribute = "Username"
 
@@ -153,6 +154,27 @@ func TestConfigIsValidDefaultAlgorithms(t *testing.T) {
 	require.Nil(t, err)
 }
 
+func TestConfigServiceProviderDefault(t *testing.T) {
+	c1 := &Config{
+		SamlSettings: *&SamlSettings{
+			Enable:             NewBool(true),
+			Verify:             NewBool(false),
+			Encrypt:            NewBool(false),
+			IdpUrl:             NewString("http://test.url.com"),
+			IdpDescriptorUrl:   NewString("http://test2.url.com"),
+			IdpCertificateFile: NewString("certificatefile"),
+			EmailAttribute:     NewString("Email"),
+			UsernameAttribute:  NewString("Username"),
+		},
+	}
+
+	c1.SetDefaults()
+	assert.Equal(t, *c1.SamlSettings.ServiceProviderIdentifier, *c1.SamlSettings.IdpDescriptorUrl)
+
+	err := c1.SamlSettings.isValid()
+	require.Nil(t, err)
+}
+
 func TestConfigIsValidFakeAlgorithm(t *testing.T) {
 	c1 := Config{}
 	c1.SetDefaults()
@@ -165,6 +187,7 @@ func TestConfigIsValidFakeAlgorithm(t *testing.T) {
 	*c1.SamlSettings.IdpDescriptorUrl = "http://test.url.com"
 	*c1.SamlSettings.IdpMetadataUrl = "http://test.url.com"
 	*c1.SamlSettings.IdpCertificateFile = "certificatefile"
+	*c1.SamlSettings.ServiceProviderIdentifier = "http://test.url.com"
 	*c1.SamlSettings.EmailAttribute = "Email"
 	*c1.SamlSettings.UsernameAttribute = "Username"