#2160 python: move authentication to separate classes
Merged 4 months ago by praiskup. Opened 4 months ago by frostyx.
copr/ frostyx/copr python-auth-class  into  main

file modified
+6 -5
@@ -8,11 +8,11 @@ 

  import pytest

  

  import copr

- from copr_cli import main

- from cli_tests_lib import mock

- 

  # pylint: disable=unused-import

  from cli_tests_lib import f_test_config

+ from cli_tests_lib import mock

+ from cli_tests_lib import config as mock_config

+ from copr_cli import main

  

  def _main(args, capsys):

      main.main(args)
@@ -142,8 +142,9 @@ 

               'webhook_rebuild': True})

  

      @staticmethod

-     @mock.patch('copr.v3.proxies.BaseProxy.auth', new_callable=mock.PropertyMock, return_value="test")

-     def test_edit_package_fail(auth, capsys):

+     @mock.patch('copr.v3.proxies.BaseProxy.auth_check', return_value=Munch(name="test"))

+     @mock.patch('copr_cli.main.config_from_file', return_value=mock_config)

+     def test_edit_package_fail(auth_check, mock_config, capsys):

          with mock.patch("copr.v3.proxies.package.PackageProxy.edit") as p1:

              p1.side_effect = copr.v3.CoprRequestException("Unable to connect to http://copr/api_3/.")

              with pytest.raises(SystemExit) as exc:

empty or binary file added
@@ -0,0 +1,153 @@ 

+ import time

+ import pytest

+ from copr.v3.auth import ApiToken, Gssapi, auth_from_config, CoprAuthException

+ from copr.v3.auth.base import BaseAuth

+ from copr.test import mock

+ 

+ 

+ class TestApiToken:

+     @staticmethod

+     def test_make_expensive():

+         """

+         Test that `ApiToken` load all necessary information from config

+         """

+         config = mock.MagicMock()

+         auth = ApiToken(config)

+         assert not auth.auth

+         assert not auth.username

+ 

+         # Make sure all auth values are loaded from the config

+         auth.make_expensive()

+         assert auth.auth

+         assert auth.username

+ 

+ 

+ class TestGssApi:

+     @staticmethod

+     @mock.patch("requests.get")

+     def test_make_expensive(mock_get):

+         """

+         Test that `Gssapi` knows what to do with information from Kerberos

+         """

+         config = mock.MagicMock()

+         auth = Gssapi(config)

+         assert not auth.username

+         assert not auth.auth

+ 

+         # Fake a response from Kerberos

+         response = mock.MagicMock()

+         response.json.return_value = {"id": 123, "name": "jdoe"}

+         response.cookies = {"session": "foo-bar-hash"}

+         mock_get.return_value = response

+ 

+         # Make sure GSSAPI cookies are set

+         auth.make_expensive()

+         assert auth.username == "jdoe"

+         assert auth.cookies == {"session": "foo-bar-hash"}

+ 

+ 

+ class TestBaseAuth:

+     @staticmethod

+     @mock.patch("copr.v3.auth.base.BaseAuth.expired")

+     @mock.patch("copr.v3.auth.base.BaseAuth.make_expensive")

+     def test_make_without_cache(make_expensive, _expired):

+         """

+         Test that auth classes remember and re-use the information from

+         `make_expensive()`

+         """

+         auth = BaseAuth(config=None)

+         auth.cache = mock.MagicMock()

+ 

+         # Even though we call make() multiple times,

+         # make_expensive() is called only once

+         for _ in range(5):

+             auth.make()

+         assert make_expensive.call_count == 1

+ 

+     @staticmethod

+     @mock.patch("copr.v3.auth.base.BaseAuth.make_expensive")

+     def test_make_reauth(make_expensive):

+         """

+         When reauth is requested, make sure we don't use any previous tokens

+         """

+         auth = BaseAuth(config=None)

+         auth.cache = mock.MagicMock()

+         for _ in range(5):

+             auth.make(reauth=True)

+         assert make_expensive.call_count == 5

+ 

+     @staticmethod

+     @mock.patch("copr.v3.auth.base.BaseAuth.make_expensive")

+     def test_make_from_cache(make_expensive):

+         """

+         If there is a cached session cookie that is still valid, use it and

+         don't make any expensive calls

+         """

