fix(plugins): multiple auth fix in an OR scenario

[Tieske]

The last auth plugin running would overwrite the results of the previous ones.
So if the first authenticates successfully, and the second fails, then the
second one would overwrite the consumer details with the details of the
anonymous consumer.

WIP: 2 failing oauth2 tests
Reason: the oauth2 endpoint is also authenticated by the configured
plugins. Hence in the AND scenario, the key-auth plugin returns a 40x, which
in turn makes the oauth2 fail

[Tieske]

The oauth2 has some peculiarities when used in an AND fashion. The token endpoints also fall within the api, hence when a token is requested, the other configured auth methods kick in. So to make this pass, the token endpoints also require credentials for the other auth methods.

Discussed this with @thefosk and we agreed on this solution.

[p0pr0ck5]

are these newlines necessary? we could fix style here in a separate commit. these changes are outside the scope of this PR, dirtying the commit history.

[Tieske]

will keep them separate next time

[bungle]

Only comment from me is that all the authentication plugins seems to share this:

function _M.execute(conf)

  if ngx.ctx.authenticated_credential and conf.anonymous ~= "" then
    -- we're already authenticated, and we're configured for using anonymous, 
    -- hence we're in a logical OR between auth methods and we're already done.
    return
  end

  local ok, err = do_authentication(conf)
  if not ok then
    if conf.anonymous ~= "" then
      -- get anonymous user
      local consumer, err = cache.get_or_set(cache.consumer_key(conf.anonymous),
                       nil, load_consumer_into_memory, conf.anonymous, true)
      if err then
        return responses.send_HTTP_INTERNAL_SERVER_ERROR(err)
      end
      set_consumer(consumer, nil)
    else
      return responses.send(err.status, err.message)
    end
  end
end

So I was thinking could we generalize this somewhere, e.g. AuthPlugin. We discussed this already with @Tieske and I don't feel strongly about this if this is about to happen with new plugin api. Can this wait until that?

Note: this is far from the only thing repeating, though.

[bungle]

Another thing is the usablity:

anonymous ~= "" → logical OR (anonymous)
anonymous == "" → logical AND (non-anonymous)
anonymous ~= "" + request_termination plugin → logical OR (anonymous, but terminates on anonymous)

If I'm correct? Pretty hard to wrap this in your head.

But what this already does is better than we had, so LGTM.

[thibaultcha]

@bungle The long term plan is to rewrite the plugins runloop in such a way that:

• we get rid of the OOP pattern plugins have
• it can run plugins or sets of plugins
• it can distinguish "types" of plugins, in this case, "Authentication", and such plugins follow an interface and have special return values.

imo, spending time getting rid of some code duplication we currently have would be a waste of time, due to this plugins runloop rewrite and the Public Lua API (that will have functions dedicated to Authentication plugins).

[thibaultcha]

This was not moved to the correct version this was merged against. Be extra wary when merging PRs.