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(hmac-auth) add 2 endpoints to interact with hmac-auth credentials #3009

Merged
merged 2 commits into from
Nov 18, 2017
Merged

feat(hmac-auth) add 2 endpoints to interact with hmac-auth credentials #3009

merged 2 commits into from
Nov 18, 2017

Conversation

hbagdi
Copy link
Member

@hbagdi hbagdi commented Nov 3, 2017

  • /hmac-auths/ to paginate through all hmac-auth credentials
  • /hmac-auths/:credential_key_or_id/consumer to retrieve the Consumer
    associated with a credential

@thibaultcha
Copy link
Member

Thanks again! Marking this one for review as well :)

thibaultcha
thibaultcha previously approved these changes Nov 17, 2017
View reviewed changes
Copy link
Member

@thibaultcha thibaultcha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing to add here, this complies to the style of this legacy part of the codebase and I see you added the test cases from my previous merge on key-auths.

Thank you for your hard work on this feature for all those plugins!

@thibaultcha thibaultcha added pr/ready This PR is considered ready and can be merged at anytime (given it received no subsequent changes) and removed pr/status/please review labels Nov 17, 2017
thibaultcha
thibaultcha reviewed Nov 18, 2017
View reviewed changes
kong/plugins/hmac-auth/api.lua Outdated
crud.paginated_set(self, dao_factory.hmacauth_credentials)
end
},
["/hmac-auths/:credential_username_or_id/consumer"] = {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here, wouldn't hmac_username be a better name as well?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to make sure /hmac-auths/:hmac_username_or_id/consumer, right?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that would be slightly better yes. "credential" is ok for key-auth or basic-auth I would say, but here I think we need users to grasp better where is this "username" attribute we are referring to. It would thus be the username of the /hmac-auth entities

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@thibaultcha Should I update Line 23 in this file as well to maintain consistency? Also, I'll update tests.

@thibaultcha 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/ready This PR is considered ready and can be merged at anytime (given it received no subsequent changes) labels Nov 18, 2017
@thibaultcha thibaultcha dismissed their stale review November 18, 2017 00:01

slightly better path name could be used

thibaultcha
thibaultcha requested changes Nov 18, 2017
View reviewed changes
Copy link
Member

@thibaultcha thibaultcha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could use a slightly better past name - and that should be all!

@hbagdi
Copy link
Member Author

hbagdi commented Nov 18, 2017 •
edited
Loading

@thibaultcha I've updated the naming in the file overall.
I've assumed we will squash and merge.
If we decide to put the renaming of other variables in a separate commit, then I can put a better commit message.
The same is true for JWT PR #3003

@hbagdi
feat(hmac-auth) add 2 endpoints to interact with hmac-auth credentials
97e534a 
* `/hmac-auths/` to paginate through all hmac-auth credentials
* `/hmac-auths/:credential_key_or_id/consumer` to retrieve the Consumer
associated with a credential
@hbagdi
style(hmac-auth) rename credential_username_or_id to hmac_username_or_id
13f76e3
thibaultcha
thibaultcha approved these changes Nov 18, 2017
View reviewed changes
Copy link
Member

@thibaultcha thibaultcha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great as is, thanks!

@thibaultcha thibaultcha merged commit b5f8c81 into Kong:master Nov 18, 2017
@thibaultcha
Copy link
Member

Thank you!

@hbagdi hbagdi deleted the list-hmacs branch November 18, 2017 02:28
@Tieske
Copy link
Member

Tieske commented Nov 19, 2017

@hbagdi yay! 🎉

hbagdi added a commit to hbagdi/getkong.org that referenced this pull request Nov 22, 2017
@hbagdi
docs(hmac-auth) add 2 new endpoints to list hmac-auths
8e11c3b 
Two new endpoints have been added to the hmac-auth plugin:
* `/hmac-auths/` to paginate through all hmac-auth credentials
* `/hmac-auths/:credential_key_or_id/consumer` to retrieve the
Consumer associated with a credential

See Kong/kong#3009
hbagdi added a commit to hbagdi/getkong.org that referenced this pull request Nov 22, 2017
@hbagdi
docs(hmac-auth) add 2 new endpoints to list hmac-auths
1df4710 
Two new endpoints have been added to the hmac-auth plugin:
* `/hmac-auths/` to paginate through all hmac-auth credentials
* `/hmac-auths/:credential_key_or_id/consumer` to retrieve the
Consumer associated with a credential

See Kong/kong#3009
@hbagdi hbagdi mentioned this pull request Nov 22, 2017
Closed
thibaultcha pushed a commit to Kong/docs.konghq.com that referenced this pull request Nov 23, 2017
@hbagdi @thibaultcha
docs(hmac-auth) describe new endpoints to list credentials
37a2b51 
Two new endpoints have been added to the hmac-auth plugin:

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

See: Kong/kong#3009
From: 555

Signed-off-by: Thibault Charbonnier <thibaultcha@me.com>
thibaultcha pushed a commit to Kong/docs.konghq.com that referenced this pull request Nov 23, 2017
@hbagdi @thibaultcha
docs(hmac-auth) describe new endpoints to list credentials
ed1624b 
Two new endpoints have been added to the hmac-auth plugin:

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

See: Kong/kong#3009
From: #555

Signed-off-by: Thibault Charbonnier <thibaultcha@me.com>
thibaultcha pushed a commit to Kong/docs.konghq.com that referenced this pull request Nov 23, 2017
@hbagdi @thibaultcha
docs(hmac-auth) describe new endpoints to list credentials
860f2df 
Two new endpoints have been added to the hmac-auth plugin:

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

See: Kong/kong#3009
From: #555

Signed-off-by: Thibault Charbonnier <thibaultcha@me.com>
thibaultcha pushed a commit to Kong/docs.konghq.com that referenced this pull request Nov 29, 2017
@hbagdi @thibaultcha
docs(hmac-auth) describe new endpoints to list credentials
735ab00 
Two new endpoints have been added to the hmac-auth plugin:

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

See: Kong/kong#3009
From: #555

Signed-off-by: Thibault Charbonnier <thibaultcha@me.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thibaultcha thibaultcha thibaultcha approved these changes

Assignees
No one assigned
Labels
pr/changes requested Changes were requested to this PR by a maintainer. Please address them and ping back once done.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@hbagdi @thibaultcha @Tieske