search

tilgovi / pyramid-oauthlib

Pyramid OAuthLib integration
BSD 2-Clause "Simplified" License
31 stars 16 forks source link

Add missing oauth params #19

Closed jer-tx closed 4 years ago

tilgovi commented 4 years ago

Looking at the base Request class from oauthlib, I'm not sure it's 100% correct to add request.client here. The request validator may use a custom sub-class of Client, for instance, that can load and persist its client_secret from a DB model.

What's sort of surprising is that the documentation recommends setting request.client in the authenticate_client or authenticate_client_id methods of the request validator, but the error you posted in #18 seems to suggest that it's required.

I think what's going on is that oauthlib is flexible, almost to a fault, here. If you were to implement custom grant types, they might not need request.client, but the built-in ones do (or at least, authorization_code does). Therefore, it's incumbent upon you to set request.client to something useful to you (which may subclass Client) if your grant types rely on it.

tilgovi commented 4 years ago

I think this is where examples / documentation would help a lot.

So, for now, maybe we can add all the OIDC parameters (and the missing OAuth 2 parameter) and just merge that.

jer-tx commented 4 years ago

So, for now, maybe we can add all the OIDC parameters (and the missing OAuth 2 parameter) and just merge that.

I can add all the OIDC params, which oauth2 param are you referring to?

tilgovi commented 4 years ago

I was referring to user, which you added. And I think, yeah, we should add all those OIDC params while we're here. Thank you!

tilgovi commented 4 years ago

Actually, before I merge this (since it's been a little while), can you confirm for me that it's still possible to set request.client, overriding the getter that add_oauth_param installs if we list client in this set?

jer-tx commented 4 years ago

As in, can we still set request.client on the validator if client exists in OAUTH_PARAMS? yes that works. If I add config.add_oauth_param('client') to config, I get a pyramid.exceptions.ConfigurationConflictError error, which is probably intended behavior.

tilgovi commented 4 years ago

Thanks, I've been in JS-land so much lately I think I was thinking of Object.defineProperty() making non-configurable properties (can't be reassigned/deleted). By adding a request property in Pyramid is not that invasive, so we're good.