vapor / auth

đŸ‘¤ Authentication and Authorization framework for Fluent.
53 stars 34 forks source link

Conforming Fluent models to BasicAuthenticable when username/password types are optional (i.e. String?) #43

Closed kevinbrewster closed 5 years ago

kevinbrewster commented 6 years ago

Consider the scenario where not all users have username/passwords and thus those properties are optional.

Currently it's not possible to conform the below model to the BasicAuthenticable protocol because you need to supply WritableKeyPath<User, String> keys, not WritableKeyPath<User, String?>.

final class User : MySQLModel {
    var id: Int?
    var username: String?
    var passwordHash: String?
}

What do you think about allowing username /password fields of type WritableKeyPath<User, String?> to also conform to BasicAuthenticable?

I think this can be pulled off like so:

public protocol BasicAuthenticatableKeyType { }
extension String : BasicAuthenticatableKeyType {}
extension Optional : BasicAuthenticatableKeyType where Wrapped == String {}

/// Authenticatable by `Basic username:password` auth.
public protocol BasicAuthenticatable: Authenticatable {
    associatedtype UsernameKeyType: BasicAuthenticatableKeyType
    associatedtype PasswordKeyType: BasicAuthenticatableKeyType

    /// Key path to the username
    typealias UsernameKey = WritableKeyPath<Self, UsernameKeyType>

    /// The key under which the user's username,
    /// email, or other identifing value is stored.
    static var usernameKey: UsernameKey { get }

    /// Key path to the password
    typealias PasswordKey = WritableKeyPath<Self, PasswordKeyType>

    /// The key under which the user's password
    /// is stored.
    static var passwordKey: PasswordKey { get }

    /// Authenticates using the supplied credentials, connection, and verifier.
    static func authenticate(using basic: BasicAuthorization, verifier: PasswordVerifier, on connection: DatabaseConnectable) -> Future<Self?>

    var basicPassword: String? { get }
}

extension BasicAuthenticatable  where PasswordKeyType == String {
    public var basicPassword: String? { return self[keyPath: Self.passwordKey] }
}
extension BasicAuthenticatable  where PasswordKeyType == String? {
    public var basicPassword: String? { return self[keyPath: Self.passwordKey] }
}

extension BasicAuthenticatable where Self: Model, Self.Database: QuerySupporting {
    public static func authenticate(using basic: BasicAuthorization, verifier: PasswordVerifier, on conn: DatabaseConnectable) -> Future<Self?> {
        do {
            let filter = ModelFilter<Self>(filter: try QueryFilter(field: usernameKey.makeQueryField(), type: QueryFilterType<Self.Database>.equals, value: .data(basic.username)))
            return Self.query(on: conn).filter(filter).first().map(to: Self?.self) { user in
                guard let user = user, let basicPassword = user.basicPassword, try verifier.verify(basic.password, created: basicPassword) else {
                    return nil
                }
                return user
            }
        } catch {
            return conn.eventLoop.newFailedFuture(error: error)
        }
    }
}
0xTim commented 6 years ago

Since Basic Authentication requires a username and password, I don’t get the benefit of allowing the models to not supply it?

kevinbrewster commented 6 years ago

Let's say you have an online store and with a Customer model. Customers can optionally create a username/password but it's not required.

When someone logs in with Basic Authentication, I want to be able to search through the Customers to find a match with the same username/password even though not all customers have credentials (i.e. Basic Authentication skips over or ignores customers with a nil username or password).

tanner0101 commented 6 years ago

@kevinbrewster an alternative way to model that would be to create a CustomerCredentials entity that is a child of a Customer. The credentials type would have non-optional values, yet it would still be optional on the customer because it may or may not have that child.

Given this is a somewhat unique situation, I think it could be preferable to handle it that way rather than make the base case more complex.

0xTim commented 5 years ago

Closing due to inactivity - feel free to reopen!