Skip to content

Make portalocker optional #117

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 1 commit into from
Jan 30, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -334,3 +334,5 @@ ASALocalRun/
.mfractor/

.eggs/
.env
Session.vim
8 changes: 4 additions & 4 deletions Dockerfile
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
# TODO: Can this Dockerfile use multi-stage build?
# https://testdriven.io/tips/6da2d9c9-8849-4386-b7f9-13b28514ded8/
# Final size 690MB. (It would be 1.16 GB if started with python:3 as base)
FROM python:3.12-slim
FROM python:3.13-slim

# Install Generic PyGObject (sans GTK)
#The following somehow won't work:
Expand All @@ -9,7 +10,6 @@ RUN apt-get update && apt-get install -y \
libcairo2-dev \
libgirepository1.0-dev \
python3-dev
RUN pip install "pygobject>=3,<4"

# Install MSAL Extensions dependencies
# Don't know how to get container talk to dbus on host,
Expand All @@ -19,10 +19,10 @@ RUN apt-get install -y \
gnome-keyring

# Not strictly necessary, but we include a pytest (which is only 3MB) to facilitate testing.
RUN pip install "pytest>=6,<7"
RUN pip install "pygobject>=3,<4" "pytest>=6,<7"

# Install MSAL Extensions. Upgrade the pinned version number to trigger a new image build.
RUN pip install "msal-extensions==1.1"
RUN pip install "msal-extensions==1.2"

# This setup is inspired from https://github.com/jaraco/keyring#using-keyring-on-headless-linux-systems-in-a-docker-container
ENTRYPOINT ["dbus-run-session", "--"]
Expand Down
5 changes: 2 additions & 3 deletions docker_run.sh
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,10 @@ docker build -t $IMAGE_NAME - < Dockerfile
echo "==== Integration Test for Persistence on Linux (libsecret) ===="
echo "After seeing the bash prompt, run the following to test encryption on Linux:"
echo " pip install -e ."
echo " pytest -s tests/chosen_test_file.py"
echo "Note that you probably need to set up ENV VAR for the test cases to run"
echo " pytest --capture=no -s tests/chosen_test_file.py"
echo "Note: It will test portalocker-based lock when portalocker is installed, or test file-based lock otherwise."
docker run --rm -it \
--privileged \
--env-file .env \
-w /home -v $PWD:/home \
$IMAGE_NAME \
$1
Expand Down
3 changes: 1 addition & 2 deletions msal_extensions/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,5 @@
KeychainPersistence,
LibsecretPersistence,
)
from .cache_lock import CrossPlatLock
from .token_cache import PersistedTokenCache
from .token_cache import PersistedTokenCache, CrossPlatLock, LockError

5 changes: 4 additions & 1 deletion msal_extensions/cache_lock.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,15 @@
import time
import logging

import portalocker
import portalocker # pylint: disable=import-error


logger = logging.getLogger(__name__)


LockError = portalocker.exceptions.LockException


