-
Notifications
You must be signed in to change notification settings - Fork 10
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.
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
Conversation
|
|
||
| -- imsAuth will validate the service token passed in "Authorization" header -- | ||
| function _M:validate_ims_token() | ||
| function _M:validateOAuthToken(oauth_token, validation_config) |
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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;
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
| UNKNOWN_ERROR = { error_code = "503010", message = "Could not validate the oauth token" } | ||
| } | ||
| local _M = BaseValidator:new({ | ||
| RESPONSES = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these responses are usually converted by validatorsHandlerErrorDecorator.lua. What would be a use-case when this validator would need different error messages ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Example use-case: I need to perform validate two different token of a single request, extracted from two different headers. The only way I see to return different error codes would be to instantiate the second validator with a different error messages map.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks ! So this is more like an edge case, in which case the current implementation works. Error codes would have to be different in such a case b/c otherwise the decorator would send back the same message.
Provides a little more flexibility in the OAuth token validator such that it is possible to support scenarios where you validate two tokens per request while using different error codes.
Still work in progress, but just wanted to open an early discussion.
@ddascal @andralungu ping