Skip to content

Prevent a non-None empty username from being passed down and used to … #336

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 2 commits into from
Jul 21, 2022

Conversation

sbSteveK
Copy link
Contributor

An empty string as a username causes a PRE_CONDITION in aws-c-mqtt to fail during the creation of a connection. This change will convert a completely empty username to remain None when passed down the bindings to native.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Copy link
Contributor

@TwistedTwigleg TwistedTwigleg 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 to me! 👍

I added a minor comment on something unrelated to the fix, but since it was changed in the PR I thought I'd mention it. Feel free to ignore though since it is unrelated.

@@ -482,10 +487,10 @@ def direct_with_custom_authorizer(
else:
username_string += auth_username

if not auth_authorizer_name is None:
if auth_authorizer_name is not None:
Copy link
Contributor

Choose a reason for hiding this comment

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

Unrelated to this PR but: We probably want to change this to != None instead of is not None because we got a GitHub issue where someone was having Python warnings in their logs using Python 3.8+ due to the use of is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

any use of '!= None' is being auto-formatted to 'is not None' for me. We should probably tackle this as a separate issue and fix it everywhere if we fix it anywhere =]

@TwistedTwigleg
Copy link
Contributor

Also - the check-docks issue is due to a URL being incorrect in the CI. It just needs a minor change to the CI file to fix it.

@sbSteveK sbSteveK merged commit 4722f3b into main Jul 21, 2022
@sbSteveK sbSteveK deleted the empty-username-fix branch July 21, 2022 21:07
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