-
Notifications
You must be signed in to change notification settings - Fork 825
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add enforcement of namespace match for async completion tokens #1086
Add enforcement of namespace match for async completion tokens #1086
Conversation
* RespondActivityTaskCompleted * RespondActivityTaskFailed * RespondActivityTaskCanceled
* RespondWorkflowTaskCompleted * RespondWorkflowTaskFailed * RespondQueryTaskCompleted
* RespondWorkflowTaskCompleted * RespondWorkflowTaskFailed * RespondQueryTaskCompleted
service/frontend/workflowHandler.go
Outdated
if !wh.config.EnableTokenNamespaceEnforcement() { | ||
return nil | ||
} | ||
if !strings.EqualFold(requestNamespace, tokenNamespace) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Namespaces are case sensitive. At least I was able to create two namespaces which are different in case only. It means someone else might already have such namespaces created and this check will be backward compatible for them.
I think this should be regular string comparison.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
namespace is case sensitive
service/frontend/workflowHandler.go
Outdated
func (wh *WorkflowHandler) RespondWorkflowTaskCompleted(ctx context.Context, | ||
request *workflowservice.RespondWorkflowTaskCompletedRequest) (_ *workflowservice.RespondWorkflowTaskCompletedResponse, | ||
retError error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
func (wh *WorkflowHandler) RespondWorkflowTaskCompleted(
ctx context.Context,
request *workflowservice.RespondWorkflowTaskCompletedRequest)
(_ *workflowservice.RespondWorkflowTaskCompletedResponse, retError error) {
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't want to start this, but yes, let's make some standart here. Seems like nor gofmt
neither Go Land
are not care much about it. So there are two cases:
- One line. All parameters are on the saneline, all return values are on the same line, no empty line before func body:
func convertString(input string) string {
return strings.ToUpper(input)
}
- Many lines. Method/func name and all parameters are on separate line, each has comma at the end and there is a blank line before body. Return values are still on the same line though as we almost don't have more than 2:
func (wh *WorkflowHandler) RespondWorkflowTaskCompleted(
ctx context.Context,
request *workflowservice.RespondWorkflowTaskCompletedRequest,
) (_ *workflowservice.RespondWorkflowTaskCompletedResponse, retError error) {
defer ...
service/frontend/workflowHandler.go
Outdated
func (wh *WorkflowHandler) RespondWorkflowTaskFailed(ctx context.Context, | ||
request *workflowservice.RespondWorkflowTaskFailedRequest) (_ *workflowservice.RespondWorkflowTaskFailedResponse, | ||
retError error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same
service/frontend/workflowHandler.go
Outdated
func (wh *WorkflowHandler) RespondActivityTaskCompleted(ctx context.Context, | ||
request *workflowservice.RespondActivityTaskCompletedRequest) (_ *workflowservice.RespondActivityTaskCompletedResponse, | ||
retError error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
namespace is case sensitive
What changed?
Added enforcement of async completion tokens matching request's namespace.
Why?
Such mismatched (by mistake) calls caused customers' confusion and investigation.
Mismatched calls also violate isolation of namespaces.
How did you test it?
Unit tests.
Potential risks
No risk as the enforcement is disabled by default (for backward compatibility).