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

Allow interactive flow to be aborted by CTRL+C even when running on Windows #404

Closed rayluo closed 2 years ago

rayluo commented 2 years ago

This PR fix #393 which only happens on Windows.

jiasli commented 2 years ago

Tested in Azure CLI with the latest MSAL dev branch. Upon pressing Ctrl+C, MSAL reported a call stack:

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

Weird thing is that if I move

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

out of the while True loop, the call stack is gone.

jiasli commented 2 years ago

Reproduced with an easier sample:

import threading
import time

from http.server import HTTPServer, SimpleHTTPRequestHandler

class MyClass:
    def __init__(self):
        self.server = HTTPServer(('127.0.0.1', 8400), SimpleHTTPRequestHandler)

    def _get_auth_response(self, result):
        while True:
            print(">>> Calling handle_request")
            self.server.handle_request()
            print("<<< handle_request exits")

    def get_auth_response(self):
        result = {}
        t = threading.Thread(target=self._get_auth_response, args=(result,))
        t.daemon = True
        t.start()
        while True:
            time.sleep(1)
            if not t.is_alive():
                break
        return result or None

    def close(self):
        """Either call this eventually; or use the entire class as context manager"""
        self.server.server_close()

    def __enter__(self):
        return self

    def __exit__(self, exc_type, exc_val, exc_tb):
        self.close()

try:
    with MyClass() as receiver:
        print(receiver.get_auth_response())
except KeyboardInterrupt:
    print("cancelled")
    # Give the daemon thread some time to exit
    time.sleep(1)

Output:

> python main.py
>>> Calling handle_request
<<< handle_request exits
cancelled
>>> Calling handle_request
Exception in thread Thread-1:
Traceback (most recent call last):
  File "C:\Users\jiasli\AppData\Local\Programs\Python\Python39\lib\threading.py", line 973, in _bootstrap_inner
    self.run()
  File "C:\Users\jiasli\AppData\Local\Programs\Python\Python39\lib\threading.py", line 910, in run
    self._target(*self._args, **self._kwargs)
  File "D:\cli\testproj\main.py", line 15, in _get_auth_response
    self.server.handle_request()
  File "C:\Users\jiasli\AppData\Local\Programs\Python\Python39\lib\socketserver.py", line 291, in handle_request
    selector.register(self, selectors.EVENT_READ)
  File "C:\Users\jiasli\AppData\Local\Programs\Python\Python39\lib\selectors.py", line 300, in register
    key = super().register(fileobj, events, data)
  File "C:\Users\jiasli\AppData\Local\Programs\Python\Python39\lib\selectors.py", line 239, in register
    key = SelectorKey(fileobj, self._fileobj_lookup(fileobj), events, data)
  File "C:\Users\jiasli\AppData\Local\Programs\Python\Python39\lib\selectors.py", line 226, in _fileobj_lookup
    return _fileobj_to_fd(fileobj)
  File "C:\Users\jiasli\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

Notice >>> Calling handle_request appears twice.

Explanation:

With the shutdown of the main thread, the first invocation of self.server.handle_request() exits -> Due to the outer while True, self.server.handle_request() is called again -> Since the server has been closed by context manager MyClass.__exit__, self.server.handle_request() raises error.

jiasli commented 2 years ago

Weirdly enough, if I make odd number of GET requests before pressing Ctrl+C, the above script never fails. If I make even number of GET requests before pressing Ctrl+C, the above script always fails.

Edit: I think this depends on Chrome holds the socket (https://github.com/AzureAD/microsoft-authentication-library-for-python/pull/405#issuecomment-916703441).

rayluo commented 2 years ago

Reproduced with an easier sample:

...

Output:

> python main.py
>>> Calling handle_request
<<< handle_request exits
cancelled
>>> Calling handle_request
Exception in thread Thread-1:
Traceback (most recent call last):
  File "C:\Users\jiasli\AppData\Local\Programs\Python\Python39\lib\threading.py", line 973, in _bootstrap_inner
    self.run()
  File "C:\Users\jiasli\AppData\Local\Programs\Python\Python39\lib\threading.py", line 910, in run
    self._target(*self._args, **self._kwargs)
  File "D:\cli\testproj\main.py", line 15, in _get_auth_response
    self.server.handle_request()
  File "C:\Users\jiasli\AppData\Local\Programs\Python\Python39\lib\socketserver.py", line 291, in handle_request
    selector.register(self, selectors.EVENT_READ)
  File "C:\Users\jiasli\AppData\Local\Programs\Python\Python39\lib\selectors.py", line 300, in register
    key = super().register(fileobj, events, data)
  File "C:\Users\jiasli\AppData\Local\Programs\Python\Python39\lib\selectors.py", line 239, in register
    key = SelectorKey(fileobj, self._fileobj_lookup(fileobj), events, data)
  File "C:\Users\jiasli\AppData\Local\Programs\Python\Python39\lib\selectors.py", line 226, in _fileobj_lookup
    return _fileobj_to_fd(fileobj)
  File "C:\Users\jiasli\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

Notice >>> Calling handle_request appears twice.

Explanation:

With the shutdown of the main thread, the first invocation of self.server.handle_request() exits -> Due to the outer while True, self.server.handle_request() is called again -> Since the server has been closed by context manager MyClass.__exit__, self.server.handle_request() raises error.

Thank you, @jiasli , for your investigation! Inspired by your experiment, I ended up using a simpler yet more accurate improvement. You can test it again using the latest MSAL dev branch.