Set namespace on API if not present #3953
Conversation
| @@ -92,6 +96,10 @@ func (ni *NamespaceValidatorInterceptor) LengthValidationIntercept( | |||
| info *grpc.UnaryServerInfo, | |||
| handler grpc.UnaryHandler, | |||
| ) (interface{}, error) { | |||
| err := ni.setNamespaceIfNotPresent(ni.namespaceRegistry, req) | |||
| if err != nil { | |||
| return nil, err | |||
There was a problem hiding this comment.
Existing behavior will also return not found in this case I think?
| namespaceName := GetNamespace(namespaceRegistry, req) | ||
| if namespaceName != "" { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Can we check if req is a NamespaceGetter and if request.GetNamespace() is empty? I feel it's hard to reason about GetNamespace function which hides the error.
Then we also don't need the empty check in setNamespace
| @@ -92,6 +96,10 @@ func (ni *NamespaceValidatorInterceptor) LengthValidationIntercept( | |||
| info *grpc.UnaryServerInfo, | |||
| handler grpc.UnaryHandler, | |||
| ) (interface{}, error) { | |||
| err := ni.setNamespaceIfNotPresent(ni.namespaceRegistry, req) | |||
| if err != nil { | |||
| return nil, err | |||
There was a problem hiding this comment.
Existing behavior will also return not found in this case I think?
| req interface{}, | ||
| ) error { | ||
| namespaceName := GetNamespace(namespaceRegistry, req) | ||
| if namespaceName != "" { |
There was a problem hiding this comment.
Why not just always set the namespace from the one in the token? Then we can drop all the token stuff from the rest of this file (extractNamespace gets simpler, checkNamespaceMatch goes away, etc.). Also this function and setNamespace gets simpler too
There was a problem hiding this comment.
Agree that we can simplify the logic here. My proposal is we can extract namespace from token, then we set it to request if it is empty. If it is not empty we validate it is the same as from token, if they are different then we return error. This will make sure the namespace from request is always the same as it from token, so we can remove all other logic around token validation in this file.
There was a problem hiding this comment.
We have to make sure this overwrite happen before authorizer interceptor because auth is done against the namespace set in the request field.
| ) { | ||
| switch request := req.(type) { | ||
| case *workflowservice.RespondQueryTaskCompletedRequest: | ||
| if request.Namespace == "" { |
There was a problem hiding this comment.
nit: looks like we no longer need this check.
What changed?
Why?
Better compatibility
How did you test it?
New tests
Potential risks
N/A
Is hotfix candidate?
N/A