feat(acl) allow acl plugin to use a consumer without credentials #2722

bungle · 2017-07-21T16:47:33Z

Summary

Sometimes Kong is not the provider of credentials, but a third-party identity provider does that. E.g. with OpenID Connect and LDAP the credentials are stored in 3rd party IdP or directory.

ACL plugin on the other hand did only work with ngx.ctx.authenticated_credential, this patch adds a support to ngx.ctx.authenticated_consumer as well.

kikito

Just a style comment. @bungle if you don't agree it can still be merged.

kikito · 2017-07-21T18:00:36Z

kong/plugins/acl/handler.lua

    consumer_id = ngx.ctx.authenticated_credential.consumer_id
-  else
+  end
+


Wouldn't it be simpler to write an elseif here?

if ngx.ctx.authenticated_consumer then consumer_id = ngx.ctx.authenticated_consumer.id elseif ngx.ctx.authenticated_credential then consumer_id = ngx.ctx.authenticated_credential.consumer_id end

Or even

local consumer_id = (ngx.ctx.authenticated_consumer and ngx.ctx.authenticated_consumer.id) or (ngx.ctx.authenticated_credential and ngx.ctx.authenticated_credential.consumer_id)

(That second one might be a bit too much)

I'd be in favor of the latter approach ^ though the parens aren't necessary I don't think.

Actually no. There is always change that ngx.ctx.authenticated_credential is set but ngx.ctx.authenticated_credential.customer_id results nil. For example right now I'm working this around by setting ngx.ctx.authenticated_credential.customer_id, but that feels wrong as I cannot set ngx.ctx.authenticated_credential.id as there is no credential (and that could screw another plugin). My approach works even if people do stupid things, and it doesn't hurt anything (maybe just one extra nil check).

What I mean here is that even if there is ngx.ctx.authenticated_consumer table or ngx.ctx. authenticated_credential table, there is no gurantee that it has any properties. We could go on, and even argue that the current and this patched code will crash if ngx.ctx.authenticated_consumer or ngx.ctx.authenticated_credential are not tables.

bungle · 2017-07-21T21:20:11Z

I pushed a new version that also doesn't crash if the used context variables are not tables (be liberal in what you accept).

thibaultcha

LGTM, except for the mix of feature/style/perf changes in the same PR/commit. It'd be great to have then separated!

thibaultcha · 2017-07-22T02:22:36Z

kong/plugins/acl/handler.lua


 local ACLHandler = BasePlugin:extend()

+


It seems like this PR has many style changes. As we've agreed upon by the past, could we limit the scope of the changes in the PRs? Or if we want those changes in the same PR, could they be in separate commits at least?

feat(acl) work with consumer

style(acl) update to new style

perf(acl) minor improvements

thibaultcha · 2017-07-22T02:26:47Z

kong/plugins/acl/handler.lua

+
+  if not consumer_id then
+    return responses.send_HTTP_FORBIDDEN(
+      "Cannot identify the consumer, add an authentication plugin to use the ACL plugin"


aside note: welp, I never saw this before, but I don't think user-facing errors/responses should talk about the notion of "plugins". This should probably be logged for the Kong admin to see (ngx.log(ngx.WARN, ...) maybe), and the 403 response shown to the user should be blank. This is, of course, out of the scope of this PR and is definitely not a blocker for this PR imo :)

thibaultcha · 2017-07-22T02:27:53Z

kong/plugins/acl/handler.lua

+    cache.acls_key(consumer_id),
+    nil,
+    load_acls_into_memory,
+    consumer_id)


Any reason for this change and the new line below? I think it was fine as it was (per the code style), except for the second line not aligning with the first argument. Such style changes should be part of another commit/PR anyways.

thibaultcha · 2017-07-22T02:28:19Z

kong/plugins/acl/handler.lua

@@ -60,14 +89,17 @@ function ACLHandler:access(conf)
  if next(conf.whitelist or empty) then
    if not next(acls or empty) then
      block = true
+


ditto: all those style changes - while appreciated, it'd be great if they could at least be included in a separate commit :) Thanks!

