Repository: owncloud/ocis
Pull request #2508 (2021-09-15 09:57:05)

allow overriding the cookie based route by claim
When determining the routing policy we now let the claim override the cookie so that users are routed to the correct backend after login.

needed for https://github.com/owncloud/ocis/pull/2302
plays together with https://github.com/owncloud/openidconnect/pull/18


Patch:
diff --git a/changelog/unreleased/proxy-policy-claim-precedence.md b/changelog/unreleased/proxy-policy-claim-precedence.md
new file mode 100644
index 00000000000..6c0317125cb
--- /dev/null
+++ b/changelog/unreleased/proxy-policy-claim-precedence.md
@@ -0,0 +1,5 @@
+Enhancement: allow overriding the cookie based route by claim
+
+When determining the routing policy we now let the claim override the cookie so that users are routed to the correct backend after login.
+
+https://github.com/owncloud/ocis/pull/2508
\ No newline at end of file
diff --git a/proxy/pkg/middleware/selector_cookie.go b/proxy/pkg/middleware/selector_cookie.go
index 04cf8cfbc38..d76054302bb 100644
--- a/proxy/pkg/middleware/selector_cookie.go
+++ b/proxy/pkg/middleware/selector_cookie.go
@@ -44,31 +44,25 @@ func (m selectorCookie) ServeHTTP(w http.ResponseWriter, req *http.Request) {
 		selectorCookieName = m.policySelector.Claims.SelectorCookieName
 	}
 
