Skip to content

Conversation

@maximumG
Copy link
Contributor

@maximumG maximumG commented Aug 7, 2018

Regarding discussion in issue #172


self.protocol.set_auth_methods(['password', 'publickey'])
self.assertTrue(self.protocol.get_auth_methods() is not None)
self.assertListEqual(self.protocol.get_auth_methods(), ['password', 'publickey'])

Choose a reason for hiding this comment

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

line too long (89 > 79 characters)

Defines the SSH2 list of authentication methods allowed
:type methods: list
:param methods: A list of authentication methods (check Exscript.protocols.ssh2.auth_type)

Choose a reason for hiding this comment

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

line too long (98 > 79 characters)

:type encoding: str
:keyword encoding: The encoding of data received from the remote host.
:type auth_methods: list
:keyword auth_methods: The SSH authentication method to process (default to all supported

Choose a reason for hiding this comment

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

line too long (97 > 79 characters)

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.1%) to 76.576% when pulling f542b1e on maximumG:auth_method_as_param into cc0b63b on knipknap:master.

@maximumG
Copy link
Contributor Author

Anyone would be able to check this pull request, please ?


def _get_auth_methods(self, allowed_types):
auth_methods = []
if self.auth_methods:
Copy link
Owner

Choose a reason for hiding this comment

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

Wouldn't it make more sense to do something like

auth_method_handlers = [] if self.auth_methods: auth_methods = [m for m in self.auth_methods if m in allowed_types] else: auth_methods = allowed_types for method in auth_methods: for type_name in auth_types[method]: auth_method_handlers.append(getattr(self, type_name)) return auth_method_handlers 

Otherwise Exscript wouldn try to authenticate using unsupported methods.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are absolutely right. Thanks for the review - I will change the code.

self.banner_timeout = banner_timeout
self.encoding = encoding
self.send_data = None
self.auth_methods = auth_methods
Copy link
Owner

@knipknap knipknap Aug 28, 2018

Choose a reason for hiding this comment

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

I would do a sanity check here, i.E.:

for method in auth_methods: if method not in auth_types: raise ValueError('unsupported auth_method: ' + repr(method)) 
Copy link
Contributor Author

@maximumG maximumG Aug 29, 2018

Choose a reason for hiding this comment

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

I thought about this check in the beginning. However the auth_type dict is in the ssh2.py module while this should be done in module protocol.py. If we import ssh2 in protocol we will end up with a circular import.

Another solution would be to have the auth_method attribute set directly inside ssh2 and not in protocol

Which one would you prefer to choose ?

Copy link
Owner

Choose a reason for hiding this comment

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

I would leave "self.auth_methods = auth_methods" in the protocol adapter, but only check the sanity in the SSH2 adapter. That would allow for code that can be interchanged between Telnet and SSH.

@knipknap knipknap mentioned this pull request Aug 28, 2018
@maximumG
Copy link
Contributor Author

Sorry for the long delay for this pull request. I will try to work on this ASAP

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

4 participants