+         auth = BaseAuth(config=None)

+         auth.cache = FakeCache(None)

+         for _ in range(5):

+             auth.make()

+         assert make_expensive.call_count == 0

+ 

+     @staticmethod

+     @mock.patch("copr.v3.auth.base.BaseAuth.make_expensive")

+     def test_make_from_cache_expired(make_expensive):

+         """

+         If there is a cached session cookie but it is expired, just ignore it

+         """

+         auth = BaseAuth(config=None)

+         auth.cache = FakeCacheExpired(None)

+         for _ in range(5):

+             auth.make()

+         assert make_expensive.call_count == 1

+ 

+ 

+ class TestAuth:

+     @staticmethod

+     def test_auth_from_config():

+         """

+         Make sure we use the expected authentication method

+         """

+         # Use (login, token) authentication if there is enough information

+         auth = auth_from_config({

+             "copr_url": "http://copr",

+             "login": "test",

+             "token": "test",

+             "username": "jdoe",

+         })

+         assert isinstance(auth, ApiToken)

+ 

+         # Otherwise use GSSAPI (if gssapi is enabled, which is by default)

+         auth = auth_from_config({

+             "copr_url": "http://copr",

+             "gssapi": True,

+         })

+         assert isinstance(auth, Gssapi)

+ 

+         # There are no other authentication methods

+         config = {

+             "copr_url": "http://copr",

+             "gssapi": False,

+         }

+         with pytest.raises(CoprAuthException):

+             auth = auth_from_config(config)

+ 

+ 

+ class FakeCache(mock.MagicMock):

+     # pylint: disable=too-many-ancestors

+     def load_session(self):

+         # pylint: disable=no-self-use

+         return {

+             "name": "jdoe",

+             "session": "foo-bar-hash",

+             "expiration": time.time() + 1

+         }

+ 

+ class FakeCacheExpired(FakeCache):

+     # pylint: disable=too-many-ancestors

+     def load_session(self):

+         return {

+             "name": "jdoe",

+             "session": "foo-bar-hash",

+             "expiration": time.time() - 1

+         }

@@ -4,6 +4,7 @@ 

  import shutil

  from json import loads

  from copr.test import mock

+ from copr.test.client_v3.test_auth import FakeCache

  from copr.v3 import ModuleProxy

  

  
@@ -27,6 +28,7 @@ 

      @mock.patch('copr.v3.requests.requests.Session.request')

      def test_module_dist_git_choice_url(self, request, distgit_opt):

          proxy = ModuleProxy(self.config_auth)

+         proxy.auth.cache = FakeCache(None)

          proxy.build_from_url('owner', 'project', 'http://test.yaml',

                               distgit=distgit_opt)

  
@@ -45,6 +47,7 @@ 

      @mock.patch('copr.v3.requests.requests.Session.request')

      def test_module_dist_git_choice_upload(self, request, distgit_opt):

          proxy = ModuleProxy(self.config_auth)

+         proxy.auth.cache = FakeCache(None)

          proxy.build_from_file('owner', 'project',

                                self.yaml_file,

                                distgit=distgit_opt)

@@ -0,0 +1,24 @@ 

+ """

+ Authentication classes for usage within APIv3

+ """

+ 

+ from copr.v3.exceptions import CoprAuthException

+ from copr.v3.auth.token import ApiToken

+ from copr.v3.auth.gssapi import Gssapi

+ 

+ 

+ def auth_from_config(config):

+     """

+     Decide what authentication method to use and return an appropriate instance

+     """

+     if config.get("token"):

+         return ApiToken(config)

+ 

+     if config.get("gssapi"):

+         return Gssapi(config)

+ 

+     msg = "GSSAPI disabled and login:token is invalid ({0}:{1})".format(

+         config.get("login", "NOT_SET"),

+         config.get("token", "NOT_SET"),

+     )

+     raise CoprAuthException(msg)

@@ -0,0 +1,161 @@ 

+ """

+ Base authentication interface

+ """

+ 

+ import os

+ import json

+ import time

+ import errno

+ from filelock import FileLock

+ 

+ try:

+     from urllib.parse import urlparse

+ except ImportError:

+     from urlparse import urlparse

+ 

+ 

+ class BaseAuth:

+     """

+     Base authentication class

+     There is a more standard way of implementing custom authentication classes,

+     please see https://docs.python-requests.org/en/latest/user/authentication/

+     We can eventually implement it using `requests.auth`.

+     """

+ 

+     def __init__(self, config):

+         self.config = config

+         self.cache = AuthCache(config)

+         self.username = None

+         self.expiration = None

+ 

+         # These attributes will be directly passed to the requests.request(...)

+         # calls. Various authentication mechanisms should set them accordingly.

+         self.auth = None

+         self.cookies = None

+ 

+     @property

+     def expired(self):

+         """

+         Are the authentication tokens, cookies, etc, expired?

+ 

+         We know this for example for cached cookies. It can be tricky because

+         it can be expired regardless of the expiration time when frontend

+         decides to e.g. revoke all tokens, but we get to know only when sending

+         a request.

+ 

+         But when expiration time is gone, we for sure know the cookie is

+         expired. That will help us avoid sending requests that we know will be

+         unsuccessful.

+         """

+         if not self.expiration:

+             return False

+         return self.expiration < time.time()

+ 

+     def make_expensive(self):

+         """

+         Perform the authentication process. This is the most expensive part.

+         The point is to set `self.username`, and some combination of

+         `self.auth` and `self.cookies`.

+         """

+         raise NotImplementedError

+ 

+     def make(self, reauth=False):

+         """

+         Perform the authentication process. It can be expensive, so we ensure

+         caching with locks, expiration checks, etc.

+         If `reauth=True` is set, then any cached cookies are ignored and the

+         authentication is done from scratch

+         """

+         # If we know an username, the authentication must have been done

+         # previously and there is no need to do it again.

+         # Unless we specifically request an re-auth

+         auth_done = bool(self.username) and not reauth

+         if auth_done:

+             return

+ 

+         with self.cache.lock:

+             # Try to load session data from cache

+             if not reauth and self._load_cache():

+                 auth_done = True

+ 

+             # If we don't have any cached cookies or they are expired

+             if not auth_done or self.expired:

+                 self.make_expensive()

+                 self._save_cache()

make() seems OK

+ 

+     def _load_cache(self):

+         session_data = self.cache.load_session()

+         if session_data:

+             token = session_data["session"]

+             self.username = session_data["name"]

+             self.cookies = {"session": token}

+             self.expiration = session_data["expiration"]

+         return bool(session_data)

+ 

+     def _save_cache(self):

+         # For now, we don't want to cache (login, token) information, only

+         # session cookies (for e.g. GSSAPI)

+         if not self.cookies:

+             return

+ 

+         data = {

+             "name": self.username,

+             "session": self.cookies["session"],

+             "expiration": self.expiration,

+         }

+         self.cache.save_session(data)

+ 

+ 

+ class AuthCache:

+     """

+     Some authentication methods are expensive and we want to use them only

+     for an initial authentication, chache their value, and use until expiration.

+     """

+ 

+     def __init__(self, config):

+         self.config = config

+ 

+     @property

+     def session_file(self):

+         """

+         Path to the cached file for a given Copr instance

+         """

+         url = urlparse(self.config["copr_url"]).netloc

+         cachedir = os.path.join(os.path.expanduser("~"), ".cache", "copr")

+         try:

+             os.makedirs(cachedir)

+         except OSError as err:

+             if err.errno != errno.EEXIST:

+                 raise

+         return os.path.join(cachedir, url + "-session")

+ 

+     @property

+     def lock_file(self):

+         """

+         Path to the lock file for a given Copr instance

+         """

+         return self.session_file + ".lock"

+ 

+     @property

+     def lock(self):

+         """

+         Allow the user to do `with cache.lock:`

+         """

+         return FileLock(self.lock_file)

+ 

+     def load_session(self):

+         """

+         Load the session data from a cache file

+         """

+         if not os.path.exists(self.session_file):

+             return None

+         with open(self.session_file, "r") as fp:

+             return json.load(fp)

+ 

+     def save_session(self, session_data):

+         """

+         Save the session data to a cache file

+         """

+         with open(self.session_file, "w") as file:

+             session_data["expiration"] = time.time() + 10 * 3600  # +10 hours

+             file.write(json.dumps(session_data, indent=4) + "\n")

@@ -0,0 +1,31 @@ 

+ """

+ Authentication via GSSAPI

+ """

+ 

+ import requests

+ import requests_gssapi

+ from future.utils import raise_from

+ from copr.v3.exceptions import CoprAuthException

+ from copr.v3.requests import munchify, handle_errors

+ from copr.v3.auth.base import BaseAuth

+ 

+ 

+ class Gssapi(BaseAuth):

+     """

+     Authentication via GSSAPI (i.e. Kerberos)

+     """

+     def make_expensive(self):

+         url = self.config["copr_url"] + "/api_3/gssapi_login/"

+         auth = requests_gssapi.HTTPSPNEGOAuth(opportunistic_auth=True)

+         try:

+             response = requests.get(url, auth=auth)

+         except requests_gssapi.exceptions.SPNEGOExchangeError as err:

+             msg = "Can not get session for {0} cookie via GSSAPI: {1}".format(

+                 self.config["copr_url"], err)

+             raise_from(CoprAuthException(msg), err)

+ 

+         handle_errors(response)

+         data = munchify(response)

+         token = response.cookies.get("session")

+         self.username = data.name

+         self.cookies = {"session": token}

@@ -0,0 +1,14 @@ 

+ """

+ Authentication via (login, token)

+ """

+ 

+ from copr.v3.auth.base import BaseAuth

+ 

+ 

+ class ApiToken(BaseAuth):

+     """

+     The standard authentication via `(login, token)` from `~/.config/copr`

+     """

+     def make_expensive(self):

+         self.auth = self.config["login"], self.config["token"]

+         self.username = self.config.get("username")

@@ -1,21 +1,8 @@ 

- import errno

- import json

- import time

  import os

- from filelock import FileLock

  

- try:

-     from urllib.parse import urlparse

- except ImportError:

-     from urlparse import urlparse

- 

- from future.utils import raise_from

- 

- import requests_gssapi

- 

- from ..requests import Request, munchify, requests, handle_errors

+ from copr.v3.auth import auth_from_config

+ from copr.v3.requests import munchify, Request

  from ..helpers import for_all_methods, bind_proxy, config_from_file

- from ..exceptions import CoprAuthException

  

  

  @for_all_methods(bind_proxy)
@@ -26,9 +13,11 @@ 

  

      def __init__(self, config):

          self.config = config

-         self._auth_token_cached = None

-         self._auth_username = None

-         self.request = Request(api_base_url=self.api_base_url, connection_attempts=config.get("connection_attempts", 1))

+         self.request = Request(

+             api_base_url=self.api_base_url,

+             connection_attempts=config.get("connection_attempts", 1)

+         )

+         self._auth = None

  

      @classmethod

      def create_from_config_file(cls, path=None):
@@ -41,95 +30,9 @@ 

  

      @property

      def auth(self):

-         if self._auth_token_cached:

-             return self._auth_token_cached

-         if self.config.get("token"):

-             self._auth_token_cached = self.config["login"], self.config["token"]

-             self._auth_username = self.config.get("username")

-         elif self.config.get("gssapi"):

-             session_data = self._get_session_cookie_via_gssapi()

-             self._auth_token_cached = session_data["session"]

-             self._auth_username = session_data["name"]

-         else:

-             msg = "GSSAPI disabled and login:token is invalid ({0}:{1})".format(

-                 self.config.get("login", "NOT_SET"),

-                 self.config.get("token", "NOT_SET"),

-             )

-             raise CoprAuthException(msg)

-         return self._auth_token_cached

- 

-     def _get_session_cookie_via_gssapi(self):

-         """

-         Return the cached session for the configured username.  If not already

-         cached, new self.get_session_via_gssapi() is performed and result is

-         cached into ~/.config/copr/<session_file>.

-         """

-         url = urlparse(self.config["copr_url"]).netloc

-         cachedir = os.path.join(os.path.expanduser("~"), ".cache", "copr")

- 

-         try:

-             os.makedirs(cachedir)

-         except OSError as err:

-             if err.errno != errno.EEXIST:

-                 raise

- 

-         session_file = os.path.join(cachedir, url + "-session")

-         session_data = self._load_or_download_session(session_file)

-         return session_data

- 

-     @staticmethod

