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

Adding handling for no auth method and null bearer #1500

Merged
merged 1 commit into from Mar 23, 2015
Merged

Adding handling for no auth method and null bearer #1500

merged 1 commit into from Mar 23, 2015

Conversation

philberg
Copy link
Contributor

I noticed a couple of scenarios the other day:

  1. When no authentication mechanism is defined, or bearer = undefined (I meant to send it, but it got set incorrectly) the error message is difficult to deciper
  2. When setting bearer: null, request does a string concat, which results in 'bearer null'. Probably not the intended behavior

Also added explanations to the bearer tests

@philberg
Copy link
Contributor Author

That line will evaluate to bearer when the bearer is set to null (but not undefined, because that enters the basic auth case).

My intent was to make the behavior less confusing.
Currently, setting bearer: null actually sends the string null as the bearer. My interpretation was that, by setting bearer: null the intent was to send bearer with nothing, in other words bearer

If the intent is so actually send the string null, then bearer: 'null' can be set

@simov
Copy link
Member

simov commented Mar 22, 2015

Ok, what I'm saying is that you can remove the req.headers.authorization === 'Bearer null' check from the test because it's misleading and it's never true.

Other than that this PR looks good to me 👍

@philberg
Copy link
Contributor Author

I operate under the principle that a change should never be made without a corresponding test, so I was trying to write a test that would have failed before but now will pass. I'm sure there is a more elegant way to do it.

Either way, I understand what you're saying, so I've removed that line and rebased into the same commit

@nylen
Copy link
Member

nylen commented Mar 23, 2015

I operate under the principle that a change should never be made without a corresponding test, so I was trying to write a test that would have failed before but now will pass.

+1000 🙇

In this case, your t.throws test would have failed before, so I think you're good.

@nylen
Copy link
Member

nylen commented Mar 23, 2015

Thanks for adding test names, too.

nylen added a commit that referenced this pull request Mar 23, 2015
Adding handling for no auth method and null bearer
@nylen nylen merged commit f33d1a6 into request:master Mar 23, 2015
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

3 participants