documentation, improvements, clean up form and validation

techknowlogick · Dec 7, 2023 · acd00f8 · acd00f8
1 parent 37322f9
commit acd00f8
Show file tree

Hide file tree

Showing 9 changed files with 24 additions and 37 deletions.
diff --git a/docs/content/usage/authentication.en-us.md b/docs/content/usage/authentication.en-us.md
@@ -375,22 +375,22 @@ Notice: Reverse Proxy Auth doesn't support the API. You still need an access tok
 - `Identity Provider Metadata URL` (optional if XML set)
 
   - The URL of the IdP metadata endpoint.
+  - This field must be set if `Identity Provider Metadata XML` is left blank.
 
 - `Identity Provider Metadata XML` (optional if URL set)
 
   - The XML returned by the IdP metadata endpoint.
+  - This field must be set if `Identity Provider Metadata URL` is left blank.
 
 - `Service Provider Certificate` (optional)
 
   - X.509-formatted certificate (with `Service Provider Private Key`) used for signing SAML requests.
+  - A certificate will be generated if this field is left blank.
 
 - `Service Provider Private Key` (optional)
 
   - DSA/RSA private key (with `Service Provider Certificate`) used for signing SAML requests.
-
-- `Sign SAML Requests` (optional)
-
-  - Sign requests made to the SAML IdP
+  - A private key will be generated if this field is left blank.
 
 - `Email Assertion Key` (optional)
 

diff --git a/options/locale/locale_en-US.ini b/options/locale/locale_en-US.ini
@@ -520,6 +520,9 @@ Content = Content
 SSPISeparatorReplacement = Separator
 SSPIDefaultLanguage = Default Language
 
+SAMLMetadata = Either SAML Identity Provider metadata URL or XML
+SAMLMetadataURL = SAML Identity Provider metadata URL is invalid
+
 require_error = ` cannot be empty.`
 alpha_dash_error = ` should contain only alphanumeric, dash ('-') and underscore ('_') characters.`
 alpha_dash_dot_error = ` should contain only alphanumeric, dash ('-'), underscore ('_') and dot ('.') characters.`
@@ -2995,11 +2998,11 @@ auths.saml_identity_provider_metadata = Identity Provider Metadata XML
 auths.saml_insecure_skip_assertion_signature_validation = [Insecure] Skip Assertion Signature Validation
 auths.saml_service_provider_certificate = Service Provider Certificate
 auths.saml_service_provider_private_key = Service Provider Private Key
-auths.saml_sign_requests = Sign SAML Requests
 auths.saml_identity_provider_email_assertion_key = Email Assertion Key
 auths.saml_identity_provider_name_assertion_key = Name Assertion Key
 auths.saml_identity_provider_username_assertion_key = Username Assertion Key
 auths.tips = Tips
+auths.tips.saml = Documentation can be found at https://docs.gitea.com/usage/authentication#saml
 auths.tips.oauth2.general = OAuth2 Authentication
 auths.tips.oauth2.general.tip = When registering a new OAuth2 authentication, the callback/redirect URL should be:
 auths.tip.oauth2_provider = OAuth2 Provider

