axelb:

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

ZachDaniel:

The way that I’m currently doing this is to redirect to the sign in page with flash on the root layout.

ZachDaniel:

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?

ZachDaniel:

<@346791515923939328> thoughts on this one?

ZachDaniel:

Actually its the weekend for you

ZachDaniel:

You’re a day ahead 😆

ZachDaniel:

I forget

axelb:

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:

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.

ZachDaniel:

However, I’m not sure if we will be able to entirely drop the failure handler

ZachDaniel:

Because it could fail on the redirect with a token that we will have to do after successful sign in.

ZachDaniel:

But that kind of failure would be exceedingly rare, and so a flash message is probably acceptable for it

frankdugan3:

Yeah, I agree w/ that assessment as well.

axelb:

Yeah, I’ve mostly played with password auth so far, so I’m speaking from this incomplete perspective.

axelb:

Agree that a flash is fine for those cases that don’t involve forms 👍

jart:

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.

jart:

So something like:

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.

jart:

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:

What kind of timing attacks do you have in mind?

ZachDaniel:

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=""&=

ZachDaniel:

And then it would validate the token as if it had been passed in an authentication header and sign you in

ZachDaniel:

So then we could submit the action in the liveview, and then redirect to /sign-in?...

jart:

The token contains enough information on its own to do that.

jart:

But also that’s a new strategy.

ZachDaniel:

ah, so then just ?token=""

jart:

I think we can just make a strategy that signs in that way.

ZachDaniel:

Does it have to be a new strategy?

jart:

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.

jart:

There is nothing wrong with making http calls. We did it for years.

jart:

I mean it’s such a trivial example you would write it in a plug in a few lines.

ZachDaniel:

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

ZachDaniel:

And by “all you’d add” I mean “all that would have to be added”

ZachDaniel:

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:

Okay maybe I am having fifi’s about people wanting to change my baby. Let me have a think about this.

ZachDaniel:

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.

ZachDaniel:

And thats not so good.

frankdugan3:

Well… Perhaps another option is to simply render the flash inside the form, right? Phoenix.Flash.get(@flash, @kind) ?

ZachDaniel:

Yeah, I actually made a branch that did that

ZachDaniel:

but it gets weird 🙂

ZachDaniel:

Normal forms will clear the flash when you have modified the form

ZachDaniel:

Well…actually I guess we can just track if the form has been altered at all and hide the message

frankdugan3:

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:

Yeah, I had it as authentication_error and then a helper function of set_authentication_error(...) in the failure handler

frankdugan3:

like :ash_auth_username_and_password or something.

ZachDaniel:

The other sort of annoying thing is that you have to pass the flash down through all of the components

ZachDaniel:

but that would technically be the “best of both worlds”

frankdugan3:

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:

Did you just come around to basically what I was thinking? Jimsy win!

ZachDaniel:

😆 well, I still don’t like the UX of it

ZachDaniel:

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

ZachDaniel:

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

ZachDaniel:

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.

ZachDaniel:

but like…not right now 😄

ZachDaniel:

So yes, Jimsy win 😆

ZachDaniel:

I think we should perhaps put some docs on this though, i.e “why don’t we log in with the liveview: “

ZachDaniel:

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)

ZachDaniel:

but we do need a way to show the errors better, and the special flash message will accomplish that 🙂

jart:

Yup I agree with that

ZachDaniel:

Can someone open a GH issue about this on the repo?

ZachDaniel:

I think this needs to happen for UX, but we don’t need this forum support page open anymore

axelb:

Done