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.

Sign up for GitHub

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

Jump to bottom

Added basic-auth path to allow testing with empty password. Fixed #502. #503

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Added basic-auth path to allow testing with empty password. Fixed #502. #503

wants to merge 3 commits into from

Conversation

javabrett
Copy link
Contributor

Fixed #502.

@javabrett javabrett force-pushed the 502-basic-auth-empty-password branch from 159001a to d4c96e1 Compare October 11, 2018 23:37
nateprewitt
nateprewitt suggested changes Oct 24, 2018
View reviewed changes
Copy link
Contributor

@nateprewitt nateprewitt left a comment

Choose a reason for hiding this comment

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

Hey @javabrett, would you mind adding some testing to demonstrate exactly what we expect to see here. I'm assuming no username or password would just result in Authorization: Basic with no checksum?

@javabrett javabrett force-pushed the 502-basic-auth-empty-password branch from d4c96e1 to 920d26c Compare October 25, 2018 00:54
@javabrett
Copy link
Contributor Author

The existing coverage is in test_set_cors_credentials_headers_after_auth_request, which is where I'd added some tests already - this method is named specifically for CORS testing, but I'm co-opting it.

The existing test didn't really check an authentication following the challenge, so I've added a commit which checks that, for all cases of foo:bar, foo: and :. You'll note that the correct auth response for an empty username/password is Basic Og== (encoded colon). Headers can be checked here: https://www.blitter.se/utils/basic-authentication-header-generator/ .

@javabrett
Added basic-auth path to allow testing with empty password. Fixed pos…
1094016 
…tmanlabs#502.
@javabrett
Also accept empty username for basic-auth (implies empty password). p…
3c97d10 
…ostmanlabs#502.
@javabrett
basic-auth test improvements:
747c11b 
- refactored test_set_cors_credentials_headers_after_auth_request to test
  with the required (and a bad) basic auth challenge response.
- do this for all present/absent username:password combos,
  foo:bar, foo:, : (neither)

Signed-off-by: Brett Randall <[email protected]>
@javabrett javabrett force-pushed the 502-basic-auth-empty-password branch from 920d26c to 747c11b Compare November 7, 2018 21:48
@javabrett
Copy link
Contributor Author

Any desire to review/merge ... else closing?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nateprewitt nateprewitt nateprewitt requested changes

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Basic auth without both username and password
2 participants
@javabrett @nateprewitt