feat(basic-auth) add endpoint to list all basic-auths #2998

hbagdi · 2017-10-31T04:12:07Z

Summary

Adding endpoints:

GET /basic-auth : lists all the basic-auths present in kong
GET /basic-auth/:credential_id/consumer : retrieve a consumer form a credential ID

Full changelog

GET /basic-auth : lists all the basic-auths present in kong
GET /basic-auth/:credential_id/consumer : retrieve a consumer form a credential ID
Add related tests

This is similar to #2955 and #2371

thibaultcha · 2017-10-31T18:45:41Z

@hbagdi Very nice! Thank you for providing solutions to this issue in an atomic fashion (per plugin) 👍 We will get a look shortly!

We should also resume the review of #2955 very shortly as well, sorry for the delay on our side.

thibaultcha · 2017-11-01T21:51:23Z

kong/plugins/basic-auth/api.lua

@@ -52,5 +52,29 @@ return {
    DELETE = function(self, dao_factory)
      crud.delete(self.basicauth_credential, dao_factory.basicauth_credentials)
    end
+  },
+  ["/basic-auth/"] = {


Should we also use the plural form /basic-auths here as well to stay consistent with what we decided for /key-auths?

thibaultcha · 2017-11-01T21:52:17Z

kong/plugins/basic-auth/api.lua

+  },
+  ["/basic-auth/:credential_id/consumer"] = {
+    before = function(self, dao_factory, helpers)
+      local credentials, err = dao_factory.basicauth_credentials:find_all { id = self.params.credential_id }


style: we should avoid exceeding the 80 columns limit for long lines as documented in our contribution guidelines. Thanks!

thibaultcha · 2017-11-01T21:56:33Z

kong/plugins/basic-auth/api.lua

+      crud.paginated_set(self, dao_factory.basicauth_credentials)
+    end
+  },
+  ["/basic-auth/:credential_id/consumer"] = {


The /consumers/:username_or_id/basic-auth/:credential_username_or_id endpoint provided by this plugin allows to retrieve a credential based on its username attribute, or its id.

Maybe we should allow the same for this endpoint as well? Any reason why we couldn't do so?

local credentials, err = crud.find_by_id_or_field(dao_factory.basicauth_credentials, nil self.params.credential_username_or_id, "username")

thibaultcha · 2017-11-01T21:57:55Z

kong/plugins/basic-auth/api.lua

+        return helpers.responses.send_HTTP_NOT_FOUND()
+      end
+
+      self.params.credential_key_or_id = nil


This instruction is not needed since the parameter is called credential_id in this endpoint. Only if we follow my other suggestion of allowing credential_username_or_id does it become useful if we rename it properly (not credential_key_*).

thibaultcha · 2017-11-01T21:59:46Z

spec/03-plugins/11-basic-auth/02-api_spec.lua

+    describe("GET", function()
+      setup(function()
+        helpers.dao:truncate_table("basicauth_credentials")
+          assert(helpers.dao.basicauth_credentials:insert {


It seems like this call is further indented for not particular reason?

thibaultcha · 2017-11-01T21:59:55Z

spec/03-plugins/11-basic-auth/02-api_spec.lua

+        consumer2 = assert(helpers.dao.consumers:insert {
+          username = "bob-the-buidler"
+        })
+          assert(helpers.dao.basicauth_credentials:insert {


ditto: indentation

thibaultcha · 2017-11-02T22:24:57Z

spec/03-plugins/11-basic-auth/02-api_spec.lua

+      it("returns 404 for a random non-existing basic-auth id", function()
+        local res = assert(admin_client:send {
+          method = "GET",
+          path = "/basic-auths/" .. utils.uuid()  .. "/consumer"


Since we modified the parameter to be :credential_username_or_id, we should also add a test case for a non-existing username this test case only covers non-existing id).

thibaultcha · 2017-11-02T22:26:00Z

spec/03-plugins/11-basic-auth/02-api_spec.lua

+        assert.is_table(json.data)
+        assert.equal(2, #json.data)
+        assert.equal(2, json.total)
+      end)


Will you also add a test case for pagination, like the one I added to /key-auths/? You can see it here: 18f3a09#diff-a053792566ffb587af457e08fd34ec14R361

* `/basic-auths/` to paginate through all basic-auth credentials * `/basic-auths/:credential_id_or_username/consumer` to retrieve the Consumer associated with a credential

thibaultcha · 2017-11-18T01:38:10Z

Awesome, thank you very much for your hard work on those plugins! We are scheduling this for our upcoming 0.11.2 release (as per our CHANGELOG).

hbagdi · 2017-11-18T02:29:30Z

@thibaultcha Thanks for accepting these contributions!
You' have been very patient with minor details I missed often.
Let's look for other features/bugs where I can help out.

hbagdi · 2017-11-18T02:45:37Z

@thibaultcha couple of things we should do before the upcoming release:

We need to update docs.
I've one already docs(key-auth) add 2 new endpoints to list key-auths docs.konghq.com#546 . Once we square it off, I'll send pulls for other plugins as well.
We should also send out feat(acl) exposing an API to access ACL groups globally #2371 in the same release. I've a branch in good state and will send out a PR soon but could you answer a question in the existing PR?

thibaultcha · 2017-11-18T02:47:30Z

Yes to all of these, definitely a must have for the release :)
We’ll take a look! Thanks for stepping forward with the docs as well.

* `/basic-auths/` to paginate through all basic-auth credentials * `/basic-auths/:credential_id_or_username/consumer` to retrieve the Consumer associated with a credential See: Kong/kong#2998

* `/basic-auths/` to paginate through all basic-auth credentials * `/basic-auths/:credential_id_or_username/consumer` to retrieve the Consumer associated with a credential See: Kong/kong#2998 From: #553 Signed-off-by: Thibault Charbonnier <thibaultcha@me.com>

thibaultcha added the pr/status/please review label Oct 31, 2017

thibaultcha requested changes Nov 1, 2017

View reviewed changes

thibaultcha added pr/changes requested Changes were requested to this PR by a maintainer. Please address them and ping back once done. and removed pr/status/please review labels Nov 1, 2017

hbagdi mentioned this pull request Nov 2, 2017

feat(jwt) add endpoint to list all JWTs #3003

Merged

thibaultcha requested changes Nov 2, 2017

View reviewed changes

feat(basic-auth) add 2 endpoints to interact with basic-auth credentials

8d013f5

* `/basic-auths/` to paginate through all basic-auth credentials * `/basic-auths/:credential_id_or_username/consumer` to retrieve the Consumer associated with a credential

hbagdi force-pushed the list-basic-auths branch from b774e56 to 8d013f5 Compare November 3, 2017 03:24

thibaultcha approved these changes Nov 18, 2017

View reviewed changes

thibaultcha removed the pr/changes requested Changes were requested to this PR by a maintainer. Please address them and ping back once done. label Nov 18, 2017

thibaultcha merged commit 54487be into Kong:master Nov 18, 2017

hbagdi deleted the list-basic-auths branch November 18, 2017 02:45

hbagdi mentioned this pull request Nov 22, 2017

docs(basic-auth) add 2 new endpoints to list basic-auths Kong/docs.konghq.com#553

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(basic-auth) add endpoint to list all basic-auths #2998

feat(basic-auth) add endpoint to list all basic-auths #2998

hbagdi commented Oct 31, 2017

thibaultcha commented Oct 31, 2017

thibaultcha Nov 1, 2017

thibaultcha Nov 1, 2017

thibaultcha Nov 1, 2017

thibaultcha Nov 1, 2017

thibaultcha Nov 1, 2017

thibaultcha Nov 1, 2017

thibaultcha Nov 2, 2017

hbagdi Nov 3, 2017

thibaultcha Nov 2, 2017

hbagdi Nov 3, 2017

thibaultcha commented Nov 18, 2017

hbagdi commented Nov 18, 2017

hbagdi commented Nov 18, 2017

thibaultcha commented Nov 18, 2017

feat(basic-auth) add endpoint to list all basic-auths #2998

feat(basic-auth) add endpoint to list all basic-auths #2998

Conversation

hbagdi commented Oct 31, 2017

Summary

Full changelog

thibaultcha commented Oct 31, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thibaultcha commented Nov 18, 2017

hbagdi commented Nov 18, 2017

hbagdi commented Nov 18, 2017

thibaultcha commented Nov 18, 2017