AE
Ash Elixir•3y ago
axelb

Display "Wrong credentials" on SignInLive

Hi, I have tried to tweak the AuthController to redirect to the SignIn with an error message in case of failure. The flash isn't available on the single liveview component, is there a built-in "Wrong credentials" message on SignInLive? This made me consider that maybe the normal use isn't to open directly the "/sign-in" route to liveview, but to embed it in a page with a layout? Cheers
27 Replies
ZachDaniel
ZachDaniel•3y ago
The way that I'm currently doing this is to redirect to the sign in page with flash on the root layout. Although I think this interaction needs a bit of work. We need to submit the action over a form post, but we could technically check the password in the liveview, and then submit the action over the form post. Would be checking their password twice, but would give the incorrect credentials message right away. Thoughts? @jart thoughts on this one? Actually its the weekend for you You're a day ahead šŸ˜† I forget
axelb
axelbOP•3y ago
Thanks for the reply, here are some thoughts gathered over the weekend. I think the flash message is not "local enough" and makes for less clear feedback and not great UX. This is why I would favor integrating the feedback in the SignInLive component. This way, we can get clear messages mapping to the _reason of failure, right next to the forms. Hopefully, the messages will be easy to override and internationalize. This is important to me at least. This brings the following considerations for the AuthController: if LiveView intercepts failure, the role of the failure function is unclear. I think it will be confusing if it is called only in "some" cases of failure. Will it ever be called, in fact? You know more about the usage of the failure function in the wild, but in my experience, 90+% of auth failure, render to the sign in form again, with some error message. No redirect is performed until validation passes and auth succeeds. So I think validation in LiveView and dropping failure function could be both convenient and lead to least surprise for devs (assuming things like rate and retry limiting, last connection, and such are handled at resource level by AshAuthentication 😁 ). What you think of these points?
ZachDaniel
ZachDaniel•3y ago
A few things: i18n will be it’s own discussion. Ash errors provide what you need to do it, but we’ll need to add things to ash authentication phoenix to support it. I agree on the UX pice as well. However, I’m not sure if we will be able to entirely drop the failure handler Because it could fail on the redirect with a token that we will have to do after successful sign in. But that kind of failure would be exceedingly rare, and so a flash message is probably acceptable for it
frankdugan3
frankdugan3•3y ago
Yeah, I agree w/ that assessment as well.
axelb
axelbOP•3y ago
Yeah, I've mostly played with password auth so far, so I'm speaking from this incomplete perspective. Agree that a flash is fine for those cases that don't involve forms šŸ‘
jart
jart•3y ago
Yeah I don’t think it’s a good idea. I am worried that we will leave ourselves exposed to timing attacks. I think perhaps we’d be better off to provide helpers so that you can put your own error message in the failure handler and we can pull it out of the conn and render it inside the form. So something like:
def failure … do
redirect_back_to_sign_in_with_error(ā€œUsername or password incorrectā€)
end
def failure … do
redirect_back_to_sign_in_with_error(ā€œUsername or password incorrectā€)
end
That’s a dumb name but you get the idea. Also the failure handler is called any time there’s a failure in any of the plugs so also on registration failures too. That’s why we pass the strategy and phase back.
ZachDaniel
ZachDaniel•3y ago
What kind of timing attacks do you have in mind? The flow I'm imagining that makes this UI better is that the /sign-in route can also be called as /sign-in?identity=user&token="<token>"&<identity_field>=<identity_value> And then it would validate the token as if it had been passed in an authentication header and sign you in So then we could submit the action in the liveview, and then redirect to /sign-in?...
jart
jart•3y ago
The token contains enough information on its own to do that. But also that’s a new strategy.
ZachDaniel
ZachDaniel•3y ago
ah, so then just ?token="<token>"
jart
jart•3y ago
I think we can just make a strategy that signs in that way.
ZachDaniel
ZachDaniel•3y ago
Does it have to be a new strategy?
jart
jart•3y ago
I think it makes the most sense that way otherwise we’re overloading the live view with a bunch of special case crud whereas at the moment they’re nice and clean. There is nothing wrong with making http calls. We did it for years. I mean it’s such a trivial example you would write it in a plug in a few lines.
ZachDaniel
ZachDaniel•3y ago
Yeah, no one is saying there is a problem with HTTP calls, but the UX of the http call in the context of a LV isn't as good. I'm not sure I see the issue with it TBH. All you'd add is
case Form.submit(form) do
{:ok, result} -> redirect(socket, to: sign_in_path(socket, ...%{token: result.__metadat__.token}))
end
case Form.submit(form) do
{:ok, result} -> redirect(socket, to: sign_in_path(socket, ...%{token: result.__metadat__.token}))
end
And by "all you'd add" I mean "all that would have to be added" and then the existing sign-in route can handle the token parameter being present, and then redirect you to where you actually want to go
jart
jart•3y ago
Okay maybe I am having fifi’s about people wanting to change my baby. Let me have a think about this.
ZachDaniel
ZachDaniel•3y ago
One thing you're right about though is that this technically allows password enumeration in the UI without the ability to rate limit using a plug. And thats not so good.
frankdugan3
frankdugan3•3y ago
Well... Perhaps another option is to simply render the flash inside the form, right? Phoenix.Flash.get(@flash, @kind)?
ZachDaniel
ZachDaniel•3y ago
Yeah, I actually made a branch that did that but it gets weird šŸ™‚ Normal forms will clear the flash when you have modified the form Well...actually I guess we can just track if the form has been altered at all and hide the message
frankdugan3
frankdugan3•3y ago
Can give it a special type of :kind as well so it doesn't get pick up by the user's normal flash UI.
ZachDaniel
ZachDaniel•3y ago
Yeah, I had it as authentication_error and then a helper function of set_authentication_error(...) in the failure handler
frankdugan3
frankdugan3•3y ago
like :ash_auth_username_and_password or something.
ZachDaniel
ZachDaniel•3y ago
The other sort of annoying thing is that you have to pass the flash down through all of the components but that would technically be the "best of both worlds"
frankdugan3
frankdugan3•3y ago
Yeah, I really do think that would probably be the more flexible and secure way to go about it... But I'm certainly no expert on the matter.
jart
jart•3y ago
Did you just come around to basically what I was thinking? Jimsy win!
ZachDaniel
ZachDaniel•3y ago
šŸ˜† well, I still don't like the UX of it but unless we add rate limiting at the action level as a default feature, then its probably not okay to submit it in the LV Although FWIW it would probably be so trivial to have ash_rate_limiter that comes with changes you can drop into an action that will rate limit based on data/context/actor/arguments It would use some existing hex dependency to do the rate limiting, and then we could set context on the actions when invoked from web interfaces, like ip/ip region and stuff like that. but like...not right now šŸ˜„ So yes, Jimsy win šŸ˜† I think we should perhaps put some docs on this though, i.e "why don't we log in with the liveview: " People will understand that we make concessions when it comes to authentication for the sake of security/securability (they still need to write these plugs if they want theM) but we do need a way to show the errors better, and the special flash message will accomplish that šŸ™‚
jart
jart•3y ago
Yup I agree with that
ZachDaniel
ZachDaniel•3y ago
Can someone open a GH issue about this on the repo? I think this needs to happen for UX, but we don't need this forum support page open anymore
axelb
axelbOP•3y ago
Done

Did you find this page helpful?