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

Added passwordRequired to options, defaults to true #31

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jackdent
Copy link

@jackdent jackdent commented Jul 1, 2014

No description provided.

@jackdent jackdent mentioned this pull request Jul 1, 2014
@paulmelnikow
Copy link

@jaredhanson Can this be merged? This would be helpful for my use case as well.

@jaredhanson
Copy link
Owner

Is this any different than doing: passport.authenticate(['basic', 'anonymous'])?

@paulmelnikow
Copy link

Yeah. These requests aren't anonymous. They're authenticated using a single API key, instead of a username + password combination. The client sends that key as the username, and a blank password.

@jackdent
Copy link
Author

@jaredhanson For example, the Stripe API uses the username as the authentication key, and leaves the password blank

@jaredhanson
Copy link
Owner

Wouldn't that be the point of passport-http-bearer?

@paulmelnikow
Copy link

Sure, seems like that would be one way to design the API. But basic auth is widely implemented in clients. The spec for http basic doesn't seem to require that the password is non-zero length, so using it in this manner seems reasonable (as well as common).

@jackdent
Copy link
Author

+1

@jaredhanson
Copy link
Owner

Cool. Can't we just always accept empty passwords and pass them back to the verify callback then? Doesn't seem like we need the option to the constructor.

@jackdent
Copy link
Author

I had backwards compatibility in mind, since some people might be relying on the failure mode for empty passwords. I don't think the change adds too much complexity, but you're right: it's nicer to just accept blank strings.

@paulmelnikow
Copy link

Wanted to bump this up. @jaredhanson, what do you think?

@ianstormtaylor
Copy link

+1 Also need this to implement a Stripe-like Basic Auth convenience. @jaredhanson can we merge this?

Backwards compatibility and the ability to opt-out of having to spend the resources to lookup a user and verify their hashed password might be good reasons to have the optional flag.

@itaysabato
Copy link

There are at least two issues and two pull requests in total on this exact issue that have been outstanding for over two years now (See #28 #30 #37). The fix seems pretty straight forward which ever way you go.

It makes me wonder: is this library actually maintained?

No offense.

@ianstormtaylor
Copy link

@jaredhanson ?...

@noahajohn
Copy link

noahajohn commented May 2, 2017

I also echo the above comments. I think allowing for the password to be optional, which would allow us to mimic a Stripe-style API, would be very helpful.

@alexchuvand
Copy link

When is that going to be implemented ? It doesn't seem to be in the version 0.3.0

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

7 participants