Skip to content

feat: TypeSafeHTTPBasic#49

Merged
ianpartridge merged 27 commits intomasterfrom
typeSafeMiddleware
Jun 5, 2018
Merged

feat: TypeSafeHTTPBasic#49
ianpartridge merged 27 commits intomasterfrom
typeSafeMiddleware

Conversation

@Andrew-Lees11
Copy link
Copy Markdown
Contributor

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 a verifyPassword function which takes a username and password as input and returns an instance of Self on success or nil on failure. This instance must contain the authentication provider (defaults to "HTTPBasic") and an id, uniquely identifying the user. The users object can then be used in TypeSafeMiddlware routes to authenticate with HTTP basic.

@Andrew-Lees11
Copy link
Copy Markdown
Contributor Author

Andrew-Lees11 commented Jun 1, 2018

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.

@Andrew-Lees11 Andrew-Lees11 requested a review from djones6 June 1, 2018 08:43
README.md Outdated

public let id: String

public static let users = ["John" : "12345", "Mary" : "qwerasdf"]
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably make this private!

@djones6
Copy link
Copy Markdown
Contributor

djones6 commented Jun 1, 2018

@Andrew-Lees11 FYI, Kitura and Credentials dependencies are merged and tagged - I have restarted the CI.

@codecov-io
Copy link
Copy Markdown

codecov-io commented Jun 1, 2018

Codecov Report

Merging #49 into master will decrease coverage by 0.34%.
The diff coverage is 78.12%.

Impacted file tree graph

@@            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
Flag Coverage Δ
#CredentialsHTTP 79.89% <78.12%> (-0.35%) ⬇️
Impacted Files Coverage Δ
Sources/CredentialsHTTP/TypeSafeHTTPBasic.swift 78.12% <78.12%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f9328fb...384fbdc. Read the comment docs.


static var allTests : [(String, (TestTypeSafeBasic) -> () throws -> Void)] {
return [
("testTypeSafeNoCredentials", testTypeSafeNoCredentials),
Copy link
Copy Markdown
Collaborator

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!

Copy link
Copy Markdown
Contributor Author

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 {
Copy link
Copy Markdown
Collaborator

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 {
Copy link
Copy Markdown
Collaborator

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)
Copy link
Copy Markdown
Collaborator

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{
Copy link
Copy Markdown
Collaborator

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 }
Copy link
Copy Markdown
Collaborator

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?

Copy link
Copy Markdown
Contributor

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) =
Copy link
Copy Markdown
Contributor

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.
Copy link
Copy Markdown
Contributor

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

@ianpartridge ianpartridge merged commit cb3445b into master Jun 5, 2018
@ianpartridge ianpartridge deleted the typeSafeMiddleware branch June 5, 2018 11:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants