Skip to content

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

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

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

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.

@@ -33,6 +33,9 @@ def json(self):


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

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

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?

@fschmi
Copy link
Author

fschmi commented Oct 4, 2018

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

@jrborbars
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.

@SpotlightKid
Copy link
Contributor

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

@jrborbars
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.

@SpotlightKid
Copy link
Contributor

This pull request was not applied?

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

@tongclement
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.

@jrborbars
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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants