Skip to content

Added support for runtime customizaiton of the oauth token validator #15

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

Merged
merged 5 commits into from
Sep 7, 2016
Merged

Added support for runtime customizaiton of the oauth token validator #15

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
84386c0
Added support for runtime customizaiton of the oauth token validator
cristianconstantin Aug 21, 2016
42cf22f
Added an integration test and updated some code based on code review …
cristianconstantin Aug 23, 2016
93890a6
Cosmetic code reordering to improve GitHub Diff layout
cristianconstantin Aug 23, 2016
37ead48
Implemented an OO solution for customizing the default config
cristianconstantin Aug 24, 2016
c83bb6f
Extensibility improvements in the user profile validator
cristianconstantin Aug 28, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions src/lua/api-gateway/validation/factory.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
-- Time: 23:36
--

local BaseValidator = require "api-gateway.validation.validator"
local ValidatorsHandler = require "api-gateway.validation.validatorsHandler"
local ApiKeyValidatorCls = require "api-gateway.validation.key.redisApiKeyValidator"
local HmacSignatureValidator = require "api-gateway.validation.signing.hmacGenericSignatureValidator"
Expand Down Expand Up @@ -101,9 +102,9 @@ local function _generateHmacSignature()
return hmacSignatureValidator:generateSignature()
end

local function _validateOAuthToken()
local function _validateOAuthToken(obj)
Copy link
Member

@ddragosd ddragosd Aug 23, 2016

Choose a reason for hiding this comment

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

it's not clear to me which code passes the obj parameter.

Copy link
Contributor Author

@cristianconstantin cristianconstantin Aug 23, 2016

Choose a reason for hiding this comment

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

Here is an example:

location /validate_x_user_token {
    internal;

    content_by_lua_block {

        ngx.apiGateway.validation.validateOAuthToken({
            authtoken = ngx.var.user_token,
            RESPONSES = {
                MISSING_TOKEN = { error_code = "403110", message = "User token is missing" },
                INVALID_TOKEN = { error_code = "401113", message = "User token is not valid" },
                TOKEN_MISSMATCH = { error_code = "401114", message = "User token not allowed in the current context" },
                SCOPE_MISMATCH = { error_code = "401115", message = "User token scope mismatch" },
                UNKNOWN_ERROR = { error_code = "503110", message = "Could not validate the user token" }
            }
        });
    }
}

Copy link
Member

@ddragosd ddragosd Aug 23, 2016

Choose a reason for hiding this comment

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

Thanks @constantincristian . I think it would be helpful to add a test with this as well.

Copy link
Contributor Author

@cristianconstantin cristianconstantin Aug 23, 2016

Choose a reason for hiding this comment

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

Just added the test.

Copy link
Member

@ddragosd ddragosd Aug 24, 2016

Choose a reason for hiding this comment

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

👍

local oauthTokenValidator = OAuthTokenValidator:new()
return oauthTokenValidator:validateRequest()
BaseValidator:exitFn(oauthTokenValidator:validateRequest(obj))
Copy link
Member

@ddragosd ddragosd Aug 23, 2016

Choose a reason for hiding this comment

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

the other _validate* methods don't use exitFn here. If we want to change this then it would be easier to maintain this behavior if we change the others as well. What was the reason for this change here ? Was it behaving incorrectly ?

Copy link
Contributor Author

@cristianconstantin cristianconstantin Aug 23, 2016

Choose a reason for hiding this comment

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

True, we should keep consistency with the other validators.
The idea behind this change was that I may want to create a custom, composite validator out of existing built-in validators. So I may need to repeatedly call validateRequest on different validators, which would not be possible right now since validateRequest writes on the response. That's why I moved the code that writes on the response in the wrapper, so for a new composite validator I would just create a new wrapper.

Copy link
Member

@ddragosd ddragosd Aug 23, 2016

Choose a reason for hiding this comment

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

The composite validator idea sounds interesting. Do you have an example in mind that the validatorsHandler wouldn't support ?

end

