New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add default implementation for CredentialsAuthenticatable #711
Conversation
public let database: DatabaseID? | ||
|
||
func authenticate(credentials: ModelCredentials, for request: Request) -> EventLoopFuture<Void> { | ||
User.query(on: request.db(self.database)).filter(\._$username == credentials.username).first().flatMapThrowing { foundUser in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe, since this is a default implementation that a lot of people will end up using, we should do something like this:
.all()
.flatMapThrowing {
#if DEBUG
assert($0.count <= 1 , "Usernames must be unique in your database")
#else
guard $0.count <= 1 else throw Abort(.conflict, reason: "Found more than one matching user")
#endif
return $0.first
}
I'd hate for it to turn into another potential footgun for the unsuspecting 😕. Granted, this would only catch the case where someone actually did end up with multiple of the same username, but at least it's something.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given this affects all of the authenticators, we should probably do that as a separate PR
@@ -99,7 +99,7 @@ private struct DatabaseSessionAuthenticator<User>: SessionAuthenticator | |||
let databaseID: DatabaseID? | |||
|
|||
func authenticate(sessionID: User.SessionID, for request: Request) -> EventLoopFuture<Void> { | |||
User.find(sessionID, on: request.db).map { | |||
User.find(sessionID, on: request.db(self.databaseID)).map { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ouch, I don't like that this was missing at all 😕
These changes are now available in 4.1.0 |
Improves the experience for users writing web applications. Adds a
ModelCredentialsAuthenticator
to automatically conformModel
types toCredentialsAuthenticatable
and provide a middleware to use.This can be used when logging in users via a web form, as shown in the tests. This also backfills some tests for ModelSessionAuthenticatable.
Also fixes a bug where the SessionAuthenticator was not using the provided
DatabaseID
Resolves #710
Resolves #701
Docs here vapor/docs#576