Skip to content
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 support for custom verify callback to servers. #226

Merged
merged 1 commit into from
Jun 16, 2020

Conversation

Lukasa
Copy link
Contributor

@Lukasa Lukasa commented Jun 16, 2020

Motivation:

In #171 when we worked on providing access to the better verification
callback, we managed to entirely miss that we had not provided that
access to servers. This meant they were stuck only with the
substantially-less-useful old-school callback, instead of the much
better new-school one.

While we're here, as we had to add multiple new initializers to
NIOSSLServerHandler, I took the opportunity to also resolve the server
handler portion of #147. The issue itself is still open because the
client handlers still have throwing inits, but all "preferred"
initializers on NIOSSLServerHandler no longer throw.

Modifications:

  • Deprecated NIOSSLServerHandler.init(context:verificationCallback:)
  • Implemented two new initializers on NIOSSLServerHandler.
  • Added tests to verify that the NIOSSLServerHandler verification
    callback is actually called.
  • Removed all now-unnecessary try keywords.

Result:

Users will be able to provide custom verification callbacks that work
much better than they currently can when on the server, and the server
is now back into feature parity with the client.

@Lukasa Lukasa added the semver/minor Adds new public API. label Jun 16, 2020
@Lukasa Lukasa added this to the 2.8.0 milestone Jun 16, 2020
@Lukasa Lukasa requested review from weissi and glbrntt June 16, 2020 13:50
@@ -34,7 +34,7 @@ final class BenchManyWrites: Benchmark {
}

func setUp() throws {
let serverHandler = try NIOSSLServerHandler(context: self.serverContext)
let serverHandler = NIOSSLServerHandler(context: self.serverContext)
Copy link
Member

Choose a reason for hiding this comment

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

🥳

Copy link
Member

@weissi weissi left a comment

Choose a reason for hiding this comment

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

Thank you! This looks good to me!

Copy link
Contributor

@glbrntt glbrntt left a comment

Choose a reason for hiding this comment

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

Looks good!

Tests/NIOSSLTests/NIOSSLIntegrationTest.swift Show resolved Hide resolved
Motivation:

In apple#171 when we worked on providing access to the better verification
callback, we managed to entirely miss that we had not provided that
access to servers. This meant they were stuck only with the
substantially-less-useful old-school callback, instead of the much
better new-school one.

While we're here, as we had to add multiple new initializers to
NIOSSLServerHandler, I took the opportunity to also resolve the server
handler portion of apple#147. The issue itself is still open because the
client handlers still have throwing inits, but all "preferred"
initializers on NIOSSLServerHandler no longer throw.

Modifications:

- Deprecated NIOSSLServerHandler.init(context:verificationCallback:)
- Implemented two new initializers on NIOSSLServerHandler.
- Added tests to verify that the NIOSSLServerHandler verification
  callback is actually called.
- Removed all now-unnecessary try keywords.

Result:

Users will be able to provide custom verification callbacks that work
much better than they currently can when on the server, and the server
is now back into feature parity with the client.
@Lukasa Lukasa force-pushed the cb-set-custom-verify-server branch from 54570a1 to ca40773 Compare June 16, 2020 14:40
@Lukasa Lukasa merged commit 5a6ad12 into apple:master Jun 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver/minor Adds new public API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants