-
Notifications
You must be signed in to change notification settings - Fork 101
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 optional parameter domain to HttpNtlmAuth __init__ #86
Conversation
This commit resolves requests#85
@@ -13,25 +13,28 @@ class HttpNtlmAuth(AuthBase): | |||
Supports pass-the-hash. | |||
""" | |||
|
|||
def __init__(self, username, password, session=None): | |||
def __init__(self, username, password, session=None, domain=None): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer to have domain
before session
but I'm not sure if that would break backwards compatibility and how important that is in this case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, we can't do that really. Sadly, positional arguments are still a thing.
self.username = username | ||
self.domain = '.' | ||
else: | ||
self.username, self.domain = (username, '.') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wasn't sure about leaving '.'
as the default domain or just an empty string. Maybe @nitzmahone knows more about this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did you refactor this code block out of its form of try...except
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did it because I thought it became indented too far. But you're right it wasn't necessary. I reverted code to the try...except
form.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This generally looks good, but I have some notes.
@@ -13,25 +13,28 @@ class HttpNtlmAuth(AuthBase): | |||
Supports pass-the-hash. | |||
""" | |||
|
|||
def __init__(self, username, password, session=None): | |||
def __init__(self, username, password, session=None, domain=None): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, we can't do that really. Sadly, positional arguments are still a thing.
"""Create an authentication handler for NTLM over HTTP. | ||
|
||
:param str username: Username in 'domain\\username' format | ||
:param str username: Username |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than remove the old note, let's just extend it to explain what's going on.
self.username = username | ||
self.domain = '.' | ||
else: | ||
self.username, self.domain = (username, '.') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did you refactor this code block out of its form of try...except
?
"""Create an authentication handler for NTLM over HTTP. | ||
|
||
:param str username: Username in 'domain\\username' format | ||
:param str username: Username. If in either 'domain\\\\username' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks ugly in code but 4 slashes are necessary for doc gen (at least that's how PyCharm works).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wait, you need four backslashes for PyCharm to render one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. I guess that's because both Python and reStructuredText treat backslash as an escape character. If HTML docs aren't generated anywhere I'm happy to leave two backslashes. This way printing __doc__
will display it in the correct format.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently we don't render HTML docs, so it might be better to just leave it in the double form.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. I also made docstring raw as suggested by @nitzmahone.
@@ -13,25 +13,34 @@ class HttpNtlmAuth(AuthBase): | |||
Supports pass-the-hash. | |||
""" | |||
|
|||
def __init__(self, username, password, session=None): | |||
def __init__(self, username, password, session=None, domain=None): | |||
"""Create an authentication handler for NTLM over HTTP. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prefacing this with r
to make it a raw string will get rid of the need forone set of escaping... Have to do this all the time in Ansible module docs for Windows paths (YAML embedded in python here-string)...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like a print statement got past you.
@@ -170,6 +179,7 @@ def __call__(self, r): | |||
r.register_hook('response', self.response_hook) | |||
return r | |||
|
|||
print(HttpNtlmAuth.__init__.__doc__) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We probably don't need this. ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I guess we don't... :)
I've set up a forest with the requisite UPN suffixes to test this- will report back... |
In my testing, the extra parameter is not necessary under any circumstances if we just fix the code to do things the right way. Buried in Microsoft's docs for LogonUser, they say that the domain should never be passed for UPN stuff anyway. IIRC, I'm the one that added it anyway, so this PR removes it. I've tested this PR against both IIS and WinRM (via Ansible) for a couple Windows versions (local/domain users), as well as with real and alternate UPN domain suffixes (ie, what the original bug report was for), and it works great. |
Closed in favour of #89. |
This commit resolves #85