Skip to content
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

Digest Authentification support for the legacy client part only #1111

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

Nicolas-Feude
Copy link

From the module "requests", but works on its own, totaly independent from it

From the module "requests", but works on its own, totaly independent from it
@Nicolas-Feude
Copy link
Author

Hello aaugustin!

First of all, sorry for not having done any actions on this PR, I've been really busy this last month...
I'd like to modify the quality tests, in order to make it ok with this new feature (digest authentification), but i don't know where to start, and what to change.
The current tests seems to handle only basic auth, which may explain why it fails with the digest auth.
Do you have any idea or suggestions about what i should do?

@Nicolas-Feude
Copy link
Author

Hello
I don't know if the tests have been run after the commit 0851a24. The modification should have solved the error, but I don't have any results.
Could you run the tests if not already done?

@Nicolas-Feude
Copy link
Author

Thanks

There is no more error message, but the coverage is not 100%. I obviously still have problems in this feature. I don't really know how to read the Tox report...
I can see that the issues are in headers.py (57%), legacy/client.py (89%) and legacy/handshake.py (95%) which are the files i modified for this PR... But i don't really understand what the "missing" column stand for, and what are the defaults/errors.

I'm sorry, I've never used Tox before, so I don't know what to do.

Is there a more detailled report ?

@Nicolas-Feude
Copy link
Author

ok, i'm reading some documentation about tox and if i interpret correctly, the missing column is about the code that is not covered by the tests. So it means that i must write some tests to cover the new feature.

Sorry for the silly questions, i'm really newbie at tox/unit tests things

@Nicolas-Feude
Copy link
Author

Is that possible to write digest auth coverage tests without having the server side modified to support digest auth ? I am not sure, but i guess that the server side digest auth is mandatory when the client side support the digest auth.

If i'm right the server must be modified to support digest auth. Am i correct ?

@ciacon
Copy link

ciacon commented Nov 6, 2022

I am seeing two different attempts of attempts of getting digest authentication into websockets - here and in #788 .
While I see some activity in both, I am not actually seeing any progress in either.

What is needed to get this ball rolling? :)

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.

3 participants