Inject remoting context to options arg [2.x]

[bajtos]

Backport of #3023

Connect to #1495

cc @fabien @PaddyMann @ebarault

[bajtos]

Quoting from #1495 (comment)

When this update is published, is there a possibility it will break 2.x projects using the workaround that already uses options?

and #1495 (comment)

The main reason for back-porting this new context propagation to 2.x is to allow the recently added access-token invalidation skip the access token of the current user making the request, which I consider as an important bug to be fixed (see #3034).
I am happy to discuss how to minimise the impact of this back-ported change on existing LoopBack 2.x applications.

In the light of the discussion we have with @doublemarked in #3042, it may be best to introduce a feature flag enabling/disabling the new context-propagation feature, so that we don't break existing applications using a workaround solution that was described in #1495.

On the other hand, such flag would complicate the fix for #3034, as the fix as I am envisioning it right now will rely on options object being set by remote calls. I suppose we could add another feature flag to disable access-token invalidation, but adding so many flags feels like over engineering to me.

To be honest, I personally consider manipulation of remote method arguments in application code as an unsupported usage of the framework. If we allowed applications to arbitrarily manipulate remoting metadata, then any change of remoting metadata in built-in LoopBack models would become a semver-major change, because we won't be able to tell if there is any app out there relying on the exact remoting metadata before our change.

Therefore I am personally inclining to land this pull request as it is now, and let users fix their context-propagation workarounds to work correctly with the new remoting metadata (ideally by using the new feature we are adding by this patch).

@tvdstaaij thoughts?

cc @ritch @raymondfeng @superkhau

[fabien]

@bajtos can we please have a flag to enable/disable context propagation? For now, I'm relying on my own workaround, until I have a chance to use your implementation.

[bajtos]

I'm leaving for vacation today, let's put this pull request on hold and revisit the discussion in January when I get back.

[fabien]

@bajtos sure - have a great vacation and nice holidays!

[tvdstaaij]

Agreed on the flag(s). I'll try to address your doubts below.

I suppose we could add another feature flag to disable access-token invalidation, but adding so many flags feels like over engineering to me.

In general it is, but I assume this would be only for 2.0. Seeing how it is unlikely that new applications will be developed with 2.0, I don't think this is much of a problem (from a user perspective at least; I don't know how much it would complicate the framework code).

It's also good to realize that existing 2.0 apps were built and tested without builtin access token invalidation. It's not just some security patch or backwards compatible feature but rather something that changes the behavior of the framework. When developing my Loopback 2 application I noticed that there was no builtin token invalidation logic, so I defined custom password change and token invalidation logic using operation hooks (relying on the aforementioned context workaround). I would assume other developers did something similar if token invalidation matters for their applications. There's no telling what would happen if a framework-defined implementation would suddenly be mashed onto an application code implementation.

To be honest, I personally consider manipulation of remote method arguments in application code as an unsupported usage of the framework. If we allowed applications to arbitrarily manipulate remoting metadata, then any change of remoting metadata in built-in LoopBack models would become a semver-major change, because we won't be able to tell if there is any app out there relying on the exact remoting metadata before our change.

Sure, it's technically an unsupported framework usage (a.k.a. "hack"), but in this particular case there was no supported way to do what was necessary. As you are aware the remote context implementation at the time was horribly broken and the docs warned not to use it. Some apps already relying on the old implementation suddenly broke, and some newer apps simply needed remote context for advanced features. Back then I wasn't eager to use the unofficial workaround at all, but when reviewed my options it was really the only viable solution.

So technically it's unsupported framework usage, but realistically developers just had to rely on this particular piece of "private" behavior.

Again, because this is an LTS branch, I expect that the majority of applications have already adapted some kind of solution that works for that application, and are not in immediate need of either feature (new context propagation method and token invalidation). Therefore I believe that pushing a change like this without being optional potentially causes more harm than good.

If a developer decides to do some maintenance on their Loopback 2 application and wants to migrate to the new context propagation, flipping a configuration switch is trivial. But as long as only minor/patch is upgraded and no configuration is changed, the framework behavior is expected not to change.

[bajtos]

@tvdstaaij thank you for a thoughtful comment!

If a developer decides to do some maintenance on their Loopback 2 application and wants to migrate to the new context propagation, flipping a configuration switch is trivial. But as long as only minor/patch is upgraded and no configuration is changed, the framework behavior is expected not to change.

