-
-
Notifications
You must be signed in to change notification settings - Fork 480
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
Improve validator registration #449
Improve validator registration #449
Conversation
f5bb56b
to
5c44d12
Compare
This looks great. Can you add some documentation? |
@bjmc Ping? |
Yeah, I can add some documentation. Do you just want docstrings? Or as part of a separate document? |
Docstrings inline and usage documentation under |
@thedrow I hope that looks okay for the docs, I'm not an expert at the sphinx syntax. |
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.
There are a few more things I'd like to change but they are rather small. I'll see if I can do them myself.
else: | ||
self._auth_validators_run_before_standard_ones.append(validator) | ||
|
||
def register_token_validator(self, validator, after_standard=True): |
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.
Why do we need a boolean flag instead of two methods? This is likely a code smell.
http://softwareengineering.stackexchange.com/questions/147977/is-it-wrong-to-use-a-boolean-parameter-to-determine-behavior
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 really went back and forth on this. I agree the boolean is a little bit clunky.
Other options:
Use two separate methods (four when you count auth validators and token validators).
The reasons I didn't do this were:
-
Using the boolean let me preserve backwards compatibility for anyone who is already using
register_*_validator()
in its existing incarnation. -
Many users just want some custom validations in the mix, and don't care about whether they run before or after the existing ones. Having a boolean with a default spares those users from having to think about it.
-
I couldn't think of method names that were informative without being so verbose it seemed unwieldy, e.g.
register_auth_validator_run_before_standard()
(before standard what?)
Just completely get rid of these registration methods and let callers append or insert to the list of validators directly.
I actually kind of like this option, because it feels more Pythonic doing away with setter methods, but I didn't do it because it seemed out of step with the rest of the code base, and obviously it somewhat locks you into an implementation for the future.
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 option would be to wrap the validators list with an API that allows you to switch the internal implementation later on.
What do you think?
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.
That's true. I would maybe make the case that the API we expose should be the standard Python MutableSequence
and we can start by just using a stock list. If in the future, anyone wants to change the implementation but preserve the API, they can use a MutableSequence
subclass.
response_types = ['code'] | ||
|
||
def __init__(self, request_validator=None, **kwargs): | ||
self.request_validator = request_validator or RequestValidator() |
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.
Can we provide a way to initialize a grant type with validators?
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.
Well, you could pass _auth_validators_run_before_standard_ones
as a keyword argument to the constructor, and that would do it. But that feels a bit hacky.
# For implicit grant, auth_validators and token_validators are | ||
# basically equivalent since the token is returned from the | ||
# authorization endpoint. | ||
for validator in chain(self._token_validators_run_before_standard_ones, |
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.
Extract to a helper method since this code is repeated on line 372 with different validators.
result = validator(request) | ||
if result is not None: | ||
request_info.update(result) | ||
|
||
return request.scopes, request_info | ||
return request_info |
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.
This method returns request_info
, but this return value is not used in any of the places where it is called.
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.
Yeah. I figure somebody else in the future might want this to return request_info
. Maybe in tests?
If you want this to be a more 'pure' function, it could construct a new dict and return that instead of mutating the existing one.
Then the calling method could do request_info = self._run_custom_validations(...)
Maybe that's better?
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 agree it's better.
09dfdfb
to
790310a
Compare
@thedrow Do you like that better? I moved all the custom validators into a container class, and you can now pass keyword args to the grant type constructor to initialize a |
LGTM! Thanks. |
This changeset is an attempt to improve and standardize the validator registration hooks introduced with the OpenIDConnect changes.
GrantTypeBase
class and removes those methods from the subclass grants.ClientCredentialsGrant
,RefreshTokenGrant
andResourceOwnerPasswordCredentialsGrant
.