-
Notifications
You must be signed in to change notification settings - Fork 13
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
feat: TypeSafeHTTPBasic #49
Conversation
…ura-CredentialsHTTP into typeSafeMiddleware
The tests are currently failing since they require typeSafeMiddleware which will only be available in Kitura 2.4. and it will need TypeSafeCredentials to be tagged. |
README.md
Outdated
|
||
public let id: String | ||
|
||
public static let users = ["John" : "12345", "Mary" : "qwerasdf"] |
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.
Probably make this private!
@Andrew-Lees11 FYI, Kitura and Credentials dependencies are merged and tagged - I have restarted the CI. |
Codecov Report
@@ Coverage Diff @@
## master #49 +/- ##
==========================================
- Coverage 80.23% 79.89% -0.35%
==========================================
Files 2 3 +1
Lines 167 199 +32
==========================================
+ Hits 134 159 +25
- Misses 33 40 +7
Continue to review full report at Codecov.
|
|
||
static var allTests : [(String, (TestTypeSafeBasic) -> () throws -> Void)] { | ||
return [ | ||
("testTypeSafeNoCredentials", testTypeSafeNoCredentials), |
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.
This array is missing some of the tests!
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.
Tests have been added and this file has been added to linux main
userid = requestUser | ||
password = requestPassword | ||
} | ||
else { |
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.
Please put the else on the previous line.
if let selfInstance = selfInstance { | ||
onSuccess(selfInstance) | ||
} | ||
else { |
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.
Same comment.
} | ||
let credentials = userAuthorization.components(separatedBy: ":") | ||
guard credentials.count >= 2 else { | ||
onFailure(.badRequest, nil) |
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.
return onFailure...
let body = try response?.readString() | ||
XCTAssertEqual(body,"{\"name\":\"Mary\",\"provider\":\"HTTPBasic\"}") | ||
} | ||
catch{ |
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.
Merge with previous line.
static var realm: String { get } | ||
|
||
/// The closure which takes a username and password and returns a TypeSafeHTTPBasic instance on success or nil on failure. | ||
static var verifyPassword: ((String, String, @escaping (Self?) -> Void) -> Void) { get } |
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.
This isn't very type-safe, did you consider replacing these Strings with something protocol-based?
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.
I think we should replace with something like
static func verifyPassword(username: String, password, String, callback: @escaping (Self?) -> Void) -> Void
Xcode will auto-complete this much nicer than the version where we define a closure signature, and apply the labels which indicates how to handle each argument.
|
||
public static let realm = "Login message" | ||
|
||
public static var verifyPassword: ((String, String, @escaping (MyHTTPBasic?) -> Void) -> Void) = |
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.
Usage example needs updating now that this is a function
/// The realm for which these credentials are valid (defaults to "User") | ||
static var realm: String { get } | ||
|
||
/// The closure which takes a username and password and returns a TypeSafeHTTPBasic instance on success or nil on failure. |
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.
Comment needs updating now that this is a function
Once typeSafeCredentials is merged we can implement HTTP basic auth using typeSafeMiddlware.
Docs description:
A
TypeSafeCredentials
plugin for HTTP basic authentication. This protocol will be implemented by a Swift object defined by the user. The plugin must implement averifyPassword
function which takes a username and password as input and returns an instance ofSelf
on success ornil
on failure. This instance must contain the authenticationprovider
(defaults to "HTTPBasic") and anid
, uniquely identifying the user. The users object can then be used in TypeSafeMiddlware routes to authenticate with HTTP basic.