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

Adds emittion of user authentication success event. #76

Merged
merged 2 commits into from Mar 3, 2021

Conversation

artemredkin
Copy link
Contributor

Motivation:
Currently there is now way to wait for successful SSH session
establishment, the only way to continue is to try to create a child
channel. Given that most clients are expecting the session to be active
when both key exchange and user authenticaiton complete, I propose to
emit UserAuthSuccessEvent. That way client can wait for that event or
fail if there is an error in the pipeline.

Modifications:

  • Adds UserAuthSuccessEvent event
  • Adds .event case to StateMachineInboundProcessResult
  • Adds emittion of event if StateMachineInboundProcessResult is .event
  • Adds two tests

Result:
Closes #39

@artemredkin artemredkin requested a review from Lukasa March 1, 2021 14:48
@artemredkin
Copy link
Contributor Author

@swift-server-bot test this please

1 similar comment
@artemredkin
Copy link
Contributor Author

@swift-server-bot test this please

@tomerd
Copy link
Member

tomerd commented Mar 2, 2021

@swift-server-bot test this please

1 similar comment
@tomerd
Copy link
Member

tomerd commented Mar 2, 2021

@swift-server-bot test this please

@Lukasa Lukasa added the 🔼 needs-minor-version-bump For PRs that when merged cause a bump of the minor version, ie. 1.x.0 -> 1.(x+1).0 label Mar 3, 2021
Copy link
Collaborator

@Lukasa Lukasa left a comment

Choose a reason for hiding this comment

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

Nice clean patch, I really like it.

@@ -16,6 +16,9 @@ protocol AcceptsUserAuthMessages {
var userAuthStateMachine: UserAuthenticationStateMachine { get set }
}

/// This event indicates that server accepted our response to authentication challenge. SSH session can be considered active after that.
public struct UserAuthSuccessEvent {}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we add a public init to this? Otherwise users can't construct instances of it. We should also make it Hashable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done!

@Lukasa
Copy link
Collaborator

Lukasa commented Mar 3, 2021

One little nit and then we're good to go.

Motivation:
Currently there is now way to wait for successful SSH session
establishment, the only way to continue is to try to create a child
channel. Given that most clients are expecting the session to be active
when both key exchange and user authenticaiton complete, I propose to
emit UserAuthSuccessEvent.  That way client can wait for that event or
fail if there is an error in the pipeline.

Modifications:
 - Adds UserAuthSuccessEvent event
 - Adds .event case to StateMachineInboundProcessResult
 - Adds emittion of event if StateMachineInboundProcessResult is .event
 - Adds two tests

Result:
Closes apple#39
@Lukasa Lukasa merged commit 7f5b8ac into apple:main Mar 3, 2021
@artemredkin artemredkin deleted the add_userauth_success_event branch March 3, 2021 14:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🔼 needs-minor-version-bump For PRs that when merged cause a bump of the minor version, ie. 1.x.0 -> 1.(x+1).0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Introduce handshake complete/failed events
3 participants