Update uri with encoded path if it was part of the original request

[Andras-Marozsi]

I've run into some issues when the path contained some special characters, like colons: :.
In this case, the path attribute of the request object was updated to the encoded one, but the uri was not, so the keys generated on the server/client side didn't match, and I got an authentication error.

Though it can be solved by using an encoded uri, it would be nice if the updates done to the request are in sync with each other and you don't have to worry about which part will be changed and which won't. This update will be only applied if the uri is provided, otherwise it won't change anything.

I'm already using the head of my fork but thought others would also benefit from this change.

[mhart]

I'm a little confused – you're passing in a uri? Even though aws4 doesn't use it?

uri/url isn't recognised by the Node.js http client, which is why I never added support for it – but there is a PR to do so, which might be useful for ppl using other HTTP clients

I think probably the best way to handle it would be by recognising it officially – this seems a little... post hoc maybe? 😸

[Andras-Marozsi]

Yeah, so I'm using request-promise, which requires a uri. So what I do is more or less like:

var uriParts = "https://host.of.app/items/module:sub-module:item.collection:12345".match(/(http(?:s|)):\/\/([^\/]+)(.+)/);
var req = {
  service: 'service',
  region: 'region',
  host: uriParts[2],
  path: uriParts[3],
  uri: uriParts[0],
  headers: {
    'Content-Type': 'application/json',
    'x-api-key': 'key'
  },
  resolveWithFullResponse: true
};

aws4.sign(req, {
    accessKeyId: 'AKI',
    secretAccessKey: 'SAK'
});

This works fine when the path doesn't have any special characters, but if it does, then that's updated, but the uri is not, which results in authentication failure.

So my purpose with this pr was not to add full support for uri, but to make the changes applied to the request object "consequent". But I can see your concern as well :)

Would you like me to include full support of uri in this pr?

[mhart]

@Andras-Marozsi is there a reason you don't use the aws option for request? https://github.com/request/request#requestoptions-callback

[Andras-Marozsi]

The only reason is that I prefer promises over callbacks, and as I use them in the framework all over, wanted to be consequent and went for the promise based solution.

[mhart]

@Andras-Marozsi I think you're misunderstanding my question. Doesn't request-promise use the same options as request? In which case, don't do any of this manually – just use the aws option when you use request-promise

[Andras-Marozsi]

Ah, I see what you mean. If I don't add in the uri, I get this:

[RequestError: Error: options.uri is a required argument]

But if I don't add host and path, then of course I have other issues :) That's why I decided to parse the parameter (uri, I used only that so far), and make all modules happy.

[mhart]

@Andras-Marozsi request (and request-promise) already handle all this stuff for you: https://github.com/request/request/blob/f009af231412fe8fee4ab5dd24d24781fd731fc3/request.js#L1283-L1312

You don't need to pass in host or path or anything – just pass in uri (and aws) options to request-promise and it will take care of the rest

[Andras-Marozsi]

Hm, I can't make it work, I'm getting 403 with that solution.
Ok, I will experiment with an alternative solution, and use my fork in the meantime.
Thanks for the tips @mhart!

[mhart]

No problems – feel free to show me your code and I can see if there's anything that jumps out