IAM Policy Mixin for HttpJson matches incorrect Mux Handler

[lqiu96]

Showcase test makes a call to getIamPolicy RPC: GET http://localhost:7469/v1beta1/users/5da0e7e4:getIamPolicy.

The userId should be users/5da0e7e4, but the error message (A user with name users/5da0e7e4:getIamPolicy not found.) suggests that the userId is matched as users/5da0e7e4:getIamPolicy

Local Gapic Showcase Server Error:

2023/06/22 11:32:55 Received GET request matching '/v1beta1/{name=users/*}': "/v1beta1/users/bbe489bd:getIamPolicy"
2023/06/22 11:32:55   urlPathParams (expect 1, have 1): map["name":"users/bbe489bd:getIamPolicy"]
2023/06/22 11:32:55   request: {
  "name": "users/bbe489bd:getIamPolicy"
}
2023/06/22 11:32:55 A user with name users/bbe489bd:getIamPolicy not found.

It looks to have matched with this handleFunc:

gapic-showcase/server/genrest/genrest.go

Line 56 in 082b527

router.HandleFunc("/v1beta1/{name:users/.+}", rest.HandleGetUser).Methods("GET")

instead of

gapic-showcase/server/genrest/genrest.go

Line 105 in 082b527

router.HandleFunc("/v1beta1/{resource:users/.+}:getIamPolicy", rest.HandleGetIamPolicy).Methods("GET")

Full Java Stacktrace:

com.google.api.gax.rpc.NotFoundException: Not Found

	at com.google.api.gax.rpc.ApiExceptionFactory.createException(ApiExceptionFactory.java:50)
	at com.google.api.gax.httpjson.HttpJsonApiExceptionFactory.createApiException(HttpJsonApiExceptionFactory.java:76)
	at com.google.api.gax.httpjson.HttpJsonApiExceptionFactory.create(HttpJsonApiExceptionFactory.java:54)
	at com.google.api.gax.httpjson.HttpJsonExceptionCallable$ExceptionTransformingFuture.onFailure(HttpJsonExceptionCallable.java:97)
	at com.google.api.core.ApiFutures$1.onFailure(ApiFutures.java:84)
	at com.google.common.util.concurrent.Futures$CallbackListener.run(Futures.java:1132)
	at com.google.common.util.concurrent.DirectExecutor.execute(DirectExecutor.java:31)
	at com.google.common.util.concurrent.AbstractFuture.executeListener(AbstractFuture.java:1270)
	at com.google.common.util.concurrent.AbstractFuture.complete(AbstractFuture.java:1038)
	at com.google.common.util.concurrent.AbstractFuture.setException(AbstractFuture.java:808)
	at com.google.api.core.AbstractApiFuture$InternalSettableFuture.setException(AbstractApiFuture.java:92)
	at com.google.api.core.AbstractApiFuture.setException(AbstractApiFuture.java:74)
	at com.google.api.gax.httpjson.HttpJsonClientCalls$HttpJsonFuture.setException(HttpJsonClientCalls.java:128)
	at com.google.api.gax.httpjson.HttpJsonClientCalls$FutureListener.onClose(HttpJsonClientCalls.java:162)
	at com.google.api.gax.httpjson.HttpJsonClientCallImpl$OnCloseNotificationTask.call(HttpJsonClientCallImpl.java:530)
	at com.google.api.gax.httpjson.HttpJsonClientCallImpl.notifyListeners(HttpJsonClientCallImpl.java:379)
	at com.google.api.gax.httpjson.HttpJsonClientCallImpl.deliver(HttpJsonClientCallImpl.java:306)
	at com.google.api.gax.httpjson.HttpJsonClientCallImpl.setResult(HttpJsonClientCallImpl.java:156)
	at com.google.api.gax.httpjson.HttpRequestRunnable.run(HttpRequestRunnable.java:149)
	at java.base/java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:515)
	at java.base/java.util.concurrent.FutureTask.run(FutureTask.java:264)
	at java.base/java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.run(ScheduledThreadPoolExecutor.java:304)
	at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1128)
	at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:628)
	at java.base/java.lang.Thread.run(Thread.java:829)
	Suppressed: com.google.api.gax.rpc.AsyncTaskException: Asynchronous task failed
		at com.google.api.gax.rpc.ApiExceptions.callAndTranslateApiException(ApiExceptions.java:57)
		at com.google.api.gax.rpc.UnaryCallable.call(UnaryCallable.java:112)
		at com.google.showcase.v1beta1.IdentityClient.getIamPolicy(IdentityClient.java:939)
		at com.google.showcase.v1beta1.it.ITIam.testHttpJson_getIamPolicy(ITIam.java:151)
		at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
		at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
		at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
		at java.base/java.lang.reflect.Method.invoke(Method.java:566)
		at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:59)
		at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
		at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:56)
		at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)
		at org.junit.internal.runners.statements.RunBefores.evaluate(RunBefores.java:26)
		at org.junit.runners.ParentRunner$3.evaluate(ParentRunner.java:306)
		at org.junit.runners.BlockJUnit4ClassRunner$1.evaluate(BlockJUnit4ClassRunner.java:100)
		at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:366)
		at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:103)
		at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:63)
		at org.junit.runners.ParentRunner$4.run(ParentRunner.java:331)
		at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:79)
		at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:329)
		at org.junit.runners.ParentRunner.access$100(ParentRunner.java:66)
		at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:293)
		at org.junit.internal.runners.statements.RunBefores.evaluate(RunBefores.java:26)
		at org.junit.internal.runners.statements.RunAfters.evaluate(RunAfters.java:27)
		at org.junit.runners.ParentRunner$3.evaluate(ParentRunner.java:306)
		at org.junit.runners.ParentRunner.run(ParentRunner.java:413)
		at org.junit.runner.JUnitCore.run(JUnitCore.java:137)
		at com.intellij.junit4.JUnit4IdeaTestRunner.startRunnerWithArgs(JUnit4IdeaTestRunner.java:69)
		at com.intellij.rt.junit.IdeaTestRunner$Repeater$1.execute(IdeaTestRunner.java:38)
		at com.intellij.rt.execution.junit.TestsRepeater.repeat(TestsRepeater.java:11)
		at com.intellij.rt.junit.IdeaTestRunner$Repeater.startRunnerWithArgs(IdeaTestRunner.java:35)
		at com.intellij.rt.junit.JUnitStarter.prepareStreamsAndStart(JUnitStarter.java:235)
		at com.intellij.rt.junit.JUnitStarter.main(JUnitStarter.java:54)
Caused by: com.google.api.client.http.HttpResponseException: 404 Not Found
GET http://localhost:7469/v1beta1/users/5da0e7e4:getIamPolicy
{"error":{"code":404,"message":"A user with name users/5da0e7e4:getIamPolicy not found.","details":[],"Body":"","Header":null,"Errors":null}}
	at com.google.api.client.http.HttpResponseException$Builder.build(HttpResponseException.java:293)
	at com.google.api.client.http.HttpRequest.execute(HttpRequest.java:1118)
	at com.google.api.gax.httpjson.HttpRequestRunnable.run(HttpRequestRunnable.java:115)
	... 6 more

Possible Fixes:

1. handleFunc RegExps match exact cases
   I'm not sure this is something Mux supports. Online references seem to suggest fixing the ordering of the handleFuncs.

2. Move the Mixin handleFuncs to the top
   Adding something like:

var mixinNames = []string{"ListLocations", "GetLocation", "SetIamPolicy", "GetIamPolicy", "TestIamPermissions", "ListOperations", "GetOperation", "DeleteOperation", "CancelOperation", "WaitOperation"}
...
registeredMixins := []*registeredHandler{}
if isMixin(handler.GoMethod) {
registeredMixins = append(registeredMixins, &registeredHandler{pathMatch, handlerName, handler.HTTPMethod})
} else {
registered = append(registered, &registeredHandler{pathMatch, handlerName, handler.HTTPMethod})
}
...
// Print out the mixin handleFuncs first

Seems to work

3. Sort the handleFuncs pattern by longest to shortest
   Adding something like:

	sort.Slice(registered, func(i, j int) bool {
		return len(registered[i].pattern) > len(registered[j].pattern)
	})

solves the current issue, but may not cover every case.

[lqiu96]

@noahdietz Would you be fine with me changing the order of handleFuncs inside the router so that Mixins (or just IAM RPCs) occur first?

[noahdietz]

Yeah I think that's fine, but kind of papers over the actual issue which is we have greedy path matchers. This could happen again for a different case.

Specifically, "/v1beta1/{name:users/.+}" doesn't need to be so greedy by using .+. Would you be OK with first looking at an improved regex? Something to only match alphanumerics & dashes/underscores? Or something that stops matching when it sees a / or :?

Worst case, we go with the ordering-based fix.

Great analysis btw, thanks.

[lqiu96]

Ah sure, good idea!

I think I can make the changes to how it set the values in the template:

gapic-showcase/util/genrest/resttools/constants.go

Lines 26 to 34 in 082b527

	// RegexURLPathSingleSegmentValue contains the regex expression for matching a single URL
	// path segment (i.e. `/` is not allowed). Since gorilla/mux hands uses the decoded paths to
	// match, we can just accept any characters.
	RegexURLPathSingleSegmentValue = ".+"
	// RegexURLPathSingleSegmentValue contains the regex expression for matching multiple URL
	// path segments (i.e. `/` is allowed). Since gorilla/mux hands uses the decoded paths to
	// match, we can just accept any characters.
	RegexURLPathMultipleSegmentValue = ".+"

to be

	RegexURLPathSingleSegmentValue = "[a-zA-Z0-9_\\-]+"
	RegexURLPathMultipleSegmentValue = "[a-zA-Z0-9_\\-\\/]+"

which should only match for alphanumerics + dash + underscore (for single segments) and + slash for (multi segments).

Running this locally changes quite a bit of the handleFuncs, but the mixins look like:

	router.HandleFunc("/v1beta1/{name:projects/[a-zA-Z0-9_\\-]+}/locations", rest.HandleListLocations).Methods("GET")
	router.HandleFunc("/v1beta1/{name:projects/[a-zA-Z0-9_\\-]+/locations/[a-zA-Z0-9_\\-]+}", rest.HandleGetLocation).Methods("GET")
	router.HandleFunc("/v1beta1/{resource:users/[a-zA-Z0-9_\\-]+}:setIamPolicy", rest.HandleSetIamPolicy).Methods("POST")
	router.HandleFunc("/v1beta1/{resource:rooms/[a-zA-Z0-9_\\-]+}:setIamPolicy", rest.HandleSetIamPolicy_1).Methods("POST")
	router.HandleFunc("/v1beta1/{resource:rooms/[a-zA-Z0-9_\\-]+/blurbs/[a-zA-Z0-9_\\-]+}:setIamPolicy", rest.HandleSetIamPolicy_2).Methods("POST")
	router.HandleFunc("/v1beta1/{resource:sequences/[a-zA-Z0-9_\\-]+}:setIamPolicy", rest.HandleSetIamPolicy_3).Methods("POST")
	router.HandleFunc("/v1beta1/{resource:users/[a-zA-Z0-9_\\-]+}:getIamPolicy", rest.HandleGetIamPolicy).Methods("GET")
	router.HandleFunc("/v1beta1/{resource:rooms/[a-zA-Z0-9_\\-]+}:getIamPolicy", rest.HandleGetIamPolicy_1).Methods("GET")
	router.HandleFunc("/v1beta1/{resource:rooms/[a-zA-Z0-9_\\-]+/blurbs/[a-zA-Z0-9_\\-]+}:getIamPolicy", rest.HandleGetIamPolicy_2).Methods("GET")
	router.HandleFunc("/v1beta1/{resource:sequences/[a-zA-Z0-9_\\-]+}:getIamPolicy", rest.HandleGetIamPolicy_3).Methods("GET")
	router.HandleFunc("/v1beta1/{resource:users/[a-zA-Z0-9_\\-]+}:testIamPermissions", rest.HandleTestIamPermissions).Methods("POST")
	router.HandleFunc("/v1beta1/{resource:rooms/[a-zA-Z0-9_\\-]+}:testIamPermissions", rest.HandleTestIamPermissions_1).Methods("POST")
	router.HandleFunc("/v1beta1/{resource:rooms/[a-zA-Z0-9_\\-]+/blurbs/[a-zA-Z0-9_\\-]+}:testIamPermissions", rest.HandleTestIamPermissions_2).Methods("POST")
	router.HandleFunc("/v1beta1/{resource:sequences/[a-zA-Z0-9_\\-]+}:testIamPermissions", rest.HandleTestIamPermissions_3).Methods("POST")
	router.HandleFunc("/v1beta1/operations", rest.HandleListOperations).Methods("GET")
	router.HandleFunc("/v1beta1/{name:operations/[a-zA-Z0-9_\\-\\/]+}", rest.HandleGetOperation).Methods("GET")
	router.HandleFunc("/v1beta1/{name:operations/[a-zA-Z0-9_\\-\\/]+}", rest.HandleDeleteOperation).Methods("DELETE")
	router.HandleFunc("/v1beta1/{name:operations/[a-zA-Z0-9_\\-\\/]+}:cancel", rest.HandleCancelOperation).Methods("POST")

Running this locally seems to work for the showcase tests.

[noahdietz]

That looks accurate to me @vchudnov-g WDYT?

good stuff @lqiu96

[vchudnov-g]

Here's one thing I notice from your description above and the genrest.go file:
You say you are matching a GET handler here instead of the intended POST handler here. That suggests that your test is issuing a GET request rather than a POST request. Is that the case?

[vchudnov-g]

In terms of changing the regexes, the proposal looks fine, but are we sure those are all the characters allowed? They probably are, but part of me leans toward making more conservative changes first: [^:]+. That said, as long as we capture all the characters, your proposal does provide a stricter (hence better) check, which I like.

[noahdietz]

toward making more conservative changes first: [^:]+

That would be fine with me, perhaps even preferred.

[lqiu96]

Here's one thing I notice from your description above and the genrest.go file: You say you are matching a GET handler here instead of the intended POST handler here. That suggests that your test is issuing a GET request rather than a POST request. Is that the case?

Ah my bad, I believe that is a typo. The test I'm trying to add exists in this PR (currently a draft testing things out): https://github.com/googleapis/sdk-platform-java/pull/1789/files#diff-6102ce1a55d25723b508257a0e715555e252ec608d718631c162414591c44cfaR172-R175. (Creating the user isn't needed -- I wasn't sure of the error for a few hours and was playing around)

It's the second call getIamPolicy which fails.

[vchudnov-g]

In terms of allowed characters, if we were to go that route: maybe using CharsField or CharsLiteral (which does seem to contain extra characters)

In terms of multiple matchers: the first match wins, and if no HTTP method is specified, any method matches (as per gorilla/mux#480)

[vchudnov-g]

OK, so the intended match is this GET method? In that case changing the regex seems like the way to go (more robust than reordering), and I suggest we go with the colon route.

Thanks for catching, investigating, and fixing this, @lqiu96 !

[lqiu96]

OK, so the intended match is this GET method? In that case changing the regex seems like the way to go (more robust than reordering), and I suggest we go with the colon route.

Thanks for catching, investigating, and fixing this, @lqiu96 !

Yep, that is the intended GET method. I created a PR with your suggestion of using the colon