-
Notifications
You must be signed in to change notification settings - Fork 43
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
Registration fix #130
Registration fix #130
Conversation
Codecov Report
@@ Coverage Diff @@
## master #130 +/- ##
==========================================
+ Coverage 72.46% 72.77% +0.31%
==========================================
Files 15 15
Lines 1551 1554 +3
Branches 268 266 -2
==========================================
+ Hits 1124 1131 +7
+ Misses 315 314 -1
+ Partials 112 109 -3
Continue to review full report at Codecov.
|
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 almost good to me. If @vodik doesn't have any comment we can merge once the typo in the test if fiexd
tests/test_registration.py
Outdated
|
||
def test_without_qop(): | ||
authenticate = auth.Auth.from_authenticate_header( | ||
AUTH['auth_with_qop'], |
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.
you probably meant auth_without_qop
here
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, and the test suddenly starts failing. Investigating.
I'm just going to quickly tweak this and get it merged upstream. I want to change the stringy handling of authentication schemes to use an enum. This currently won't crash when we see an unsupported authentication scheme, which was part of the problem in the first place. If it has crashed then, we would have been able to fix this a lot faster. |
0cdca70
to
eacc4b4
Compare
Use enumerations for specifying authentication support. This will quickly throw a value error should we hit an unknown authentication scheme instead of falling back on default behaviours that might not be correct. Always throw a key error if part of the authentication header is missing, instead of defaulting to None or empty strings. This should also help quickly catch errors if we send out invalid authentication headers.
eacc4b4
to
a72237d
Compare
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'm happy with this. I'll merge this tonight unless you guys got anything more to complain about. Then I'll start looking at getting back to my dialog work.
@vodik I'm glad to see you are back & I hope everything is ok. Just to let you know what we have been up to, I recently took a bit of time to work on the client to imagine and test an API that would work nicely. You can find that there (https://github.com/Eyepea/aiosip/compare/client) . It breaks a lot of things since I only wanted to experiment but it might be interesting for your dialog work. The test.py file contains a working example |
This pull request cherry pick the sip auth fix from #125 since it is needed for our testing.
The unit test is added to verify the expected digest response.