Logged in users can visit /auth/login

[pharpend]

Steps to reproduce:

1. Build Snowdrift, run it via stack exec -- yesod devel.
2. Visit http://localhost:3000/auth/login, log in as admin, password admin.
3. Visit http://localhost:3000/auth/login again. You can even log in again.

We tried to fix this, thinking it was a bug in Snowdrift, but it appears to be a bug in yesod-auth.

Snowdrift ticket: https://github.com/snowdriftcoop/snowdrift/issues/144

Thanks,

Peter Harpending

[gregwebs]

Thanks for reporting. I think this hasn't been reported as an issue before because in most applications it is rare for a user to go straight to the login page. If you have an /auth/login page you would normally have links on your home page to enter the app, and then if they are not logged in they would get redirected to /auth/login.

I think fixing this is probably pretty easy though.

[wolftune]

@gregwebs Snowdrift is the same in that the links in the navigation change when you are logged in, so it remains rare for this to happen. It's a weird edge case we came across. But it's still awkward thing that should be fixed.

[paul-rouse]

I am a little concerned that there isn't a single obviously correct behaviour. If an authenticated user goes explicitly to /auth/login, do you redirect the same way as after a successful login (as decided by redirectToReferer and loginDest), or do you have a special page reminding them they are logged in, or do you give an error response? I can see reasons why any of those, or even the status quo, might be correct in a particular application, and conversely why any of them might seem odd to the user.

As an alternative, it is fairly easy to fix this in the application, and make whichever choice you want. The application can define loginHandler for the YesodAuth instance, and start with something like:

ma <- lift maybeAuthId
when (isJust ma) $
    lift $ redirect HomeR

However, this is a little harder than it needs to be because you probably want to drop into the default implementation straight after this, unless there are other reasons for overriding it. It would be made easier if we exposed the default implementation, say as defaultLoginHandler. If that's an acceptable answer, I am quite happy to PR it.

[wolftune]

My thinking would be: redirect as per a successful log-in but have an alert show that says "you are already logged in, click here to sign out if you want to log in as a different user" or something like that.

[paul-rouse]

Yes, in some cases that would probably be right. But would you be happy to implement it on a per-application basis if, as I suggested, it was made easier to re-use the default loginHandler code in your own extended loginHandler?

[wolftune]

I'm a fan of everything being/going upstream whenever possible as a guideline unless there's good reason not to…

[paul-rouse]

My reason against was simply that I think the required behaviour will depend on the application.

[gregwebs]

I think @wolftune makes a good case for changing the default behavior. But it is potentially a breaking change for anyone that likes the current behavior. We can always add a new typeclass method to configure the behavior for this.

[wolftune]

@paul-rouse I should clarify that if it were easy to implement the solution in our own specific app, we would indeed do so and be happy enough.

[paul-rouse]

OK, I'll have another look at it tomorrow (UK time).

[paul-rouse]

How about this, then. @gregwebs noted that fixing the default behaviour is a breaking change, and I agree, so shall we do it in two steps?

1. Separate out the default implementation of loginHandler straight away, so that @wolftune has an easy fix along the lines of

   instance YesodAuth App where
   ...
      loginHandler = do
          ma <- lift maybeAuthId
          when (isJust ma) $
              lift $ redirect HomeR   -- or any other Handler code you want
          defaultLoginHandler

   I think this is good anyway, so I'll go ahead and PR it.

2. At the next opportunity for breaking changes, provide an extra method in the YesodAuth typeclass to do something like this, and invoke it from defaultLoginHandler. I suggest the default implementation should be fairly simple - probably silently redirecting as if just logged in - and leave anything more complicated to an override in the application. ( @gregwebs - would I just submit a PR with a big comment saying it needs to be held for the next breaking change?)

[gregwebs]

Rather than providing a new typeclass configuration method, why not just encourage the above overriding? Perhaps put it in the haddock as an example for overriding loginHandler

[paul-rouse]

As you probably guessed yesterday, I am unconvinced by the need for my second step! Another alternative would be to put this in the scaffolding, which would have the effect of being educational while also changing the default for those who are new to Yesod. What do you think?

[gregwebs]

yep, scaffolding is fine. Although obviously you need to use the auth login redirect route instead of HomeR

[paul-rouse]

Yes, of course! I'll get something together soon.

[paul-rouse]

@gregwebs After some reflection, I think my idea of using the scaffolding to document this is OTT, so your suggestion of putting it in the haddock seems better. PR to follow in a minute!

@wolftune I thought, rather belatedly, to look at the Snowdrift code. It seems you have your own loginHandler in any case. Unless I am missing something, I think you have to add the test yourself (along the lines of my code above), since I can't see how it would be reasonable for yesod-auth to intervene and change the behaviour of a handler you have defined. (To be pedantic, it does wrap your handler, but only to track the referrer in the session, not to change anything else about it.) Is that a reasonable view?

[chreekat]

I'm closing this on Snowdrift.coop's behalf. :)