-     def _load_session_from_file(session_file):

-         session_data = None

-         if os.path.exists(session_file):

-             with open(session_file, "r") as file:

-                 session_data = json.load(file)

- 

-         if session_data and session_data["expiration"] > time.time():

-             return session_data

-         return None

- 

-     def _load_or_download_session(self, session_file):

-         lock = FileLock(session_file + ".lock")

-         with lock:

-             session = BaseProxy._load_session_from_file(session_file)

-             if session:

-                 return session

-             # TODO: create Munch sub-class that returns serializable dict, we

-             # have something like that in Cli: cli/copr_cli/util.py:serializable()

-             session_data = self.get_session_via_gssapi()

-             session_data = session_data.__dict__

-             session_data.pop("__response__", None)

-             session_data.pop("__proxy__", None)

-             BaseProxy._save_session(session_file, session_data)

-             return session_data

- 

-     @staticmethod

-     def _save_session(session_file, session_data):

-         with open(session_file, "w") as file:

-             session_data["expiration"] = time.time() + 10 * 3600  # +10 hours

-             file.write(json.dumps(session_data, indent=4) + "\n")

- 

-     def get_session_via_gssapi(self):

-         """

-         Obtain a _new_ session using GSSAPI route

- 

-         :return: Munch, provides user's "id", "name", "session" cookie, and

-             "expiration".

-         """

-         url = self.config["copr_url"] + "/api_3/gssapi_login/"

-         session = requests.Session()

-         auth = requests_gssapi.HTTPSPNEGOAuth(opportunistic_auth=True)

-         try:

-             response = session.get(url, auth=auth)

-         except requests_gssapi.exceptions.SPNEGOExchangeError as err:

-             msg = "Can not get session for {0} cookie via GSSAPI: {1}".format(

-                 self.config["copr_url"], err)

-             raise_from(CoprAuthException(msg), err)

- 

-         handle_errors(response)

-         retval = munchify(response)

-         retval.session = response.cookies.get("session")

-         return retval

+         if not self._auth:

+             self._auth = auth_from_config(self.config)

+         return self._auth

  

      def home(self):

          """
@@ -156,7 +59,4 @@ 

          Return the username (string) assigned to this configuration.  May

          contact the server and authenticate if needed.

          """

-         if not self._auth_username:

-             # perform authentication as a side effect

-             _ = self.auth

-         return self._auth_username

+         return self.auth.username

file modified
+19 -11
@@ -39,32 +39,32 @@ 

  

      def send(self, endpoint, method=GET, data=None, params=None, headers=None,

               auth=None):

-         session = requests.Session()

-         if not isinstance(auth, tuple):

-             # api token not available, set session cookie obtained via gssapi

-             session.cookies.set("session", auth)

  

          request_params = self._request_params(

              endpoint, method, data, params, headers, auth)

  

-         response = self._send_request_repeatedly(session, request_params)

+         response = self._send_request_repeatedly(request_params, auth)

+ 

          handle_errors(response)

          return response

  

-     def _send_request_repeatedly(self, session, request_params):

+     def _send_request_repeatedly(self, request_params, auth):

          """

          Repeat the request until it succeeds, or connection retry reaches its limit.

          """

          sleep = 5

          for i in range(1, self.connection_attempts + 1):

              try:

-                 response = session.request(**request_params)

+                 response = requests.request(**request_params)

              except requests_gssapi.exceptions.SPNEGOExchangeError as e:

                  raise_from(CoprAuthException("GSSAPI authentication failed."), e)

              except requests.exceptions.ConnectionError:

                  if i < self.connection_attempts:

                      time.sleep(sleep)

              else:

+                 if response.status_code == 401:

+                     self._update_auth_params(request_params, auth, reauth=True)

+                     continue

                  return response

          raise CoprRequestException("Unable to connect to {0}.".format(self.api_base_url))

  
@@ -77,12 +77,20 @@ 

              "params": params,

              "headers": headers,

          }

-         # We usually use a tuple (login, token). If this is not available,

-         # we use gssapi auth, which works with cookies.

-         if isinstance(auth, tuple):

-             params["auth"] = auth

+         self._update_auth_params(params, auth)

Can you move this method into BaseAuth, and do auth.update_auth_params(params) so the logic is encapsulated there?