-	_, err := req.Cookie(selectorCookieName)
-	if err != nil {
-		// no cookie there - try to add one
-		if oidc.FromContext(req.Context()) != nil {
+	// update cookie
+	if oidc.FromContext(req.Context()) != nil {
 
-			selectorFunc, err := policy.LoadSelector(&m.policySelector)
-			if err != nil {
-				m.logger.Err(err)
-			}
+		selectorFunc, err := policy.LoadSelector(&m.policySelector)
+		if err != nil {
+			m.logger.Err(err)
+		}
 
-			selector, err := selectorFunc(req)
-			if err != nil {
-				m.logger.Err(err)
-			}
+		selector, err := selectorFunc(req)
+		if err != nil {
+			m.logger.Err(err)
+		}
 
-			cookie := http.Cookie{
-				Name:     selectorCookieName,
-				Value:    selector,
-				Domain:   req.Host,
-				Path:     "/",
-				MaxAge:   60 * 60,
-				HttpOnly: true,
-			}
-			http.SetCookie(w, &cookie)
+		cookie := http.Cookie{
+			Name:  selectorCookieName,
+			Value: selector,
+			Path:  "/",
 		}
+		http.SetCookie(w, &cookie)
 	}
 
 	m.next.ServeHTTP(w, req)
diff --git a/proxy/pkg/proxy/policy/selector.go b/proxy/pkg/proxy/policy/selector.go
index 9a9ac5aafa0..c877eb083b1 100644
--- a/proxy/pkg/proxy/policy/selector.go
+++ b/proxy/pkg/proxy/policy/selector.go
@@ -164,13 +164,8 @@ func NewMigrationSelector(cfg *config.MigrationSelectorConf, ss accounts.Account
 // This selector can be used in migration-scenarios where some users have already migrated from ownCloud10 to OCIS and
 func NewClaimsSelector(cfg *config.ClaimsSelectorConf) Selector {
 	return func(r *http.Request) (s string, err error) {
-		// use cookie first if provided
-		selectorCookie, err := r.Cookie(cfg.SelectorCookieName)
-		if err == nil {
-			return selectorCookie.Value, nil
-		}
 
-		// if no cookie is present, try to route by selector
+		// first, try to route by selector
 		if claims := oidc.FromContext(r.Context()); claims != nil {
 			if p, ok := claims[oidc.OcisRoutingPolicy].(string); ok && p != "" {
 				// TODO check we know the routing policy?
@@ -179,6 +174,12 @@ func NewClaimsSelector(cfg *config.ClaimsSelectorConf) Selector {
 			return cfg.DefaultPolicy, nil
 		}
 
+		// use cookie if provided
+		selectorCookie, err := r.Cookie(cfg.SelectorCookieName)
+		if err == nil {
+			return selectorCookie.Value, nil
+		}
+
 		return cfg.UnauthenticatedPolicy, nil
 	}
 }


Test patch:
diff --git a/proxy/pkg/proxy/policy/selector_test.go b/proxy/pkg/proxy/policy/selector_test.go
index 96da63e695f..101eaae73e6 100644
--- a/proxy/pkg/proxy/policy/selector_test.go
+++ b/proxy/pkg/proxy/policy/selector_test.go
@@ -3,6 +3,7 @@ package policy
 import (
 	"context"
 	"fmt"
+	"net/http"
 	"net/http/httptest"
 	"testing"
 
@@ -129,6 +130,7 @@ func mockAccSvc(retErr bool) proto.AccountsService {
 type testCase struct {
 	Name     string
 	Context  context.Context
+	Cookie   *http.Cookie
 	Expected string
 }
 
@@ -139,12 +141,17 @@ func TestClaimsSelector(t *testing.T) {
 	})
 
 	var tests = []testCase{
-		{"unatuhenticated", context.Background(), "unauthenticated"},
-		{"default", oidc.NewContext(context.Background(), map[string]interface{}{oidc.OcisRoutingPolicy: ""}), "default"},
-		{"claim-value", oidc.NewContext(context.Background(), map[string]interface{}{oidc.OcisRoutingPolicy: "ocis.routing.policy-value"}), "ocis.routing.policy-value"},
+		{"unatuhenticated", context.Background(), nil, "unauthenticated"},
+		{"default", oidc.NewContext(context.Background(), map[string]interface{}{oidc.OcisRoutingPolicy: ""}), nil, "default"},
+		{"claim-value", oidc.NewContext(context.Background(), map[string]interface{}{oidc.OcisRoutingPolicy: "ocis.routing.policy-value"}), nil, "ocis.routing.policy-value"},
+		{"cookie-only", context.Background(), &http.Cookie{Name: SelectorCookieName, Value: "cookie"}, "cookie"},
+		{"claim-can-override-cookie", oidc.NewContext(context.Background(), map[string]interface{}{oidc.OcisRoutingPolicy: "ocis.routing.policy-value"}), &http.Cookie{Name: SelectorCookieName, Value: "cookie"}, "ocis.routing.policy-value"},
 	}
 	for _, tc := range tests {
 		r := httptest.NewRequest("GET", "https://example.com", nil)
+		if tc.Cookie != nil {
+			r.AddCookie(tc.Cookie)
+		}
 		nr := r.WithContext(tc.Context)
 		got, err := sel(nr)
 		if err != nil {
@@ -172,16 +179,16 @@ func TestRegexSelector(t *testing.T) {
 	})
 
 	var tests = []testCase{
-		{"unauthenticated", context.Background(), "unauthenticated"},
-		{"default", revactx.ContextSetUser(context.Background(), &userv1beta1.User{}), "default"},
-		{"mail-ocis", revactx.ContextSetUser(context.Background(), &userv1beta1.User{Mail: "marie@example.org"}), "ocis"},
-		{"mail-oc10", revactx.ContextSetUser(context.Background(), &userv1beta1.User{Mail: "einstein@example.org"}), "oc10"},
-		{"username-einstein", revactx.ContextSetUser(context.Background(), &userv1beta1.User{Username: "einstein"}), "ocis"},
-		{"username-feynman", revactx.ContextSetUser(context.Background(), &userv1beta1.User{Username: "feynman"}), "ocis"},
-		{"username-marie", revactx.ContextSetUser(context.Background(), &userv1beta1.User{Username: "marie"}), "oc10"},
-		{"id-nil", revactx.ContextSetUser(context.Background(), &userv1beta1.User{Id: &userv1beta1.UserId{}}), "default"},
-		{"id-1", revactx.ContextSetUser(context.Background(), &userv1beta1.User{Id: &userv1beta1.UserId{OpaqueId: "4c510ada-c86b-4815-8820-42cdf82c3d51"}}), "ocis"},
-		{"id-2", revactx.ContextSetUser(context.Background(), &userv1beta1.User{Id: &userv1beta1.UserId{OpaqueId: "f7fbf8c8-139b-4376-b307-cf0a8c2d0d9c"}}), "oc10"},
+		{"unauthenticated", context.Background(), nil, "unauthenticated"},
+		{"default", revactx.ContextSetUser(context.Background(), &userv1beta1.User{}), nil, "default"},
+		{"mail-ocis", revactx.ContextSetUser(context.Background(), &userv1beta1.User{Mail: "marie@example.org"}), nil, "ocis"},
+		{"mail-oc10", revactx.ContextSetUser(context.Background(), &userv1beta1.User{Mail: "einstein@example.org"}), nil, "oc10"},
+		{"username-einstein", revactx.ContextSetUser(context.Background(), &userv1beta1.User{Username: "einstein"}), nil, "ocis"},
+		{"username-feynman", revactx.ContextSetUser(context.Background(), &userv1beta1.User{Username: "feynman"}), nil, "ocis"},
+		{"username-marie", revactx.ContextSetUser(context.Background(), &userv1beta1.User{Username: "marie"}), nil, "oc10"},
+		{"id-nil", revactx.ContextSetUser(context.Background(), &userv1beta1.User{Id: &userv1beta1.UserId{}}), nil, "default"},
+		{"id-1", revactx.ContextSetUser(context.Background(), &userv1beta1.User{Id: &userv1beta1.UserId{OpaqueId: "4c510ada-c86b-4815-8820-42cdf82c3d51"}}), nil, "ocis"},
+		{"id-2", revactx.ContextSetUser(context.Background(), &userv1beta1.User{Id: &userv1beta1.UserId{OpaqueId: "f7fbf8c8-139b-4376-b307-cf0a8c2d0d9c"}}), nil, "oc10"},
 	}
 
 	for _, tc := range tests {