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

added allowRuntimeProviderParams to allow runtime query params #129

Merged
merged 1 commit into from Aug 22, 2015

Conversation

@AdriVanHoudt
Copy link
Contributor

AdriVanHoudt commented Aug 20, 2015

to be send to auth url // allows runtime changing query params // fixes #125

we can discuss naming ;)

@AdriVanHoudt

This comment has been minimized.

Copy link
Contributor Author

AdriVanHoudt commented Aug 20, 2015

2 things:

  • tests already failed with a clean clone (not sure why it is failing)
  • why not just use the built in lint config of Lab?
@ldesplat ldesplat added the feature label Aug 20, 2015
@ldesplat ldesplat added this to the 5.1.0 milestone Aug 20, 2015
@ldesplat ldesplat closed this Aug 20, 2015
@ldesplat ldesplat reopened this Aug 20, 2015
@AdriVanHoudt

This comment has been minimized.

Copy link
Contributor Author

AdriVanHoudt commented Aug 20, 2015

so any thoughts on this?

query.scope = scope.join(settings.provider.scopeSeparator || ' ');
}
// Validate runtime query params
return Joi.validate(request.query, settings.runtimeQuerySchema, function (err, value) {

This comment has been minimized.

Copy link
@ldesplat

ldesplat Aug 20, 2015

Contributor

I know it's the default but can you explicitly state { allowUnknown: false } in the Joi.validate options.

This comment has been minimized.

Copy link
@AdriVanHoudt

AdriVanHoudt Aug 20, 2015

Author Contributor

will do

@ldesplat

This comment has been minimized.

Copy link
Contributor

ldesplat commented Aug 20, 2015

@AdriVanHoudt It looks good to me. I'll try tomorrow with the google provider. Please provide some documentation :)

@hueniverse Can you please review this? I really want to make sure we're not introducing a security issue here. My main worry is that we're not verifying the values sent are what the application wanted to send. I don't think it's an issue here but still would like an expert to review this!

@AdriVanHoudt

This comment has been minimized.

Copy link
Contributor Author

AdriVanHoudt commented Aug 20, 2015

@ldesplat ok, was holding off until i was sure it was good

@AdriVanHoudt AdriVanHoudt force-pushed the AdriVanHoudt:runtime-queryparams branch from 4857da8 to 4c1e6e3 Aug 20, 2015
@AdriVanHoudt

This comment has been minimized.

Copy link
Contributor Author

AdriVanHoudt commented Aug 20, 2015

adding allowUnknown seems to throw for some reason @Marsup ?

@ldesplat

This comment has been minimized.

Copy link
Contributor

ldesplat commented Aug 20, 2015

Well I am dumb sometimes. Even if we verify the content it does not matter because the client is redirected so an attacker can do whatever they want with it.

I wonder if your original idea of just hoek.merge all the world is the right way to do it. The checks I've made you do are useless...

@ldesplat

This comment has been minimized.

Copy link
Contributor

ldesplat commented Aug 20, 2015

@AdriVanHoudt You need to put the options after the schema.

@AdriVanHoudt

This comment has been minimized.

Copy link
Contributor Author

AdriVanHoudt commented Aug 20, 2015

derp :P updated now

@ldesplat

This comment has been minimized.

Copy link
Contributor

ldesplat commented Aug 20, 2015

I'll merge this (with your documentation) after the weekend if nothing bad comes up (in comments here or my testing).

: (a[1] > b[1] ? 1 : 0))));
: (a[0] > b[0] ? 1
: (a[1] < b[1] ? -1
: (a[1] > b[1] ? 1 : 0))));
});

This comment has been minimized.

Copy link
@hueniverse

hueniverse Aug 20, 2015

Member

More readable the way it was.

internals.encode(baseUri) + '&' +
internals.encode(normalizedParam);
internals.encode(baseUri) + '&' +
internals.encode(normalizedParam);

This comment has been minimized.

Copy link
@hueniverse

hueniverse Aug 20, 2015

Member

More readable the way it was.