I agree this is an important point to keep in mind.

I assume this would be only for 2.0. Seeing how it is unlikely that new applications will be developed with 2.0, I don't think this is much of a problem (from a user perspective at least; I don't know how much it would complicate the framework code).

FWIW, based on the emails and GitHub notifications I am seeing, it looks like there are still users building applications on LoopBack 2.x now.

It's also good to realize that existing 2.0 apps were built and tested without builtin access token invalidation. It's not just some security patch or backwards compatible feature but rather something that changes the behavior of the framework. When developing my Loopback 2 application I noticed that there was no builtin token invalidation logic, so I defined custom password change and token invalidation logic using operation hooks (relying on the aforementioned context workaround). I would assume other developers did something similar if token invalidation matters for their applications.

I personally think most people don't realise the importance of token invalidation, and what's even worse, they don't know what they don't know. As framework authors, we need a way how to let these users know that their applications may be vulnerable.

I am thinking about adding a feature flag for token invalidation, and printing a deprecation warning if this flag is not set (i.e. the value is undefined). That way the behaviour of applications does not change after upgrade, yet there is a warning for users that an action is needed on their side. If the users decide the issue is something they have already handled themselves, then they can set this new flag to false to get rid of the deprecation warning.

Thoughts?

[bajtos]

I have added one more commit de34d55 that will be landed only to 2.x. In this commit, I am adding a new model-level setting injectOptionsFromRemoteContext that acts as a feature flag controlling whether built-in methods include "options" argument in their remoting metadata. The flag is disabled by default for backwards compatibility.

Usage:

// common/models/customer.json
{
  "name": "Customer",
  "base": "User",
  "injectOptionsFromRemoteContext": true,
  "properties": {
    // ...
  }
}

@fabien @PaddyMann @ebarault @tvdstaaij @doublemarked thoughts?

@superkhau @ritch PTAL at the proposed code changes

[bajtos]

One more commit to implement the new feature flag for sharedCtor too: 23bb38d

[fabien]

I am thinking about adding a feature flag for token invalidation, and printing a deprecation warning if this flag is not set (i.e. the value is undefined).

+1 Would be great!

[bajtos]

I am thinking about adding a feature flag for token invalidation, and printing a deprecation warning if this flag is not set (i.e. the value is undefined).

+1 Would be great!

I created a new issue to keep track of this work, see #3068.

Pull requests are welcome!

[bajtos]

I am thinking about adding a feature flag for token invalidation, and printing a deprecation warning if this flag is not set (i.e. the value is undefined).

See #3109

[bitmage]

I'm trying to utilize this to implement 1) multi-tenancy, 2) caching.

I have to say it's pretty confusing at this point... I've spent the last 4 days combing through issue tracker threads and trying a variety of methods including getCurrentContext(), middleware, app.remotes().phases.addBefore, and app.remotes().before('*.*').

I finally was able to inject standard tenancy parameters into the options arg in a consistent way using the last approach. And then I went to combine this with my caching middleware, and found that they run in the wrong order.

I guess what I would want is just more consistency. It seems like there should be 1 general middleware implementation, and everything else should fall out from that. And the ability to have some kind of introspection/printout is a god send as well, so I can see the middleware stacks that got created for my endpoints.

I'm on Loopback 2.x at the moment, as I saw some stability issues with generators in 3.x last I tried. Was middleware cleanup a major thrust of 3.x? Would I benefit from an upgrade? Is there any other cleanup/standardization/documentation effort in the works?

[bajtos]

And the ability to have some kind of introspection/printout is a god send as well, so I can see the middleware stacks that got created for my endpoints.

I can see how this can be useful! I don't think we have any such tool in LoopBack core, have you found any community-maintained one? Would you like to start one yourself?

I'm on Loopback 2.x at the moment, as I saw some stability issues with generators in 3.x last I tried. Was middleware cleanup a major thrust of 3.x? Would I benefit from an upgrade? Is there any other cleanup/standardization/documentation effort in the works?

IIRC, there was no significant changes related to middleware in LoopBack 3.0. The release was focused mostly on cleanup tasks that break backwards compatibility, see https://loopback.io/doc/en/lb3/3.0-Release-Notes.html. The subsequent 3.x releases contain mostly security-related improvements.