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

Make Handling of Missing or Incorrect Api Key Consistent With Fever Api #454

Merged
merged 2 commits into from Aug 7, 2017

Conversation

Projects
None yet
4 participants
@jbrayton
Contributor

jbrayton commented Aug 6, 2017

I am the developer of Unread, an RSS reader for iOS:

https://www.goldenhillsoftware.com/unread/

I recently implemented Fever URL validation. Unread performs this validation before asking for the customer's credentials. This validation attempts to confirm that the Fever URL is correct by issuing a request to the Fever API without an API key and verifying that the API returns the following JSON:

{ "api_version": 3, "auth": 0 }

This works with Fever, but it is not working with Stringer because Stringer responds with a 403 and no response body when the request does not contain the correct API key. This pull request makes Stringer consistent with Fever in this regard.

When the api_key is not specified or is incorrect, the Fever API returns
a response code of 200 and the following JSON:

{"api_version":3,"auth":0}

This change emulates that behavior in Stringer.
@queuebit

This comment has been minimized.

Show comment
Hide comment
@queuebit

queuebit Aug 6, 2017

Contributor

@jbrayton nicely done. I downloaded the new version of Unread after hearing the podcast and when I tried to login it to my stringer backend it failed and didn't have time to look into it at the time.

Contributor

queuebit commented Aug 6, 2017

@jbrayton nicely done. I downloaded the new version of Unread after hearing the podcast and when I tried to login it to my stringer backend it failed and didn't have time to look into it at the time.

@swanson

This comment has been minimized.

Show comment
Hide comment
@swanson

swanson Aug 7, 2017

Owner

This looks great -- one question: would it still work if we returned a 403, but with the body including the API version? It's unclear to me on if returning a 200 when the caller is unauthorized is mirroring the Fever API, but it feels slightly wrong semantically.

Owner

swanson commented Aug 7, 2017

This looks great -- one question: would it still work if we returned a 403, but with the body including the API version? It's unclear to me on if returning a 200 when the caller is unauthorized is mirroring the Fever API, but it feels slightly wrong semantically.

@gglanzani

This comment has been minimized.

Show comment
Hide comment
@gglanzani

gglanzani Aug 7, 2017

I've "deployed" this version, and I can confirm that it's working now.

@swanson I guess if that's consistent with the Fever API, we should keep it this way?

gglanzani commented Aug 7, 2017

I've "deployed" this version, and I can confirm that it's working now.

@swanson I guess if that's consistent with the Fever API, we should keep it this way?

@jbrayton

This comment has been minimized.

Show comment
Hide comment
@jbrayton

jbrayton Aug 7, 2017

Contributor

@swanson Unread is looking for both the 200 response code that the Fever API returns as well as the JSON response body.

Contributor

jbrayton commented Aug 7, 2017

@swanson Unread is looking for both the 200 response code that the Fever API returns as well as the JSON response body.

@swanson

This comment has been minimized.

Show comment
Hide comment
@swanson

swanson Aug 7, 2017

Owner

Thanks -- agree with matching the Fever API so let's go with the 200 code.

Owner

swanson commented Aug 7, 2017

Thanks -- agree with matching the Fever API so let's go with the 200 code.

@swanson swanson merged commit 4b21f68 into swanson:master Aug 7, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment