Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat(acl) exposing an API to access ACL groups globally #2371

Closed
wants to merge 7 commits into from
Closed

feat(acl) exposing an API to access ACL groups globally #2371

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
d56aee9
feat(acl) exposing an API to access ACL groups globally
subnetmarco Apr 11, 2017
8dc2a96
test(acl) checking decoded table instead of string
subnetmarco Apr 18, 2017
aefbaf8
refactor(acl) using length in DAO response check
subnetmarco Apr 18, 2017
2dfbbd9
Merge branch 'master' into feat/acls-api
subnetmarco Apr 26, 2017
be8d1c5
changelog
subnetmarco Apr 28, 2017
93a1553
Merge branch 'master' into feat/acls-api
subnetmarco May 1, 2017
7d2b7cc
Merge branch 'master' into feat/acls-api
subnetmarco Oct 19, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 44 additions & 0 deletions kong/plugins/acl/api.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,50 @@ return {
crud.patch(self.params, dao_factory.acls, self.acl)
end,

DELETE = function(self, dao_factory)
crud.delete(self.acl, dao_factory.acls)
end
},

["/acls"] = {
GET = function(self, dao_factory)
crud.paginated_set(self, dao_factory.acls)
end,

PUT = function(self, dao_factory)
crud.put(self.params, dao_factory.acls)
end,

POST = function(self, dao_factory)
crud.post(self.params, dao_factory.acls)
end
},

["/acls/:id"] = {
before = function(self, dao_factory, helpers)
local filter_keys = {
id = self.params.id,
consumer_id = self.params.consumer_id,
Copy link
Contributor

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?

}

local acls, err = dao_factory.acls:find_all(filter_keys)
if err then
return helpers.yield_error(err)
elseif #acls == 0 then
return helpers.responses.send_HTTP_NOT_FOUND()
end

self.acl = acls[1]
end,

GET = function(self, dao_factory, helpers)
return helpers.responses.send_HTTP_OK(self.acl)
end,

PATCH = function(self, dao_factory)
crud.patch(self.params, dao_factory.acls, self.acl)
end,

DELETE = function(self, dao_factory)
crud.delete(self.acl, dao_factory.acls)
end
Expand Down
192 changes: 192 additions & 0 deletions spec/03-plugins/19-acl/01-api_spec.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -229,6 +229,7 @@ describe("Plugin: acl (API)", function()
}
})
local body = assert.res_status(400, res)

local json = cjson.decode(body)
assert.same({ group = "ACL group already exist for this consumer" }, json)
end)
Expand Down Expand Up @@ -268,4 +269,195 @@ describe("Plugin: acl (API)", function()
end)
end)
end)

describe("/acls/", function()

after_each(function()
helpers.dao:truncate_table("acls")
end)

describe("POST", function()
it("creates an ACL group", function()
local res = assert(admin_client:send {
method = "POST",
path = "/acls",
body = {
group = "hello-group",
consumer_id = consumer.id
},
headers = {
["Content-Type"] = "application/json"
}
})
local body = cjson.decode(assert.res_status(201, res))
Copy link
Contributor

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?

assert.equal(consumer.id, body.consumer_id)
assert.equal("hello-group", body.group)
end)
describe("errors", function()
it("returns bad request", function()
local res = assert(admin_client:send {
method = "POST",
path = "/acls",
body = {},
headers = {
["Content-Type"] = "application/json"
}
})
local body = assert.res_status(400, res)
assert.are.same({
consumer_id = "consumer_id is required",
group = "group is required"
}, cjson.decode(body))
end)
end)
end)

describe("PUT", function()
it("creates an ACL group", function()
local res = assert(admin_client:send {
method = "PUT",
path = "/acls",
body = {
group = "hello-group",
consumer_id = consumer.id
},
headers = {
["Content-Type"] = "application/json"
}
})
local body = cjson.decode(assert.res_status(201, res))
assert.equal(consumer.id, body.consumer_id)
assert.equal("hello-group", body.group)
end)
describe("errors", function()
it("returns bad request", function()
local res = assert(admin_client:send {
method = "PUT",
path = "/acls",
body = {},
headers = {
["Content-Type"] = "application/json"
}
})
local body = assert.res_status(400, res)
assert.are.same({
consumer_id = "consumer_id is required",
group = "group is required"
}, cjson.decode(body))
end)
end)
end)

describe("GET", function()
Copy link
Contributor

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?

setup(function()
for i = 1, 3 do
assert(helpers.dao.acls:insert {
group = "hello-group"..i,
consumer_id = consumer.id
})
end
end)
teardown(function()
helpers.dao:truncate_table("acls")
end)
it("retrieves the first page", function()
local res = assert(admin_client:send {
method = "GET",
path = "/acls"
})
local body = assert.res_status(200, res)
local json = cjson.decode(body)
assert.is_table(json.data)
assert.equal(3, #json.data)
assert.equal(3, json.total)
end)
end)

describe("/acls/:id", function()
local acl
before_each(function()
helpers.dao:truncate_table("acls")
acl = assert(helpers.dao.acls:insert {
group = "hello-group",
consumer_id = consumer.id
})
end)

describe("GET", function()
it("retrieves ACL group by id", function()
Copy link
Contributor

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)

local res = assert(admin_client:send {
method = "GET",
path = "/acls/"..acl.id
})
local body = assert.res_status(200, res)
local json = cjson.decode(body)
assert.equal(acl.id, json.id)
end)
end)

describe("PATCH", function()
it("updates an ACL group by id", function()
local previous_group = acl.group

local res = assert(admin_client:send {
method = "PATCH",
path = "/acls/"..acl.id,
body = {
group = "updated"
},
headers = {
["Content-Type"] = "application/json"
}
})
local body = assert.res_status(200, res)
local json = cjson.decode(body)
assert.not_equal(previous_group, json.group)
end)
describe("errors", function()
it("handles invalid input", function()
local res = assert(admin_client:send {
method = "PATCH",
path = "/acls/"..acl.id,
body = {
group = ""
},
headers = {
["Content-Type"] = "application/json"
}
})
local body = assert.res_status(400, res)
assert.are.same({
group = "group is required"
}, cjson.decode(body))
end)
end)
end)

describe("DELETE", function()
it("deletes an ACL group", function()
local res = assert(admin_client:send {
method = "DELETE",
path = "/acls/"..acl.id,
})
assert.res_status(204, res)
end)
describe("errors", function()
it("returns 400 on invalid input", function()
local res = assert(admin_client:send {
method = "DELETE",
path = "/acls/blah"
})
assert.res_status(400, res)
end)
it("returns 404 if not found", function()
local res = assert(admin_client:send {
method = "DELETE",
path = "/acls/00000000-0000-0000-0000-000000000000"
})
assert.res_status(404, res)
end)
end)
end)
end)
end)
end)