Bug in Loading Strategies

[rdegges]

So, our Python contractor (@avojnovicDk) has been working on porting this library over to Python, and came across a bug in this code here: https://github.com/stormpath/stormpath-node-config/blob/master/lib/strategy/LoadAPIKeyConfigStrategy.js#L37-L39

So, here's what's happening. First, take a look at the order in which things are supposed to be loaded from our sdk spec: https://github.com/stormpath/stormpath-sdk-spec/blob/master/specifications/config.md#loading-order

The order in which things should be loaded goes from most 'implicit' to most 'explicit'. So, the more explicit a user is about their settings, we'll prefer those explicit settings over the more generic ones. This makes sense, because it means a user will usually get what they expect in a magical way.

Here's the scenario in which the APIKeyConfigStrategy is broken:

Let's say I'm a user, and I have the following:

• An API key file named ~/.stormpath/apiKey.properties,
• An API key file named apiKey.properties inside of my local application folder that I'm currently working on.

Per our spec, we should first load the settings from ~/.stormpath/apiKey.properties, and later override it with the settings from apiKey.properties in the local directory since it also exists.

This is not happening though.

These lines of code: https://github.com/stormpath/stormpath-node-config/blob/master/lib/strategy/LoadAPIKeyConfigStrategy.js#L37-L39 show that if those API key values are already loaded from the first API key file, than we'll skip processing and use those old values INSTEAD of overwriting them like we should.

I think the solution is most likely to just remove these lines of code, but we should write some tests / etc. to cover this scenario first to make sure we aren't missing anything.

[typerandom]

Interesting.

I think this was originally a refactor from: https://github.com/stormpath/stormpath-sdk-node/blob/abe164b88538cf357c18ac53545796cd48fa0e03/lib/Config.js#L46

But I can definitely see how this is an issue. I thought I had tests for it, but I'll add some and fix this issue :)

@rdegges @avojnovicDk Big thanks on discovering/reporting it! :)

[rdegges]

Thanks @typerandom! All credits go to @avojnovicDk, she's really great.

[avojnovicDk]

Thanks! <3 It's always good to have another pair of eyes.

[robertjd]

Thanks @avojnovicDk for pointing this out - we'll have to update this, the way it should work is defined here:

https://github.com/stormpath/stormpath-sdk-spec/blob/master/specifications/config.md#loading-order

[avojnovicDk]

Actually, there are some more things that bother me in that Strategy:

https://github.com/stormpath/stormpath-node-config/blob/master/lib/strategy/LoadAPIKeyConfigStrategy.js#L31-L34
This is in else clause, so it says that, if API key config file exists, we should extend the current config with default client API key fields. I think this will overwrite the currently loaded API keys with blank keys.
For example, if user specifies his/hers API keys in stormpath.json file from ~/.stormpath directory (loading order no. 3), and has apiKey.properties file in the application directory (loading order no. 4) that is empty. It seems to me that these lines will overwrite the previously loaded keys. And it is stated here: https://github.com/stormpath/stormpath-sdk-spec/blob/master/specifications/config.md#apikeyproperties that

If the file exists but is empty it should be ignored.

I checked the extend function in this fiddle: http://jsfiddle.net/L11q3LqL/ . This is jQuery, but npm's extend should be it's port: https://www.npmjs.com/package/extend .

Also, now I think that this condition https://github.com/stormpath/stormpath-node-config/blob/master/lib/strategy/LoadAPIKeyConfigStrategy.js#L37 will never be True, as we have just set config.client.apiKey to {}.

Another thing that confuses me with the case when the file is empty is this line: https://github.com/stormpath/stormpath-node-config/blob/master/lib/strategy/LoadAPIKeyConfigStrategy.js#L53
It seems to me that it will ignore the whole previously loaded config. Also, I don't know why mustExist variable is checked in this if condition.

Please note that I'm not a Node.js developer, so I'm sorry if I misinterpreted some lines.