query.scope = scope.join(settings.provider.scopeSeparator || ' ');
}
// Validate runtime query params
return Joi.validate(request.query, settings.runtimeQuerySchema, { allowUnknown: false }, function (err, value) {

This comment has been minimized.

Copy link
@hueniverse

hueniverse Aug 20, 2015

Member

Rework this without using the validate callback (use the returned value format instead). It's easier to read.

@hueniverse

This comment has been minimized.

Copy link
Member

hueniverse commented Aug 20, 2015

I am not following the requirements here. Why set up a joi schema? I understand supporting per-request query additions but this feels complex for no reason. Are you then going to document every provider's additional query params?!

@ldesplat

This comment has been minimized.

Copy link
Contributor

ldesplat commented Aug 20, 2015

@hueniverse Thanks for reviewing it.

I did not want a user attempt to use the login endpoint with extra parameters that we may not want to pass along. But we can't protect against that obviously since the whole oauth flow is not designed so.

ie. I am not used to taking query params and passing them along with no verification. But in this case it makes sense.

@AdriVanHoudt I am sorry, you can remove the verification of parameters.

@AdriVanHoudt

This comment has been minimized.

Copy link
Contributor Author

AdriVanHoudt commented Aug 21, 2015

no problem, will make the changes according to the comments
Also the indent thing is just me indenting the whole file the same (habit, will change it back)

@AdriVanHoudt

This comment has been minimized.

Copy link
Contributor Author

AdriVanHoudt commented Aug 21, 2015

should be done now, let me know your feedback and if it is ok I will squash for you ;)

README.md Outdated
@@ -113,6 +113,7 @@ Each strategy accepts the following optional settings:
- Facebook supports `display` ('page', 'popup', or 'touch'), `auth_type`, `auth_nonce`.
- Google supports `access_type`, `approval_prompt`, `prompt`, `login_hint`, `user_id`, `hd`.
- Twitter supports `force_login`, `screen_name`.
- `allowRuntimeQueryParams` - will allow you to pass along query params to the auth request that change on every request. Bassicly merging the query params you pass along with the predefined ones. Be aware that this will override predefined query params! Defaults to `false`.

This comment has been minimized.

Copy link
@hueniverse

hueniverse Aug 21, 2015

Member

Should be edited to be more consistent with the other text. E.g. 'allows passing query parameters...'

@@ -55,6 +55,7 @@ internals.schema = Joi.object({
ttl: Joi.number(),
domain: Joi.string().allow(null),
providerParams: Joi.object(),
allowRuntimeQueryParams: Joi.boolean().default(false),

This comment has been minimized.

Copy link
@ldesplat

ldesplat Aug 21, 2015

Contributor

We've only implemented it for oauth2 so adding .when('provider.protocol', { is: 'oauth2', otherwise: Joi.forbidden() }) might be useful so people don't wonder why it is not working.

This comment has been minimized.

Copy link
@ldesplat

ldesplat Aug 22, 2015

Contributor

Optional: Renaming it to allowRuntimeProviderParams might make the documentation easier since they are related.

This comment has been minimized.

Copy link
@AdriVanHoudt

AdriVanHoudt Aug 22, 2015

Author Contributor

do you also want it for oath1? (not used it so not sure if it applies)

README.md Outdated
@@ -113,6 +113,7 @@ Each strategy accepts the following optional settings:
- Facebook supports `display` ('page', 'popup', or 'touch'), `auth_type`, `auth_nonce`.
- Google supports `access_type`, `approval_prompt`, `prompt`, `login_hint`, `user_id`, `hd`.
- Twitter supports `force_login`, `screen_name`.
- `allowRuntimeQueryParams` - allows passing query parameters to the auth request by adding them to the first call to the endpoint. It will merge the query params you pass along with the predefined ones. Be aware that this will override predefined query params! Defaults to `false`.

This comment has been minimized.

Copy link
@ldesplat

ldesplat Aug 22, 2015

Contributor

Maybe?
A boolean that allows passing query parameters from a bell protected endpoint to the auth request. It will merge the query params you pass along with the providerParams and any other predefined ones. Be aware that this will override all other query parameters! Default to false.

@ldesplat

This comment has been minimized.

Copy link
Contributor

ldesplat commented Aug 22, 2015

Thanks! This looks great. Tried it out with the google provider and works great. I am ready to merge when you squash it :)

…s to be send to the auth url // fixes #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
@AdriVanHoudt AdriVanHoudt force-pushed the AdriVanHoudt:runtime-queryparams branch from f2d6092 to b5438bc Aug 22, 2015
@AdriVanHoudt AdriVanHoudt changed the title added runtimeQueryScheme to allow runtime query params added allowRuntimeProviderParams to allow runtime query params Aug 22, 2015
@AdriVanHoudt

This comment has been minimized.

Copy link
Contributor Author

AdriVanHoudt commented Aug 22, 2015

Ok squashed and allowed on oauth1

@ldesplat

This comment has been minimized.

Copy link
Contributor

ldesplat commented Aug 22, 2015

You forced me to read the oauth1 spec finally!

Looks like it's allowed though I have not found any providers allowing extra parameters just yet. It looks good so I am merging it!

Thank you so very much for this PR and bearing with me!

ldesplat added a commit that referenced this pull request Aug 22, 2015
added allowRuntimeProviderParams to allow runtime query params 
Closes #125
@ldesplat ldesplat merged commit 9fffd61 into hapijs:master Aug 22, 2015
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@ldesplat

This comment has been minimized.

Copy link
Contributor

ldesplat commented Aug 22, 2015

It's in the 5.1.0 release.

@AdriVanHoudt

This comment has been minimized.

Copy link
Contributor Author

AdriVanHoudt commented Aug 22, 2015

haha sorry 😜 @hueniverse will be happy (as far as I recall he helped write it)

And no problem, it was my pleasure

@AdriVanHoudt AdriVanHoudt deleted the AdriVanHoudt:runtime-queryparams branch Aug 22, 2015
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.