Fix: treat empty access token string as undefined [2.x]

[andrey-abramow]

Related issues

• Fixes AccessToken incorrectly handles an empty string value with loopback-datasource-juggler 2.56.0 #4078

[slnode]

Can one of the admins verify this patch? To accept patch and trigger a build add comment ".ok\W+to\W+test."

[bajtos]

@slnode ok to test

[bajtos]

@andrey-abramow thank you for the pull request. Please add a test to verify the fix - the test should fail agains the current master and pass with your fix in place.

See the existing test suite for inspiration:

loopback/test/access-token.test.js

Lines 413 to 446 in b064b6d

	describe('.findForRequest()', function() {
	beforeEach(createTestingToken);
	it('supports two-arg variant with no options', function(done) {
	var expectedTokenId = this.token.id;
	var req = mockRequest({
	headers: { 'authorization': expectedTokenId }
	});
	Token.findForRequest(req, function(err, token) {
	if (err) return done(err);
	expect(token.id).to.eql(expectedTokenId);
	done();
	});
	});
	function mockRequest(opts) {
	return extend(
	{
	method: 'GET',
	url: '/a-test-path',
	headers: {},
	_params: {},
	// express helpers
	param: function(name) { return this._params[name]; },
	header: function(name) { return this.headers[name]; }
	},
	opts);
	}
	});
	});

[bajtos]

@andrey-abramow also please

1. Sign our CLA:
   https://cla.strongloop.com/agreements/strongloop/loopback
2. Fix your commit message to follow our guidelines: https://loopback.io/doc/en/contrib/git-commit-messages.html.

Here are the git commands to run, in case you are not familiar with git history manipulation:

$ git commit --amend
# edit the commit message
$ git push --force-with-lease

[bajtos]

Last but not least, make sure that npm test is passing for you locally. I am seeing linter errors reported by our CI, e.g. here: https://travis-ci.org/strongloop/loopback/jobs/458326249

Running "jscs:common" (jscs) task
disallowMultipleLineBreaks:  http://tools.ietf.org/html/rfc6750 and  To prevent Error: Model::findById requires the id argument should have at most 2 line(s) between them at common/models/access-token.js :
   209 |      if (typeof id === 'string') {
   210 |        // Add support for oAuth 2.0 bearer token
   211 |        // http://tools.ietf.org/html/rfc6750
-----------------------------------------------------^
   212 |
   213 |
>> 1 code style errors found!
Warning: Task "jscs:common" failed.� Use --force to continue.

Quite a few comments to address, but hopefully most of the problems will be easy & quick to fix. Let me know if you need any help. I am looking forward to see your pull request accepted ❤️

[bajtos]

☝️

[andrey-abramow]

@bajtos Thanks! Done!

[andrey-abramow]

@bajtos Hello! Please, let me know if I missed something or made it wrong.

• npm test is ok locally but I see a lot of failed checks, even required one (loopback — Failed! (fe79966)).
• I added package-lock.json, should I remove it?

[bajtos]

I added package-lock.json, should I remove it?

Yes please!

Sorry for the delay, I am overwhelmed by GH notifications. I'll take a closer look at this pull request early next week.

[bajtos]

The windows test is failing with the following error, I think it's unrelated to changes made in this pull request.

<testcase classname="User Verification user.verify(options, fn)" 
  name="Verify a user&#x27;s email address with custom token generator" time="0"
><failure>0 == true
AssertionError: 0 == true
    at test\user.test.js:1523:13
    at common\models\user.js:150:273
    at Nodemailer.&#x3C;anonymous&#x3E; (node_modules\nodemailer\lib\nodemailer.js:329:26)
    at StubTransport.&#x3C;anonymous&#x3E; (node_modules\nodemailer-stub-transport\src\stub-transport.js:78:13)
    at Immediate._onImmediate (node_modules\async-listener\glue.js:188:31)</failure></testcase>

[bajtos]

Make compatible with .datasource-juggler 2.56.0

I find this commit message as not very useful to people reading git history, because it does not describe what is being changed in the user-observable behavior.

I'll edit the commit message for you to speed up the process of landing this pull request.

[bajtos]

Here is the new commit message:

Fix: treat empty access token string as undefined

Fix AccessToken's method tokenIdForRequest to treat an empty string
as if no access token was provided.

This is needed to accomodate the changes made in
loopback-datasource-juggler@2.56.0.

Let's wait for CI results before landing.

[bajtos]

Landed, thank you for your first contribution! ❤️

I have published the fix in loopback@2.41.1. Enjoy 🎉

[bajtos]

@andrey-abramow It makes me wonder, is this issue present in 3.x version of LoopBack too? Would you mind forward-porting your patch and opening a new pull request against master branch?

[andrey-abramow]

@bajtos Thanks! I made a new one from master, look please #4083