Merge pull request #2690 from openziti/fix.2689.fix.form.id.body.vs.q…

…uery.string fixes #2689 form data doesn't respect query string authRequestID
openziti · Jan 24, 2025 · 2b9ac8a · 2b9ac8a
2 parents e80a109 + cd26cbf
commit 2b9ac8a
Show file tree

Hide file tree

Showing 4 changed files with 81 additions and 9 deletions.
diff --git a/controller/oidc_auth/login.go b/controller/oidc_auth/login.go
@@ -22,7 +22,8 @@ import (
 )
 
 const (
-	queryAuthRequestID = "authRequestID"
+	queryAuthRequestID    = "authRequestID"
+	queryAuthRequestIdAlt = "id"
 
 	//page specific resource keys
 	pageLogin = "login"

diff --git a/controller/oidc_auth/parse.go b/controller/oidc_auth/parse.go
@@ -192,8 +192,6 @@ func parsePayload(r *http.Request, out AuthRequestIdHolder) error {
 			return err
 		}
 
-		return nil
-
 	} else if contentType == JsonContentType {
 		body, err := io.ReadAll(r.Body)
 
@@ -216,8 +214,13 @@ func parsePayload(r *http.Request, out AuthRequestIdHolder) error {
 		}
 	}
 
+	//prefer body, if not set use > query string queryAuthRequestID > query string queryAuthRequestIdAlt
 	if out.GetAuthRequestId() == "" {
-		out.SetAuthRequestId(r.URL.Query().Get("id"))
+		if queryAuthRequestId := r.URL.Query().Get(queryAuthRequestID); queryAuthRequestId != "" {
+			out.SetAuthRequestId(queryAuthRequestId)
+		} else if queryId := r.URL.Query().Get(queryAuthRequestIdAlt); queryId != "" {
+			out.SetAuthRequestId(queryId)
+		}
 	}
 
 	return nil

diff --git a/go.sum b/go.sum
@@ -595,10 +595,6 @@ github.com/openziti/cobra-to-md v1.0.1 h1:WRinNoIRmwWUSJm+pSNXMjOrtU48oxXDZgeCYQ
 github.com/openziti/cobra-to-md v1.0.1/go.mod h1:FjCpk/yzHF7/r28oSTNr5P57yN5VolpdAtS/g7KNi2c=
 github.com/openziti/dilithium v0.3.5 h1:+envGNzxc3OyVPiuvtxivQmCsOjdZjtOMLpQBeMz7eM=
 github.com/openziti/dilithium v0.3.5/go.mod h1:XONq1iK6te/WwNzkgZHfIDHordMPqb0hMwJ8bs9EfSk=
-github.com/openziti/edge-api v0.26.36 h1:zy2DjmIz/B+WxPpIzhFOAxi/LhM/yeKa8s1Vz2h8cQk=
-github.com/openziti/edge-api v0.26.36/go.mod h1:sYHVpm26Jr1u7VooNJzTb2b2nGSlmCHMnbGC8XfWSng=
-github.com/openziti/edge-api v0.26.37 h1:MQGDQusrya6U5IEW/t6x6IRzBqzBVFF00YNcuPMNtnY=
-github.com/openziti/edge-api v0.26.37/go.mod h1:sYHVpm26Jr1u7VooNJzTb2b2nGSlmCHMnbGC8XfWSng=
 github.com/openziti/edge-api v0.26.38 h1:3xDWC5SFn3qUVR428TIBpRc2lrjVV7Gz0Rx4pQx0JSg=
 github.com/openziti/edge-api v0.26.38/go.mod h1:sYHVpm26Jr1u7VooNJzTb2b2nGSlmCHMnbGC8XfWSng=
 github.com/openziti/foundation/v2 v2.0.56 h1:YXqBmkrN0fYr3TqIlWZSZGluE2QpJxlA29Z6okZyQ5I=

diff --git a/tests/auth_oidc_test.go b/tests/auth_oidc_test.go
@@ -165,7 +165,7 @@ func Test_Authenticate_OIDC_Auth(t *testing.T) {
 		ctx.Req.Equal(http.StatusUnsupportedMediaType, resp.StatusCode())
 	})
 
