Skip to content
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

Provide variable providerParams #125

Closed
AdriVanHoudt opened this issue Aug 17, 2015 · 8 comments · Fixed by #129
Closed

Provide variable providerParams #125

AdriVanHoudt opened this issue Aug 17, 2015 · 8 comments · Fixed by #129
Labels
Milestone

Comments

@AdriVanHoudt
Copy link
Contributor

@AdriVanHoudt AdriVanHoudt commented Aug 17, 2015

My provider accepts extra query params on the auth route, which you can set with providerParams. But afaik I don't see an option to change these values on every request. For example I can provide a login_hint query param but this hint is different on every request

@ldesplat

This comment has been minimized.

Copy link
Contributor

@ldesplat ldesplat commented Aug 17, 2015

Yes, you are right. Looks like this can be useful. I want to add more support for OpenID Connect and this is part of it so I would appreciate a PR if you need it right now :)

https://github.com/hapijs/bell/blob/master/lib/oauth.js#L149

Looks like, we can easily add it. Just not sure, how to pass it to the endpoint (query params? cookie? ...)

@ldesplat ldesplat added the request label Aug 17, 2015
@AdriVanHoudt

This comment has been minimized.

Copy link
Contributor Author

@AdriVanHoudt AdriVanHoudt commented Aug 17, 2015

I need the query params so I would do Hoek.merge(query, request.query) there

@ldesplat ldesplat added the support label Aug 17, 2015
@ldesplat

This comment has been minimized.

Copy link
Contributor

@ldesplat ldesplat commented Aug 17, 2015

I am trying to think about this. What would be a good API for this.

Yes, I think that's the right way, but maybe enforcing the spec may be safer http://openid.net/specs/openid-connect-core-1_0.html#AuthRequest . By that I mean, only allowing the options defined in the spec.

I am also trying to decide if maybe we should create a new protocol (currently OAuth 1, OAuth 2 and now OpenID Connect) based on OAuth 2 but adding OpenID specific things to it, or just when scope specifies 'openid' then we do OpenID specific things. Those dynamic parameters is one of them.

Maybe, we can have a new OpenID implementation with just that feature included. And add OpenID functionality gradually in minor releases.

@ldesplat ldesplat added this to the 5.1.0 milestone Aug 17, 2015
@AdriVanHoudt

This comment has been minimized.

Copy link
Contributor Author

@AdriVanHoudt AdriVanHoudt commented Aug 18, 2015

I am all for implementing the spec and using OpenID (as far as I know about from reading the intro and your link :P) but this does not help me in particular since my auth provider is not following the spec by having it's own query params.
Would you still accept a PR for the merge thing?

@ldesplat

This comment has been minimized.

Copy link
Contributor

@ldesplat ldesplat commented Aug 18, 2015

Yes, I would love a PR for it.

I don't want a blind merge though. If we could specify a setting in providers to say which params are dynamic and then only merge those. I would accept that.

Ideally, the application would set a signed cookie with the param names and values that we then retrieve and use. At least, we could ensure our application has approved those values. But maybe it's not necessary since at this step of the auth, we are just using our clientId and not our clientSecret. But, this makes it much harder to use as well.

Thanks!

@AdriVanHoudt

This comment has been minimized.

Copy link
Contributor Author

@AdriVanHoudt AdriVanHoudt commented Aug 19, 2015

Ok so it would look like

  • declare allowed runtime query params in config
  • send request to bell route with extra query params
  • check if all are allowed (fail when one is not or just ignore it? Joi would be an option for this, like allowing to specify a schema to test against)
  • merge the extra params with current query (override runtime with preconfigured or other way around?)
  • store query in cookie (like now or separated in some runtime property?)
  • send auth request
@ldesplat

This comment has been minimized.

Copy link
Contributor

@ldesplat ldesplat commented Aug 19, 2015

check if all are allowed (fail when one is not or just ignore it? Joi would be an option for this, like allowing to specify a schema to test against)

I would prefer we fail if the checks fail

merge the extra params with current query (override runtime with preconfigured or other way around?)

The runtime ones take over but only if they are allowed as runtime params :)

store query in cookie (like now or separated in some runtime property?)

Unfortunately, that's too late to do so. At this stage, we should be verifying that the cookie match the query. Maybe providing a callback function that the application can implement where they are given the query params and values along with the current request. Then the application can verify the correctness by either using a cookie or going to fetch some state somewhere else.

I think it's best to leave this part alone for now and we can implement it later.

Thank you very much.

@AdriVanHoudt

This comment has been minimized.

Copy link
Contributor Author

@AdriVanHoudt AdriVanHoudt commented Aug 19, 2015

Fail seems good, runtime nly taking over if allowed is done with the previous check

And leave what exactly alone?
Let me do a PR and then we can go from there

AdriVanHoudt added a commit to AdriVanHoudt/bell that referenced this issue Aug 20, 2015
…auth url // allows runtime changing query params // fixes hapijs#125
AdriVanHoudt added a commit to AdriVanHoudt/bell that referenced this issue Aug 20, 2015
…auth url // allows runtime changing query params // fixes hapijs#125
AdriVanHoudt added a commit to AdriVanHoudt/bell that referenced this issue Aug 22, 2015
…s to be send to the auth url // fixes hapijs#125

specifically set allowunknown to false

rtfm

removed validation // added option allowRuntimeQueryParams to not introduce unexpected behaviour // added docs // reverted some style changes

more logical test // fixed tests indices

cleanup

better explanation

renamed setting // updated doc // allow runtime params on oauth1
ldesplat added a commit that referenced this issue Aug 22, 2015
added allowRuntimeProviderParams to allow runtime query params 
Closes #125
@Marsup Marsup added feature and removed request labels Sep 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.