vapor / auth

👤 Authentication and Authorization framework for Fluent.
53 stars 34 forks source link

Unable to remove sessions with AuthenticationSessionsMiddleware enabled #59

Closed mikkelu closed 5 years ago

mikkelu commented 5 years ago

The problem

When using AuthenticationSessionsMiddleware it is not possible to delete sessions because of the way #52 has been fixed. I think I have found the problem, but I am not entirely sure. I don't think I am the first since this could be the same: https://github.com/vapor/vapor/pull/1667 and https://github.com/vapor/vapor/issues/1661.

I am also using Redis as sessions storage.

What happens?

When calling req.destroySession() and req.unauthenticate(User.self) the session and authenticated user in the cache are set to nil. This works as expected, however, when the AuthenticationSessionsMiddleware responds, it sees that the user is no longer authenticated, and calls req.unauthenticateSession(A.self) as implemented by #52. This in turns calls session() that sees the cache is set to nil and creates a new session.

For this reason, the SessionMiddleware(which responds after the auth session middleware) thinks we have created a new session, and stores that in the session storage. Not only does this mean that the previous session is not deleted, a new one is created, without any user context.

Potential fix

For the time being, I have been using a custom middleware that works like AuthenticationSessionsMiddleware but does not do any responding. Thus, the middleware never calls session() and I am able to correctly delete the session without a new one being created.

I have not created a PR, because I don't know how this should be handled appropriately. Because the middleware does not respond, you will have to call try req.authenticateSession(user) manually, and not req.authenticate(user) in your signin method.

Our temporary middleware looks something like:

final class UserSessionsMiddleware: Middleware {

    init() {}

    public func respond(to req: Request, chainingTo next: Responder) throws -> Future<Response> {
        if let id = try req.authenticatedSession(User.self) {
            // try to find user with id from session
            return User.authenticate(sessionID: id, on: req).flatMap { user in
                // if the user was found, auth it
                guard let user = user else {
                    throw SomeError
                }

                try req.authenticate(user)

                // return done
                return try next.respond(to: req)
            }
        } else {
            // no need to authenticate
            return try next.respond(to: req)
        }
    }
}
tanner0101 commented 5 years ago

I think what we need to do here is check that session exists before attempting to remove the user during req.unauthenticateSession(_:). That way we don't create an empty session if it has already been destroyed.

tanner0101 commented 5 years ago

Thanks for reporting this issue @mikkelu. It should be fixed in Auth 2.0.2. Please update and let me know if you have any problems.

mikkelu commented 5 years ago

@tanner0101 thank you! It works perfectly now! 🚀