-
Notifications
You must be signed in to change notification settings - Fork 21
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
Fix #377: Allow basic auth-scheme #384
Conversation
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.
Thank you for taking the time to contribute!
I gave a few points to improve before we can merge, but they are details
Thank you for sharing the details. Will work on them and send a new pr.
…On Tue, 18 Feb 2025 at 12:53, Mathieu Leplatre ***@***.***> wrote:
***@***.**** requested changes on this pull request.
Thank you for taking the time to contribute!
I gave a few points to improve before we can merge, but they are details
------------------------------
In tests/test_session.py
<#384 (comment)>:
> @@ -250,6 +250,12 @@ def test_auth_can_be_passed_as_basic_header(session_setup: Tuple[MagicMock, Sess
assert isinstance(session.auth, kinto_http.BearerTokenAuth)
assert session.auth.type == "Bearer+OIDC"
assert session.auth.token == "abcdef"
+ session = create_session(auth="Basic asdfghjkl;")
I know that this part very related, but we try to maintain a test per
scope. Could you please split this test into a dedicated one?
------------------------------
In src/kinto_http/__init__.py
<#384 (comment)>:
> @@ -37,5 +37,6 @@ def __init__(self, token, type=None):
self.type = type or "Bearer"
def __call__(self, r):
+ # Sets auth-scheme to either Bearer or Basic
I think we need to rename this class into TokenAuth for better
readability.
And we can add:
class BearerTokenAuth(TokenAuth):
pass
for retrocompatibility
—
Reply to this email directly, view it on GitHub
<#384 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AQGHB4FLHPYXQRAPANX2D532QL7LBAVCNFSM6AAAAABXGJIMTKVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDMMRSHE3DSOBUGY>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
you should be able to push new commits to this one! |
Worked on said improvements, awaiting merge approval. |
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.
Two minor nitpicks, and we're good to merge 👏
Fix #377