class CrossPlatLock(object):
"""Offers a mechanism for waiting until another process is finished interacting with a shared
resource. This is specifically written to interact with a class of the same name in the .NET
Expand Down
62 changes: 62 additions & 0 deletions msal_extensions/filelock.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
"""A cross-process lock based on exclusive creation of a given file name"""
import os
import sys
import errno
import time
import logging


logger = logging.getLogger(__name__)


class LockError(RuntimeError):
"""It will be raised when unable to obtain a lock"""


class CrossPlatLock(object):
Copy link
Contributor

@jiasli jiasli Oct 27, 2022

Choose a reason for hiding this comment

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

Well, Azure CLI doesn't really need such a complex lock at the comment. Previously ADAL didn't have locking mechanism and worked mostly fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair point. But if the current PR's non-portalocker lock is harmless, then we might as well just use it.

We are open to the dummy lock idea, too. We may work on it at a later time. It can be done, but it is more work, in terms of needing to expand the current MSAL EX api to allow a "no lock" option. (The current file-based lock, on the contrary, is a drop-in replacement for portalocker, and keeps the MSAL EX api intact.)

If the file-based lock does not work well for Azure CLI on SAW, we will prioritize the dummy lock work accordingly.

"""This implementation relies only on ``open(..., 'x')``"""
def __init__(self, lockfile_path):
self._lockpath = lockfile_path

def __enter__(self):
self._create_lock_file('{} {}'.format(
os.getpid(),
sys.argv[0],
).encode('utf-8')) # pylint: disable=consider-using-f-string
return self

def _create_lock_file(self, content):
timeout = 5
check_interval = 0.25
current_time = getattr(time, "monotonic", time.time)
timeout_end = current_time() + timeout
while timeout_end > current_time():
try:
with open(self._lockpath, 'xb') as lock_file: # pylint: disable=unspecified-encoding
lock_file.write(content)
return None # Happy path
except ValueError: # This needs to be the first clause, for Python 2 to hit it
raise LockError("Python 2 does not support atomic creation of file")
except FileExistsError: # Only Python 3 will reach this clause
logger.debug(
"Process %d found existing lock file, will retry after %f second",
os.getpid(), check_interval)
time.sleep(check_interval)
raise LockError(
"Unable to obtain lock, despite trying for {} second(s). "
"You may want to manually remove the stale lock file {}".format(
timeout,
self._lockpath,
))

def __exit__(self, *args):
try:
os.remove(self._lockpath)
except OSError as ex: # pylint: disable=invalid-name
if ex.errno in (errno.ENOENT, errno.EACCES):
# Probably another process has raced this one
# and ended up clearing or locking the file for itself.
logger.debug("Unable to remove lock file")
else:
raise

4 changes: 1 addition & 3 deletions msal_extensions/libsecret.py
Original file line number Diff line number Diff line change
Expand Up @@ -40,9 +40,7 @@
class LibSecretAgent(object):
"""A loader/saver built on top of low-level libsecret"""
# Inspired by https://developer.gnome.org/libsecret/unstable/py-examples.html
def __init__(
# pylint: disable=too-many-arguments
# pylint: disable=too-many-positional-arguments
def __init__( # pylint: disable=too-many-arguments,too-many-positional-arguments
self,
schema_name,
attributes, # {"name": "value", ...}
Expand Down
8 changes: 7 additions & 1 deletion msal_extensions/token_cache.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,13 @@

import msal

from .cache_lock import CrossPlatLock
try: # It needs portalocker
from .cache_lock import ( # pylint: disable=unused-import
CrossPlatLock,
LockError, # We don't use LockError in this file, but __init__.py uses it.
)
except ImportError: # Falls back to file-based lock
from .filelock import CrossPlatLock, LockError # pylint: disable=unused-import
from .persistence import _mkdir_p, PersistenceNotFound


Expand Down
6 changes: 5 additions & 1 deletion setup.py
Original file line number Diff line number Diff line change
Expand Up @@ -20,10 +20,14 @@
python_requires=">=3.7",
install_requires=[
'msal>=1.29,<2', # Use TokenCache.search() from MSAL Python 1.29+
'portalocker<4,>=1.4',

## We choose to NOT define a hard dependency on this.
# "pygobject>=3,<4;platform_system=='Linux'",
],
extras_require={
"portalocker": [
'portalocker<4,>=1.4',
],
},
tests_require=['pytest'],
)
Empty file added tests/__init__.py
Empty file.
7 changes: 4 additions & 3 deletions tests/cache_file_generator.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,10 @@
import sys
import time

from portalocker import exceptions
from msal_extensions import FilePersistence, CrossPlatLock, LockError

from msal_extensions import FilePersistence, CrossPlatLock

print("Testing with {}".format(CrossPlatLock))


def _acquire_lock_and_write_to_cache(cache_location, sleep_interval):
Expand All @@ -31,7 +32,7 @@ def _acquire_lock_and_write_to_cache(cache_location, sleep_interval):
time.sleep(sleep_interval)
data += "> " + str(os.getpid()) + "\n"
cache_accessor.save(data)
except exceptions.LockException as e:
except LockError as e:
logging.warning("Unable to acquire lock %s", e)


Expand Down
12 changes: 12 additions & 0 deletions tests/http_client.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
class MinimalResponse(object): # Not for production use
def __init__(self, requests_resp=None, status_code=None, text=None, headers=None):
self.status_code = status_code or requests_resp.status_code
self.text = text if text is not None else requests_resp.text
self.headers = {} if headers is None else headers
self._raw_resp = requests_resp

def raise_for_status(self):
if self._raw_resp is not None: # Turns out `if requests.response` won't work
# cause it would be True when 200<=status<400
self._raw_resp.raise_for_status()

20 changes: 12 additions & 8 deletions tests/test_agnostic_backend.py
Original file line number Diff line number Diff line change
@@ -1,12 +1,15 @@
import json
import os
import shutil
import tempfile
from unittest.mock import patch
import sys

import msal
import pytest

from msal_extensions import *
from .http_client import MinimalResponse


@pytest.fixture
Expand All @@ -16,18 +19,19 @@ def temp_location():
shutil.rmtree(test_folder, ignore_errors=True)

def _test_token_cache_roundtrip(persistence):
client_id = os.getenv('AZURE_CLIENT_ID')
client_secret = os.getenv('AZURE_CLIENT_SECRET')
if not (client_id and client_secret):
pytest.skip('no credentials present to test TokenCache round-trip with.')

desired_scopes = ['https://graph.microsoft.com/.default']
apps = [ # Multiple apps sharing same persistence
msal.ConfidentialClientApplication(
client_id, client_credential=client_secret,
"fake_client_id", client_credential="fake_client_secret",
token_cache=PersistedTokenCache(persistence)) for i in range(2)]
token1 = apps[0].acquire_token_for_client(scopes=desired_scopes)
assert token1["token_source"] == "identity_provider", "Initial token should come from IdP"
with patch.object(apps[0].http_client, "post", return_value=MinimalResponse(
status_code=200, text=json.dumps({
"token_type": "Bearer",
"access_token": "app token",
"expires_in": 3600,
}))) as mocked_post:
token1 = apps[0].acquire_token_for_client(scopes=desired_scopes)
assert token1["token_source"] == "identity_provider", "Initial token should come from IdP"
token2 = apps[1].acquire_token_for_client(scopes=desired_scopes) # Hit token cache in MSAL 1.23+
assert token2["token_source"] == "cache", "App2 should hit cache written by app1"
assert token1['access_token'] == token2['access_token'], "Cache should hit"
Expand Down
2 changes: 1 addition & 1 deletion tests/test_cache_lock_file_perf.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@

import pytest

from cache_file_generator import _acquire_lock_and_write_to_cache
from .cache_file_generator import _acquire_lock_and_write_to_cache


@pytest.fixture
Expand Down
3 changes: 2 additions & 1 deletion tests/test_crossplatlock.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import pytest
from msal_extensions.cache_lock import CrossPlatLock
from msal_extensions import CrossPlatLock


def test_ensure_file_deleted():
Expand All @@ -10,6 +10,7 @@ def test_ensure_file_deleted():
except NameError:
FileNotFoundError = IOError

print("Testing with {}".format(CrossPlatLock))
with CrossPlatLock(lockfile):
pass

Expand Down
2 changes: 1 addition & 1 deletion tox.ini
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ passenv =
GITHUB_ACTIONS

commands =
pytest
{posargs:pytest --color=yes}

[testenv:lint]
deps =
Expand Down
Loading