fix(authentication-oauth): Fix bug introduced by PR #2631

[cciocov]

This fixes a bug introduced with PR #2631 , which makes it impossible to override Grant defaults.

[johnheenan]

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.

[cciocov]

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.

[johnheenan]

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?

[cciocov]

I need to do some tests with _.defaultsDeep(object, [sources]) before I address this further. Are sources being deep copied in order or are they in fact merged before all being deep copied to object or not?

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 grant.defaults.origin is to define oauth.defaults.host and oauth.defaults.protocol. If I simply define oauth.defaults.origin it's not being picked up, and this is the bug I'm fixing.

The same will be the case for response, which in the current code is hardcoded to ['tokens', 'raw', 'profile'] and cannot be set from the app's config.

In fact, the only reason we're able to change prefix, host, and protocol is that we're using oauth.defaults.prefix, oauth.defaults.host and oauth.defaults.protocol as defaults when setting the defaults themselves... which kinda makes no sense if you think about it :).

[cciocov]

@johnheenan the configuration you outlined here should work just fine with #2659

But instead of defining oauth.defaults.host and oauth.defaults.protocol you should also be able to just define oauth.defaults.origin as https://some.website.io.

[cciocov]

@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.

[johnheenan]

@cciocov @daffl

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 "a": { "b": 3, "c": 3, "d": 3 } for _.defaultsDeep({}, x, y, z)'
with result "a": { "b": 3 } for _.defaultsDeep({}, x, {y, z})

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
      }
    }
  }
}
*/

[cciocov]

?

[daffl]

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.

[cciocov]

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"
    }
  }
}

[daffl]

Great, that looks like the correct fix and order for merging those parameters to me. Thank you for fixing that!