PR#2160: python: move authentication to separate classes - copr/copr

copr / copr

#2160 python: move authentication to separate classes

Merged 2 years ago by praiskup. Opened 2 years ago by frostyx.

copr/ frostyx/copr python-auth-class into main

python: move authentication to separate classes

Jakub Kadlcik • 2 years ago

6c8f909

cli/tests/test_distgit.py

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:`

python/copr/test/client_v3/__init__.py

file added

empty or binary file added

python/copr/test/client_v3/test_auth.py

file added

+153

		`@@ -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`
		`+ }`

python/copr/test/client_v3/test_modules.py

file modified

		`@@ -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)`

python/copr/v3/auth/__init__.py

file added

+24

		`@@ -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)`

python/copr/v3/auth/base.py

file added

+161

		`@@ -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()`
praiskup commented 2 years ago 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")`

python/copr/v3/auth/gssapi.py

file added

+31

		`@@ -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}`

python/copr/v3/auth/token.py

file added

+14

		`@@ -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")`

python/copr/v3/proxies/__init__.py

file modified

+11 -111

		`@@ -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`

python/copr/v3/requests.py

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)`
praiskup commented 2 years ago Can you move this method into BaseAuth, and do `auth.update_auth_params(params)` so the logic is encapsulated there?
praiskup commented 2 years ago 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,`
praiskup commented 2 years ago 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):`

frostyx commented 2 years ago

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

zuul commented 2 years ago

Build succeeded.

csdiff-pylint : SUCCESS in 1m 41s

rebased onto 808ba5eb7b5b049d6bd28ceb9bb3888ae260a6d7

2 years ago

rebased onto 96d6239160f554ba71eedbdd8d3ee1aa363febb0

2 years ago

rebased onto 8645bf80ddc1dd8ea3660dbb0fc5cc8002b2a2a0

2 years ago

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

2 years ago

zuul commented 2 years ago

Build succeeded.

csdiff-pylint : SUCCESS in 2m 22s

rebased onto bb6d960e5663923a83bfb5aef7f8bc7d0415f901

2 years ago

zuul commented 2 years ago

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

csdiff-pylint : FAILURE in 1m 42s

rebased onto 929069d3c6c04844803c2e7b40fe1a8c5e445347

2 years ago

frostyx commented 2 years ago

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

zuul commented 2 years ago

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

csdiff-pylint : FAILURE in 1m 37s

rebased onto 73d1c4307d99b0d9938ab2a61f92c657a632d308

2 years ago

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

2 years ago

zuul commented 2 years ago

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

csdiff-pylint : FAILURE in 9m 31s

rebased onto 6c1158caeb6f874944809dc5a922c1d9bf81f2f0

2 years ago

zuul commented 2 years ago

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

csdiff-pylint : FAILURE in 2m 18s

rebased onto 9de7f2c35ee9dfe7b57938f61e278468e8e2e16b

2 years ago

zuul commented 2 years ago

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

csdiff-pylint : FAILURE in 1m 40s

rebased onto e19d6facd4fee15efd82659bb636a9dda594456a

2 years ago

zuul commented 2 years ago

Build succeeded.

csdiff-pylint : SUCCESS in 1m 38s

rebased onto d2902e5e7eb5b16421008c2640cd95d2652ab3eb

2 years ago

zuul commented 2 years ago

Build succeeded.

csdiff-pylint : SUCCESS in 1m 28s

frostyx commented 2 years ago

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

2 years ago

zuul commented 2 years ago

Build succeeded.

csdiff-pylint : SUCCESS in 1m 35s

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

2 years ago

praiskup commented on line 67 of python/copr/v3/auth/cached_cookie.py 2 years 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.

praiskup commented 2 years ago

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

2 years ago

zuul commented 2 years ago

Build succeeded.

csdiff-pylint : SUCCESS in 1m 44s

rebased onto c7470847fe2913a97dee3485163127f5d6150e6d

2 years ago

rebased onto eebb04f4bbb2fb7142ceeca7d88708135e846c5d

2 years ago

rebased onto 907f19ca8ab010cb63fdca64ea84c8c480bec971

2 years ago

zuul commented 2 years ago

Build succeeded.

csdiff-pylint : SUCCESS in 1m 32s

frostyx commented 2 years ago

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.

praiskup commented on line 85 of python/copr/v3/auth/base.py 2 years ago

make() seems OK

praiskup commented on line 146 of python/copr/v3/proxies/__init__.py 2 years ago

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

praiskup commented on line 49 of python/copr/v3/requests.py 2 years ago

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

praiskup commented on line 49 of python/copr/v3/requests.py 2 years ago

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

praiskup commented 2 years ago

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.

praiskup commented on line 60 of python/copr/v3/requests.py 2 years ago

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

rebased onto 8e5703a6da9428ed7dabcd93e74689124fc5c73b

2 years ago

zuul commented 2 years ago

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

csdiff-pylint : FAILURE in 1m 54s

rebased onto a3289c2e718b196494abdbc89113cb0ebacd7944

2 years ago

zuul commented 2 years ago

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

csdiff-pylint : FAILURE in 1m 33s

rebased onto 333c1632f462b67b69a51d58eb8f6cfd27b3f854

2 years ago

frostyx commented 2 years ago

Updated, and added some tests.
PTAL

The Zuul errors are only from tests.

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

2 years ago

zuul commented 2 years ago

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

csdiff-pylint : FAILURE in 2m 28s

rebased onto bbf5d2ed581cdeb428307f77c0e28f78ab916b58

2 years ago

zuul commented 2 years ago

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

csdiff-pylint : FAILURE in 2m 40s

Metadata Update from @praiskup:
- Request assigned

2 years ago

rebased onto fc6bbbc9175d130acd3a2669281e0acec4713495

2 years ago

zuul commented 2 years ago

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

csdiff-pylint : FAILURE in 3m 11s

praiskup commented 2 years ago

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

2 years ago

zuul commented 2 years ago

Build succeeded.

csdiff-pylint : SUCCESS in 2m 26s

frostyx commented 2 years ago

[copr-build]

frostyx commented 2 years ago

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

praiskup commented 2 years ago

I think rawhide chroot is broken

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

praiskup commented 2 years ago

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

schlupov commented 2 years ago

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.

schlupov commented on line 21 of python/copr/v3/proxies/__init__.py 2 years ago

You can remove this import

schlupov commented 2 years ago

Otherwise, +1

rebased onto 831269b38d17a768166166cf87beedd0d07203ed

2 years ago

zuul commented 2 years ago

Build succeeded.