-
-
Notifications
You must be signed in to change notification settings - Fork 497
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
Allow passport to return authentication failure messages via variable #4
Comments
That's basically how I'm doing it too, except I'm not returning json. As long as the new function allows you to return any variable and not just a string and you can pass that variable to res.xxx() in your router (so you can pass any auth errors to your views easily) I'd be happy. Edit: |
passport-local v0.1.2 address this issue. See this comment on Passport issue #12 for details. Also note that the above syntax will work
but the preferred form is to use an options hash
as it is more extensible for future use. |
Can you change around how the message is returned? Using req.flash() is silly. Perhaps you can just make it throw an err and let the developer decide how to handle it? |
@nickjj I agree completely with all your points. The arguments to done in the verify callback are carried through exactly to the custom authenticate callback, so when you call
That last argument is the info argument to the authenticate callback:
From there you can call any message you want on the request ( I'm not the biggest fan of flash myself, and am aware that it's being deprecated in Express. (I suspect you'll see middleware to polyfill it back in, though.) That said, if you never use the option, it won't affect you. I wan't to keep Passport framework agnostic, and these options are really just shortcuts for common patterns, Express or otherwise. Flash messages are common enough that it makes sense to include. I'm open to adding additional options if there are better ways of messaging or other standard operations. Outside of that, custom callbacks are the way to go. Does that seem OK to you? |
In 3.x, express now has view-specific middleware where you can inject site wide variables to all of your views with app.locals.use(...). The workflow to use that (for example) is: In app.js...
source: https://github.com/visionmedia/express/blob/master/examples/blog/app.js#L32 In one of your routes you would just do req.session.message = info.message with the new setup but the route doesn't have access to info. Are you saying we would still need to use a custom callback to output the message in a way that doesn't depend on req.flash()? I'm still pretty new to node/express/JS in general, sorry if my questions are retarded. |
Your questions are great. You've got a good handle on what is going on.
If you are using Express 3, the yes, for the time being. Here's how that would work:
Now, I admit that is a fair amount of extra effort just to set a simple message. Here's my philosophy on this: Passport is designed to be generic authentication middleware, usable in any app framework. It does this by delegating control back to the application for tasks that are logically under the application's responsibility. Rendering, redirecting, messaging, etc all fall under that category, so any custom work that is unique to your application or your application's underlying framework should be handled in the callback. Where common patterns emerge across frameworks, Passport will provide options to make those patterns easier (thus the existence of failureRedirect, failureFlash, etc.) Flash messaging is one such common pattern, so its useful to have Passport support it. I'm personally in favor of @visionmedia's decision to remove If a different messaging pattern emerges that is useful across a variety of apps, then I'll consider adding a built-in option to Passport (say |
Ok thank you. That clears up pretty much everything. I'm looking forward to sticking with Passport. |
@nickjj I just published connect-flash, which is middleware you can drop into an Express 3.x app: If you use it, you can also use the |
I'm anti req.flash() too, so I likely won't be using that however I do think a lot of people will enjoy it going by the sheer number of times I've seen req.flash() in example express apps. On a side note your connect-flash middleware is a really good/basic example of how to properly implement "real" middleware in express. |
Whenever auth fails it always says "Missing credentials" which seems to be hard coded into the passport lib itself. When I overwrite it by following the custom callback example and output info.message it always reports "Missing credentials" still, like it's ignoring whatever is in my passport.use(LocalStrategy...). Also I seem to always get "failed to deserialize user out of session" whenever I successfully login. I've followed your examples and tutorials. I've tried using both the standard express session store and redis. The page I'm working on worked before I upgraded passport/passport-local to the latest version. Any idea what's going? Should I post all of the code? |
@nickjj Have you figured out the cause of your issues? Everything is working fine for me, and if Passport were broken on the whole, I'd be hearing about it from other people. Since I'm not, I'm guessing it is something specific to your setup. Post a gist with code that reproduces the problem, and et me know if I can provide any help. |
Yes, I was using a version of express 3.x that had a buggy version of connect (I think it was 2.0.0 or 2.0.1). Tj fixed a bug in connect recently that did something to fix sessions. After upgrading to the latest connect passport is back to working. I just ported your example app in your passport-local repo to an express 3.x app and it works. It works using both the standard connect session store and a redis session store (connect-redis module). The other issue I had was my fault. When I changed your mock data to access a real database (mongodb), it was having issues deserializing the data because I was passing the id from deserializeUser() directly into my mongo findOne() as a string but it needs to be converted to an ObjectID first. I'm still not 100% comfy with this new stack so when I read your tutorial guide and followed along, it showed you using findOne() during a deserializeUser(). I just figured "oh ok, he's using findOne() so he must be using mongo too and just passes the id no problem". Everything seems to be good to go now for using passport-local. The real problem now is, if I'm latched into using a custom callback to get custom messages with express 3.x and I have to define my own local strategy depending on what's checking the data then why am I using passport-local? Why not just use a standard function to authenticate against my database and write even less code in my route to detect if it's a good or bad login attempt? I definitely want to use passport-xxx if I decide to support logging in with 3rd party services, but I'm not sure why local (with custom callbacks) is even worth using? Can you sell me on using it? |
The only good way to put a failure message I found was this way. app.post("/login", passport.authenticate("local",
{ successRedirect: "/",
failureRedirect: "/login",
failureMessage: "Invalid username or password" })); Then the message will be stored in req.session.messages, which is an array, the only problem is that it is static and I think there is a bug that the array will concatenate the same message over and over again. This was fixed by the following code, after the response was rendered :P res.render("login", { login_errors: req.session.messages || [] });
req.session.messages = []; |
What's the point of using
|
How can I return multiple messages, for instance if the login fails I want to return an appropriate error message and the email that was entered so I can feed the email input again.
This does not work. |
What is this supposed to do? Resetting the session messages? I don't think this will work. I just display the last of the messages in the array. Like so in EJS:
` |
@GEOLYTIX yes, this approach will work also, it's just that each time I refreshed the site there was one more of the same message in the array, so I cleaned it each time, but your way also works |
@jaredhanson I have a programming situation where I want to know if the user exists, because I am gong to take different application action based on if they already exist or not. So I wish there would be a passport.js URL to provide support for /exist , where I provide a username and passport returns a 0 or 1. Ok, this is not available. So I post to /login with a username and password. The password can be anything. In the passport.use function if the username is not found I return-
If the user was found I instead return with- done(null, false, { "rtnCode": 2 })
Now, in passport.authenticate('local-login', function(err, user, info) That means that in
Now I can send a specific return code to my app which provides me the information that the user exists or not. Here is the problem. Even though the execution flow ends with a return and so does not ever run- If I now issue an ajax GET with the username that I know exists and a bogus password, I get in, I am able to read the database. That is very unexpected as I have never issued req.logIn(). And of course it is a security issue. The point here is that upon a "bad" username or password in my /login I do not want to go to a webpage, I want to take a different course of action in code.
|
Im using angular4 with passport, is there anyway i can get res without the redirect feature in passport? |
So it happens that passport has a function to do solve that in their documentation, |
//routes examples //ejs handler <%= msg %>
<% }); %>
<% } %>
<% if (alerts.success) { %>
<% alerts.success.forEach(function(msg) { %>
<%= msg %>
<% }); %>
<% } %>`
|
thanks @jaredhanson !! you saved me hhhh |
There is abundant confusion on the web regarding how the information passed as
In other words, the default action of the passport-local middleware is to ignore what's being passed to This confuses users who expect to use passport.authenticate() as middleware before their own request handlers. Exacerbating the matter is the fact that passport.authenticate takes several options, none of which have the desired behavior of passing along the information to the client. I'm wondering if this could/should be revised. It seems, to me, a perfectly established pattern to - by default - take the object passed as |
It's been 7 years and I don't think this feature will ever get implemented. That said, you can create a workaround to keep the strategy as a middleware function and still have custom messages sent back to the client (and the function can be made reusable for other controllers if desired): Express route:
Passport Middleware
Express Controller
The biggest advantage of this approach is that you can now unit test the authenticate strategy separately from the controller. The disadvantage is that this still doesn't support passport sessions (which doesn't appear to be supported when utilizing any custom callbacks). |
Thank you, this was very useful and clean. |
As per my discussion with Jared via IM, this issue is to require the ability to pass back authentication failure messages to the passport.authenticate function. Perhaps via:
passport.authenticate(... func(err, user, info))
The issue right now is that there is no good way to return back an error message from the LocalStrategy. The only option is to either return a server failure via 'err' which will cause the server to simply return a 500 HTTP error, or to use a workaround which uses the user object to return back the error message.
As a result, the done function will probably look something more like this:
done(null, false, 'bad password')
Here is a jsfiddle where I'm running into the issue. In this example, the server simply returns a 500 error for all failures -- and I want it to return an error string instead:
http://jsfiddle.net/nd8LZ/1/
The text was updated successfully, but these errors were encountered: