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

Apply new logic for parsing WWW-Authenticate header #467

Merged
merged 1 commit into from
Jul 8, 2024

Conversation

hovaesco
Copy link
Member

Description

Resolves #444

Non-technical explanation

Release notes

( ) This is not user-visible or docs only and no release notes are required.
( ) Release notes are required, please propose a release note for me.
( ) Release notes are required, with the following suggested text:

* Fix some things. ({issue}`issuenumber`)

@cla-bot cla-bot bot added the cla-signed label Jun 27, 2024
@hovaesco hovaesco requested a review from hashhar June 27, 2024 11:13
@hovaesco hovaesco force-pushed the hovaesco/oauth-header branch from 88282f9 to 01b937c Compare June 27, 2024 12:12
trino/auth.py Outdated Show resolved Hide resolved
trino/auth.py Outdated
key = comps[0].strip(' "')
value = comps[1].strip(' "')
if key:
auth_info_headers[key.lower()] = value
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the dict would overwrite if same key is present (e.g. if there are multiple Bearer realm entries or multiple Bearer x_redirect_server).

Copy link
Member Author

@hovaesco hovaesco Jun 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it's possible that www-authenticate would have duplicate keys, based on https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/WWW-Authenticate

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The entire challenge cannot be a duplicate I think but here the key is the part split on =.

So the example at https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/WWW-Authenticate#digest_authentication_with_sha-256_and_md5 has two challenges for which key = 'Digest realm'.

(Sorry I'm AFK and did not test the static method against that example 🙏).

@hovaesco hovaesco force-pushed the hovaesco/oauth-header branch from 01b937c to ca7ad24 Compare June 27, 2024 20:39
tests/unit/test_client.py Outdated Show resolved Hide resolved
Copy link
Member

@hashhar hashhar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good % question about duplicate keys

@hovaesco hovaesco force-pushed the hovaesco/oauth-header branch from ca7ad24 to 63ffa53 Compare July 1, 2024 08:43
Copy link
Member

@hashhar hashhar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

works for our use-case where we just want x_redirect_server and x_token_server value. Sadly Python is not enterprise enough to have good quality libraries for HTTP 401 handling.

@hashhar hashhar merged commit 5f10177 into trinodb:master Jul 8, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

Cannot connect to Trino via OAUTH2 with SSO credentials
2 participants