Updated Jim Studt's PR to support servers seeing user's usernames #125
Conversation
|
Can one of the admins verify this patch? |
Joannis
force-pushed
the
feature/jo-usernames
branch
from
f5e22a8
to
d69936d
Compare
| @@ -57,11 +57,21 @@ struct SSHConnectionStateMachine { | |||
| case sentDisconnect(SSHConnectionRole) | |||
| } | |||
|
|
|||
| class Attributes { | |||
There was a problem hiding this comment.
Is there any reason this isn't a struct?
There was a problem hiding this comment.
Ah, I see: it's not a struct because we need some action at a distance to store the value.
I don't love that. I wonder if we can refactor the code a bit. We have a few options.
The first option is that we could force the user auth delegate to tell us which username they're accepting. That's kind of tricky to do and a bit awkward.
The second option is that we could store a class only for the lifetime of the key exchange, and use that. Then reify this into a struct.
The third option is that we could use a promise to resolve the username. I don't love that much, but it's do-able without being too painful.
The fourth option is to change the return value convention here somewhat so that we can also resolve a side-effect. Do you have any thoughts here?
There was a problem hiding this comment.
Hey Cory, thanks for the quick follow-up! I concur that it's sub-optimal. I merely updated the PR so you could leave your feedback, but will take it from here.
I don't think option 1 and 3 is a great API. It's going to be an awkward API to use for sure. One good option - in my eyes - is sending an event on the channel that indicates which user was authenticated. This leaves catching it to the implementations like Citadel.
There was a problem hiding this comment.
I'm open to sending an event on the channel.
| /// The state of this state machine. | ||
| private var state: State | ||
|
|
||
| /// Attributes of the connection which can be changed by messages handlers | ||
| private let attributes: Attributes |
There was a problem hiding this comment.
As each of the states has a reference to the attributes, I think I'd recommend making this computed and switching over the states to get hold of the value.
| /// Attributes of the connection which can be changed by messages handlers | ||
| private let attributes: Attributes | ||
|
|
||
| var username: String? { attributes.username } |
There was a problem hiding this comment.
Not sure we need a computed property for this exactly here.
Fixes #57 and obsoletes #58