{"action":"created","issue":{"url":"https://api.github.com/repos/stormpath/stormpath-node-config/issues/6","labels_url":"https://api.github.com/repos/stormpath/stormpath-node-config/issues/6/labels{/name}","comments_url":"https://api.github.com/repos/stormpath/stormpath-node-config/issues/6/comments","events_url":"https://api.github.com/repos/stormpath/stormpath-node-config/issues/6/events","html_url":"https://github.com/stormpath/stormpath-node-config/issues/6","id":114084728,"number":6,"title":"Bug in Loading Strategies","user":{"login":"rdegges","id":90247,"avatar_url":"https://avatars.githubusercontent.com/u/90247?v=3","gravatar_id":"","url":"https://api.github.com/users/rdegges","html_url":"https://github.com/rdegges","followers_url":"https://api.github.com/users/rdegges/followers","following_url":"https://api.github.com/users/rdegges/following{/other_user}","gists_url":"https://api.github.com/users/rdegges/gists{/gist_id}","starred_url":"https://api.github.com/users/rdegges/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/rdegges/subscriptions","organizations_url":"https://api.github.com/users/rdegges/orgs","repos_url":"https://api.github.com/users/rdegges/repos","events_url":"https://api.github.com/users/rdegges/events{/privacy}","received_events_url":"https://api.github.com/users/rdegges/received_events","type":"User","site_admin":false},"labels":[{"url":"https://api.github.com/repos/stormpath/stormpath-node-config/labels/bug","name":"bug","color":"fc2929"}],"state":"open","locked":false,"assignee":null,"milestone":null,"comments":5,"created_at":"2015-10-29T16:09:06Z","updated_at":"2015-11-13T18:20:03Z","closed_at":null,"body":"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\r\n\r\nSo, 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\r\n\r\nThe 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.\r\n\r\nHere's the scenario in which the APIKeyConfigStrategy is broken:\r\n\r\nLet's say I'm a user, and I have the following:\r\n\r\n- An API key file named `~/.stormpath/apiKey.properties`,\r\n- An API key file named `apiKey.properties` inside of my local application folder that I'm currently working on.\r\n\r\nPer 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.\r\n\r\nThis is not happening though.\r\n\r\nThese 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.\r\n\r\nI 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."},"comment":{"url":"https://api.github.com/repos/stormpath/stormpath-node-config/issues/comments/156507790","html_url":"https://github.com/stormpath/stormpath-node-config/issues/6#issuecomment-156507790","issue_url":"https://api.github.com/repos/stormpath/stormpath-node-config/issues/6","id":156507790,"user":{"login":"avojnovicDk","id":5756635,"avatar_url":"https://avatars.githubusercontent.com/u/5756635?v=3","gravatar_id":"","url":"https://api.github.com/users/avojnovicDk","html_url":"https://github.com/avojnovicDk","followers_url":"https://api.github.com/users/avojnovicDk/followers","following_url":"https://api.github.com/users/avojnovicDk/following{/other_user}","gists_url":"https://api.github.com/users/avojnovicDk/gists{/gist_id}","starred_url":"https://api.github.com/users/avojnovicDk/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/avojnovicDk/subscriptions","organizations_url":"https://api.github.com/users/avojnovicDk/orgs","repos_url":"https://api.github.com/users/avojnovicDk/repos","events_url":"https://api.github.com/users/avojnovicDk/events{/privacy}","received_events_url":"https://api.github.com/users/avojnovicDk/received_events","type":"User","site_admin":false},"created_at":"2015-11-13T18:20:03Z","updated_at":"2015-11-13T18:20:03Z","body":"Actually, there are some more things that bother me in that Strategy:\r\n\r\nhttps://github.com/stormpath/stormpath-node-config/blob/master/lib/strategy/LoadAPIKeyConfigStrategy.js#L31-L34\r\nThis 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.\r\nFor 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\r\n>If the file exists but is empty it should be ignored.\r\n\r\nI 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 .\r\n\r\nAlso, 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 `{}`.\r\n\r\nAnother 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 \r\nIt 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.\r\n\r\nPlease note that I'm not a Node.js developer, so I'm sorry if I misinterpreted some lines."}}