thibaultcha · 2017-07-22T02:28:47Z

kong/plugins/acl/handler.lua

@@ -83,7 +115,9 @@ function ACLHandler:access(conf)
  for _, v in ipairs(acls) do
    table_insert(str_acls, v.group)
  end
-  ngx.req.set_header(constants.HEADERS.CONSUMER_GROUPS, table_concat(str_acls, ", "))
+
+  set_header(constants.HEADERS.CONSUMER_GROUPS, table_concat(str_acls, ", "))


Such a change should also be separated in its own perf(acl) commit as well

bungle added the pr/status/please review label Jul 21, 2017

bungle force-pushed the feat/acl-plugin-consumer branch from 08fb9a7 to 7ebdaba Compare July 21, 2017 16:50

kikito reviewed Jul 21, 2017

View reviewed changes

bungle force-pushed the feat/acl-plugin-consumer branch from 7ebdaba to ddf2adf Compare July 21, 2017 21:19

thibaultcha requested changes Jul 22, 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 Jul 22, 2017

feat(acl) allow acl plugin to use a consumer without credentials

1225a7d

Tieske force-pushed the feat/acl-plugin-consumer branch from ddf2adf to 1225a7d Compare July 26, 2017 11:49

fix(acl) do not leak config info

8e1382a

Tieske force-pushed the feat/acl-plugin-consumer branch from 87d43d5 to 8e1382a Compare July 26, 2017 14:16

Tieske mentioned this pull request Jul 26, 2017

perf(acl) rewrite for performance, jit and caching #2736

Merged

thibaultcha approved these changes Jul 26, 2017

View reviewed changes

thibaultcha merged commit 2461e04 into master Jul 26, 2017

Tieske added pr/ready This PR is considered ready and can be merged at anytime (given it received no subsequent changes) and removed pr/changes requested Changes were requested to this PR by a maintainer. Please address them and ping back once done. labels Jul 27, 2017

Tieske deleted the feat/acl-plugin-consumer branch July 27, 2017 07:18

This was referenced Jul 27, 2017

Anonymous auth breaks ACL plugin #2547

Closed

Allow multiple authentication methods per API and/or Consumer #590

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(acl) allow acl plugin to use a consumer without credentials #2722

feat(acl) allow acl plugin to use a consumer without credentials #2722

bungle commented Jul 21, 2017

kikito left a comment

kikito Jul 21, 2017

p0pr0ck5 Jul 21, 2017

bungle Jul 21, 2017 •

edited

Loading

bungle Jul 21, 2017 •

edited

Loading

bungle commented Jul 21, 2017 •

edited

Loading

thibaultcha left a comment

thibaultcha Jul 22, 2017

thibaultcha Jul 22, 2017

thibaultcha Jul 22, 2017

thibaultcha Jul 22, 2017

thibaultcha Jul 22, 2017

feat(acl) allow acl plugin to use a consumer without credentials #2722

feat(acl) allow acl plugin to use a consumer without credentials #2722

Conversation

bungle commented Jul 21, 2017

Summary

kikito left a comment

Choose a reason for hiding this comment

kikito Jul 21, 2017

Choose a reason for hiding this comment

p0pr0ck5 Jul 21, 2017

Choose a reason for hiding this comment

bungle Jul 21, 2017 • edited Loading

Choose a reason for hiding this comment

bungle Jul 21, 2017 • edited Loading

Choose a reason for hiding this comment

bungle commented Jul 21, 2017 • edited Loading

thibaultcha left a comment

Choose a reason for hiding this comment

thibaultcha Jul 22, 2017

Choose a reason for hiding this comment

thibaultcha Jul 22, 2017

Choose a reason for hiding this comment

thibaultcha Jul 22, 2017

Choose a reason for hiding this comment

thibaultcha Jul 22, 2017

Choose a reason for hiding this comment

thibaultcha Jul 22, 2017

Choose a reason for hiding this comment

bungle Jul 21, 2017 •

edited

Loading

bungle Jul 21, 2017 •

edited

Loading

bungle commented Jul 21, 2017 •

edited

Loading