fix custom token model in token middleware

[ebarault]

cc @bajtos @beeman

Description

Specifying a custom AccessToken model via the auth middleware options is broken when the app is using a single global registry.

The faulty line are

if (registry === loopback.registry) {
  TokenModel = options.model || loopback.AccessToken;
} 

where options.model is the model name in the token middleware config, not the model itself
more comments inlined in the code itself

Note: New tests should be added to cover properly the scenario where a custom AccessToken is specified using the token middleware.

Related issues

loopbackio/loopback.io#297 (comment)

• None

Checklist

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

[bajtos]

New tests should be added to cover properly the scenario where a custom AccessToken is specified using the token middleware.

Yes please!

[bajtos]

Good catch!

I think we need to support the case where options.model is a model instance too, see e.g.

loopback/server/middleware/rest.js

Lines 52 to 53 in ed4f56c

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

I am proposing to use loopback.getModel (registry.getModel) instead of app.models, because the former is already able to handle both string and constructor arguments. In fact, that's already done in the code below. I think it will be best to simplify this whole code to the following:

if (registry === loopback.registry && !options.model) {
  TokenModel = loopback.AccessToken;
} else {
  TokenModel = registry.getModel(options.model || 'AccessToken');
}

[ebarault]

yes, i thought that too
simplifying was my next move:

i think we can even simplify to just this:

TokenModel = registry.getModel(options.model || 'AccessToken'); 

as it does the exact same thing, thoughts?

[bajtos]

Agreed +1

[ebarault]

on my way adding missing tests: it occurred to me that we don't currently have a test category for all the configuration that can be done with middleware.json.

I think the good way to do this is with fixtures: i added a new folder for middleware-config tests right here

Now i wonder whether we should dispatch each related tests where it belongs from a functional PoV (for example : access token config with middleware.json in access-token-test.js) or if it'd be better to group them all in a new middleware-json.test.js file.

@bajtos thoughts?

done

[bajtos]

Thank you for taking the time to think about how to test our built-in middleware better and writing down code to show it in action! 👏

I think the good way to do this is with fixtures

I am afraid I disagree with the current proposal for fixtures, see my earlier 716ed45#commitcomment-15931869

My main objection against test fixtures using full loopback-boot project layout is that such fixtures make it difficult to understand what a given test is executing, because there are too many files to read.

The are also adding a lot of maintenance costs by containing lot of duplication. Besides that, the project layout scaffolded by lb app (loopback-workspace) is evolving over time and we don't have bandwidth to update the fixtures to stay current.

In general, I strongly believe in keeping the test fixtures/data minimal, containing only the necessary things to make the test fail/pass.

Now i wonder whether we should dispatch each related tests where it belongs from a functional PoV (for example : access token config with middleware.json in access-token-test.js) or if it'd be better to group them all in a new middleware-json.test.js file.

+1 for the former - group the tests based on what function/feature they are testing.

I think this is another problem with "big" test fixtures - because they are difficult to write and maintain, developers tend to (incorrectly) structure the test code around the fixtures. IMO, we should first structure the tests based on what makes most sense from functional point of view, and then find a way how to write fixtures in such way that makes it easy to write tests the way we would like to.

[bajtos]

The code changes LGTM 👍 Could you please fix the commit message to follow our 50/72 rule?

2f3daac - fix custom token model in token middleware: line 3 longer than 72 characters.

split commit message

[ebarault]

@bajtos should be good now

[bajtos]

Great, thank you!