Skip to content

Conversation

@dvolodin7
Copy link
Contributor

Session.userauth_keyboardinteractive method

@pkittenis
Copy link
Member

Hey there,

Thanks for the PR and your contribution!

Can you please remove the libssh2 1.9.0 upgrade as part of this PR. That should be done separately from adding keyboard interactive implementation if you want to make a separate PR for it, or I can.

The kbd interactive changes look good but need a short test to go in.

@dvolodin7
Copy link
Contributor Author

Greetings,

I've cleaned the PR. Please take a look

@pkittenis
Copy link
Member

Looks good, thanks for making it. Can you add a short test for the new userauth_keyboardinteractive session function? See existing session tests.

@dvolodin7
Copy link
Contributor Author

dvolodin7 commented Jul 4, 2019

Looks good, thanks for making it. Can you add a short test for the new userauth_keyboardinteractive session function? See existing session tests.

Username and password needed. Embedded OpenSSHServer does not provide one.
I guess it will try against CI container.

@pkittenis
Copy link
Member

pkittenis commented Jul 5, 2019

A test for exception is ok, just need to exercise the new code path. It should be raising exception on failure.

@dvolodin7
Copy link
Contributor Author

Tried to. Got AuthenticationError not captured by assertRaises

@dvolodin7
Copy link
Contributor Author

BTW, we're using own custom container for ssh tests - https://code.getnoc.com/infrastructure/tests/testsshd

It allows to build more complicated tests, considering positive and negative scenarios:
https://code.getnoc.com/noc/noc/merge_requests/1990/diffs#0c47f52f2a6192b16db8ccde786bbca459d9c3a2_0_53

@pkittenis pkittenis merged commit e5fdd3e into ParallelSSH:master Jul 5, 2019
@pkittenis
Copy link
Member

pkittenis commented Jul 5, 2019

Thanks for that container pointer, looks good. Was looking for something similar - the embedded server can be a bit flaky, and not suited to more complex stuff. Made #81 to migrate to it.

Thanks again for the contribution.

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.

2 participants