Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Actually post data as application/json, not form-data #25

Merged
merged 5 commits into from Jan 28, 2017
Merged

Actually post data as application/json, not form-data #25

merged 5 commits into from Jan 28, 2017

Conversation

holm
Copy link
Contributor

@holm holm commented Jan 20, 2017

After trying to get logos to work in approval request for way too long, I found that the data submitted to Authy was not actually being sent as JSON. It was a bit confusing since the path was /json and the headers was set, but in reality the form property was used instead of body on the request methods.

Logos didn't work because Authy expects them in the form logos[][res]..., while the json was being encoded to form-data as logos[0][res]....

Rather than try to fix that, I think the right solution is to actually send as json. This PR changes this and adjusts the tests.

@ruimarinho
Copy link
Owner

@holm it doesn't seem like Authy has changed their data input method to JSON, at least judging by the documentation. It's still application/x-www-form-urlencoded, so form is the appropriate way of sending requests. I would rather fix this particular non-standard way of sending collections (array values with brackets only) to suit the needs of the Authy API.

I think the fix is simply:

❯ git diff
diff --git i/src/client.js w/src/client.js
index c45b518..938b1a2 100755
--- i/src/client.js
+++ w/src/client.js
@@ -102,6 +102,9 @@ export default class Client {
           message,
           seconds_to_expire: ttl
         },
+        qsStringifyOptions: {
+          arrayFormat: 'brackets'
+        },
         uri: esc`users/${authyId}/approval_requests`
       })
       .bind(this)

@holm
Copy link
Contributor Author

holm commented Jan 23, 2017

I think it is just very odd that you send it as form-data, when the API is in general using JSON. The path in the URL indicates JSON, ttps://github.com/seegno/authy-client/blob/master/src/client.js#L66), and there is nothing in their documentation to indicate they support form-data.

While the other change will work, I just find it clearer and more concise to just use json.

@ruimarinho
Copy link
Owner

Are you sure you're looking at the right documentation? E.g. Everywhere in https://www.twilio.com/docs/api/authy/authy-totp I only see application/x-www-form-urlencoded values, not application/json.

@@ -37,7 +37,10 @@ export default function signatureAssert({ key, request } = {}) {
const url = parse(`${protocol}://${host}${path}`, true);

// Stringify body using sorted keys and encoding spaces as "+" instead of "%20".
const encoded = qs.stringify(body, { sort: (a, b) => a.localeCompare(b) }).replace(/%20/g, '+');
const encoded = qs.stringify(body, {
arrayFormat: 'brackets',
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure the returned values are using this method? Last time I checked it returned indexed ones.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it is confirmed.

@holm
Copy link
Contributor Author

holm commented Jan 24, 2017

I can see curl examples that uses form data, but nothing else. I still think this makes everything cleaner and simpler.

The posted back data on verification retains the data types in hidden_details, so I'm pretty sure json is the underlying format they use.

@ruimarinho
Copy link
Owner

Sounds good. I'll give it a more thorough look in the upcoming days. I'm adding a cli-based utility belt in between.

@ruimarinho
Copy link
Owner

Thanks for pointing this out @holm. After some testing, I have confirmed all changes.

@ruimarinho ruimarinho merged commit 355b6e7 into ruimarinho:master Jan 28, 2017
@holm
Copy link
Contributor Author

holm commented Jan 28, 2017

Thanks for the merge. We have been running it in production for over a week, and haven't seen any issues either.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants