Fix: fix bug that group path variable value would be fixed by first u…

…ser resource request group variable is a pointer that outside of closure, referenced it in the request filter will caused fixed by fist request Signed-off-by: jtcheng <[email protected]>
katanomi · Jul 28, 2023 · 0ffb60f · 0ffb60f
1 parent 9d5982a
commit 0ffb60f
Show file tree

Hide file tree

Showing 2 changed files with 54 additions and 7 deletions.
diff --git a/user/filter_test.go b/user/filter_test.go
@@ -201,4 +201,47 @@ func TestUserOwnedResourcePermissionFilter(t *testing.T) {
 		})
 	}
 
+	t.Run("gvr is nil, and gvr should set by request path in multi request with same filter", func(t *testing.T) {
+		g := gomega.NewWithT(t)
+
+		ctx = kclient.WithUser(ctx, &user.DefaultInfo{Name: "jackson"})
+		filter := UserOwnedResourcePermissionFilter(ctx, nil)
+
+		request, response, recorder := mockRequestAndResponse(ctx, map[string]string{
+			"group":     "batch-fake",
+			"version":   "v1",
+			"resource":  "jobs",
+			"name":      "job1",
+			"namespace": "default",
+		})
+		filter(request, response, chain)
+		g.Expect(recorder.Code).ShouldNot(gomega.Equal(200))
+
+		// refresh path params for assert groupVersionResource should not be fixed in closure
+		request, response, recorder = mockRequestAndResponse(ctx, map[string]string{
+			"group":     "batch",
+			"version":   "v1",
+			"resource":  "jobs",
+			"name":      "job1",
+			"namespace": "default",
+		})
+		filter(request, response, chain)
+		g.Expect(recorder.Code).Should(gomega.Equal(200))
+	})
+}
+
+func mockRequestAndResponse(ctx context.Context, pathParams map[string]string) (
+	*restful.Request, *restful.Response, *httptest.ResponseRecorder) {
+	_req := httptest.NewRequest("GET", "http://localhost", nil)
+	request := restful.NewRequest(_req)
+
+	for key, value := range pathParams {
+		request.PathParameters()[key] = value
+	}
+
+	request.Request = request.Request.WithContext(ctx)
+	recorder := httptest.NewRecorder()
+	response := restful.NewResponse(recorder)
+
+	return request, response, recorder
 }
diff --git a/user/filters.go b/user/filters.go
@@ -54,7 +54,7 @@ func UserInfoFilter(req *restful.Request, res *restful.Response, chain *restful.
 // the user owned resource could use annotation to annotated owner name
 // and the filter will check if current user could execcute the `verb` for the resource
 // more doc about it please see Spec: user owned resource permission check
-func UserOwnedResourcePermissionFilter(appCtx context.Context, gvr *schema.GroupVersionResource) restful.FilterFunction {
+func UserOwnedResourcePermissionFilter(appCtx context.Context, groupVersionResource *schema.GroupVersionResource) restful.FilterFunction {
 	appClient := kclient.Client(appCtx)
 	restMapper := appClient.RESTMapper()
 
@@ -63,13 +63,17 @@ func UserOwnedResourcePermissionFilter(appCtx context.Context, gvr *schema.Group
 		log := logging.FromContext(ctxInReq).Named("user-owned-resource")
 		logging.WithLogger(ctxInReq, log)
 
-		if gvr == nil {
-			gvr = &schema.GroupVersionResource{
+		gvr := schema.GroupVersionResource{}
+		if groupVersionResource == nil {
+			gvr = schema.GroupVersionResource{
 				Group:    req.PathParameter("group"),
 				Version:  req.PathParameter("version"),
 				Resource: req.PathParameter("resource"),
 			}
+		} else {
+			gvr = *groupVersionResource
 		}
+
 		log = log.With("gvr", gvr)
 
 		// check rbac for current user
@@ -122,14 +126,14 @@ func verbFromReq(req *restful.Request) string {
 }
 
 func resourecRBACAllowed(appCtx context.Context, req *restful.Request,
-	gvr *schema.GroupVersionResource, restMapper meta.RESTMapper) (*authv1.SubjectAccessReviewStatus, string, *unstructured.Unstructured, error) {
+	gvr schema.GroupVersionResource, restMapper meta.RESTMapper) (*authv1.SubjectAccessReviewStatus, string, *unstructured.Unstructured, error) {
 	log := logging.FromContext(req.Request.Context())
 	ctxInReq := req.Request.Context()
 
 	verb := verbFromReq(req)
-	log = log.With("gvr", *gvr).With("verb", verb)
+	log = log.With("gvr", gvr).With("verb", verb)
 
-	gvk, err := restMapper.KindFor(*gvr)
+	gvk, err := restMapper.KindFor(gvr)
 	if err != nil {
 		log.Errorw("get kind from groupversionresource error", "err", err)
 		return nil, "", nil, err
@@ -143,7 +147,7 @@ func resourecRBACAllowed(appCtx context.Context, req *restful.Request,
 	req.Request = req.Request.WithContext(WithEntity(ctxInReq, obj))
 
 	clientInReq := kclient.Client(ctxInReq)
-	status, err := resourceRBACCheck(ctxInReq, clientInReq, verb, *gvr, obj.GetNamespace(), obj.GetName())
+	status, err := resourceRBACCheck(ctxInReq, clientInReq, verb, gvr, obj.GetNamespace(), obj.GetName())
 	if err != nil {
 		log.Errorw("resource permission check error", "namespace", obj.GetNamespace(), "name", obj.GetName(), "err", err)
 		return nil, "", nil, err