AzureAD / microsoft-authentication-library-for-python

Microsoft Authentication Library (MSAL) for Python makes it easy to authenticate to Microsoft Entra ID. General docs are available here https://learn.microsoft.com/entra/msal/python/ Stable APIs are documented here https://msal-python.readthedocs.io. Questions can be asked on www.stackoverflow.com with tag "msal" + "python".
https://stackoverflow.com/questions/tagged/azure-ad-msal+python
Other
788 stars 194 forks source link

Fix: Ctrl+C results in handle_request failure #405

Closed jiasli closed 2 years ago

jiasli commented 2 years ago

Refine https://github.com/AzureAD/microsoft-authentication-library-for-python/pull/404

Symptom

_AuthCodeReceiver is used as a context manager:

https://github.com/AzureAD/microsoft-authentication-library-for-python/blob/f97cb7c69d1f5c2eed6c85e28dd828b2434263b4/msal/oauth2cli/oauth2.py#L636

When Ctrl+C is pressed, Python calls

https://github.com/AzureAD/microsoft-authentication-library-for-python/blob/f97cb7c69d1f5c2eed6c85e28dd828b2434263b4/msal/oauth2cli/authcode.py#L279-L280

then

https://github.com/AzureAD/microsoft-authentication-library-for-python/blob/f97cb7c69d1f5c2eed6c85e28dd828b2434263b4/msal/oauth2cli/authcode.py#L272-L274

This shuts down the server and causes

https://github.com/AzureAD/microsoft-authentication-library-for-python/blob/f97cb7c69d1f5c2eed6c85e28dd828b2434263b4/msal/oauth2cli/authcode.py#L264

to return.

Because of the outer loop

https://github.com/AzureAD/microsoft-authentication-library-for-python/blob/f97cb7c69d1f5c2eed6c85e28dd828b2434263b4/msal/oauth2cli/authcode.py#L261 ,

https://github.com/AzureAD/microsoft-authentication-library-for-python/blob/f97cb7c69d1f5c2eed6c85e28dd828b2434263b4/msal/oauth2cli/authcode.py#L264

is called again. However, as the server has been shut down, it fails with

Exception in thread Thread-1:
Traceback (most recent call last):
  File "C:\Users\user1\AppData\Local\Programs\Python\Python39\lib\threading.py", line 973, in _bootstrap_inner
    self.run()
  File "C:\Users\user1\AppData\Local\Programs\Python\Python39\lib\threading.py", line 910, in run
    self._target(*self._args, **self._kwargs)
  File "d:\cli-msal\microsoft-authentication-library-for-python\msal\oauth2cli\authcode.py", line 264, in _get_auth_response
    self._server.handle_request()
  File "C:\Users\user1\AppData\Local\Programs\Python\Python39\lib\socketserver.py", line 291, in handle_request
    selector.register(self, selectors.EVENT_READ)
  File "C:\Users\user1\AppData\Local\Programs\Python\Python39\lib\selectors.py", line 300, in register
    key = super().register(fileobj, events, data)
  File "C:\Users\user1\AppData\Local\Programs\Python\Python39\lib\selectors.py", line 239, in register
    key = SelectorKey(fileobj, self._fileobj_lookup(fileobj), events, data)
  File "C:\Users\user1\AppData\Local\Programs\Python\Python39\lib\selectors.py", line 226, in _fileobj_lookup
    return _fileobj_to_fd(fileobj)
  File "C:\Users\user1\AppData\Local\Programs\Python\Python39\lib\selectors.py", line 42, in _fileobj_to_fd
    raise ValueError("Invalid file descriptor: {}".format(fd))
ValueError: Invalid file descriptor: -1

Change

If a request is handled by msal.oauth2cli.authcode._AuthCodeHandler.do_GET, self.server.auth_response will always be set. Later on, if self._server.handle_request() returns leaving an empty self.server.auth_response, that means the server is shutdown before any request is handled, so we break the loop.

jiasli commented 2 years ago

Azure CLI doesn't have this issue because Azure CLI doesn't close the server upon receiving Ctrl+C at all!

jiasli commented 2 years ago

My initial implementation has a flaw: if some noises reach first, self.server.auth_response will be set, and self._server.handle_request() will be called again, triggering the failure. This can be reproed with curl:

  1. python -m azure.cli login --debug
  2. In another terminal, curl localhost:xxxxx
  3. Go back to the previous terminal, press Ctrl+C

However, when I visit localhost:xxxxx in Chrome, after pressing Ctrl+C, self._server.handle_request() never exits and the failure is not triggered. Meanwhile, localhost:xxxxx doesn't respond to curl anymore. Guess Chrome is holding the socket for some reason, preventing self._server.handle_request() from exiting. Once Chrome is closed, everything resumes to normal.

rayluo commented 2 years ago

Closing this, since we currently choose a boolean flag approach.