Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

urequests: Added Basic Authentication to requests. #310

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

Closed
fschmi wants to merge 1 commit into micropython:master from fschmi:master

Conversation

@fschmi
Copy link

@fschmi fschmi commented Oct 4, 2018

No description provided.

Copy link
Contributor

@SpotlightKid SpotlightKid left a comment

Choose a reason for hiding this comment

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

These changes were obviously not tested, since the code does not work as is.

raise TypeError("expected bytes, not %s"
% altchars.__class__.__name__)
assert len(altchars) == 2, repr(altchars)
return encoded.translate(bytes.maketrans(b'+/', altchars))
Copy link
Contributor

@SpotlightKid SpotlightKid Oct 4, 2018

Choose a reason for hiding this comment

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

The translate method is not supported on MicroPython, neither is maketrans.

Copy link
Author

Choose a reason for hiding this comment

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

Good to know. Shouldn't we resolve it here then? As mentioned, it's a simply a copy.

% altchars.__class__.__name__)
assert len(altchars) == 2, repr(altchars)
return encoded.translate(bytes.maketrans(b'+/', altchars))
return encoded
Copy link
Contributor

@SpotlightKid SpotlightKid Oct 4, 2018

Choose a reason for hiding this comment

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

  • This whole b64encode function is superfluous. The altchars parameter is never used by the rest of the module and the only other actual functionality left then is being a wrapper for ubinascii.b2a_base64. This can be done in encode_basic_auth directly.
  • Stripping the trailing newline is better done with .rstrip(b'\n').

Copy link
Author

Choose a reason for hiding this comment

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

Very good remark. Seems like I wasn't paying much attention to the functionality of b64encode. Simply knew I needed it ;-)


def encode_basic_auth(auth):
auth_encoded = b64encode(b"%s:%s" % (auth[0], auth[1])).decode("ascii")
return {'Authorization' : 'Basic %s' % auth_encoded}
Copy link
Contributor

@SpotlightKid SpotlightKid Oct 4, 2018

Choose a reason for hiding this comment

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

Several problems with this function too:

  • Should take user and password as separate params, not as a single auth param, to increase clarity.
  • Decoding the return value of b64encode is not necessary, since the header dict should contain byte strings anyway.
  • Non-pep-8-compliant whitespace.

Suggested version:

def encode_basic_auth(user, password):
 from ubinascii import b2a_base64
 auth_encoded = b2a_base64(b"%s:%s" % (user, password)).rstrip(b'\n')
 return {b'Authorization': b'Basic %s' % auth_encoded}

Using the % string formatting operator here allows for user and password to be passed as either str or bytes, which is a bit unclean and wouldn't work in CPython, but works in MicroPython.


def request(method, url, data=None, json=None, headers={}, stream=None):
if auth is not None:
headers.update(encode_basic_auth(auth))
Copy link
Contributor

@SpotlightKid SpotlightKid Oct 4, 2018

Choose a reason for hiding this comment

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

  • An auth parameter needs to be added to the request function signature (defaulting to None).

  • If following my suggested changes for encode_basic_auth, this should be changed to the following then:

    if auth:
     headers.update(encode_basic_auth(auth[0], auth[1]))
    

    (Also changing if auth is not None to if auth so that passing an empty tuple works as well.)

Copy link
Contributor

@SpotlightKid SpotlightKid Oct 4, 2018

Choose a reason for hiding this comment

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

It might be also a good idea to allow passing a callable for auth (but this would be an extension not supported by the original requests.request().

if auth:
 headers.update(auth() if callable(auth) else encode_basic_auth(auth[0], auth[1]))

Copy link
Author

Choose a reason for hiding this comment

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

Obviously that mistake had to happen to me 🤦
Seems like I messed up when developing...

Copy link
Author

Choose a reason for hiding this comment

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

... this would be an extension not supported by the original requests.request().

How are we dealing with that? Do we actually want to extend the original or simply replicate it's functionality? Is there any guidelines for that?

Copy link
Author

fschmi commented Oct 4, 2018
edited
Loading

I have decided to open a new pull request with a working version and some of the changes suggested by @SpotlightKid.
#311

Copy link

I'm getting this error with urequests. (micropython 1.12 from repo, ESP32)

File "urequests.py", line 108, in get
TypeError: unexpected keyword argument 'auth'

Sorry about to open this thread again, but I cannot find the "auth" parameter in the actual code, even in the #311.
Thank you in advance.

Copy link
Contributor

def request(method, url, data=None, json=None, headers={}, stream=None, auth=None):

Copy link

The line in the master branch is as follow:

def request(method, url, data=None, json=None, headers={}, stream=None):

This pull request was not applied?
Thanks in advance.

Copy link
Contributor

This pull request was not applied?

No, as you can see from the PR still being open.

Copy link

To whoever it was who asked me whether I found a solution or not, the answer is sort of. I am using JWT (Token Auth) instead of Basic Auth for my esp32 Micropython project now. It gets and refreshes the token every 5 minutes from the server with the username and password passed as clear text params and then the tokens are used in the header of each request. Hope it helps.

Copy link

Good catch. Can you provide some basic example?
For documentation purposes.
Thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@SpotlightKid SpotlightKid SpotlightKid requested changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /