-
Notifications
You must be signed in to change notification settings - Fork 138
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 requireAdmin() to Sync Function (Fixes #3276) #3277
Conversation
runner, err := NewSyncRunner(funcSource) | ||
assert.Equals(t, err, nil) | ||
var result interface{} | ||
result, _ = runner.Call(parse(`{}`), parse(`{}`), parse(`{}`)) |
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.
AFAIK, this is equivalent to how a request from the Admin port will hit the Sync Function, but would appreciate a double-check on this.
assertNotRejected(t, result) | ||
result, _ = runner.Call(parse(`{}`), parse(`{}`), parse(`{"name": ""}`)) | ||
assertRejected(t, result, base.HTTPErrorf(403, "admin required")) | ||
result, _ = runner.Call(parse(`{}`), parse(`{}`), parse(`{"name": "GUEST"}`)) |
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.
As above, AFAIK this should represent a request as the GUEST
user, but a double-check of my logic here would be good.
74d4fd8
to
d21a473
Compare
31d869b
to
23e772a
Compare
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 don't know enough about how the runner gets called to verify your comments, so I'll wait for Adam or Traun to review, but in looks good in general 👍
@JFlath This looks good to me, but it looks like it pulled in an unnecessary manifest change somewhere along the way. Do you mind rebasing to get rid of that, then ping me and I can merge? |
Hey, when I tried the first time, I think I pulled in 200-odd commits, it's an improvement 😂! Will sort it out when I get a sec, thanks! |
23e772a
to
df19aaf
Compare
df19aaf
to
7a8f1dd
Compare
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.
LGTM. Sanity checked against a local SG and everything works as advertised.
/me adds Golang to LinkedIn skills |
Fixes #3276 This adds a new callback to the Sync Function:
This allows rejecting mutations that don't come via the Admin port.