Repository: avenga/couper
Pull request #593 (2022-09-14 13:06:56)

Empty scope
* handle empty `scope` properly
* enhance tests for `oauth2_authorization_url()` function

---
<details>
    <summary>Reviewer checklist</summary>
    <ul>
        <li>Read PR description: a summary about the changes is required</li>
        <li>Changelog updated</li>
        <li>Documentation: docs/{Reference, Cli, ...}, Docker and cli help/usage</li>
        <li>Pulled branch, manually tested</li>
        <li>Verified requirements are met</li>
        <li>Reviewed the code</li>
        <li>Reviewed the related tests</li>
    </ul>
</details>



Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index c8ce5d86..86586352 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -5,7 +5,8 @@
 Unreleased changes are available as `avenga/couper:edge` container.
 
 * **Fixed**
-  * Aligned the evaluation of `beta_oauth2`/`oidc` `redirect_uri` to `saml` `sp_acs_url` ([#589](https://github.com/avenga/couper/pull/589))
+  * Aligned the evaluation of [`beta_oauth2`](https://docs.couper.io/configuration/block/oauth2_ac)/[`oidc`](https://docs.couper.io/configuration/block/oidc) `redirect_uri` to `saml` `sp_acs_url` ([#589](https://github.com/avenga/couper/pull/589))
+  * Proper handling of empty [`beta_oauth2`](https://docs.couper.io/configuration/block/oauth2_ac)/[`oidc`](https://docs.couper.io/configuration/block/oidc) `scope` ([#593](https://github.com/avenga/couper/pull/593))
 
 ---
 
diff --git a/config/ac_oauth2.go b/config/ac_oauth2.go
index 3512ef7e..4ea5f21d 100644
--- a/config/ac_oauth2.go
+++ b/config/ac_oauth2.go
@@ -1,6 +1,8 @@
 package config
 
 import (
+	"strings"
+
 	"github.com/hashicorp/hcl/v2"
 	"github.com/hashicorp/hcl/v2/gohcl"
 
@@ -28,7 +30,7 @@ type OAuth2AC struct {
 	Name                    string   `hcl:"name,label"`
 	RedirectURI             string   `hcl:"redirect_uri" docs:"The Couper endpoint for receiving the authorization code. Relative URL references are resolved against the origin of the current request URL. The origin can be changed with the {accept_forwarded_url}([settings](settings)) attribute if Couper is running behind a proxy."`
 	Remain                  hcl.Body `hcl:",remain"`
-	Scope                   *string  `hcl:"scope,optional" docs:"A space separated list of requested scope values for the access token."`
+	Scope                   string   `hcl:"scope,optional" docs:"A space separated list of requested scope values for the access token."`
 	TokenEndpoint           string   `hcl:"token_endpoint" docs:"The authorization server endpoint URL used for requesting the token."`
 	TokenEndpointAuthMethod *string  `hcl:"token_endpoint_auth_method,optional" default:"client_secret_basic" docs:"Defines the method to authenticate the client at the token endpoint. If set to {client_secret_post}, the client credentials are transported in the request body. If set to {client_secret_basic}, the client credentials are transported via Basic Authentication."`
 	VerifierMethod          string   `hcl:"verifier_method" docs:"The method to verify the integrity of the authorization code flow. Available values: {ccm_s256} ({code_challenge} parameter with {code_challenge_method} {S256}), {state} ({state} parameter)"`
@@ -101,10 +103,7 @@ func (oa *OAuth2AC) GetRedirectURI() string {
 }
 
 func (oa *OAuth2AC) GetScope() string {
-	if oa.Scope == nil {
-		return ""
-	}
-	return *oa.Scope
+	return strings.TrimSpace(oa.Scope)
 }
 
 func (oa *OAuth2AC) GetAuthorizationEndpoint() (string, error) {
diff --git a/config/ac_oidc.go b/config/ac_oidc.go
index 89aa7f53..bee78ad8 100644
--- a/config/ac_oidc.go
+++ b/config/ac_oidc.go
@@ -1,6 +1,8 @@
 package config
 
 import (
+	"strings"
+
 	"github.com/hashicorp/hcl/v2"
 	"github.com/hashicorp/hcl/v2/gohcl"
 
@@ -27,7 +29,7 @@ type OIDC struct {
 	Name                    string   `hcl:"name,label"`
 	Remain                  hcl.Body `hcl:",remain"`
 	RedirectURI             string   `hcl:"redirect_uri" docs:"The Couper endpoint for receiving the authorization code. Relative URL references are resolved against the origin of the current request URL. The origin can be changed with the {accept_forwarded_url}({settings} block) attribute if Couper is running behind a proxy."`
-	Scope                   *string  `hcl:"scope,optional" docs:"A space separated list of requested scope values for the access token."`
+	Scope                   string   `hcl:"scope,optional" docs:"A space separated list of requested scope values for the access token."`
 	TokenEndpointAuthMethod *string  `hcl:"token_endpoint_auth_method,optional" docs:"Defines the method to authenticate the client at the token endpoint. If set to {client_secret_post}, the client credentials are transported in the request body. If set to {client_secret_basic}, the client credentials are transported via Basic Authentication." default:"client_secret_basic"`
 	ConfigurationTTL        string   `hcl:"configuration_ttl,optional" docs:"The duration to cache the OpenID configuration located at {configuration_url}." type:"duration" default:"1h"`
 	ConfigurationMaxStale   string   `hcl:"configuration_max_stale,optional" docs:"Duration a cached OpenID configuration stays valid after its TTL has passed." type:"duration" default:"1h"`
@@ -126,10 +128,12 @@ func (o *OIDC) GetRedirectURI() string {
 }
 
 func (o *OIDC) GetScope() string {
-	if o.Scope == nil {
+	scope := strings.TrimSpace(o.Scope)
+	if scope == "" {
 		return "openid"
 	}
-	return "openid " + *o.Scope
+
+	return "openid " + scope
 }
 
 func (o *OIDC) GetTokenEndpointAuthMethod() *string {


Test patch:
diff --git a/eval/lib/oauth2_test.go b/eval/lib/oauth2_test.go
index 258e0c30..1d35e7be 100644
--- a/eval/lib/oauth2_test.go
+++ b/eval/lib/oauth2_test.go
@@ -46,15 +46,53 @@ func TestNewOAuthAuthorizationURLFunction(t *testing.T) {
 		_, err = rw.Write(b)
 		helper.Must(err)
 	}))
-	u := origin.URL + "/.well-known/openid-configuration"
-	backendConfig := ` server {}
+	confURL := origin.URL + "/.well-known/openid-configuration"
+	authURL := origin.URL + "/auth"
+	tokenURL := origin.URL + "/token"
+
+	log, _ := test.NewLogger()
+	logger := log.WithContext(context.Background())
+
+	type testCase struct {
+		name      string
+		config    string
+		wantRedir string
+		wantScope string
+		wantNonce bool
+		wantState bool
+		wantPKCE  bool
+	}
+
+	for _, tc := range []testCase{
+		{
+			"oidc",
+			`server {}
 definitions {
   oidc "auth-ref" {
- 	client_id = "test-id"
-	client_secret = "test-s3cr3t"
-    configuration_url = "` + u + `"
-	redirect_uri = split(" ", env.REDIR_URIS)[0]
-	verifier_value = "asdf"
+    client_id = "test-id"
+    client_secret = "test-s3cr3t"
+    configuration_url = "` + confURL + `"
+    redirect_uri = "/cb"
+    verifier_value = "asdf"
+  }
+}
+`,
+			"https://couper.io/cb",
+			"openid",
+			true,
+			false,
+			false,
+		},
+		{
+			"oidc, redir URI from env var and using function",
+			`server {}
+definitions {
+  oidc "auth-ref" {
+    client_id = "test-id"
+    client_secret = "test-s3cr3t"
+    configuration_url = "` + confURL + `"
+    redirect_uri = split(" ", env.REDIR_URIS)[0]
+    verifier_value = "asdf"
   }
 }
 defaults {
@@ -62,42 +100,322 @@ defaults {
     REDIR_URIS = "/cb /cb2"
   }
 }
-`
+`,
+			"https://couper.io/cb",
+			"openid",
+			true,
+			false,
+			false,
+		},
+		{
+			"oidc, absolute redir URI",
+			`server {}
+definitions {
+  oidc "auth-ref" {
+    client_id = "test-id"
+    client_secret = "test-s3cr3t"
+    configuration_url = "` + confURL + `"
+    redirect_uri = "https://example.com/cb"
+    verifier_value = "asdf"
+  }
+}
+`,
+			"https://example.com/cb",
+			"openid",
+			true,
+			false,
+			false,
+		},
+		{
+			"oidc, empty scope",
+			`server {}
+definitions {
+  oidc "auth-ref" {
+    client_id = "test-id"
+    client_secret = "test-s3cr3t"
+    configuration_url = "` + confURL + `"
+    redirect_uri = "/cb"
+    verifier_value = "asdf"
+    scope = ""
+  }
+}
+`,
+			"https://couper.io/cb",
+			"openid",
+			true,
+			false,
+			false,
+		},
+		{
+			"oidc, email scope",
+			`server {}
+definitions {
+  oidc "auth-ref" {
+    client_id = "test-id"
+    client_secret = "test-s3cr3t"
+    configuration_url = "` + confURL + `"
+    redirect_uri = "/cb"
+    verifier_value = "asdf"
+    scope = "email"
+  }
+}
+`,
+			"https://couper.io/cb",
+			"openid email",
+			true,
+			false,
+			false,
+		},
+		{
+			"oidc, verifier_method ccm_s256",
+			`server {}
+definitions {
+  oidc "auth-ref" {
+    client_id = "test-id"
+    client_secret = "test-s3cr3t"
+    configuration_url = "` + confURL + `"
+    redirect_uri = "/cb"
+    verifier_method = "ccm_s256"
+    verifier_value = "asdf"
+  }
+}
+`,
+			"https://couper.io/cb",
+			"openid",
+			false,
+			false,
+			true,
+		},
+		{
+			"oauth2 authorization code",
+			`server {}
+definitions {
+  beta_oauth2 "auth-ref" {
+    grant_type = "authorization_code"
+    client_id = "test-id"
+    client_secret = "test-s3cr3t"
+    authorization_endpoint = "` + authURL + `"
+    token_endpoint = "` + tokenURL + `"
+    redirect_uri = "/cb"
+    verifier_method = "state"
+    verifier_value = "asdf"
+  }
+}
+`,
+			"https://couper.io/cb",
+			"",
+			false,
+			true,
+			false,
+		},
+		{
+			"oauth2 authorization code, redir URI from env var",
+			`server {}
+definitions {
+  beta_oauth2 "auth-ref" {
+    grant_type = "authorization_code"
+    client_id = "test-id"
+    client_secret = "test-s3cr3t"
+    authorization_endpoint = "` + authURL + `"
+    token_endpoint = "` + tokenURL + `"
+    redirect_uri = env.REDIR_URI
+    verifier_method = "state"
+    verifier_value = "asdf"
+  }
+}
+defaults {
+  environment_variables = {
+    REDIR_URI = "/cb"
+  }
+}
+`,
+			"https://couper.io/cb",
+			"",
+			false,
+			true,
+			false,
+		},
+		{
+			"oauth2 authorization code, absolute redir URI",
+			`server {}
+definitions {
+  beta_oauth2 "auth-ref" {
+    grant_type = "authorization_code"
+    client_id = "test-id"
+    client_secret = "test-s3cr3t"
+    authorization_endpoint = "` + authURL + `"
+    token_endpoint = "` + tokenURL + `"
+    redirect_uri = "https://example.com/cb"
+    verifier_method = "state"
+    verifier_value = "asdf"
+  }
+}
+`,
+			"https://example.com/cb",
+			"",
+			false,
+			true,
+			false,
+		},
+		{
+			"oauth2 authorization code, empty scope",
+			`server {}
+definitions {
+  beta_oauth2 "auth-ref" {
+    grant_type = "authorization_code"
+    client_id = "test-id"
+    client_secret = "test-s3cr3t"
+    authorization_endpoint = "` + authURL + `"
+    token_endpoint = "` + tokenURL + `"
+    redirect_uri = "/cb"
+    verifier_method = "state"
+    verifier_value = "asdf"
+    scope = ""
+  }
+}
+`,
+			"https://couper.io/cb",
+			"",
+			false,
+			true,
+			false,
+		},
+		{
+			"oauth2 authorization code, 'foo bar' scope",
+			`server {}
+definitions {
+  beta_oauth2 "auth-ref" {
+    grant_type = "authorization_code"
+    client_id = "test-id"
+    client_secret = "test-s3cr3t"
+    authorization_endpoint = "` + authURL + `"
+    token_endpoint = "` + tokenURL + `"
+    redirect_uri = "/cb"
+    verifier_method = "state"
+    verifier_value = "asdf"
+    scope = "foo bar"
+  }
+}
+`,
+			"https://couper.io/cb",
+			"foo bar",
+			false,
+			true,
+			false,
+		},
+		{
+			"oauth2 authorization code, verifier_method ccm_s256",
+			`server {}
+definitions {
+  beta_oauth2 "auth-ref" {
+    grant_type = "authorization_code"
+    client_id = "test-id"
+    client_secret = "test-s3cr3t"
+    authorization_endpoint = "` + authURL + `"
+    token_endpoint = "` + tokenURL + `"
+    redirect_uri = "/cb"
+    verifier_method = "ccm_s256"
+    verifier_value = "asdf"
+  }
+}
+`,
+			"https://couper.io/cb",
+			"",
+			false,
+			false,
+			true,
+		},
+	} {
+		t.Run(tc.name, func(subT *testing.T) {
+			h := test.New(subT)
 
-	log, _ := test.NewLogger()
-	logger := log.WithContext(context.Background())
+			couperConf, err := configload.LoadBytes([]byte(tc.config), "test.hcl")
+			h.Must(err)
+
+			quitCh := make(chan struct{}, 1)
+			defer close(quitCh)
+			memStore := cache.New(logger, quitCh)
+
+			ctx, cancel := context.WithCancel(couperConf.Context)
+			couperConf.Context = ctx
+			defer cancel()
+
+			_, err = runtime.NewServerConfiguration(couperConf, logger, memStore)
+			helper.Must(err)
 
-	couperConf, err := configload.LoadBytes([]byte(backendConfig), "test.hcl")
-	helper.Must(err)
+			req, rerr := http.NewRequest(http.MethodGet, "https://couper.io/", nil)
+			helper.Must(rerr)
+			req = req.Clone(context.Background())
 
-	quitCh := make(chan struct{}, 1)
-	defer close(quitCh)
-	memStore := cache.New(logger, quitCh)
+			hclCtx := couperConf.Context.(*eval.Context).
+				WithClientRequest(req).
+				HCLContext()
 
-	ctx, cancel := context.WithCancel(couperConf.Context)
-	couperConf.Context = ctx
-	defer cancel()
+			val, furr := hclCtx.Functions[lib.FnOAuthAuthorizationURL].Call([]cty.Value{cty.StringVal("auth-ref")})
+			helper.Must(furr)
+
+			authURL := seetie.ValueToString(val)
+			authURLObj, perr := url.Parse(authURL)
+			helper.Must(perr)
+
+			query := authURLObj.Query()
+
+			if rt := query.Get("response_type"); rt != "code" {
+				subT.Errorf("response_type want: %v; got: %v", "code", rt)
+			}
 
-	_, err = runtime.NewServerConfiguration(couperConf, logger, memStore)
-	helper.Must(err)
+			if clID := query.Get("client_id"); clID != "test-id" {
+				subT.Errorf("client_id want: %v; got: %v", "test-id", clID)
+			}
 
-	req, rerr := http.NewRequest(http.MethodGet, "https://couper.io/", nil)
-	helper.Must(rerr)
-	req = req.Clone(context.Background())
+			if redir := query.Get("redirect_uri"); redir != tc.wantRedir {
+				subT.Errorf("redirect_uri want: %v; got: %v", tc.wantRedir, redir)
+			}
 
-	hclCtx := couperConf.Context.(*eval.Context).
-		WithClientRequest(req).
-		HCLContext()
+			if tc.wantScope == "" && query.Has("scope") {
+				subT.Error("scope not expected")
+			}
+			if scope := query.Get("scope"); scope != tc.wantScope {
+				subT.Errorf("scope want: %v; got: %v", tc.wantScope, scope)
+			}
 
-	val, furr := hclCtx.Functions[lib.FnOAuthAuthorizationURL].Call([]cty.Value{cty.StringVal("auth-ref")})
-	helper.Must(furr)
+			nonce := query.Get("nonce")
+			if tc.wantNonce {
+				if nonce == "" {
+					subT.Error("missing nonce")
+				}
+			} else {
+				if nonce != "" {
+					subT.Error("nonce not expected")
+				}
+			}
 
-	authURL := seetie.ValueToString(val)
-	authURLObj, perr := url.Parse(authURL)
-	helper.Must(perr)
+			state := query.Get("state")
+			if tc.wantState {
+				if state == "" {
+					subT.Error("missing state")
+				}
+			} else {
+				if state != "" {
+					subT.Error("state not expected")
+				}
+			}
 
-	want := "https://couper.io/cb"
-	if value := authURLObj.Query().Get("redirect_uri"); value != want {
-		t.Errorf("Want: %v; got: %v", want, value)
+			codeChallenge := query.Get("code_challenge")
+			codeChallengeMethod := query.Get("code_challenge_method")
+			if tc.wantPKCE {
+				if codeChallenge == "" {
+					subT.Error("missing code_challenge")
+				}
+				if codeChallengeMethod != "S256" {
+					subT.Errorf("code_challenge want: %v; got: %v\n", "S256", codeChallengeMethod)
+				}
+			} else {
+				if codeChallenge != "" {
+					subT.Error("code_challenge not expected")
+				}
+			}
+		})
 	}
+
 }