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.

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(plugins) anonymous authentication in auth plugins #1666

Merged
merged 3 commits into from
Nov 19, 2016

Conversation

subnetmarco
Copy link
Member

Summary

Adds support for anonymous authentication in every authentication plugin. When config.anonymous=true, even non-authenticated requests are allowed and the plugins will append a X-Anonymous-Consumer: true header to the upstream request.

Full changelog

  • basic-auth: support for anonymous consumers
  • key-auth: support for anonymous consumers
  • oauth2: support for anonymous consumers
  • hmac-auth: support for anonymous consumers
  • jwt: support for anonymous consumers
  • ldap-auth: support for anonymous consumers

Issues resolved

Fix #853.

@subnetmarco subnetmarco added this to the 0.10 milestone Sep 21, 2016
@Tieske
Copy link
Member

Tieske commented Sep 21, 2016

wouldn't this automatically add multi-auth support (in a logical OR fashion?)

@subnetmarco
Copy link
Member Author

@Tieske yes, if all of them have config.anonymous=true (which means ultimately the consumer can make a request without specifying any authentication). No, if you want at least one authentication to authenticate the consumer.

Copy link
Member

@Tieske Tieske left a comment

Choose a reason for hiding this comment

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

Approving, because the comments are only minor, overall LGTM.

@@ -101,7 +105,7 @@ function _M.execute(conf)
end

if not credential or not validate_credentials(credential, given_password) then
return responses.send_HTTP_FORBIDDEN("Invalid authentication credentials")
return false, {status = 403, message = "Invalid authentication credentials"}
Copy link
Member

Choose a reason for hiding this comment

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

why not just return success, statuscode, message without creating the intermediate table? More efficient and less GC.

same for the other plugins

ngx_set_header(constants.HEADERS.CONSUMER_ID, consumer.id)
ngx_set_header(constants.HEADERS.CONSUMER_CUSTOM_ID, consumer.custom_id)
ngx_set_header(constants.HEADERS.CONSUMER_USERNAME, consumer.username)
ngx_set_header(constants.HEADERS.CREDENTIAL_USERNAME, credential.username)
Copy link
Member

Choose a reason for hiding this comment

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

can't we cache those constants locally? the above are 15 table lookups

same for the other plugins

@@ -26,7 +26,8 @@ return {
RATELIMIT_REMAINING = "X-RateLimit-Remaining",
CONSUMER_GROUPS = "X-Consumer-Groups",
FORWARDED_HOST = "X-Forwarded-Host",
FORWARDED_PREFIX = "X-Forwarded-Prefix"
FORWARDED_PREFIX = "X-Forwarded-Prefix",
ANONYMOUS = "X-Anonymous-Consumer"
Copy link
Member

Choose a reason for hiding this comment

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

please use trailing comma for table constructors, to keep the diff clean

same for the other plugins

@subnetmarco subnetmarco merged commit f231623 into next Nov 19, 2016
@subnetmarco subnetmarco deleted the feat/plugins-anonymous-auth branch November 19, 2016 01:41
@Suraj-Kumar
Copy link

+1

@rajskhanna
Copy link

Thanks Tieske. So it looks like if I want to force one authentication or the other it is not possible. Excluding the anonymous access. So as I see it we should be defining 2 paths to accomplish this. right?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants