Validate User model config at app boot #3265

ebarault · 2017-03-09T12:44:16Z

Description

Add a hasMultipleUserModelsConfig property on loopback application to reflect the User setup throughout the application
Perform sanity check on the go on user model config to detect and warn the user (and eventually stop the application in case the config is not workable)

This continues the work started by @bajtos in #3231
First draft provided for general discussion.

Related issues

#3140 (warn on misconfigured user/accessToken model relations)
#3231 (enable multiple owner $owner resolving)
#2971 (enable loopback to handle multiple user models)

Checklist

New tests added or existing tests modified to cover all changes
Code conforms with the style
guide

ebarault · 2017-03-09T13:23:17Z

lib/application.js

-      scheduleVerification(Model, verifyUserRelations);
-    }
+  verifyUserModelsSetup().then(()=> {
+    verifyRelationsSetup();


general idea of why not make the whole two together:

first we detect the user models config status

then we can use it in verifyRelationsSetup to even go further in the bad config detection since things get to change according to one config or the other

Sounds good.

Code-wise, this line can be simplified to verifyUserModelsSetup.then(verifyRelationsSetup).

ebarault · 2017-03-09T13:23:28Z

lib/application.js

-      scheduleVerification(Model, verifyUserRelations);
-    }
+  verifyUserModelsSetup().then(()=> {
+    verifyRelationsSetup();


general idea of why not make the whole two together:

first we detect the user models config status

then we can use it in verifyRelationsSetup to even go further in the bad config detection since things get to change according to one config or the other

ebarault · 2017-03-09T13:25:09Z

lib/application.js

+        });
+      }
+    });
+  }


i needed to change @bajtos first implem to add async support as we now expect on this function to resolve when done, so we can use it to first check user models config and then check how the relations are set up

bajtos · 2017-03-21T13:00:10Z

lib/application.js

