-
Notifications
You must be signed in to change notification settings - Fork 17
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
fix(RBAC): fix scope for release tags #237
Conversation
Merging these commits will result in the following changelog entries: Changelogsgo-lib-micro (rbac-fix)New changes in go-lib-micro since master: Bug Fixes
|
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.
one double-check kiind of comment.
rbac/rbac.go
Outdated
@@ -49,7 +49,7 @@ func WithContext(ctx context.Context, scope *Scope) context.Context { | |||
func ExtractScopeFromHeader(r *http.Request) *Scope { | |||
groupStr := r.Header.Get(ScopeHeader) | |||
tagsStr := r.Header.Get(ScopeReleaseTagsHeader) | |||
if len(groupStr) > 0 { | |||
if len(groupStr) || len(tagsStr) > 0 { |
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.
just to double check: are we sure that for else to this if we are returning nil?
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.
sorry, but I don't understand the comment; can you rephrase?
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.
sorry, I am on fire a bit. I meant:
if this:
if len(groupStr) || len(tagsStr) > 0 {
is not true, then we return nil
. I wanted just to point your attention to it and double check if that is something we want.
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.
that is something we were already doing (btw, I fixed the condition) and is something we don't want to change
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.
copy that
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 noticed the condition fix. and I was copying this code to the comment -- still failed to notice the new syntax ;-)
Changelog: Title Ticket: None Signed-off-by: Krzysztof Jaskiewicz <krzysztof.jaskiewicz@northern.tech>
No description provided.