feat(key-auth) add endpoint to list all key-auths #2955

hbagdi · 2017-10-13T19:04:29Z

Summary

Adding an endpoint to list all the keys of all consumers.
This is an initial commit to get better feedback from the community.

The endpoint could be changed to something else.
Grouping of multiple keys belonging to the same consumer is something I'm considering
A similar thing could be done for the ACL plugin.

Tests are missing right now.

Full changelog

Implement an endpoint GET /plugins/key-auth/consumers to list all the consumers with their key(s).

thibaultcha

Hi,

Thank you for giving this a try! This is a feature we are very interested in. If you are willing to work this us on updating this PR, we would gladly accept it.

I gave some suggestions on the endpoints themselves. We will also require of you that you write tests for these new endpoints. Those tests will have to belong to the key-auth plugin test suite.

Thank you!

thibaultcha · 2017-10-13T20:44:26Z

kong/plugins/key-auth/api.lua

@@ -52,5 +52,10 @@ return {
    DELETE = function(self, dao_factory)
      crud.delete(self.keyauth_credential, dao_factory.keyauth_credentials)
    end
+  },
+  ["/plugins/key-auth/consumers"] = {


It seems to me like the intent of such an endpoint is to retrieve a paginated list key-auth credentials. One of the use-cases that this solves is "retrieving a particular key-auth in order to read it's consumer_id and retrieve the Consumer associated with a key-auth credential" (as the title of this PR suggests). I think this would mean that users need to make the following request:

/plugins/key-auth/consumers?id=... or /plugins/key-auth/consumers?key=...

However, such a request could only return a single result, as a key-auth credential can only be associated with a single Consumer. Additionally, the current implementation seems to return key-auth credential, and makes forces the client to make a subsequent request to retrieve the full Consumer entity. Finally, the URL of this request is confusing in the sense that user isn't sure if the response will be a list of Plugins, key-auth, or Consumers...

If we want to add both a paginated set of all key-auth credentials, and the ability to retrieve the Consumer associated with a credential, it might be friendlier and more appropriate (RESTful) to solve those two requirements with two endpoints:

/key-auth/ /consumers/key-auth/:credential_key_or_id

The reasoning is that in our RESTful interface, we try to prefix a resource's endpoint with the type of entities the endpoint will return. The first endpoint returns a list of key-auth credentials. The second one directly returns a single Consumer associated with this credential (or 404 Not Found) without the need for the client to send 2 requests (the first one for the key-auth credential, and the next one to finally request the Consumer from the retrieved consumer_id).

What do you think?

thibaultcha · 2017-10-13T20:57:16Z

Grouping of multiple keys belonging to the same consumer is something I'm considering

That'd be great too! The endpoint could relate to the one I suggested in my review, which would get us 2 new endpoints:

/key-auth/
/consumers/:consumer/key-auth/

A similar thing could be done for the ACL plugin.

We are also interested similar endpoints for the ACL, basic-auth, hmac and JWT plugins if they do not exist yet! But let's process step-by-step. We should keep the scope of this PR limited to the key-auth plugin only :) We will gladly accept more contributions for other credentials-based plugins once we're square on this first plugin.

Tests are missing right now.

Great!

hbagdi · 2017-10-13T22:01:44Z

@thibaultcha Thanks a lot for the feedback.

/key-auth/ endpoint to list all the key-auth credential makes a lot of sense really since what it returns is key-auths. Other auth plugins could have similar GET all endpoints. I'll get this one done first.
The endpoint you metion/consumers/key-auth/:credential_key_or_id to retrieve a consumer for a given key-credential is a very useful endpoint to have.
However, consider /consumers/:consumer_id_or_name/key-auth, which returns all the key-auths for the specific consumer. To maintain consistency, we can instead have an endpoint /key-auth/: credential_key_or_id/consumer to retrieve an consumer for a key or key_id.

Thoughts?

I'll try to get this into a good shape and once we're done here, we can take on other plugins as well.

thibaultcha · 2017-10-14T00:11:45Z

@hbagdi Yes, sorry, I meant /consumers/:consumer/key-auth/. I updated my comment to reflect this. Your proposal resonates to me. If I understand correctly, this would add the following two endpoints to our key-auth plugin:

1. /key-auth/
2. /key-auth/:credential_key_or_id/consumer

List all key-auth credentials in a paginated fashion. It's also a way to retrieve all credentials belonging to a Consumer, via filtering (/key-auth?consumer_id=...).
In a single call, retrieve the Consumer associated with a given key-auth credential.

Am I missing anything?

hbagdi · 2017-10-14T00:21:17Z

Sounds good to me.

So, this PR will add the following:

/key-auth/
/key-auth/:credential_key_or_id/consumer
Tests for 1 and 2

I'm done with 2 and half of 1 (need filtering).
I'll add tests and let you know.

Thanks for being prompt!

hbagdi · 2017-10-14T05:22:49Z

@thibaultcha I've updated the PR to reflect our discussion.

thibaultcha

This looks great! I haven't run it myself yet, but for now, I don't think the endpoints are missing anything. I do think we are missing a few tests though.

Thanks!

thibaultcha · 2017-10-15T23:29:32Z

spec/03-plugins/10-key-auth/01-api_spec.lua

@@ -310,4 +311,117 @@ describe("Plugin: key-auth (API)", function()
      assert.equal(key_name, body.config.key_names[1])
    end)
  end)