+    // we use Promise.reflect() here to let all the tests go without throwing,
+    // this way we can get all the logs for all models we are checking
+    return Promise.map(userModels, (model) => {
+      return scheduleVerification(model, hasMultipleUserModelsConfig).reflect();


This can be simplified (although I am not sure if the line won't become too long):

return Promise.map(userModels, model => scheduleVerification(model, hasMultipleUserModelsConfig).reflect());

bajtos · 2017-03-21T13:00:28Z

lib/application.js

+    .then((inspections) => {
+      // proceed to next checkups if no error
+      let hasErrors = inspections.filter(inspection => {
+        return inspection.isRejected();


ditto:

inspections.filter(inspection => inspection.isRejected());

bajtos · 2017-03-21T13:01:06Z

lib/application.js

+        );
+      });
+
+      delete self.hasMultipleUserModelsConfig;


Since this is in an arrow function =>, you should be able to use this. Arrow functions preserve the value of this reference.

bajtos · 2017-03-21T13:01:31Z

lib/application.js

+
+      delete self.hasMultipleUserModelsConfig;
+
+      if (!dismissRejection) {


I think this is another example of a double-negative.

bajtos · 2017-03-21T13:02:20Z

lib/application.js

+      if (Model.dataSource) {
+        try {
+          resolve(verifyFn(Model));
+        } catch (e) {


I believe the Promise constructor is required to catch errors thrown by the factory function, therefore it shouldn't be needed to catch them ourselves.

no, i checked. If i just do resolve(veryFn(Model))
then the verifyFn throws an error, the following error will be caught when trying to reflect the inspection result in scheduleVerification(model, hasMultipleUserModelsConfig).reflect();:

TypeError: cannot get fulfillment value of a non-fulfilled promise

That's weird. This is what Bluebird's docs say about the error:

https://github.com/petkaantonov/bluebird/blob/master/docs/docs/error-explanations.md#error-cannot-get-fulfillment-value-of-a-non-fulfilled-promise

You can get this error when you're trying to call .value or .error when inspecting a promise where the promise has not been fulfilled or rejected yet.

For example:

var p = Promise.delay(1000); p.inspect().value();

Consider using .isPending() .isFulfilled() and .isRejected() in order to inspect the promise for status.

This looks like a possible bug in bluebird to me.

nevermind... thanks for digging, the error was on my side...

ebarault · 2017-03-23T09:25:42Z

lib/application.js

+  return this._verifyAuthModelRelations(app._warnOnBadAuthModelRelations)
+    .then(() => {
+      this.isAuthEnabled = true;
+    });


as _verifyAuthModelRelations is now async and returns a promise, this change is required
also i made enableAuth() return a promise to, as it's de facto async too now, it's not a breaking change to me, the method can still be called in an app while not caring about the returned promise if most if not all use cases.

This is useful in the tests so we can wait for enableAuth to return before checking the displayed warnings

I am quite concerned about this change. We use app.isAuthEnabled in other places (e.g.

loopback/server/middleware/rest.js

Lines 51 to 54 in c3ab932

if (app.isAuthEnabled) {

var AccessToken = registry.getModelByType('AccessToken');

handlers.push(loopback.token({model: AccessToken, app: app}));

}

), but possibly in any 3rd party LoopBack code to detect whether the app has authentication enabled. While our REST middleware makes this check few ticks later (at which time this promise chain has already finished), I think it's not safe to assume that everybody else is doing the same.

Having said that, I agree that making enableAuth async & promise returning is a good direction.

What is your reasoning for enabling isAuthEnabled only after auth-model relations have been verified? What would happen if we keep only _verifyAuthModelRelations async and set app.isAuthEnabled right on the first tick, as we are doing now?

BTW in the current proposal, the app will end up in an inconsistent state when _verifyAuthModelRelations -models and the remoting authorization hook are all set up, but the flag is saying otherwise.

Yes, i agree this is the main point to discuss.

In loopback i think this is not a problem since the features/modules relying on this flag comes few ticks later, but i agree we should care about 3rd party apps.
I dropped this implementation quickly to make this visible and get your feedback.

In fact i noted when implementing the tests that i could not easily test directly the _verifyAuthModelRelations method alone since it require the auth models and relations are all set up. Which is usually a benefit of using enableAuth(), so i needed to call _verifyAuthModelRelations through enableAuth, and then way for the latter's returned promise to resolve to properly get the warning messages. So at first i just modified enableAuth to return a promise. Then i considered the meaning of the flag and decided to set it after the health check has resolved.

The rationale:
Leaving aside backward compat for a sec: a valid auth setup health check function would not result in setting an isAuthEnabled flag true if the auth model was incorrect is some ways. We could project for v4 that the app deliberately stops in situations that now trigger only warnings, until the user fixes the issues or activate some failover flags to bypass the situation. Thoughts?

👉 For now: Setting app.isAuthEnabling to true before the call _verifyAuthModelRelations would probably not have a big impact actually and could keep a variant where it still set it sync and let the method return async after the checks have completed:

either the relations are not well set for single user model and the flag will be set and the app will warn, this is the current behavior, also this does not mean that the flag should actually be set since the app is not configured properly

or the multiple users config if not good (including relations), the flag will have been set but the app will crash, so there's not risk in setting the flag sync

So this could be

// ... // still sync up to this point this.isAuthEnabled = true; return this._verifyAuthModelRelations(app._warnOnBadAuthModelRelations); // async }

As the enableAuth() method currently returns nothing, there's no risk that it will conflict in existing 3rd party apps.

Of course we can additionaly already implement the target implement with a flag and set the flag only latter and decide to halt the app if warnings occur with a feature flag (wrapping your other proposal about an option to return sync or async)

I dropped this implementation quickly to make this visible and get your feedback.

👍

Leaving aside backward compat for a sec: a valid auth setup health check function would not result in setting an isAuthEnabled flag true if the auth model was incorrect is some ways. We could project for v4 that the app deliberately stops in situations that now trigger only warnings, until the user fixes the issues or activate some failover flags to bypass the situation. Thoughts?

I agree that if the backwards compatibility was not an issue (e.g. in v4), then enableAuth should fail when the configuration is invalid.

// still sync up to this point this.isAuthEnabled = true; return this._verifyAuthModelRelations(app._warnOnBadAuthModelRelations); // async

👍 let's take this approach now.

ebarault · 2017-03-23T09:26:56Z

lib/application.js

+  const warnOnBadSetup = this.get('_warnOnBadAuthModelsSetup') !== false;
+  // Prevent the app from being killed although the configuration is inconsistent
+  const abortOnBadUserSetup = this.get('_abortOnBadUserSetup') !== false;



reworded the flags to avoid double negations

ebarault · 2017-03-23T09:33:28Z

lib/application.js

-        Model.modelName, userName, userName);
+    const modelTo = (relationsConfig.user || {}).model;
+    if (modelTo) {
+      warnFn('CUSTOM_USER_MODEL_NOT_AVAILABLE', {modelFrom, modelTo});


i suggest changing the var names to modelFrom and modelTo, rather than modelName and userName, as it makes the tests more readable.
if you'd like to keep the original names, i will map the original names so the tests are readable

ebarault · 2017-03-23T09:33:53Z

lib/application.js

+    if (modelTo) {
+      warnFn('CUSTOM_ACCESS_TOKEN_MODEL_NOT_AVAILABLE', {modelFrom, modelTo});
+      return;
+    }


ebarault · 2017-03-23T09:34:47Z

lib/application.js

+};
+
+// function warnOnBadAuthModelRelations(code, args) {
+app._warnOnBadAuthModelRelations = function(code, args) {


built-in warning function ("private")
i actually need it to be attached on the app object so i can swap it in the tests when calling enableAuth()

Have you considered passing the custom implementation via enableAuth options instead?

Not a big deal though.

bajtos · 2017-03-23T12:23:15Z

lib/application.js

-  this._verifyAuthModelRelations();
-
-  this.isAuthEnabled = true;
+  return this._verifyAuthModelRelations(app._warnOnBadAuthModelRelations)


In existing applications that are not waiting for enableAuth to resolve, when the verification code throws an error, the rejected promise will end up as unhandled promise rejection. This typically produces only a console warning, it does not crash the application as an uncaught error would.

(node:90355) UnhandledPromiseRejectionWarning: Unhandled promise rejection (rejection id: 3): Error

I am proposing to introduce an options flag to control the sync/async behaviour. For example:

// old way - throws error app.enableAuth({dataSource: 'db'}); // new way - returns rejected promise app.enableAuth({dataSource: 'db', async: true}) .then(...) .catch(...); // alternatively, replace the flag with a new method name app.enableAuthAsync({dataSource: 'db'});

(The "Async" prefix is already used by Bluebird's promisify API)

bajtos · 2017-03-23T12:32:06Z

test/app.test.js

-      app.enableAuth();
-      expect(app.isAuthEnabled).to.equal(true);
+      return app.enableAuth().then(() => {
+        expect(app.isAuthEnabled).to.equal(true);


This goes back to my first comment https://github.com/strongloop/loopback/pull/3265/files#r107652656.

When we have to change a test in a significant way, as we are doing here, then I think it's a very strong sign that we are breaking backwards compatibility.

bajtos · 2017-03-23T12:32:43Z

test/app.test.js

-      app.enableAuth({dataSource: 'db'});
-
-      expect(Object.keys(app.models)).to.include.members(AUTH_MODELS);
+      return app.enableAuth({dataSource: 'db'}).then(() => {


This change shouldn't be needed, because we are attaching the models synchronously in the current implementation. Unless I am missing something?

bajtos · 2017-03-23T12:32:57Z

test/app.test.js

      AccessToken.settings.relations.user.model = 'Customer';

-      app.enableAuth({dataSource: 'db'});
+      return app.enableAuth({dataSource: 'db'}).then(() => {


ditto, I think this change is not needed.

bajtos · 2017-03-23T12:35:21Z

test/user.test.js

      // setup a new LoopBack app, we don't want to use shared models
      app = loopback({localRegistry: true, loadBuiltinModels: true});
-      app.set('_verifyAuthModelRelations', false);
+      app.set('_warnOnBadAuthModelsSetup', false);


Would you mind reverting this rename? I don't really care about the name, it's just to avoid code churn.

it would mean that it's the exact same name as the app method
app. _verifyAuthModelRelations()
is it problematic?

the rationale for this change is: the _verifyAuthModelRelations method, for a same name, has now a larger scope than the equally named flag

it would mean that it's the exact same name as the app method

That shouldn't matter, because the flag is stored in a config hash-map.

the rationale for this change is: the _verifyAuthModelRelations method, for a same name, has now a larger scope than the equally named flag

I see your point, let's make the proposed change then.

stale · 2017-08-23T16:07:29Z

This issue has been closed due to continued inactivity. Thank you for your understanding. If you believe this to be in error, please contact one of the code owners, listed in the CODEOWNERS file at the top-level of this repository.

first commit for general discussion

8441475

0candy added the #wip label Mar 9, 2017

crandmck added the review label Mar 9, 2017

bajtos self-requested a review March 9, 2017 12:45

bajtos assigned ebarault Mar 9, 2017

ebarault changed the title ~~first commit for general discussion~~ Detect User model config at app boot Mar 9, 2017

ebarault commented Mar 9, 2017

View reviewed changes

bajtos reviewed Mar 21, 2017

View reviewed changes

starting unit tests

a657e8a

ebarault commented Mar 23, 2017

View reviewed changes

bajtos reviewed Mar 23, 2017

View reviewed changes

ebarault changed the title ~~Detect User model config at app boot~~ Validate User model config at app boot Mar 24, 2017

ebarault mentioned this pull request Mar 24, 2017

Add a new property to model to store user-desired relations config loopbackio/loopback-datasource-juggler#1282

Closed

1 task

superkhau added in-progress and removed #wip in-progress labels Mar 28, 2017

bajtos mentioned this pull request Jul 13, 2017

Fix OWNER role to handle multiple relations #3140

Merged

stale bot added the stale label Aug 23, 2017

stale bot closed this Aug 23, 2017

0candy removed the review label Aug 23, 2017


		delete self.hasMultipleUserModelsConfig;

		if (!dismissRejection) {

	if (app.isAuthEnabled) {
	var AccessToken = registry.getModelByType('AccessToken');
	handlers.push(loopback.token({model: AccessToken, app: app}));
	}

Validate User model config at app boot #3265

Validate User model config at app boot #3265

Uh oh!

Conversation

ebarault commented Mar 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related issues

Checklist

Uh oh!

ebarault Mar 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ebarault Mar 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ebarault Mar 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ebarault Mar 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stale bot commented Aug 23, 2017

Uh oh!

ebarault commented Mar 9, 2017 •

edited

Loading

ebarault Mar 9, 2017 •

edited

Loading

ebarault Mar 23, 2017 •

edited

Loading

ebarault Mar 23, 2017 •

edited

Loading

ebarault Mar 23, 2017 •

edited

Loading