-	t.Run("updb", func(t *testing.T) {
+	t.Run("updb with auth request id in body", func(t *testing.T) {
 		ctx.testContextChanged(t)
 
 		client := resty.NewWithClient(ctx.NewHttpClient(ctx.NewTransport()))
@@ -201,6 +201,78 @@ func Test_Authenticate_OIDC_Auth(t *testing.T) {
 		ctx.Req.NotEmpty(outTokens.RefreshToken)
 	})
 
+	t.Run("updb with auth request id in query string", func(t *testing.T) {
+		ctx.testContextChanged(t)
+
+		client := resty.NewWithClient(ctx.NewHttpClient(ctx.NewTransport()))
+		client.SetRedirectPolicy(resty.DomainCheckRedirectPolicy("127.0.0.1", "localhost"))
+		resp, err := client.R().Get(rpServer.LoginUri)
+
+		ctx.Req.NoError(err)
+		ctx.Req.Equal(http.StatusOK, resp.StatusCode())
+
+		authRequestId := resp.Header().Get(oidc_auth.AuthRequestIdHeader)
+		ctx.Req.NotEmpty(authRequestId)
+
+		opLoginUri := "https://" + resp.RawResponse.Request.URL.Host + "/oidc/login/username?authRequestID=" + authRequestId
+
+		resp, err = client.R().SetFormData(map[string]string{"username": ctx.AdminAuthenticator.Username, "password": ctx.AdminAuthenticator.Password}).Post(opLoginUri)
+
+		ctx.Req.NoError(err)
+		ctx.Req.Equal(http.StatusOK, resp.StatusCode())
+
+		var outTokens *oidc.Tokens[*oidc.IDTokenClaims]
+
+		select {
+		case tokens := <-rpServer.TokenChan:
+			outTokens = tokens
+		case <-time.After(5 * time.Second):
+			ctx.Fail("no tokens received, hit timeout")
+		}
+
+		ctx.Req.NotNil(outTokens)
+		ctx.Req.NotEmpty(outTokens.IDToken)
+		ctx.Req.NotEmpty(outTokens.IDTokenClaims)
+		ctx.Req.NotEmpty(outTokens.AccessToken)
+		ctx.Req.NotEmpty(outTokens.RefreshToken)
+	})
+
+	t.Run("updb with id in query string", func(t *testing.T) {
+		ctx.testContextChanged(t)
+
+		client := resty.NewWithClient(ctx.NewHttpClient(ctx.NewTransport()))
+		client.SetRedirectPolicy(resty.DomainCheckRedirectPolicy("127.0.0.1", "localhost"))
+		resp, err := client.R().Get(rpServer.LoginUri)
+
+		ctx.Req.NoError(err)
+		ctx.Req.Equal(http.StatusOK, resp.StatusCode())
+
+		authRequestId := resp.Header().Get(oidc_auth.AuthRequestIdHeader)
+		ctx.Req.NotEmpty(authRequestId)
+
+		opLoginUri := "https://" + resp.RawResponse.Request.URL.Host + "/oidc/login/username?id=" + authRequestId
+
+		resp, err = client.R().SetFormData(map[string]string{"username": ctx.AdminAuthenticator.Username, "password": ctx.AdminAuthenticator.Password}).Post(opLoginUri)
+
+		ctx.Req.NoError(err)
+		ctx.Req.Equal(http.StatusOK, resp.StatusCode())
+
+		var outTokens *oidc.Tokens[*oidc.IDTokenClaims]
+
+		select {
+		case tokens := <-rpServer.TokenChan:
+			outTokens = tokens
+		case <-time.After(5 * time.Second):
+			ctx.Fail("no tokens received, hit timeout")
+		}
+
+		ctx.Req.NotNil(outTokens)
+		ctx.Req.NotEmpty(outTokens.IDToken)
+		ctx.Req.NotEmpty(outTokens.IDTokenClaims)
+		ctx.Req.NotEmpty(outTokens.AccessToken)
+		ctx.Req.NotEmpty(outTokens.RefreshToken)
+	})
+
 	t.Run("cert", func(t *testing.T) {
 		ctx.testContextChanged(t)