Update credential requirements #13
Conversation
jborean93
force-pushed
the
credential
branch
2 times, most recently
from
9406cf3
to
dca9623
Compare
src/sspilib/_sec_context.py
Outdated
| credential: A credential to use for authentication, defaults to the | ||
| current user context if not set. |
There was a problem hiding this comment.
There is no defaulting anymore.
There was a problem hiding this comment.
Thanks for the pickup, has been updated.
jborean93
force-pushed
the
credential
branch
2 times, most recently
from
d2dceb9
to
fd0c16b
Compare
Update the documentation that specifies a credential MUST be specified when creating a security context. This is technically a breaking change but it should be a minimal one.
| channel_bindings: Optional channel bindings to tie the context to. | ||
| """ | ||
|
|
||
| def __init__( | ||
| self, | ||
| credential: raw.CredHandle, |
There was a problem hiding this comment.
Sorry, I did not catch this earlier, but placing credential after target_name is probably a less disruptive change. target_name was a single required argument, so in my code I had ClientSecurityContext(spn, credential=UserCredential()), which will be broken by this change. I now updated to ClientSecurityContext(target_name=spn, credential=UserCredential()), but making credential the second argument would work without any changes.
There was a problem hiding this comment.
Yea I did consider this and found that having it first aligns this with the server context object and as it was already a breaking change and I couldn't find any active examples that used the failing way in GitHub search I decided to just go with it.
There was a problem hiding this comment.
I found one example in pyinstaller's tests: https://github.com/pyinstaller/pyinstaller-hooks-contrib/blob/5a553ff210635443d41ed5dab97f47196fcfa8e3/src/_pyinstaller_hooks_contrib/tests/test_libraries.py#L1770
There was a problem hiding this comment.
Which is actually the only use I've found outside of this repo :)
There was a problem hiding this comment.
Thanks for finding that, I'll submit a PR to use the syntax that will work with both versions. I prefer to keep it consistent especially in the early stages of this project and in IMO people would probably expect the credentials to be the first thing they would supply.
There was a problem hiding this comment.
Update the documentation that specifies a credential MUST be specified when creating a security context. This is technically a breaking change but it should be a minimal one.
I've done a search on github for any users of this library and could not find any situations where the change in positional arguments would cause any issues. That doesn't rule it out completely but I'm fairly confident this should be a minimally impacted change. To have code that works with both then using the keyword in the constructor will work with both for example:
Fixes: #11