maybeAuthId and maybeAuth may be inconsistent

[meteficha]

I'd expect the following property consistent to always hold.

consistent = do
  (a,b) <- runDB $ (,) <$> maybeAuthId <*> maybeAuth
  isJust a `shouldBe` isJust b

Unfortunately, it may not hold on some corner cases (although not corner enough that they were already seen):

1. User A logs in.
2. User A is removed from the database (cookies are not cleared).
3. User A visits the site.

I don't see any way of fixing this bug besides doing a roundtrip to the DB on maybeAuthId, but it is not desirable to do so in general. Thoughts?

[snoyberg]

I've actually run into this myself on occasion. I actually try to avoid using maybeAuthId at all in my code. The problem is that there are definitely some use cases where maybeAuth isn't a possibility, such as when the user information is not stored in Persistent. Perhaps our solution should be to document this behavior and simply not export maybeAuthId from the standard modules or in the scaffolding.

[meteficha]

The problem I've run into was due to a use of maybeAuthId on yesod-auth-fb, so you'd recommend me to change it into a maybeAuth even though I won't use that information at all?

[meteficha]

If we could cache the result of maybeAuth on the request-local storage it would become cheap to implement maybeAuthId = fmap entityKey <$> maybeAuth. And the cache would be beneficial anyway. Finally, it seems that the current CacheKey suffices to implement this feature.

[snoyberg]

That's a good idea. I'd actually like to implement #268 for the 1.2 release. So let's approach this by implementing the caching behavior for maybeAuth right now using the current infrastructure and then upgrading to the new approach in the 1.2 branch. This can work as a proving case that we've implemented the feature properly.

[meteficha]

Now that #268 is solved it should be fairly easy to solve this issue.

[snoyberg]

Good point; I've refactored slightly to make this work. Can you review my commit in b5a1d76?

[meteficha]

Looks good, there are just a few places that I think you've used the wrong function.

[snoyberg]

It wasn't by mistake: maybeAuthId makes more requirements than maybeAuthIdRaw does. I was playing around with the idea of using DefaultSignatures to solve the problem, and eventually got it working in 7748a19.

[meteficha]

Ok, now I see it. I'd just like to see a big fat warning on cachedAuth stating that it must not be called with different arguments on the same request.

[snoyberg]

I don't mind adding the warning, but neither cachedAuth nor CachedMaybeAuth is exported, which makes it impossible for users to accidentally mess things up. The only way to have a problem is if Yesod.Auth itself had a bug.

[gregwebs]

Should this ticket be closed?

[snoyberg]

Yes, thanks.