feat(authentication): add support for multiple strategies on same method

[nflaig]

Resolves #5310

This change allows to specify multiple strategies, e.g. @authenticate(['basicAuth', 'localAuth'])

Edit: authenticate decorator signature was updated to @authenticate('basicAuth', 'localAuth')

The logic on how the strategies are executed is similiar to how passportjs does it.

• The authentication strategies will be executed in the provided order.
• If at least one authentication strategy succeeds, the request will be further processed without throwing an error.
• Once a strategy succeeds or redirects, all subsequent strategies will not be evaluated.
• if all strategies fail, a error will be thrown with the error message of the first provided strategy
• it is not feasible to use multiple strategies that redirect (e.g. oauth login redirects) since the first redirect will halt the execution chain

I tried to make this change backwards compatible and therefore I did not rename bindings and it is still possible to set the strategy as a single string.

Edit: changes are no longer backwards compatible, see commit

The API Documentation in code and the Documentation in /docs/site still needs to be updated. And probably some examples need to be updated or added.

Edit: API documentation in code is updated

Checklist

👉 Read and sign the CLA (Contributor License Agreement) 👈

• npm test passes on your machine
• New tests added or existing tests modified to cover all changes
• Code conforms with the style guide
• API Documentation in code was updated
• Documentation in /docs/site was updated
• Affected artifact templates in packages/cli were updated
• Affected example projects in examples/* were updated

👉 Check out how to submit a PR 👈

[raymondfeng]

@nflaig Thank you for the contribution.

[raymondfeng]

Let's improve the user experience here. I would like to see the following styles:

1. Allow one or more strategy names

@authenticate('my-strategy-1', 'my-strategy-2')

2. Allow one or more meta data objects

@authenticate(metaForStrategy1, metaForStrategy2)

3. Allow multiple applications of @authenticate on the same method: (nice-to-have)

@authenticate('my-strategy-1');
@authenticate(metaForMyStrategy2);

The signature can be:

export function authenticate(...strategies: (string | AuthenticationMetadata) []) { ... }

It allows one or more string or AuthenticationMetadata.

Please note that different strategy may have different metadata.

[nflaig]

I did look into that but this means that the changes will be breaking for sure since there are no more options in the decorator, e.g. this will fail now

it('can add authenticate metadata to target method with options', () => {
      class TestClass {
        @authenticate('my-strategy', {option1: 'value1', option2: 'value2'})
        whoAmI() {}
      }

      const metaData = getAuthenticateMetadata(TestClass, 'whoAmI');
      expect(metaData).to.eql({
        strategy: 'my-strategy',
        options: {option1: 'value1', option2: 'value2'},
      });
    });

One more thing I am not sure about is how the metadata for each strategy should be injected into a strategy since it is now an array of metadata object.

@inject(AuthenticationBindings.METADATA)
 private metadata: AuthenticationMetadata[], // this is now an array

The solution could be to iterate through the metadata array and find the according metadata based on the strategy name, maybe a helper function could be added or can this be done in the inject somehow?

[raymondfeng]

Great question.

For @inject, I don't think we can resolve it to the metadata for a given strategy name, which is unknown.

Maybe we have to use AuthenticationMetadata[] as the type and introduce a helper function to get AuthenticationMetadata by strategy name.

[jannyHou]

@nflaig Awesome feature, thank you 👍

Agree with Raymond's suggested signature in comment https://github.com/strongloop/loopback-next/pull/5735/files#r439852618, the new signature should be backward compatible.

Once a strategy succeeds or redirects, all subsequent strategies will not be evaluated.

Since only one strategy will be executed, "it is not feasible to use multiple strategies that redirect (e.g. oauth login redirects) since the first redirect will halt the execution chain" would not be a concern. A failed oauth authentication should not execute the redirect, but let other strategies try instead.

[jannyHou]

according to https://github.com/strongloop/loopback-next/pull/5735/files#r439852618, let's still keep the name as type string. One metadata object configures one strategy.

[nflaig]

@jannyHou I agree, the signature indeed needs to be changed. I will take a look at it once I find the time or maybe someone else wants to look into it.

A failed oauth authentication should not execute the redirect, but let other strategies try instead.

This is unlikely to happen unless there is a configuration or programming error. Usually, strategies such as saml or oauth (oidc) will immediately redirect the user to the identity provider to authenticate the user and once this redirect happens you can not execute further strategies.

One more thing I was wondering about that is unrelated to this PR is that a strategy etiher needs to redirect or return a UserProfile else a error will be thrown. I feel like this limits the amount of strategies you can use since some just don't return anything.

[raymondfeng]

One more thing I was wondering about that is unrelated to this PR is that a strategy etiher needs to redirect or return a UserProfile else a error will be thrown. I feel like this limits the amount of strategies you can use since some just don't return anything.

In case that a strategy does not return a user profile, do you expect it to pass control to other strategies or simply return?

[nflaig]

I think it should simply return and only in case the strategy throws an error it should execute the next strategy in the chain.

An example could be a strategy using api keys which might only check if the key is valid and the key itself might not correlate to a user.

[raymondfeng]

I think it should simply return and only in case the strategy throws an error it should execute the next strategy in the chain.
An example could be a strategy using api keys which might only check if the key is valid and the key itself might not correlate to a user.

So the return value should be UserProfile | Redirect | undefined. That's fine with you as long as we make it clear in documentation.

[raymondfeng]

@nflaig Ping. Do you need further input to move forward?

[nflaig]

@raymondfeng I have few local changes that I did not push yet becasue of the problem mentioned above.

[raymondfeng]

@nflaig Great progress.

For the commit message:

BREAKING CHANGE: signature of authenticate decorator changed, options are no longer a separate input
parameter but instead have to be provided in metadata object if required

We probably need to make it clear there are a few breaking changes:

• The @authenticate signature
• The metadata value is now AuthenticationMetadata[]

[nflaig]

@raymondfeng where exactly should I document the breaking changes? Should I amend the commit message?

[raymondfeng]

where exactly should I document the breaking changes? Should I amend the commit message?

Yes, amending the commit message. Please also make sure related .md files are updated in /docs/site .

[raymondfeng]

@nflaig I have rebased your PR against latest master and pushed it back to your branch.

[raymondfeng]

@nflaig Great UX improvement!

[jannyHou]

Awesome 👍 and appreciate the newly added unit tests.
A few nitpicks to consider, not a blocker for this PR.

[jannyHou]

A reminder for myself: I realized the doc is missing authenticate.skip()'s usage.

[nflaig]

I looked through the documentation and not sure how to go about updating it. There are a lot of code samples which are outdated now and even images that include code.

Probably a new section needs to be added which describes how to use multiple strategies and what kind of implications and limitations exist.

[dhmlau]

Landing this PR as there are 2 approvals already and I'll be switching to DCO soon.

@nflaig, thanks for your contribution. Your PR has landed.
Please open a new PR for any follow up work. Thanks!

[jannyHou]

Thank you @nflaig again for the contribution!

I looked through the documentation and not sure how to go about updating it. There are a lot of code samples which are outdated now and even images that include code.
Probably a new section needs to be added which describes how to use multiple strategies and what kind of implications and limitations exist.

I am submitting a PR to update the auth action doc, and I can add new behaviour for this PR too.