-
-
Notifications
You must be signed in to change notification settings - Fork 751
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
fix(authentication-oauth): Fix bug introduced by PR #2631 #2659
Conversation
Your fix brings back the regression bug I fixed and tested I had fixed. The problem is that the defaults, as labelled, are not complete and cannot be finalised until just before passed to Grant. My fix patches the defaults to be correct. |
Hey @johnheenan, could you please explain what the issue was, because I don't understand and the current code doesn't make sense. With the current code, if I define "authentication.oauth.defaults.origin" in my app's config, it doesn't get set in Grant's config, which is clearly a bug. |
I need to do some tests with _.defaultsDeep(object, [sources]) before I address this further. Are sources being defaultsDeeped in order or are they in fact merged before all being defaultDeeped to object? |
Not sure what you mean, but intuitively, the order should be the inverse of the order you'd use with _.merge(). To further explain the issue: with the current code, the only way to set The same will be the case for In fact, the only reason we're able to change |
@johnheenan the configuration you outlined here should work just fine with #2659 But instead of defining |
@johnheenan @daffl I've made some updates to this PR, imo, this should be the "final" code. It should work with John's configuration and allow all Grant settings to be overwritten from app's config. |
I have done some tests and confirm defaultsDeep works as expected, but that it is very easy to set it up incorrectly. Have a look at the test results below with defaultsDeep. Put objects in the wrong place and additional defaults are missing from where they should be. This is a strong clue. Compare result I don't think we can declare with confidence anything has been fixed without testing. John Heenan let _ = require('lodash');
console.log(_.VERSION) // 4.17.21
const showme = obj => console.log(JSON.stringify(obj, null, 2))
const x = { 'a': { 'b': 3 } }
const y = { 'a': { 'b': 2, 'c': 3 } }
const z = { 'a': { 'b': 1, 'c': 2, 'd': 3 } }
showme({ '_.defaultsDeep({}, x, y, z)': _.defaultsDeep({}, x, y, z) })
showme({ '_.defaultsDeep({}, x, {y, z})': _.defaultsDeep({}, x, {y, z}) })
/*
node .\main.js
4.17.21
{
"_.defaultsDeep({}, x, y, z)": {
"a": {
"b": 3,
"c": 3,
"d": 3
}
}
}
{
"_.defaultsDeep({}, x, {y, z})": {
"a": {
"b": 3
},
"y": {
"a": {
"b": 2,
"c": 3
}
},
"z": {
"a": {
"b": 1,
"c": 2,
"d": 3
}
}
}
}
*/ |
? |
I was wondering if that fix wasn't going to introduce other problems. Can both of you please provide a full example configuration and what you'd expect it to do? That way we can write a breaking test and fix it for both cases. |
Hey @daffl , I'd expect this configuration to work, but it currently doesn't because the redirect_uri generated by Grant does not take "oauth.defaults.origin" into account. (This PR fixes this issue) {
"host": "localhost",
"port": 3030,
"public": "./public/",
"paginate": {
"default": 10,
"max": 50
},
"ts": true,
"authentication": {
"oauth": {
"redirect": "https://some.website.io",
"defaults": {
"origin": "https://some.website.io"
},
"google": {
"key": "some_key",
"secret": "some_secret",
"scope": [
"email",
"profile",
"openid"
],
"nonce": true
}
},
"entity": "user",
"service": "users",
"secret": "some_secret",
"authStrategies": [
"jwt",
"local"
],
"jwtOptions": {
"header": {
"typ": "access"
},
"audience": "https://some.website.io",
"issuer": "feathers",
"algorithm": "HS256",
"expiresIn": "1d"
},
"local": {
"usernameField": "email",
"passwordField": "password"
}
}
} |
Great, that looks like the correct fix and order for merging those parameters to me. Thank you for fixing that! |
This fixes a bug introduced with PR #2631 , which makes it impossible to override Grant defaults.