-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
feat(acl) exposing an API to access ACL groups globally #2371
Conversation
kong/plugins/acl/api.lua
Outdated
id = self.params.id, | ||
consumer_id = self.params.consumer_id, | ||
} | ||
self.params.group_or_id = 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.
slightly confused, where is group_or_id
populated?
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.
good catch - it's a leftover from a first implementation when I had :group_or_id
in the URL.
} | ||
}) | ||
local body = assert.res_status(400, res) | ||
assert.equal([[{"consumer_id":"consumer_id is required","group":"group is required"}]], body) |
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.
it might make more sense to test table equality directly, by decoding body
, rather than a string representation of the JSON? table keys are not encoded deterministically (in our environment, to my knowledge), so this test can fail at random.
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.
@p0pr0ck5 in the past tests we have been comparing the string directly. Maybe @thibaultcha has an opinion on this?
} | ||
}) | ||
local body = assert.res_status(400, res) | ||
assert.equal([[{"consumer_id":"consumer_id is required","group":"group is required"}]], body) |
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.
idem
} | ||
}) | ||
local body = assert.res_status(400, res) | ||
assert.equal([[{"group":"group is required"}]], body) |
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.
idem
kong/plugins/acl/api.lua
Outdated
local acls, err = dao_factory.acls:find_all(filter_keys) | ||
if err then | ||
return helpers.yield_error(err) | ||
elseif next(acls) == nil then |
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.
should this be elseif #acls == 0
, to match existing style, and as a more performant/idiomatic approach?
Yes, better to decode too I think, since those tests have indeed proven themselves to occasionally fail on different platforms due to the behavior @p0pr0ck5 described |
The last commit addresses the review. |
Would have been better to amend previous commits to keep the history clean. Will fixup during merge |
I kept the history in the PR so that reviews can be tracked, but the idea is to squash and merge once everything is ready. |
@@ -51,7 +51,9 @@ describe("Plugin: acl (API)", function() | |||
} | |||
}) | |||
local body = assert.res_status(400, res) | |||
assert.equal([[{"group":"group is required"}]], body) | |||
assert.are.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.
these JSON encoding changes are being handled in #2402; it's not necessary here i dont think, and it's also out of scope for this commit
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.
Should we also implement an endpoint /acls/:consumer_id
that shows all the groups a user is associated with?
before = function(self, dao_factory, helpers) | ||
local filter_keys = { | ||
id = self.params.id, | ||
consumer_id = self.params.consumer_id, |
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.
Since id
is the PK on acls
, searching by consumer_id
seems unnecessary?
end) | ||
end) | ||
|
||
describe("GET", function() |
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.
This test nesting feels really awkward. We have
/acls GET
and then
/acls GET /acls/:id GET
/acls GET /acls/:id PATCH
/acls GET /acls/:id DELETE
Can we (should we?) clean this up?
end) | ||
|
||
describe("GET", function() | ||
it("retrieves ACL group by id", function() |
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.
It would be good to expand this test to see the route in question filter out some unrelated entries (e.g. add some ACL entries that are not associated with this consumer)
CHANGELOG.md
Outdated
@@ -46,6 +46,9 @@ | |||
- file-log: New `config.reopen` property to close and reopen the log file on | |||
every request, in otder to effectively rotate the logs. | |||
[#2348](https://github.com/Mashape/kong/pull/2348) | |||
- acl: a new `/acls` endpoint has been exposed in the Admin API to globally |
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 think this isnt going to make it into 0.10.2, so this will need to be moved elsewhere?
["Content-Type"] = "application/json" | ||
} | ||
}) | ||
local body = cjson.decode(assert.res_status(201, res)) |
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.
in some cases this is wrapping cjson.decode
around res_status
, and in other cases the decoding is being done as part of the body check assertion (as in the next test below). can we keep this behavior consistent?
@thefosk any update? |
@thefosk any update? |
@thefosk @thibaultcha @p0pr0ck5 I feel having an endpoint The problem I'm facing, it seems there is no straightforward way of doing: select distinct group from acls; Is there away around of doing this, or we will need to update the core DB logic? I could take a stab at it if there is another use case for such a query. IMHO, this would be pretty useful for admin purposes. |
#3039 has just been merged and includes such an Closing this then! |
Full changelog
/acls
endpoint to globally access ACL groupsIssues resolved
Fix #2188
TODO: requires documentation