Ah, I see ... there would have to be NoAuth or something.

          return params

  

+     def _update_auth_params(self, request_params, auth, reauth=False):

+         # pylint: disable=no-self-use

+         if not auth:

+             return

+ 

+         auth.make(reauth)

+         request_params.update({

+             "auth": auth.auth,

+             "cookies": auth.cookies,

Just a note: If cookies were already set for any reason by the client (in the future?) we would replace them.

+         })

+ 

  

  class FileRequest(Request):

      def __init__(self, files=None, progress_callback=None, **kwargs):

I am trying to touch the internals of the Kerberos code as least as possible. Only moving it to a separate class.

Build succeeded.

rebased onto 808ba5eb7b5b049d6bd28ceb9bb3888ae260a6d7

4 months ago

rebased onto 96d6239160f554ba71eedbdd8d3ee1aa363febb0

4 months ago

rebased onto 8645bf80ddc1dd8ea3660dbb0fc5cc8002b2a2a0

4 months ago

Metadata Update from @frostyx:
- Pull-request tagged with: needs-tests

4 months ago

Build succeeded.

rebased onto bb6d960e5663923a83bfb5aef7f8bc7d0415f901

4 months ago

Build failed. More information on how to proceed and troubleshoot errors available at https://fedoraproject.org/wiki/Zuul-based-ci

rebased onto 929069d3c6c04844803c2e7b40fe1a8c5e445347

4 months ago

Sorry, I made a couple more changes and rebased. There is now a new CachedCookie(Auth) class.

Build failed. More information on how to proceed and troubleshoot errors available at https://fedoraproject.org/wiki/Zuul-based-ci

rebased onto 73d1c4307d99b0d9938ab2a61f92c657a632d308

4 months ago

Metadata Update from @frostyx:
- Pull-request tagged with: wip

4 months ago

Build failed. More information on how to proceed and troubleshoot errors available at https://fedoraproject.org/wiki/Zuul-based-ci

rebased onto 6c1158caeb6f874944809dc5a922c1d9bf81f2f0

4 months ago

Build failed. More information on how to proceed and troubleshoot errors available at https://fedoraproject.org/wiki/Zuul-based-ci

rebased onto 9de7f2c35ee9dfe7b57938f61e278468e8e2e16b

4 months ago

Build failed. More information on how to proceed and troubleshoot errors available at https://fedoraproject.org/wiki/Zuul-based-ci

rebased onto e19d6facd4fee15efd82659bb636a9dda594456a

4 months ago

Build succeeded.

rebased onto d2902e5e7eb5b16421008c2640cd95d2652ab3eb

4 months ago

Build succeeded.

I really hope I didn't overengineer this too much. I just don't know how to make this any simpler.

The agreed goal was to make separate classes for Token auth, GSSAPI, and cached cookie. I let them inherit from BaseAuth to define a united interface, and I also added NoAuth class that does nothing but mimic the common interface, so we can use it instead of checking if auth is not None.

With this, I still found myself needing a lot of code in the BaseProxy to manage caching, and switching the authentication methods if they are expired or something. Therefore I invented Auth class, which knows about all the possible authentication methods, and does all the mentioned plumbing, and provides a simple interface for the *Proxy and Request.

rebased onto 0c0e5daff032d9845cc6e849c7590061c802f827

4 months ago

Build succeeded.

Metadata Update from @frostyx:
- Pull-request untagged with: wip

4 months ago

This needs to be protected by lock, see the previous implementation. All the tasks a) reading the file, b) fallback to gssapi auth and c) writing to the file was implemented in one critical section.

I went through this proposal and I sort of like the idea. But it is a bit different design
than I originally thought - so I dumped my previous idea to a debate file.

I don't object here in this PR, it may be merged but I would like to know what
you think about that original idea. Sorry I did not do this before. What I like about
that is the simplicity and the fact that adding new auth mechanisms would be
pretty trivial (e.g. per-project, build-only tokens that generate sessions, too, or OIDC).

rebased onto 26d3b9988cc6dab187d674bb0779c142d4701929

4 months ago

Build succeeded.

rebased onto c7470847fe2913a97dee3485163127f5d6150e6d

4 months ago

rebased onto eebb04f4bbb2fb7142ceeca7d88708135e846c5d

