Skip to content

Don't send empty credentials to SR in Authorization Header #863

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

Merged
merged 7 commits into from
May 14, 2020

Conversation

rnpridgeon
Copy link
Contributor

No description provided.

@rnpridgeon rnpridgeon requested a review from edenhill May 13, 2020 14:08
Copy link
Contributor

@edenhill edenhill left a comment

Choose a reason for hiding this comment

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

What's testconf.json.swp?

headers = request._request.headers

# Pass request to downstream matchers
if 'Authorization' not in headers.keys():
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure how this works under the hood, but this seems like it needs to scan keys() for 'Authorization', would suggest using ..keys().get('Authorization', None)

if 'Authorization' not in headers.keys():
return None

userinfo = headers.get('Authorization').split(" ")[1]
Copy link
Contributor

Choose a reason for hiding this comment

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

And here's a second lookup. Just do one lookup.

Copy link
Contributor

Choose a reason for hiding this comment

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

Inconsistent use of single/double quotes on the same line.

Copy link
Contributor

Choose a reason for hiding this comment

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

Will throw an IndexError if the Authorization header does not contain a space, but maybe that's okay here.

# Authenticated, pass downstream
return None

unauhtorized = {'error_code': 401,
Copy link
Contributor

Choose a reason for hiding this comment

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

typo

'message': "401 Unauthorized"}
return create_response(request=request,
status_code=401,
json=unauhtorized)
Copy link
Contributor

Choose a reason for hiding this comment

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

typo


Verify that the package works, should print the expected Python client
and librdkafka versions:

$ python -c 'import confluent_kafka as ck ; print "py:", ck.version(), "c:", ck.libversion()'
$ python -c 'import confluent_kafka as ck ; print "py: {} c: {}" \
Copy link
Contributor

Choose a reason for hiding this comment

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

This will not work for python3

@rnpridgeon rnpridgeon merged commit 0593c37 into master May 14, 2020
@rnpridgeon rnpridgeon deleted the authz_misc branch May 14, 2020 11:57
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.

2 participants