local function _validateUserProfile()
Expand Down
55 changes: 30 additions & 25 deletions src/lua/api-gateway/validation/oauth2/oauthTokenValidator.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,8 +57,8 @@ local RESPONSES = {
local LOCAL_CACHE_TTL = 60

-- Hook to override the logic verifying if a token is valid
function _M:istokenValid(json)
return json.valid or false, RESPONSES.INVALID_TOKEN
function _M:isTokenValid(json, validation_config)
return json.valid or false, validation_config.RESPONSES.INVALID_TOKEN
end

-- override this if other checks need to be in place
Expand Down Expand Up @@ -129,11 +129,11 @@ end

-- TODO: cache invalid tokens too for a short while
-- Check in the response if the token is valid --
function _M:checkResponseFromAuth(res, cacheLookupKey)
function _M:checkResponseFromAuth(res, cacheLookupKey, validation_config)
local json = cjson.decode(res.body)
if json ~= nil then

local tokenValidity, error = self:istokenValid(json)
local tokenValidity, error = self:isTokenValid(json, validation_config)
if not tokenValidity and error ~= nil then
return tokenValidity, error
end
Expand Down Expand Up @@ -166,15 +166,8 @@ function _M:getTokenFromCache(cacheLookupKey)
return nil;
end

-- imsAuth will validate the service token passed in "Authorization" header --
function _M:validate_ims_token()
function _M:validateOAuthToken(oauth_token, validation_config)
Copy link
Member

@ddragosd ddragosd Aug 23, 2016

Choose a reason for hiding this comment

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

if validation_config is nil or validation_config.RESPONSES is nil this method will fail with NPE.
We could either create a getter for validation_config that can be used in other methods too, or make sure that it's not breaking this method.
The getter could be:

local function get_validation_config( validation_config )
    local cfg = validation_config or {}
    cfg.RESPONSES = validation_config.RESPONSES or RESPONSES
    return cfg
end

WDYT ?

Copy link
Contributor Author

@cristianconstantin cristianconstantin Aug 23, 2016

Choose a reason for hiding this comment

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

I added think exact kind of initialization in the validateRequest method, which is calling this code, so this should cover your observation, right?

function _M:validateRequest(validation_config)
    validation_config = validation_config or {}
    validation_config.RESPONSES = validation_config.RESPONSES or RESPONSES;

Copy link
Member

@ddragosd ddragosd Aug 23, 2016

Choose a reason for hiding this comment

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

Yes, that's where I copied the code but the fact that the validateOAuthToken is public made me think that it would make the code better if it were to have protection for such a use case.

Copy link
Contributor Author

@cristianconstantin cristianconstantin Aug 23, 2016

Choose a reason for hiding this comment

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

I didn't think so much of the validateOAuthToken being public. But it makes perfect sense to use it from a composite validator, so I can then revert the exitFn call back to the validator class.
I will update the code.

Copy link
Contributor Author

@cristianconstantin cristianconstantin Aug 23, 2016

Choose a reason for hiding this comment

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

Another note: I initially wanted to add the validation_config object as a instance member, as I thought this would make more sense. But I'm not really proficient in Lua OO, so I followed an alternate route. WDYT?

Copy link
Member

@ddragosd ddragosd Aug 23, 2016

Choose a reason for hiding this comment

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

Yes, Lua is not OO friendly 😄 . If you'd rather define it as an instance member I think you can do something like:

OAuthTokenValidator:new({
  validation_config = foo
})

B/c the new(o) method is defined in the Base Validator and it decorates o with the methods and properties defined by that class.

If you take this route please write a test to verify what I'm saying here, in case I'm wrong.

local oauth_host = ngx.var.oauth_host
local oauth_token = ngx.var.authtoken

-- ngx.var.authtoken needs to be set before calling this method
if oauth_token == nil or oauth_token == "" then
return self:exitFn(RESPONSES.MISSING_TOKEN.error_code, cjson.encode(RESPONSES.MISSING_TOKEN))
end

--1. try to get token info from the cache first ( local or redis cache )
local oauth_token_hash = ngx.md5(oauth_token)
Expand All @@ -190,37 +183,49 @@ function _M:validate_ims_token()
ngx.log(ngx.DEBUG, "Caching locally a new token for " .. tostring(local_expire_in) .. " s, out of a total validity of " .. tostring(tokenValidity ) .. " s.")
self:setKeyInLocalCache(cacheLookupKey, cachedToken, local_expire_in , "cachedOauthTokens")
self:setContextProperties(obj)
return self:exitFn(ngx.HTTP_OK)
return ngx.HTTP_OK
end
-- at this point the cached token is not valid
ngx.log(ngx.WARN, "Invalid OAuth Token found in cache. OAuth host=" .. tostring(oauth_host))
if (error == nil) then
error = RESPONSES.INVALID_TOKEN
error = validation_config.RESPONSES.INVALID_TOKEN
end
error.error_code = error.error_code or RESPONSES.INVALID_TOKEN.error_code
return self:exitFn(error.error_code, cjson.encode(error))
error.error_code = error.error_code or validation_config.RESPONSES.INVALID_TOKEN.error_code
return error.error_code, cjson.encode(error)
end

-- 2. validate the token with the OAuth endpoint
local res = ngx.location.capture("/validate-token", { share_all_vars = true })
local res = ngx.location.capture("/validate-token", {
share_all_vars = true,
args = { authtoken = oauth_token}
})
if res.status == ngx.HTTP_OK then
local tokenValidity, error = self:checkResponseFromAuth(res, cacheLookupKey)
local tokenValidity, error = self:checkResponseFromAuth(res, cacheLookupKey, validation_config)
if (tokenValidity == true) then
return self:exitFn(ngx.HTTP_OK)
return ngx.HTTP_OK
end
-- at this point the token is not valid
ngx.log(ngx.WARN, "Invalid OAuth Token returned. OAuth host=" .. tostring(oauth_host))
if (error == nil) then
error = RESPONSES.INVALID_TOKEN
error = validation_config.RESPONSES.INVALID_TOKEN
end
error.error_code = error.error_code or RESPONSES.INVALID_TOKEN.error_code
return self:exitFn(error.error_code, cjson.encode(error))
error.error_code = error.error_code or validation_config.RESPONSES.INVALID_TOKEN.error_code
return error.error_code, cjson.encode(error)
end
return self:exitFn(res.status, cjson.encode(RESPONSES.UNKNOWN_ERROR));
return res.status, cjson.encode(validation_config.RESPONSES.UNKNOWN_ERROR);
end

function _M:validateRequest(obj)
return self:validate_ims_token()
function _M:validateRequest(validation_config)
validation_config = validation_config or {}
validation_config.RESPONSES = validation_config.RESPONSES or RESPONSES;

local oauth_token = validation_config.authtoken or ngx.var.authtoken

if oauth_token == nil or oauth_token == "" then
return validation_config.RESPONSES.MISSING_TOKEN.error_code, cjson.encode(validation_config.RESPONSES.MISSING_TOKEN)
end

return self:validateOAuthToken(oauth_token, validation_config)
end


Expand Down