diff --git a/routers/web/admin/auths.go b/routers/web/admin/auths.go
@@ -249,23 +249,18 @@ func parseSSPIConfig(ctx *context.Context, form forms.AuthenticationForm) (*sspi
 }
 
 func parseSAMLConfig(ctx *context.Context, form forms.AuthenticationForm) (*saml.Source, error) {
-	if util.IsEmptyString(form.ServiceProviderCertificate) {
-		ctx.Data["Err_SSPISeparatorReplacement"] = true
-		return nil, errors.New(ctx.Tr("form.require_error"))
-	}
-	if util.IsEmptyString(form.ServiceProviderPrivateKey) {
-		ctx.Data["Err_SSPISeparatorReplacement"] = true
-		return nil, errors.New(ctx.Tr("form.require_error"))
-	}
 	if util.IsEmptyString(form.IdentityProviderMetadata) && util.IsEmptyString(form.IdentityProviderMetadataURL) {
-		return nil, fmt.Errorf("Identity Provider Metadata needed (either raw XML or URL)")
+		return nil, errors.New(ctx.Tr("form.SAMLMetadata") + ctx.Tr("form.require_error"))
 	}
+
 	if !util.IsEmptyString(form.IdentityProviderMetadataURL) {
 		_, err := url.Parse(form.IdentityProviderMetadataURL)
 		if err != nil {
-			return nil, fmt.Errorf("Identity Provider Metadata URL is an invalid URL")
+			return nil, errors.New(ctx.Tr("form.SAMLMetadataURL"))
 		}
 	}
+
+	// check the integrity of the certificate and private key (autogenerated if these form fields are blank)
 	if !util.IsEmptyString(form.ServiceProviderCertificate) && !util.IsEmptyString(form.ServiceProviderPrivateKey) {
 		keyPair, err := tls.X509KeyPair([]byte(form.ServiceProviderCertificate), []byte(form.ServiceProviderPrivateKey))
 		if err != nil {
@@ -276,14 +271,14 @@ func parseSAMLConfig(ctx *context.Context, form forms.AuthenticationForm) (*saml
 			return nil, err
 		}
 	}
+
 	return &saml.Source{
 		IdentityProviderMetadata:                 form.IdentityProviderMetadata,
 		IdentityProviderMetadataURL:              form.IdentityProviderMetadataURL,
 		InsecureSkipAssertionSignatureValidation: form.InsecureSkipAssertionSignatureValidation,
 		NameIDFormat:                             saml.NameIDFormat(form.NameIDFormat),
 		ServiceProviderCertificate:               form.ServiceProviderCertificate,
 		ServiceProviderPrivateKey:                form.ServiceProviderPrivateKey,
-		SignRequests:                             form.SignRequests,
 		EmailAssertionKey:                        form.EmailAssertionKey,
 		NameAssertionKey:                         form.NameAssertionKey,
 		UsernameAssertionKey:                     form.UsernameAssertionKey,

diff --git a/services/auth/source/saml/source.go b/services/auth/source/saml/source.go
@@ -27,7 +27,7 @@ import (
 // /   _____/  /  _  \    /     \ |    |
 // \_____  \  /  /_\  \  /  \ /  \|    |
 // /        \/    |    \/    Y    \    |___
-///_______  /\____|__  /\____|__  /_______ \
+// /_______  /\____|__  /\____|__  /_______ \
 //        \/         \/         \/        \/
 
 // Source holds configuration for the SAML login source.
@@ -46,8 +46,6 @@ type Source struct {
 	ServiceProviderIssuer string
 	// ServiceProviderPrivateKey description: The SAML Service Provider private key in PKCS#8 encoding (begins with "-----BEGIN PRIVATE KEY-----"). This private key is used to sign AuthnRequests and LogoutRequests. It corresponds to the Service Provider's certificate (`serviceProviderCertificate`). To escape the value into a JSON string, you may want to use a tool like https://json-escape-text.now.sh.
 	ServiceProviderPrivateKey string
-	// SignRequests description: Sign AuthnRequests and LogoutRequests sent to the Identity Provider using the Service Provider's private key (`serviceProviderPrivateKey`). It defaults to true if the `serviceProviderPrivateKey` and `serviceProviderCertificate` are set, and false otherwise.
-	SignRequests bool
 
 	CallbackURL string
 
@@ -134,6 +132,7 @@ func (source *Source) initSAMLSp() error {
 		SkipSignatureValidation:     source.InsecureSkipAssertionSignatureValidation,
 		NameIdFormat:                source.NameIDFormat.String(),
 		IDPCertificateStore:         &certStore,
+		SignAuthnRequests:           true,
 		SPKeyStore:                  keyStore,
 		ServiceProviderIssuer:       setting.AppURL + "user/saml/" + url.PathEscape(source.authSource.Name) + "/metadata",
 	}

diff --git a/services/forms/auth_form.go b/services/forms/auth_form.go
@@ -91,7 +91,6 @@ type AuthenticationForm struct {
 	InsecureSkipAssertionSignatureValidation bool
 	ServiceProviderCertificate               string
 	ServiceProviderPrivateKey                string
-	SignRequests                             bool
 	EmailAssertionKey                        string
 	NameAssertionKey                         string
 	UsernameAssertionKey                     string

diff --git a/templates/admin/auth/edit.tmpl b/templates/admin/auth/edit.tmpl
@@ -409,13 +409,6 @@
 						<textarea rows=2 id="service_provider_private_key" name="service_provider_private_key">{{$cfg.ServiceProviderPrivateKey}}</textarea>
 					</div>
 
-					<div class="inline field">
-						<div class="ui checkbox">
-							<label><strong>{{ctx.Locale.Tr "admin.auths.saml_sign_requests"}}</strong></label>
-							<input name="sign_requests" type="checkbox" {{if $cfg.SignRequests}}checked{{end}}>
-						</div>
-					</div>
-
 					<div class="field">
 						<label for="email_assertion_key">{{ctx.Locale.Tr "admin.auths.saml_identity_provider_email_assertion_key"}}</label>
 						<input id="email_assertion_key" name="email_assertion_key" value="{{if not $cfg.EmailAssertionKey}}http://schemas.xmlsoap.org/ws/2005/05/identity/claims/emailaddress{{else}}{{$cfg.EmailAssertionKey}}{{end}}">
@@ -506,6 +499,9 @@
 			<h5>GMail Settings:</h5>
 			<p>Host: smtp.gmail.com, Port: 587, Enable TLS Encryption: true</p>
 
+			<h5>SAML Settings:</h5>
+			<p>{{ctx.Locale.Tr "admin.auths.tips.saml"}}</p>
+
 			<h5 class="oauth2">{{ctx.Locale.Tr "admin.auths.tips.oauth2.general"}}:</h5>
 			<p class="oauth2">{{ctx.Locale.Tr "admin.auths.tips.oauth2.general.tip"}} <b id="oauth2-callback-url"></b></p>
 		</div>

diff --git a/templates/admin/auth/new.tmpl b/templates/admin/auth/new.tmpl
@@ -88,6 +88,9 @@
 			<h5>GMail Settings:</h5>
 			<p>Host: smtp.gmail.com, Port: 587, Enable TLS Encryption: true</p>
 
+			<h5>SAML Settings:</h5>
+			<p>{{ctx.Locale.Tr "admin.auths.tips.saml"}}</p>
+
 			<h5 class="oauth2">{{ctx.Locale.Tr "admin.auths.tips.oauth2.general"}}:</h5>
 			<p class="oauth2">{{ctx.Locale.Tr "admin.auths.tips.oauth2.general.tip"}} <b id="oauth2-callback-url"></b></p>
 

diff --git a/templates/admin/auth/source/saml.tmpl b/templates/admin/auth/source/saml.tmpl
@@ -30,22 +30,15 @@
 		</div>
 	</div>
 
-	<div class=" field">
+	<div class="field">
 		<label for="service_provider_certificate">{{ctx.Locale.Tr "admin.auths.saml_service_provider_certificate"}}</label>
 		<textarea rows=2 id="service_provider_certificate" name="service_provider_certificate" value="{{.ServiceProviderCertificate}}"></textarea>
 	</div>
-	<div class=" field">
+	<div class="field">
 		<label for="service_provider_private_key">{{ctx.Locale.Tr "admin.auths.saml_service_provider_private_key"}}</label>
 		<textarea rows=2 id="service_provider_private_key" name="service_provider_private_key" value="{{.ServiceProviderPrivateKey}}"></textarea>
 	</div>
 
-	<div class="inline field">
-		<div class="ui checkbox">
-			<label><strong>{{ctx.Locale.Tr "admin.auths.saml_sign_requests"}}</strong></label>
-			<input name="sign_requests" type="checkbox" {{if .SignRequests}}checked{{end}}>
-		</div>
-	</div>
-
 	<div class="field">
 		<label for="email_assertion_key">{{ctx.Locale.Tr "admin.auths.saml_identity_provider_email_assertion_key"}}</label>
 		<input id="email_assertion_key" name="email_assertion_key" value="{{if not .EmailAssertionKey}}http://schemas.xmlsoap.org/ws/2005/05/identity/claims/emailaddress{{else}}{{.EmailAssertionKey}}{{end}}">

diff --git a/tests/integration/saml_test.go b/tests/integration/saml_test.go
@@ -52,7 +52,6 @@ func TestSAMLRegistration(t *testing.T) {
 			NameIDFormat:                             4,
 			ServiceProviderCertificate:               "",
 			ServiceProviderPrivateKey:                "",
-			SignRequests:                             false,
 			EmailAssertionKey:                        "http://schemas.xmlsoap.org/ws/2005/05/identity/claims/emailaddress",
 			NameAssertionKey:                         "http://schemas.xmlsoap.org/claims/CommonName",
 			UsernameAssertionKey:                     "http://schemas.xmlsoap.org/ws/2005/05/identity/claims/name",