4 months ago

rebased onto 907f19ca8ab010cb63fdca64ea84c8c480bec971

4 months ago

Build succeeded.

So I toned down the overengineering a little, and I am much happier with the current approach.
The Auth facade is gone, the NoAuth is gone, and the caching is done in BaseAuth (even though I am delegating the file manipulation to a separate class to hopefully avoid mocking in tests).

The locking was fixed, or at least I think so.

And also, when the cached cookie is not expired per se but is not recognized by the frontend, a re-auth is performed.

I think we can do this unconditionaly. This check for username is done inside the make() call, too.

Can you move this method into BaseAuth, and do auth.update_auth_params(params) so the logic is encapsulated there?

Ah, I see ... there would have to be NoAuth or something.

Thank you very much for the update!

I would probably like this even more:
https://docs.python-requests.org/en/latest/user/authentication/#new-forms-of-authentication
But that can be done later, if needed.

Just a note: If cookies were already set for any reason by the client (in the future?) we would replace them.

rebased onto 8e5703a6da9428ed7dabcd93e74689124fc5c73b

4 months ago

Build failed. More information on how to proceed and troubleshoot errors available at https://fedoraproject.org/wiki/Zuul-based-ci

rebased onto a3289c2e718b196494abdbc89113cb0ebacd7944

4 months ago

Build failed. More information on how to proceed and troubleshoot errors available at https://fedoraproject.org/wiki/Zuul-based-ci

rebased onto 333c1632f462b67b69a51d58eb8f6cfd27b3f854

4 months ago

Updated, and added some tests.
PTAL

The Zuul errors are only from tests.

Metadata Update from @frostyx:
- Pull-request untagged with: needs-tests

4 months ago

Build failed. More information on how to proceed and troubleshoot errors available at https://fedoraproject.org/wiki/Zuul-based-ci

rebased onto bbf5d2ed581cdeb428307f77c0e28f78ab916b58

4 months ago

Build failed. More information on how to proceed and troubleshoot errors available at https://fedoraproject.org/wiki/Zuul-based-ci

Metadata Update from @praiskup:
- Request assigned

4 months ago

rebased onto fc6bbbc9175d130acd3a2669281e0acec4713495

4 months ago

Build failed. More information on how to proceed and troubleshoot errors available at https://fedoraproject.org/wiki/Zuul-based-ci

Can you try to rebase against https://pagure.io/copr/copr/pull-request/2183 ?

These remain then:

../build_aux/linter
================
 Added warnings 
================

Error: PYLINT_WARNING:
copr/v3/auth/base.py:144:15: E0110[abstract-class-instantiated]: AuthCache.lock: Abstract class 'WindowsFileLock' with abstract methods instantiated

Error: PYLINT_WARNING:
copr/v3/auth/base.py:144:15: E0110[abstract-class-instantiated]: AuthCache.lock: Abstract class 'UnixFileLock' with abstract methods instantiated

Error: PYLINT_WARNING:
copr/v3/auth/gssapi.py:30:24: E1101[no-member]: Gssapi.make_expensive: Instance of 'List' has no 'name' member
make: *** [Makefile:14: lint] Error 1

rebased onto fc1f1f766df42685659ad76a0d4f35ade1188e9a

4 months ago

Build succeeded.

Rebased and Zuul fixed.
I think rawhide chroot is broken though

I think rawhide chroot is broken

Yes, that's probably this bug. It should be fixed already (and thus should get to mirrors today).

Ok, thank you for the update. I'll hand over the rest of the review to @schlupov now.

I really like these changes :)
The API tests leave the copr-session.lock file in ~ /.cache/copr/, it probably doesn't matter, only the unit tests shouldn't leave anything behind, so the teardown method or fixture is usually used.

rebased onto 831269b38d17a768166166cf87beedd0d07203ed

4 months ago

Build succeeded.

rebased onto 6c8f909

4 months ago

Build succeeded.

Thank you for the review.

The API tests leave the copr-session.lock file in ~ /.cache/copr/

Interestingly enough, the problem wasn't in test_auth.py but in test_modules.py. Fixed.

You can remove this import

Fixed

Commit 5802a70 fixes this pull-request

Pull-Request has been merged by praiskup

4 months ago