Merge pull request #2508 from owncloud/proxy-policy-claim-precedence

allow overriding the cookie based route by claim
owncloud · Sep 15, 2021 · c53b5bf · c53b5bf
2 parents b0f0d6a + b2d53b9
commit c53b5bf
Show file tree

Hide file tree

Showing 4 changed files with 47 additions and 40 deletions.
diff --git a/changelog/unreleased/proxy-policy-claim-precedence.md 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
diff --git 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
@@ -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
 	}
 }

diff --git 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 {