+  describe("/Key-auth", function()


Please name this test suite as per the endpoint (not capitalized): /key-auth

thibaultcha · 2017-10-15T23:31:38Z

spec/03-plugins/10-key-auth/01-api_spec.lua

+          method = "GET",
+          path = "/key-auth",
+          body = {
+            consumer_id = consumer.id


I think this might be the same test case as above ("/key-auth?consumer_id=" .. consumer.id) with a different syntax allowed by lua-resty-http/our spec helpers? If not, then GET requests shouldn't contain a body anyways. Better remove this test-case regardless: it's either a duplicate of the above one, or a use-case we don't wish to support (and that should be fixed and tested in lua-resty-http or our helpers functions)

thibaultcha · 2017-10-15T23:35:37Z

spec/03-plugins/10-key-auth/01-api_spec.lua

+          method = "GET",
+          path = "/key-auth",
+          body = {
+            consumer_id = utils.uuid()


ditto, please specify URI arguments with the following syntax to avoid confusion:

path = "/key-auth?consumer_id=" .. utils.uuid()

thibaultcha · 2017-10-15T23:36:02Z

spec/03-plugins/10-key-auth/01-api_spec.lua

+  end)
+  describe("/key-auth/:credential_key_or_id/consumer", function()
+    describe("GET", function()
+      local credential;


style: please remove this semicolon

thibaultcha · 2017-10-15T23:37:29Z

spec/03-plugins/10-key-auth/01-api_spec.lua

+      it("retrieve all the key-auths", function()
+        local res = assert(admin_client:send {
+          method = "GET",
+          path = "/key-auth"


Could we add a test case with a URI containing a trailing slash (/key-auth/)? We've had issues with our Lapis routes and trailing slashes in the last few months and we want to avoid it from happening again. A test will help prevent that :) Thanks!

thibaultcha · 2017-10-15T23:39:34Z

kong/plugins/key-auth/api.lua

+      self.params.username_or_id = credentials[1].consumer_id
+      crud.find_consumer_by_username_or_id(self, dao_factory, helpers)
+    end,
+    GET = function(self, dao_factory,helpers)


style: we are missing a single line jump above this handler declaration (see other handlers in this file)

thibaultcha · 2017-10-15T23:43:01Z

spec/03-plugins/10-key-auth/01-api_spec.lua

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


Could we also add a test case with some non-UUID garbage? The reason is that if the value is not recognized as a valid UUID, then the admin API will treat it as the credential key. We need to make sure a non-existing key also returns HTTP 404. Thanks!

thibaultcha · 2017-10-15T23:43:41Z

spec/03-plugins/10-key-auth/01-api_spec.lua

+        local json = cjson.decode(body)
+        assert.same(consumer,json)
+      end)
+      it("retrieve consumer from a keyid", function()


It seems this test should rather be named: "retrieves a Consumer from a credential's 'key'"

hbagdi · 2017-10-16T03:39:46Z

@thibaultcha I've added and corrected the tests.

thibaultcha · 2017-10-16T06:30:13Z

spec/03-plugins/10-key-auth/01-api_spec.lua

+      it("retrieve all the key-auths", function()
+        local res = assert(admin_client:send {
+          method = "GET",
+          path = "/key-auth/"


We should test with, and without the trailing slash here is what I meant :) Sorry if it wasn't clear in my previous comment!

@thibaultcha Sorry, my bad. I intended to do what you suggested but it somehow ended up being something else; fixed it.

hbagdi · 2017-10-19T02:50:57Z

@thibaultcha having the endpoint as /key-auths makes more sense?

thibaultcha · 2017-11-01T21:48:29Z

having the endpoint as /key-auths makes more sense?

Hmmm yeah, probably. This would stay consistent with the /acls endpoint proposed in #2371. Unfortunately our existing key-auth endpoints use a singular form (/consumers/:username_or_id/key-auth/), but let's try to avoid this mistake again.

If you update this PR to use the plural form you suggested, I don't see any further blockers to merge it. Thanks!

hbagdi · 2017-11-02T01:45:16Z

@thibaultcha I'll send PRs for similar endpoints for JWT and hmac soon then.

* `/key-auths/` to paginate through key-auth credentials * `/key-auths/:credential_key_or_id/consumer` to retrieve the Consumer associated with a credential From #2955 Signed-off-by: Thibault Charbonnier <thibaultcha@me.com>

thibaultcha · 2017-11-02T22:10:50Z

Manually merged to master with an updated commit message and some minor edits, including a new test ensuring the /key-auths endpoint is paginated :)

This is awesome, thank you very much for giving a stab at those! Onwards to JWT, basic-auth, and acls 😉

thibaultcha · 2017-11-02T22:30:35Z

@hbagdi Oh, I was almost forgetting: will you also contribute the appropriate documentation on our open source docs repository: https://github.com/Kong/getkong.org/ ? Would be greatly appreciated!

You can find the key-auth plugin documentation here: https://getkong.org/plugins/key-authentication/ with, in the sidebar, some anchors that lead to various sections of the page describing the additional endpoints provided by this plugin. Maybe 2 more sections would be appropriate here:

Browse all key-auth credentials
Retrieve the Consumer associated with a key-auth credential

We do value complete contributions including code, tests and documentation :)

hbagdi · 2017-11-03T02:57:57Z

@thibaultcha I did have documentation on mind. I'll send out a PR soon.

Two new endpoints have been added to the key-auth plugin: * `/key-auths/` to paginate through key-auth credentials * `/key-auths/:credential_key_or_id/consumer` to retrieve the Consumer associated with a credential See: Kong/kong#2955

Two new endpoints have been added to the key-auth plugin: * `/key-auths/` to paginate through key-auth credentials * `/key-auths/:credential_key_or_id/consumer` to retrieve the Consumer associated with a credential See: Kong/kong#2955 From: #546 Signed-off-by: Thibault Charbonnier <thibaultcha@me.com>

thibaultcha requested changes Oct 13, 2017

View reviewed changes

hbagdi force-pushed the list-consumer-keys branch 2 times, most recently from 3b198a1 to bd3d64e Compare October 14, 2017 05:14

thibaultcha requested changes Oct 15, 2017

View reviewed changes

hbagdi force-pushed the list-consumer-keys branch 2 times, most recently from 7b902aa to e9e6bc1 Compare October 16, 2017 03:37

thibaultcha reviewed Oct 16, 2017

View reviewed changes

Add endpoint to list consumers using key-auth plugin

4eceb7e

hbagdi force-pushed the list-consumer-keys branch from e9e6bc1 to 4eceb7e Compare October 16, 2017 15:04

hbagdi mentioned this pull request Oct 19, 2017

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

Closed

hbagdi changed the title ~~Add endpoint to list all consumers using key-auth plugin and their keys~~ feat(key-auth) Add endpoint to list all consumers using key-auth plugin and their keys Oct 31, 2017

hbagdi changed the title ~~feat(key-auth) Add endpoint to list all consumers using key-auth plugin and their keys~~ feat(key-auth) Add endpoint to list all key-auths Oct 31, 2017

hbagdi mentioned this pull request Oct 31, 2017

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

Merged

hbagdi changed the title ~~feat(key-auth) Add endpoint to list all key-auths~~ feat(key-auth) add endpoint to list all key-auths Nov 1, 2017

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

change endpoint to key-auths

4dd4050

hbagdi mentioned this pull request Nov 2, 2017

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

Merged

thibaultcha closed this Nov 2, 2017

hbagdi deleted the list-consumer-keys branch November 3, 2017 02:54

hbagdi mentioned this pull request Nov 7, 2017

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

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(key-auth) add endpoint to list all key-auths #2955

feat(key-auth) add endpoint to list all key-auths #2955

hbagdi commented Oct 13, 2017 •

edited

Loading

thibaultcha left a comment

thibaultcha Oct 13, 2017

thibaultcha commented Oct 13, 2017 •

edited

Loading

hbagdi commented Oct 13, 2017

thibaultcha commented Oct 14, 2017

hbagdi commented Oct 14, 2017

hbagdi commented Oct 14, 2017

thibaultcha left a comment

thibaultcha Oct 15, 2017

thibaultcha Oct 15, 2017

thibaultcha Oct 15, 2017

thibaultcha Oct 15, 2017

thibaultcha Oct 15, 2017

thibaultcha Oct 15, 2017

thibaultcha Oct 15, 2017

thibaultcha Oct 15, 2017

hbagdi commented Oct 16, 2017

thibaultcha Oct 16, 2017

hbagdi Oct 16, 2017

hbagdi commented Oct 19, 2017

thibaultcha commented Nov 1, 2017

hbagdi commented Nov 2, 2017

thibaultcha commented Nov 2, 2017

thibaultcha commented Nov 2, 2017

hbagdi commented Nov 3, 2017

feat(key-auth) add endpoint to list all key-auths #2955

feat(key-auth) add endpoint to list all key-auths #2955

Conversation

hbagdi commented Oct 13, 2017 • edited Loading

Summary

Full changelog

thibaultcha left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thibaultcha commented Oct 13, 2017 • edited Loading

hbagdi commented Oct 13, 2017

thibaultcha commented Oct 14, 2017

hbagdi commented Oct 14, 2017

hbagdi commented Oct 14, 2017

thibaultcha left a 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

hbagdi commented Oct 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hbagdi commented Oct 19, 2017

thibaultcha commented Nov 1, 2017

hbagdi commented Nov 2, 2017

thibaultcha commented Nov 2, 2017

thibaultcha commented Nov 2, 2017

hbagdi commented Nov 3, 2017

hbagdi commented Oct 13, 2017 •

edited

Loading

thibaultcha commented Oct 13, 2017 •

edited

Loading