From 3a812535906484e1534761edab3fc2c8b88555a3 Mon Sep 17 00:00:00 2001 From: Jana Cupova Date: Jan 25 2023 14:24:19 +0000 Subject: [PATCH 1/16] Create new session when old session was timeout Fixes: https://pagure.io/koji/issue/3394 --- diff --git a/koji/__init__.py b/koji/__init__.py index 50162f8..883550a 100644 --- a/koji/__init__.py +++ b/koji/__init__.py @@ -2408,6 +2408,9 @@ def grab_session_options(options): 'upload_blocksize', 'no_ssl_verify', 'serverca', + 'keytab', + 'principal', + 'ccache', ) # cert is omitted for now if isinstance(options, dict): @@ -2444,6 +2447,8 @@ class ClientSession(object): self.rsession = None self.new_session() self.opts.setdefault('timeout', DEFAULT_REQUEST_TIMEOUT) + self.exclusive = False + self.hostip = None @property def multicall(self): @@ -2475,13 +2480,16 @@ class ClientSession(object): self.callnum = None # do we need to do anything else here? self.authtype = None + self.session_key = None else: self.logged_in = True self.callnum = 0 + self.session_key = sinfo['session-key'] self.sinfo = sinfo def login(self, opts=None): - sinfo = self.callMethod('login', self.opts['user'], self.opts['password'], opts) + sinfo = self.callMethod('login', self.opts['user'], self.opts['password'], + self.opts['session_key'], opts) if not sinfo: return False self.setSession(sinfo) @@ -2494,7 +2502,7 @@ class ClientSession(object): return type(self)(self.baseurl, self.opts, sinfo) def gssapi_login(self, principal=None, keytab=None, ccache=None, - proxyuser=None, proxyauthtype=None): + proxyuser=None, proxyauthtype=None, session_key=None): if not reqgssapi: raise PythonImportError( "Please install python-requests-gssapi to use GSSAPI." @@ -2539,7 +2547,7 @@ class ClientSession(object): # will fail with a handshake failure, which is retried by default. # For this case we're now using retry=False and test errors for # this exact usecase. - kwargs = {'proxyuser': proxyuser} + kwargs = {'proxyuser': proxyuser, 'session_key': session_key} if proxyauthtype is not None: kwargs['proxyauthtype'] = proxyauthtype for tries in range(self.opts.get('max_retries', 30)): @@ -2587,7 +2595,8 @@ class ClientSession(object): self.authtype = AUTHTYPES['GSSAPI'] return True - def ssl_login(self, cert=None, ca=None, serverca=None, proxyuser=None, proxyauthtype=None): + def ssl_login(self, cert=None, ca=None, serverca=None, proxyuser=None, proxyauthtype=None, + session_key=None): cert = cert or self.opts.get('cert') serverca = serverca or self.opts.get('serverca') if cert is None: @@ -2616,7 +2625,7 @@ class ClientSession(object): self.opts['serverca'] = serverca e_str = None try: - kwargs = {'proxyuser': proxyuser} + kwargs = {'proxyuser': proxyuser, 'session_key': session_key} if proxyauthtype is not None: kwargs['proxyauthtype'] = proxyauthtype sinfo = self._callMethod('sslLogin', [], kwargs) @@ -2833,6 +2842,32 @@ class ClientSession(object): result = result[0] return result + def _renew_session(self): + session_key = self.session_key + self.setSession(None) + if self.authtype == 'SSL' or \ + (self.opts.get('cert') and os.path.isfile(self.opts['cert'])): + self.ssl_login(cert=self.opts['cert'], + serverca=self.opts['serverca'], + session_key=session_key) + elif self.authtype == 'NORMAL' or self.opts.get('user'): + self.login(user=self.opts['user'], password=self.opts['password'], + session_key=session_key) + elif self.authtype in ['KERBEROS', 'GSSAPI'] or \ + self.opts.get('krb_principal'): + authtype = self.authtype or AUTHTYPES['GSSAPI'] + principal = self.opts.get('principal') + keytab = self.opts.get('keytab') + ccache = self.opts.get('ccache') + if authtype == 'KERBEROS': + self.krb_login(principal=principal, keytab=keytab, + ccache=ccache, session_key=session_key) + elif authtype == 'GSSAPI': + self.gssapi_login(self, principal=principal, keytab=keytab, + ccache=ccache, session_key=session_key) + if self.exclusive: + self.exclusiveSession() + def _callMethod(self, name, args, kwargs=None, retry=True): """Make a call to the hub with retries and other niceties""" @@ -2871,7 +2906,16 @@ class ClientSession(object): # server correctly reporting an outage tries = 0 continue - raise err + elif isinstance(err, AuthExpired): + if self.logged_in: + self._renew_session() + return self._callMethod(name, args, kwargs, retry) + else: + raise AuthError("Session ID %s is unlogged and expired." % + self.sinfo['session-id']) + else: + raise err + except (SystemExit, KeyboardInterrupt): # (depending on the python version, these may or may not be subclasses of # Exception) @@ -3183,6 +3227,11 @@ class ClientSession(object): result = self.callMethod('downloadTaskOutput', taskID, fileName, **dlopts) return base64.b64decode(result) + def exclusiveSession(self, force=False): + """Make this session exclusive""" + self._callMethod('exclusiveSession', {'force': force}) + self.exclusive = True + class MultiCallHack(object): """Workaround of a terribly overloaded namespace diff --git a/koji/auth.py b/koji/auth.py index 74b41a3..2da0e37 100644 --- a/koji/auth.py +++ b/koji/auth.py @@ -280,7 +280,7 @@ class Session(object): if result['status'] != koji.USER_STATUS['NORMAL']: raise koji.AuthError('logins by %s are not allowed' % result['name']) - def login(self, user, password, opts=None): + def login(self, user, password, session_key=None, opts=None): """create a login session""" if opts is None: opts = {} @@ -301,7 +301,8 @@ class Session(object): self.checkLoginAllowed(user_id) # create session and return - sinfo = self.createSession(user_id, hostip, koji.AUTHTYPES['NORMAL']) + sinfo = self.createSession(user_id, hostip, koji.AUTHTYPES['NORMAL'], + session_key=session_key) context.cnx.commit() return sinfo @@ -326,7 +327,7 @@ class Session(object): return (local_ip, local_port, remote_ip, remote_port) - def sslLogin(self, proxyuser=None, proxyauthtype=None): + def sslLogin(self, proxyuser=None, proxyauthtype=None, session_key=None): """Login into brew via SSL. proxyuser name can be specified and if it is allowed in the configuration file then connection is allowed to login as @@ -404,7 +405,7 @@ class Session(object): hostip = self.get_remote_ip() - sinfo = self.createSession(user_id, hostip, authtype) + sinfo = self.createSession(user_id, hostip, authtype, session_key=session_key) return sinfo def makeExclusive(self, force=False): @@ -483,12 +484,22 @@ class Session(object): update.execute() context.cnx.commit() - def createSession(self, user_id, hostip, authtype, master=None): + def createSession(self, user_id, hostip, authtype, master=None, session_key=None): """Create a new session for the given user. Return a map containing the session-id and session-key. If master is specified, create a subsession """ + if session_key: + query = QueryProcessor(tables=['sessions'], columns=['master'], + clauses=['key=%(session_key)d'], + values={'session_key': session_key}) + row = query.executeOne(strict=False) + if not row: + raise koji.GenericError("Don't allow to renew subsession, " + "subsession doesn't exist.") + master = row['master'] + # generate a random key alnum = string.ascii_letters + string.digits key = "%s-%s" % (user_id, diff --git a/tests/test_lib/test_gssapi.py b/tests/test_lib/test_gssapi.py index 6249221..1c7d918 100644 --- a/tests/test_lib/test_gssapi.py +++ b/tests/test_lib/test_gssapi.py @@ -27,7 +27,7 @@ class TestGSSAPI(unittest.TestCase): old_environ = dict(**os.environ) self.session.gssapi_login() self.session._callMethod.assert_called_with( - 'sslLogin', [], {'proxyuser': None}, retry=False) + 'sslLogin', [], {'proxyuser': None, 'session_key': None}, retry=False) self.assertEqual(old_environ, dict(**os.environ)) @mock.patch('koji.reqgssapi.HTTPKerberosAuth') @@ -47,7 +47,7 @@ class TestGSSAPI(unittest.TestCase): koji.reqgssapi.__version__ = accepted_version rv = self.session.gssapi_login(principal, keytab, ccache) self.session._callMethod.assert_called_with( - 'sslLogin', [], {'proxyuser': None}, retry=False) + 'sslLogin', [], {'proxyuser': None, 'session_key': None}, retry=False) self.assertEqual(old_environ, dict(**os.environ)) self.assertTrue(rv) self.session._callMethod.reset_mock() @@ -84,7 +84,7 @@ class TestGSSAPI(unittest.TestCase): with self.assertRaises(koji.GSSAPIAuthError): self.session.gssapi_login() self.session._callMethod.assert_called_with( - 'sslLogin', [], {'proxyuser': None}, retry=False) + 'sslLogin', [], {'proxyuser': None, 'session_key': None}, retry=False) self.assertEqual(old_environ, dict(**os.environ)) def test_gssapi_login_http(self): From c136b74e77be26d6aac6ae87f0922a2d9be2683f Mon Sep 17 00:00:00 2001 From: Tomas Kopecek Date: Jan 25 2023 14:24:19 +0000 Subject: [PATCH 2/16] store original auth method --- diff --git a/koji/__init__.py b/koji/__init__.py index 883550a..d116636 100644 --- a/koji/__init__.py +++ b/koji/__init__.py @@ -2408,9 +2408,6 @@ def grab_session_options(options): 'upload_blocksize', 'no_ssl_verify', 'serverca', - 'keytab', - 'principal', - 'ccache', ) # cert is omitted for now if isinstance(options, dict): @@ -2427,7 +2424,13 @@ def grab_session_options(options): class ClientSession(object): - def __init__(self, baseurl, opts=None, sinfo=None): + def __init__(self, baseurl, opts=None, sinfo=None, auth_method=None): + """ + :param baseurl str: hub url + :param dict opts: dictionary with content varying according to authentication method + :param dict sinfo: session info returned by login method + :param dict auth_method: method for reauthentication, shouldn't be ever set manually + """ assert baseurl, "baseurl argument must not be empty" if opts is None: opts = {} @@ -2448,7 +2451,7 @@ class ClientSession(object): self.new_session() self.opts.setdefault('timeout', DEFAULT_REQUEST_TIMEOUT) self.exclusive = False - self.hostip = None + self.auth_method = auth_method @property def multicall(self): @@ -2487,9 +2490,18 @@ class ClientSession(object): self.session_key = sinfo['session-key'] self.sinfo = sinfo - def login(self, opts=None): + def login(self, opts=None, session_key=None): + """ + Username/password based login method + + :param dict opts: dict used by hub "login" call, currently can + contain only "host_ip" key. + :returns bool True: success or raises exception + """ + # store calling parameters + self.auth_method = {'method': 'login', 'kwargs': {'opts': opts}} sinfo = self.callMethod('login', self.opts['user'], self.opts['password'], - self.opts['session_key'], opts) + opts=opts, session_key=session_key) if not sinfo: return False self.setSession(sinfo) @@ -2499,14 +2511,33 @@ class ClientSession(object): def subsession(self): "Create a subsession" sinfo = self.callMethod('subsession') - return type(self)(self.baseurl, self.opts, sinfo) + return type(self)(self.baseurl, opts=self.opts, sinfo=sinfo, auth_method=self.auth_method) def gssapi_login(self, principal=None, keytab=None, ccache=None, proxyuser=None, proxyauthtype=None, session_key=None): + """ + GSSAPI/Kerberos login method + + :param str principal: Kerberos principal + :param str keytab: path to keytab file + :param str ccache: path to ccache file/dir + :param str proxyuser: name of proxied user (e.g. forwarding by web ui) + :param int proxyauthtype: AUTHTYPE used by proxied user (can be different from ours) + :param str session_key: used for session renewal + :returns bool True: success or raises exception + """ if not reqgssapi: raise PythonImportError( "Please install python-requests-gssapi to use GSSAPI." ) + # store calling parameters + self.auth_method = { + 'method': 'gssapi_login', + 'kwargs': { + 'principal': principal, 'keytab': keytab, 'ccache': ccache, 'proxyuser': proxyuser, + 'proxyauthtype': proxyauthtype, 'session_key': session_key + } + } # force https old_baseurl = self.baseurl uri = six.moves.urllib.parse.urlsplit(self.baseurl) @@ -2597,6 +2628,26 @@ class ClientSession(object): def ssl_login(self, cert=None, ca=None, serverca=None, proxyuser=None, proxyauthtype=None, session_key=None): + """ + SSL cert based login + + :param str cert: path to SSL certificate + :param str ca: deprecated, not used anymore + :param str serverca: path for CA public cert, otherwise system-wide CAs are used + :param str proxyuser: name of proxied user (e.g. forwarding by web ui) + :param int proxyauthtype: AUTHTYPE used by proxied user (can be different from ours) + :param str session_key: used for session renewal + :returns bool: success + """ + # store calling parameters + self.logger.error("ssl_login---------------") + self.auth_method = { + 'method': 'ssl_login', + 'kwargs': { + 'cert': cert, 'ca': ca, 'serverca': serverca, 'proxyuser': proxyuser, + 'proxyauthtype': proxyauthtype, 'session_key': session_key, + } + } cert = cert or self.opts.get('cert') serverca = serverca or self.opts.get('serverca') if cert is None: @@ -2843,28 +2894,15 @@ class ClientSession(object): return result def _renew_session(self): - session_key = self.session_key + if not hasattr(self, 'auth_method'): + raise GenericError("Missing info for reauthentication") + # will be deleted by setSession + auth_method = self.auth_method['method'] + args = self.auth_method.get('args', []) + kwargs = self.auth_method.get('kwargs', {}) + kwargs['session_key'] = self.session_key self.setSession(None) - if self.authtype == 'SSL' or \ - (self.opts.get('cert') and os.path.isfile(self.opts['cert'])): - self.ssl_login(cert=self.opts['cert'], - serverca=self.opts['serverca'], - session_key=session_key) - elif self.authtype == 'NORMAL' or self.opts.get('user'): - self.login(user=self.opts['user'], password=self.opts['password'], - session_key=session_key) - elif self.authtype in ['KERBEROS', 'GSSAPI'] or \ - self.opts.get('krb_principal'): - authtype = self.authtype or AUTHTYPES['GSSAPI'] - principal = self.opts.get('principal') - keytab = self.opts.get('keytab') - ccache = self.opts.get('ccache') - if authtype == 'KERBEROS': - self.krb_login(principal=principal, keytab=keytab, - ccache=ccache, session_key=session_key) - elif authtype == 'GSSAPI': - self.gssapi_login(self, principal=principal, keytab=keytab, - ccache=ccache, session_key=session_key) + auth_method(*args, **kwargs) if self.exclusive: self.exclusiveSession() diff --git a/koji/auth.py b/koji/auth.py index 2da0e37..a5ac19f 100644 --- a/koji/auth.py +++ b/koji/auth.py @@ -280,7 +280,7 @@ class Session(object): if result['status'] != koji.USER_STATUS['NORMAL']: raise koji.AuthError('logins by %s are not allowed' % result['name']) - def login(self, user, password, session_key=None, opts=None): + def login(self, user, password, opts=None, session_key=None): """create a login session""" if opts is None: opts = {} @@ -491,14 +491,14 @@ class Session(object): If master is specified, create a subsession """ if session_key: + if master: + raise koji.GenericError("Can't call createSession with both master + session_key.") query = QueryProcessor(tables=['sessions'], columns=['master'], clauses=['key=%(session_key)d'], values={'session_key': session_key}) - row = query.executeOne(strict=False) - if not row: - raise koji.GenericError("Don't allow to renew subsession, " - "subsession doesn't exist.") - master = row['master'] + master = query.singleValue(strict=False) + if not master: + raise koji.GenericError("Don't allow to renew non-existent subsession") # generate a random key alnum = string.ascii_letters + string.digits From 9b8e8ceeb1913669198178a59a9b7d3124e2db13 Mon Sep 17 00:00:00 2001 From: Jana Cupova Date: Jan 25 2023 14:24:19 +0000 Subject: [PATCH 3/16] Fix call auth_method --- diff --git a/koji/__init__.py b/koji/__init__.py index d116636..9014661 100644 --- a/koji/__init__.py +++ b/koji/__init__.py @@ -2897,7 +2897,7 @@ class ClientSession(object): if not hasattr(self, 'auth_method'): raise GenericError("Missing info for reauthentication") # will be deleted by setSession - auth_method = self.auth_method['method'] + auth_method = getattr(self, self.auth_method['method']) args = self.auth_method.get('args', []) kwargs = self.auth_method.get('kwargs', {}) kwargs['session_key'] = self.session_key diff --git a/koji/auth.py b/koji/auth.py index a5ac19f..a480a76 100644 --- a/koji/auth.py +++ b/koji/auth.py @@ -496,9 +496,10 @@ class Session(object): query = QueryProcessor(tables=['sessions'], columns=['master'], clauses=['key=%(session_key)d'], values={'session_key': session_key}) - master = query.singleValue(strict=False) - if not master: + row = query.executeOne(strict=False) + if not row: raise koji.GenericError("Don't allow to renew non-existent subsession") + master = row['master'] # generate a random key alnum = string.ascii_letters + string.digits From 2a73491a6fd44c5b487092510725d108ac2e0fc2 Mon Sep 17 00:00:00 2001 From: Jana Cupova Date: Jan 25 2023 14:24:19 +0000 Subject: [PATCH 4/16] Add closed column to session table and use it in session --- diff --git a/docs/schema-update-1.31-1.32.sql b/docs/schema-update-1.31-1.32.sql new file mode 100644 index 0000000..86edce2 --- /dev/null +++ b/docs/schema-update-1.31-1.32.sql @@ -0,0 +1,11 @@ +-- upgrade script to migrate the Koji database schema +-- from version 1.31 to 1.32 + +BEGIN; + + -- fix duplicate extension in archivetypes + UPDATE archivetypes SET extensions = 'vhdx.gz vhdx.xz' WHERE name = 'vhdx-compressed'; + + -- for tag if session is closed or not + ALTER TABLE sessions ADD COLUMN closed BOOLEAN NOT NULL DEFAULT 'false'; +COMMIT; diff --git a/docs/schema.sql b/docs/schema.sql index c2ea695..6d6a24a 100644 --- a/docs/schema.sql +++ b/docs/schema.sql @@ -119,6 +119,7 @@ CREATE TABLE sessions ( start_time TIMESTAMPTZ NOT NULL DEFAULT NOW(), update_time TIMESTAMPTZ NOT NULL DEFAULT NOW(), exclusive BOOLEAN CHECK (exclusive), + closed BOOLEAN NOT NULL DEFAULT FALSE, CONSTRAINT no_exclusive_subsessions CHECK ( master IS NULL OR "exclusive" IS NULL), CONSTRAINT exclusive_expired_sane CHECK ( diff --git a/koji/auth.py b/koji/auth.py index a480a76..107160d 100644 --- a/koji/auth.py +++ b/koji/auth.py @@ -465,7 +465,8 @@ class Session(object): ses_id = session_id else: ses_id = self.id - update = UpdateProcessor('sessions', data={'expired': True, 'exclusive': None}, + update = UpdateProcessor('sessions', + data={'expired': True, 'exclusive': None, 'closed': True}, clauses=['id = %(id)i OR master = %(id)i'], values={'id': ses_id}) update.execute() @@ -494,11 +495,11 @@ class Session(object): if master: raise koji.GenericError("Can't call createSession with both master + session_key.") query = QueryProcessor(tables=['sessions'], columns=['master'], - clauses=['key=%(session_key)d'], + clauses=['key=%(session_key)d', 'closed=FALSE'], values={'session_key': session_key}) row = query.executeOne(strict=False) if not row: - raise koji.GenericError("Don't allow to renew non-existent subsession") + raise koji.GenericError("Don't allow to renew non-existent or logged out session") master = row['master'] # generate a random key From 78dd6048125a053e0032725db38f1c63368445c4 Mon Sep 17 00:00:00 2001 From: Jana Cupova Date: Jan 25 2023 14:24:19 +0000 Subject: [PATCH 5/16] Add decorator for renew expired session --- diff --git a/koji/__init__.py b/koji/__init__.py index 9014661..fd3c0e8 100644 --- a/koji/__init__.py +++ b/koji/__init__.py @@ -2688,6 +2688,7 @@ class ClientSession(object): sinfo = None finally: self.opts = old_opts + if not sinfo: err = 'unable to obtain a session' if e_str: @@ -2894,6 +2895,7 @@ class ClientSession(object): return result def _renew_session(self): + """Renew expirated session or subsession.""" if not hasattr(self, 'auth_method'): raise GenericError("Missing info for reauthentication") # will be deleted by setSession @@ -2906,9 +2908,19 @@ class ClientSession(object): if self.exclusive: self.exclusiveSession() + def renew_expired_session(func): + """Decorator to renew expirated session or subsession.""" + def _renew_expired_session(*args, **kwargs): + try: + return func(*args, **kwargs) + except AuthExpired: + args[0]._renew_session() + return func(*args, **kwargs) + return _renew_expired_session + + @renew_expired_session def _callMethod(self, name, args, kwargs=None, retry=True): """Make a call to the hub with retries and other niceties""" - if self.multicall: if kwargs is None: kwargs = {} @@ -2944,13 +2956,6 @@ class ClientSession(object): # server correctly reporting an outage tries = 0 continue - elif isinstance(err, AuthExpired): - if self.logged_in: - self._renew_session() - return self._callMethod(name, args, kwargs, retry) - else: - raise AuthError("Session ID %s is unlogged and expired." % - self.sinfo['session-id']) else: raise err From 2d87a495cde5998ee5624a1130627495bb34437b Mon Sep 17 00:00:00 2001 From: Jana Cupova Date: Jan 25 2023 14:24:19 +0000 Subject: [PATCH 6/16] Fix unit tests --- diff --git a/tests/test_lib/test_auth.py b/tests/test_lib/test_auth.py index 36a6473..64a9b7a 100644 --- a/tests/test_lib/test_auth.py +++ b/tests/test_lib/test_auth.py @@ -434,7 +434,7 @@ class TestAuthSession(unittest.TestCase): self.assertEqual(update.table, 'sessions') self.assertEqual(update.values, {'id': 123, 'id': 123}) self.assertEqual(update.clauses, ['id = %(id)i OR master = %(id)i']) - self.assertEqual(update.data, {'expired': True, 'exclusive': None}) + self.assertEqual(update.data, {'closed': True, 'expired': True, 'exclusive': None}) self.assertEqual(update.rawdata, {}) def test_logoutChild_not_logged(self): @@ -668,15 +668,6 @@ class TestAuthSession(unittest.TestCase): self.assertEqual(query.clauses, ['active = TRUE', 'user_id=%(user_id)s']) self.assertEqual(query.columns, ['name']) - def test_logout_not_logged(self): - s, cntext = self.get_session() - - # not logged - s.logged_in = False - with self.assertRaises(koji.AuthError) as ex: - s.logout() - self.assertEqual("Not logged in", str(ex.exception)) - @mock.patch('koji.auth.context') def test_logout_logged_not_owner(self, context): s, cntext = self.get_session() From 4fe39dea6e3ed41338eafffab4874b1502f6e706 Mon Sep 17 00:00:00 2001 From: Tomas Kopecek Date: Jan 25 2023 14:24:19 +0000 Subject: [PATCH 7/16] remove passing session-id --- diff --git a/docs/schema-update-1.31-1.32.sql b/docs/schema-update-1.31-1.32.sql index 86edce2..1b6a17d 100644 --- a/docs/schema-update-1.31-1.32.sql +++ b/docs/schema-update-1.31-1.32.sql @@ -7,5 +7,6 @@ BEGIN; UPDATE archivetypes SET extensions = 'vhdx.gz vhdx.xz' WHERE name = 'vhdx-compressed'; -- for tag if session is closed or not - ALTER TABLE sessions ADD COLUMN closed BOOLEAN NOT NULL DEFAULT 'false'; + ALTER TABLE sessions ADD COLUMN closed BOOLEAN NOT NULL DEFAULT FALSE; + ALTER TABLE sessions ADD CONSTRAINT no_closed_exclusive CHECK (closed IS FALSE OR "exclusive" IS NULL); COMMIT; diff --git a/koji/__init__.py b/koji/__init__.py index fd3c0e8..e12b0b3 100644 --- a/koji/__init__.py +++ b/koji/__init__.py @@ -2483,14 +2483,12 @@ class ClientSession(object): self.callnum = None # do we need to do anything else here? self.authtype = None - self.session_key = None else: self.logged_in = True self.callnum = 0 - self.session_key = sinfo['session-key'] self.sinfo = sinfo - def login(self, opts=None, session_key=None): + def login(self, opts=None, renew=False): """ Username/password based login method @@ -2500,8 +2498,8 @@ class ClientSession(object): """ # store calling parameters self.auth_method = {'method': 'login', 'kwargs': {'opts': opts}} - sinfo = self.callMethod('login', self.opts['user'], self.opts['password'], - opts=opts, session_key=session_key) + sinfo = self.callMethod('login', self.opts['user'], self.opts['password'], opts=opts, + renew=renew) if not sinfo: return False self.setSession(sinfo) @@ -2514,7 +2512,7 @@ class ClientSession(object): return type(self)(self.baseurl, opts=self.opts, sinfo=sinfo, auth_method=self.auth_method) def gssapi_login(self, principal=None, keytab=None, ccache=None, - proxyuser=None, proxyauthtype=None, session_key=None): + proxyuser=None, proxyauthtype=None, renew=False): """ GSSAPI/Kerberos login method @@ -2523,7 +2521,6 @@ class ClientSession(object): :param str ccache: path to ccache file/dir :param str proxyuser: name of proxied user (e.g. forwarding by web ui) :param int proxyauthtype: AUTHTYPE used by proxied user (can be different from ours) - :param str session_key: used for session renewal :returns bool True: success or raises exception """ if not reqgssapi: @@ -2535,7 +2532,7 @@ class ClientSession(object): 'method': 'gssapi_login', 'kwargs': { 'principal': principal, 'keytab': keytab, 'ccache': ccache, 'proxyuser': proxyuser, - 'proxyauthtype': proxyauthtype, 'session_key': session_key + 'proxyauthtype': proxyauthtype } } # force https @@ -2578,7 +2575,7 @@ class ClientSession(object): # will fail with a handshake failure, which is retried by default. # For this case we're now using retry=False and test errors for # this exact usecase. - kwargs = {'proxyuser': proxyuser, 'session_key': session_key} + kwargs = {'proxyuser': proxyuser, 'renew': renew} if proxyauthtype is not None: kwargs['proxyauthtype'] = proxyauthtype for tries in range(self.opts.get('max_retries', 30)): @@ -2627,7 +2624,7 @@ class ClientSession(object): return True def ssl_login(self, cert=None, ca=None, serverca=None, proxyuser=None, proxyauthtype=None, - session_key=None): + renew=False): """ SSL cert based login @@ -2636,16 +2633,14 @@ class ClientSession(object): :param str serverca: path for CA public cert, otherwise system-wide CAs are used :param str proxyuser: name of proxied user (e.g. forwarding by web ui) :param int proxyauthtype: AUTHTYPE used by proxied user (can be different from ours) - :param str session_key: used for session renewal :returns bool: success """ # store calling parameters - self.logger.error("ssl_login---------------") self.auth_method = { 'method': 'ssl_login', 'kwargs': { - 'cert': cert, 'ca': ca, 'serverca': serverca, 'proxyuser': proxyuser, - 'proxyauthtype': proxyauthtype, 'session_key': session_key, + 'cert': cert, 'ca': ca, 'serverca': serverca, + 'proxyuser': proxyuser, 'proxyauthtype': proxyauthtype, } } cert = cert or self.opts.get('cert') @@ -2676,7 +2671,7 @@ class ClientSession(object): self.opts['serverca'] = serverca e_str = None try: - kwargs = {'proxyuser': proxyuser, 'session_key': session_key} + kwargs = {'proxyuser': proxyuser, 'renew': renew} if proxyauthtype is not None: kwargs['proxyauthtype'] = proxyauthtype sinfo = self._callMethod('sslLogin', [], kwargs) @@ -2768,24 +2763,28 @@ class ClientSession(object): return self._prepUpload(*args, **kwargs) args = encode_args(*args, **kwargs) headers = [] - if self.logged_in: + + sinfo = None + if getattr(self, 'sinfo') is not None: + # session renewal (not logged in, but have session data) + # makes sense only for new method/server sinfo = self.sinfo.copy() sinfo['callnum'] = self.callnum self.callnum += 1 - if sinfo.get('header-auth'): - handler = self.baseurl - headers += [ - ('Koji-Session-Id', str(self.sinfo['session-id'])), - ('Koji-Session-Key', str(self.sinfo['session-key'])), - ('Koji-Session-Callnum', str(sinfo['callnum'])), - ] - else: - # old server - handler = "%s?%s" % (self.baseurl, six.moves.urllib.parse.urlencode(sinfo)) - elif name == 'sslLogin': + headers += [ + ('Koji-Session-Id', str(sinfo['session-id'])), + ('Koji-Session-Key', str(sinfo['session-key'])), + ('Koji-Session-Callnum', str(sinfo['callnum'])), + ] + + if self.logged_in and not self.sinfo.get('header-auth'): + # old server + handler = "%s?%s" % (self.baseurl, six.moves.urllib.parse.urlencode(sinfo)) + elif name in 'sslLogin': handler = self.baseurl + '/ssllogin' else: handler = self.baseurl + request = dumps(args, name, allow_none=1) if six.PY3: # For python2, dumps() without encoding specified means return a str @@ -2898,12 +2897,11 @@ class ClientSession(object): """Renew expirated session or subsession.""" if not hasattr(self, 'auth_method'): raise GenericError("Missing info for reauthentication") - # will be deleted by setSession auth_method = getattr(self, self.auth_method['method']) args = self.auth_method.get('args', []) kwargs = self.auth_method.get('kwargs', {}) - kwargs['session_key'] = self.session_key - self.setSession(None) + kwargs['renew'] = True + self.logged_in = False auth_method(*args, **kwargs) if self.exclusive: self.exclusiveSession() diff --git a/koji/auth.py b/koji/auth.py index 107160d..d381848 100644 --- a/koji/auth.py +++ b/koji/auth.py @@ -56,6 +56,8 @@ RetryWhitelist = [ 'repoProblem', ] +AUTH_METHODS = ['login', 'sslLogin'] + logger = logging.getLogger('koji.auth') @@ -82,8 +84,8 @@ class Session(object): args = environ.get('QUERY_STRING', '') # prefer new header-based sessions if 'HTTP_KOJI_SESSION_ID' in environ: - id = int(environ['HTTP_KOJI_SESSION_ID']) - key = environ['HTTP_KOJI_SESSION_KEY'] + self.id = int(environ['HTTP_KOJI_SESSION_ID']) + self.key = environ['HTTP_KOJI_SESSION_KEY'] try: callnum = int(environ['HTTP_KOJI_CALLNUM']) except KeyError: @@ -96,8 +98,8 @@ class Session(object): return args = urllib.parse.parse_qs(args, strict_parsing=True) try: - id = int(args['session-id'][0]) - key = args['session-key'][0] + self.id = int(args['session-id'][0]) + self.key = args['session-key'][0] except KeyError as field: raise koji.AuthError('%s not specified in session args' % field) try: @@ -119,23 +121,26 @@ class Session(object): query = QueryProcessor(tables=['sessions'], columns=columns, aliases=aliases, clauses=['id = %(id)i', 'key = %(key)s', 'hostip = %(hostip)s'], - values={'id': id, 'key': key, 'hostip': hostip}, + values={'id': self.id, 'key': self.key, 'hostip': hostip}, opts={'rowlock': True}) session_data = query.executeOne(strict=False) if not session_data: query = QueryProcessor(tables=['sessions'], columns=['key', 'hostip'], - clauses=['id = %(id)i'], values={'id': id}) + clauses=['id = %(id)i'], values={'id': self.id}) row = query.executeOne(strict=False) if row: - if key != row['key']: - logger.warning("Session ID %s is not related to session key %s.", id, key) + if self.key != row['key']: + logger.warning("Session ID %s is not related to session key %s.", + self.id, self.key) elif hostip != row['hostip']: - logger.warning("Session ID %s is not related to host IP %s.", id, hostip) + logger.warning("Session ID %s is not related to host IP %s.", self.id, hostip) raise koji.AuthError('Invalid session or bad credentials') # check for expiration if session_data['expired']: - raise koji.AuthExpired('session "%i" has expired' % id) + if getattr(context, 'method') not in AUTH_METHODS: + raise koji.AuthExpired(f'session "{self.id}" has expired') + # check for callnum sanity if callnum is not None: try: @@ -145,8 +150,7 @@ class Session(object): lastcall = session_data['callnum'] if lastcall is not None: if lastcall > callnum: - raise koji.SequenceError("%d > %d (session %d)" - % (lastcall, callnum, id)) + raise koji.SequenceError(f"{lastcall} > {callnum} (session {self.id})") elif lastcall == callnum: # Some explanation: # This function is one of the few that performs its own commit. @@ -159,8 +163,11 @@ class Session(object): method = getattr(context, 'method', 'UNKNOWN') if method not in RetryWhitelist: raise koji.RetryError( - "unable to retry call %d (method %s) for session %d" - % (callnum, method, id)) + f"unable to retry call {callnum} " + f"(method {method}) for session {self.id}") + + if session_data['expired']: + return # read user data # historical note: @@ -200,21 +207,19 @@ class Session(object): # update timestamp update = UpdateProcessor('sessions', rawdata={'update_time': 'NOW()'}, - clauses=['id = %(id)i'], values={'id': id}) + clauses=['id = %(id)i'], values={'id': self.id}) update.execute() context.cnx.commit() # update callnum (this is deliberately after the commit) # see earlier note near RetryError if callnum is not None: update = UpdateProcessor('sessions', data={'callnum': callnum}, - clauses=['id = %(id)i'], values={'id': id}) + clauses=['id = %(id)i'], values={'id': self.id}) update.execute() # we only want to commit the callnum change if there are other commits context.commit_pending = False # record the login data - self.id = id - self.key = key self.hostip = hostip self.callnum = callnum self.user_id = session_data['user_id'] @@ -327,7 +332,7 @@ class Session(object): return (local_ip, local_port, remote_ip, remote_port) - def sslLogin(self, proxyuser=None, proxyauthtype=None, session_key=None): + def sslLogin(self, proxyuser=None, proxyauthtype=None, renew=False): """Login into brew via SSL. proxyuser name can be specified and if it is allowed in the configuration file then connection is allowed to login as @@ -405,7 +410,7 @@ class Session(object): hostip = self.get_remote_ip() - sinfo = self.createSession(user_id, hostip, authtype, session_key=session_key) + sinfo = self.createSession(user_id, hostip, authtype, renew=renew) return sinfo def makeExclusive(self, force=False): @@ -485,37 +490,48 @@ class Session(object): update.execute() context.cnx.commit() - def createSession(self, user_id, hostip, authtype, master=None, session_key=None): + def createSession(self, user_id, hostip, authtype, master=None, renew=False): """Create a new session for the given user. Return a map containing the session-id and session-key. If master is specified, create a subsession """ - if session_key: - if master: - raise koji.GenericError("Can't call createSession with both master + session_key.") - query = QueryProcessor(tables=['sessions'], columns=['master'], - clauses=['key=%(session_key)d', 'closed=FALSE'], - values={'session_key': session_key}) - row = query.executeOne(strict=False) - if not row: - raise koji.GenericError("Don't allow to renew non-existent or logged out session") - master = row['master'] - # generate a random key alnum = string.ascii_letters + string.digits key = "%s-%s" % (user_id, ''.join([random.choice(alnum) for x in range(1, 20)])) # use sha? sha.new(phrase).hexdigest() - # get a session id - session_id = nextval('sessions_id_seq') - - # add session id to database - insert = InsertProcessor('sessions', - data={'id': session_id, 'user_id': user_id, 'key': key, - 'hostip': hostip, 'authtype': authtype, 'master': master}) - insert.execute() + if renew and self.id is not None: + # just update key + session_id = self.id + self.key = key + if self.master: + # check if master session died meanwhile + query = QueryProcessor(tables=['sessions'], + clauses=['id = %(master_id)d', + 'expired IS FALSE', + 'closed IS FALSE'], + values={'master_id': self.master}, + opts={'countOnly': True}) + if query.executeOne() == 0: + return None + + update = UpdateProcessor('sessions', + clauses=['id=%(id)i'], + rawdata={'update_time': 'NOW()'}, + data={'key': self.key, 'expired': False}, + values={'id': self.id}) + update.execute() + else: + # get a session id + session_id = nextval('sessions_id_seq') + # add session id to database + insert = InsertProcessor('sessions', + data={'id': session_id, 'user_id': user_id, 'key': key, + 'hostip': hostip, 'authtype': authtype, + 'master': master}) + insert.execute() context.cnx.commit() # return session info @@ -532,8 +548,7 @@ class Session(object): master = self.master if master is None: master = self.id - return self.createSession(self.user_id, self.hostip, self.authtype, - master=master) + return self.createSession(self.user_id, self.hostip, self.authtype, master=master) def getPerms(self): if not self.logged_in: From 440e6085f560ec70b2056aeb9751a143c9530dcb Mon Sep 17 00:00:00 2001 From: Tomas Kopecek Date: Jan 25 2023 14:24:19 +0000 Subject: [PATCH 8/16] renew exclusive status as part of login --- diff --git a/koji/__init__.py b/koji/__init__.py index e12b0b3..966b374 100644 --- a/koji/__init__.py +++ b/koji/__init__.py @@ -2498,8 +2498,11 @@ class ClientSession(object): """ # store calling parameters self.auth_method = {'method': 'login', 'kwargs': {'opts': opts}} - sinfo = self.callMethod('login', self.opts['user'], self.opts['password'], opts=opts, - renew=renew) + kwargs = {'opts': opts} + if renew: + kwargs['renew'] = True + kwargs['exclusive'] = self.exclusive + sinfo = self.callMethod('login', self.opts['user'], self.opts['password'], **kwargs) if not sinfo: return False self.setSession(sinfo) @@ -2575,7 +2578,10 @@ class ClientSession(object): # will fail with a handshake failure, which is retried by default. # For this case we're now using retry=False and test errors for # this exact usecase. - kwargs = {'proxyuser': proxyuser, 'renew': renew} + kwargs = {'proxyuser': proxyuser} + if renew: + kwargs['renew'] = True + kwargs['exclusive'] = self.exclusive if proxyauthtype is not None: kwargs['proxyauthtype'] = proxyauthtype for tries in range(self.opts.get('max_retries', 30)): @@ -2671,7 +2677,10 @@ class ClientSession(object): self.opts['serverca'] = serverca e_str = None try: - kwargs = {'proxyuser': proxyuser, 'renew': renew} + kwargs = {'proxyuser': proxyuser} + if renew: + kwargs['renew'] = True + kwargs['exclusive'] = self.exclusive if proxyauthtype is not None: kwargs['proxyauthtype'] = proxyauthtype sinfo = self._callMethod('sslLogin', [], kwargs) @@ -2903,8 +2912,6 @@ class ClientSession(object): kwargs['renew'] = True self.logged_in = False auth_method(*args, **kwargs) - if self.exclusive: - self.exclusiveSession() def renew_expired_session(func): """Decorator to renew expirated session or subsession.""" diff --git a/koji/auth.py b/koji/auth.py index d381848..efcb1b6 100644 --- a/koji/auth.py +++ b/koji/auth.py @@ -120,7 +120,8 @@ class Session(object): columns, aliases = zip(*fields) query = QueryProcessor(tables=['sessions'], columns=columns, aliases=aliases, - clauses=['id = %(id)i', 'key = %(key)s', 'hostip = %(hostip)s'], + clauses=['id = %(id)i', 'key = %(key)s', 'hostip = %(hostip)s', + 'closed IS FALSE'], values={'id': self.id, 'key': self.key, 'hostip': hostip}, opts={'rowlock': True}) session_data = query.executeOne(strict=False) @@ -146,7 +147,7 @@ class Session(object): try: callnum = int(callnum) except (ValueError, TypeError): - raise koji.AuthError("Invalid callnum: %r" % callnum) + raise koji.AuthError(f"Invalid callnum: {callnum!r}") lastcall = session_data['callnum'] if lastcall is not None: if lastcall > callnum: @@ -285,7 +286,7 @@ class Session(object): if result['status'] != koji.USER_STATUS['NORMAL']: raise koji.AuthError('logins by %s are not allowed' % result['name']) - def login(self, user, password, opts=None, session_key=None): + def login(self, user, password, opts=None, renew=False, exclusive=False): """create a login session""" if opts is None: opts = {} @@ -307,7 +308,9 @@ class Session(object): # create session and return sinfo = self.createSession(user_id, hostip, koji.AUTHTYPES['NORMAL'], - session_key=session_key) + renew=renew) + if sinfo and exclusive and not self.exclusive: + self.makeExclusive() context.cnx.commit() return sinfo @@ -332,7 +335,7 @@ class Session(object): return (local_ip, local_port, remote_ip, remote_port) - def sslLogin(self, proxyuser=None, proxyauthtype=None, renew=False): + def sslLogin(self, proxyuser=None, proxyauthtype=None, renew=False, exclusive=None): """Login into brew via SSL. proxyuser name can be specified and if it is allowed in the configuration file then connection is allowed to login as @@ -411,6 +414,8 @@ class Session(object): hostip = self.get_remote_ip() sinfo = self.createSession(user_id, hostip, authtype, renew=renew) + if sinfo and exclusive and not self.exclusive: + self.makeExclusive() return sinfo def makeExclusive(self, force=False): From 395ae782efc6d70c5f36cd123c8bbd2292159344 Mon Sep 17 00:00:00 2001 From: Tomas Kopecek Date: Jan 25 2023 14:24:19 +0000 Subject: [PATCH 9/16] minor fixes --- diff --git a/docs/schema.sql b/docs/schema.sql index 6d6a24a..1910fac 100644 --- a/docs/schema.sql +++ b/docs/schema.sql @@ -124,6 +124,8 @@ CREATE TABLE sessions ( master IS NULL OR "exclusive" IS NULL), CONSTRAINT exclusive_expired_sane CHECK ( expired IS FALSE OR "exclusive" IS NULL), + CONSTRAINT no_closed_exclusive CHECK ( + closed IS FALSE OR "exclusive" IS NULL), UNIQUE (user_id,exclusive) ) WITHOUT OIDS; CREATE INDEX sessions_master ON sessions(master); diff --git a/koji/__init__.py b/koji/__init__.py index 966b374..b6875c1 100644 --- a/koji/__init__.py +++ b/koji/__init__.py @@ -2745,8 +2745,6 @@ class ClientSession(object): self.new_session() # forget our login session, if any - if not self.logged_in: - return self.setSession(None) # we've had some trouble with this method causing strange problems @@ -2789,7 +2787,7 @@ class ClientSession(object): if self.logged_in and not self.sinfo.get('header-auth'): # old server handler = "%s?%s" % (self.baseurl, six.moves.urllib.parse.urlencode(sinfo)) - elif name in 'sslLogin': + elif name == 'sslLogin': handler = self.baseurl + '/ssllogin' else: handler = self.baseurl @@ -2961,9 +2959,7 @@ class ClientSession(object): # server correctly reporting an outage tries = 0 continue - else: - raise err - + raise err except (SystemExit, KeyboardInterrupt): # (depending on the python version, these may or may not be subclasses of # Exception) diff --git a/koji/auth.py b/koji/auth.py index efcb1b6..3959700 100644 --- a/koji/auth.py +++ b/koji/auth.py @@ -439,8 +439,9 @@ class Session(object): excl_id = query.singleValue(strict=False) if excl_id: if force: - # expire the previous exclusive session and try again - update = UpdateProcessor('sessions', data={'expired': True, 'exclusive': None}, + # close the previous exclusive sessions and try again + update = UpdateProcessor('sessions', + data={'expired': True, 'exclusive': None, 'closed': True}, clauses=['id=%(excl_id)s'], values={'excl_id': excl_id},) update.execute() else: From b536aad77e3852406f999bd91cb6746a153f5628 Mon Sep 17 00:00:00 2001 From: Tomas Kopecek Date: Jan 25 2023 14:24:19 +0000 Subject: [PATCH 10/16] retain expired session exclusivity --- diff --git a/docs/schema-update-1.31-1.32.sql b/docs/schema-update-1.31-1.32.sql index 1b6a17d..a3c6fc5 100644 --- a/docs/schema-update-1.31-1.32.sql +++ b/docs/schema-update-1.31-1.32.sql @@ -9,4 +9,5 @@ BEGIN; -- for tag if session is closed or not ALTER TABLE sessions ADD COLUMN closed BOOLEAN NOT NULL DEFAULT FALSE; ALTER TABLE sessions ADD CONSTRAINT no_closed_exclusive CHECK (closed IS FALSE OR "exclusive" IS NULL); + ALTER TABLE sessions DROP CONSTRAINT exclusive_expired_sane; COMMIT; diff --git a/docs/schema.sql b/docs/schema.sql index 1910fac..007e786 100644 --- a/docs/schema.sql +++ b/docs/schema.sql @@ -122,10 +122,8 @@ CREATE TABLE sessions ( closed BOOLEAN NOT NULL DEFAULT FALSE, CONSTRAINT no_exclusive_subsessions CHECK ( master IS NULL OR "exclusive" IS NULL), - CONSTRAINT exclusive_expired_sane CHECK ( - expired IS FALSE OR "exclusive" IS NULL), - CONSTRAINT no_closed_exclusive CHECK ( - closed IS FALSE OR "exclusive" IS NULL), + CONSTRAINT no_closed_exclusive CHECK ( + closed IS FALSE OR "exclusive" IS NULL), UNIQUE (user_id,exclusive) ) WITHOUT OIDS; CREATE INDEX sessions_master ON sessions(master); diff --git a/koji/auth.py b/koji/auth.py index 3959700..2189592 100644 --- a/koji/auth.py +++ b/koji/auth.py @@ -190,7 +190,7 @@ class Session(object): # see if an exclusive session exists query = QueryProcessor(tables=['sessions'], columns=['id'], clauses=['user_id=%(user_id)s', 'exclusive = TRUE', - 'expired = FALSE'], + 'closed = FALSE'], values=session_data) excl_id = query.singleValue(strict=False) @@ -307,8 +307,7 @@ class Session(object): self.checkLoginAllowed(user_id) # create session and return - sinfo = self.createSession(user_id, hostip, koji.AUTHTYPES['NORMAL'], - renew=renew) + sinfo = self.createSession(user_id, hostip, koji.AUTHTYPES['NORMAL'], renew=renew) if sinfo and exclusive and not self.exclusive: self.makeExclusive() context.cnx.commit() @@ -431,9 +430,9 @@ class Session(object): query = QueryProcessor(tables=['users'], columns=['id'], clauses=['id=%(user_id)s'], values={'user_id': user_id}, opts={'rowlock': True}) query.execute() - # check that no other sessions for this user are exclusive + # check that no other sessions for this user are exclusive (including expired) query = QueryProcessor(tables=['sessions'], columns=['id'], - clauses=['user_id=%(user_id)s', 'expired = FALSE', + clauses=['user_id=%(user_id)s', 'closed = FALSE', 'exclusive = TRUE'], values={'user_id': user_id}, opts={'rowlock': True}) excl_id = query.singleValue(strict=False) @@ -461,7 +460,7 @@ class Session(object): context.cnx.commit() def logout(self, session_id=None): - """expire a login session""" + """close a login session""" if not self.logged_in: # XXX raise an error? raise koji.AuthError("Not logged in") @@ -486,11 +485,12 @@ class Session(object): self.logged_in = False def logoutChild(self, session_id): - """expire a subsession""" + """close a subsession""" if not self.logged_in: # XXX raise an error? raise koji.AuthError("Not logged in") - update = UpdateProcessor('sessions', data={'expired': True, 'exclusive': None}, + update = UpdateProcessor('sessions', + data={'expired': True, 'exclusive': None, 'closed': True}, clauses=['id = %(session_id)i', 'master = %(master)i'], values={'session_id': session_id, 'master': self.id}) update.execute() @@ -513,11 +513,9 @@ class Session(object): session_id = self.id self.key = key if self.master: - # check if master session died meanwhile + # check if master session died meanwhile (expired is ok) query = QueryProcessor(tables=['sessions'], - clauses=['id = %(master_id)d', - 'expired IS FALSE', - 'closed IS FALSE'], + clauses=['id = %(master_id)d', 'closed IS FALSE'], values={'master_id': self.master}, opts={'countOnly': True}) if query.executeOne() == 0: From 203be1e52a8e3ee7b88241387b4819e7f3a7b78e Mon Sep 17 00:00:00 2001 From: Tomas Kopecek Date: Jan 25 2023 14:24:19 +0000 Subject: [PATCH 11/16] fixes --- diff --git a/koji/__init__.py b/koji/__init__.py index b6875c1..912b395 100644 --- a/koji/__init__.py +++ b/koji/__init__.py @@ -2773,8 +2773,8 @@ class ClientSession(object): sinfo = None if getattr(self, 'sinfo') is not None: - # session renewal (not logged in, but have session data) - # makes sense only for new method/server + # send sinfo in headers if we have it + # still needed if not logged in for renewal case sinfo = self.sinfo.copy() sinfo['callnum'] = self.callnum self.callnum += 1 @@ -2911,13 +2911,14 @@ class ClientSession(object): self.logged_in = False auth_method(*args, **kwargs) + @staticmethod def renew_expired_session(func): """Decorator to renew expirated session or subsession.""" - def _renew_expired_session(*args, **kwargs): + def _renew_expired_session(self, *args, **kwargs): try: return func(*args, **kwargs) except AuthExpired: - args[0]._renew_session() + self._renew_session() return func(*args, **kwargs) return _renew_expired_session diff --git a/koji/auth.py b/koji/auth.py index 2189592..ff7d136 100644 --- a/koji/auth.py +++ b/koji/auth.py @@ -436,6 +436,10 @@ class Session(object): 'exclusive = TRUE'], values={'user_id': user_id}, opts={'rowlock': True}) excl_id = query.singleValue(strict=False) + # get lock for our session + query = QueryProcessor(tables=['sessions'], clauses=['id=%(sesions_id)s'], + values={'session_id': session_id}, opts={'rowlock': True}) + query.execute() if excl_id: if force: # close the previous exclusive sessions and try again From ffe4e57d7e5c45bcb92740c4c2387b602caeeb31 Mon Sep 17 00:00:00 2001 From: Tomas Kopecek Date: Jan 25 2023 14:24:19 +0000 Subject: [PATCH 12/16] remove redundant lock --- diff --git a/koji/auth.py b/koji/auth.py index ff7d136..2189592 100644 --- a/koji/auth.py +++ b/koji/auth.py @@ -436,10 +436,6 @@ class Session(object): 'exclusive = TRUE'], values={'user_id': user_id}, opts={'rowlock': True}) excl_id = query.singleValue(strict=False) - # get lock for our session - query = QueryProcessor(tables=['sessions'], clauses=['id=%(sesions_id)s'], - values={'session_id': session_id}, opts={'rowlock': True}) - query.execute() if excl_id: if force: # close the previous exclusive sessions and try again From 2b8b79bc6f269346d138dd2fa22151c5879e1233 Mon Sep 17 00:00:00 2001 From: Tomas Kopecek Date: Jan 25 2023 14:24:19 +0000 Subject: [PATCH 13/16] fix decorator --- diff --git a/koji/__init__.py b/koji/__init__.py index 912b395..5dbd853 100644 --- a/koji/__init__.py +++ b/koji/__init__.py @@ -2916,10 +2916,10 @@ class ClientSession(object): """Decorator to renew expirated session or subsession.""" def _renew_expired_session(self, *args, **kwargs): try: - return func(*args, **kwargs) + return func(self, *args, **kwargs) except AuthExpired: self._renew_session() - return func(*args, **kwargs) + return func(self, *args, **kwargs) return _renew_expired_session @renew_expired_session From 8eff52ce68cc63f1bde5ccc168a75db5a92087c5 Mon Sep 17 00:00:00 2001 From: Tomas Kopecek Date: Jan 25 2023 14:24:19 +0000 Subject: [PATCH 14/16] fix tests --- diff --git a/tests/test_lib/test_auth.py b/tests/test_lib/test_auth.py index 64a9b7a..cb688e5 100644 --- a/tests/test_lib/test_auth.py +++ b/tests/test_lib/test_auth.py @@ -140,7 +140,8 @@ class TestAuthSession(unittest.TestCase): query = self.queries[0] self.assertEqual(query.tables, ['sessions']) self.assertEqual(query.joins, None) - self.assertEqual(query.clauses, ['hostip = %(hostip)s', 'id = %(id)i', 'key = %(key)s']) + self.assertEqual(query.clauses, ['closed IS FALSE', 'hostip = %(hostip)s', 'id = %(id)i', + 'key = %(key)s']) self.assertEqual(query.columns, ['authtype', 'callnum', 'exclusive', 'expired', 'master', 'start_time', "date_part('epoch', start_time)", 'update_time', "date_part('epoch', update_time)", @@ -160,7 +161,7 @@ class TestAuthSession(unittest.TestCase): query = self.queries[2] self.assertEqual(query.tables, ['sessions']) self.assertEqual(query.joins, None) - self.assertEqual(query.clauses, ['exclusive = TRUE', 'expired = FALSE', + self.assertEqual(query.clauses, ['closed = FALSE', 'exclusive = TRUE', 'user_id=%(user_id)s']) self.assertEqual(query.columns, ['id']) @@ -190,7 +191,8 @@ class TestAuthSession(unittest.TestCase): query = self.queries[0] self.assertEqual(query.tables, ['sessions']) self.assertEqual(query.joins, None) - self.assertEqual(query.clauses, ['hostip = %(hostip)s', 'id = %(id)i', 'key = %(key)s']) + self.assertEqual(query.clauses, ['closed IS FALSE', 'hostip = %(hostip)s', 'id = %(id)i', + 'key = %(key)s']) self.assertEqual(query.columns, ['authtype', 'callnum', 'exclusive', 'expired', 'master', 'start_time', "date_part('epoch', start_time)", 'update_time', "date_part('epoch', update_time)", @@ -210,7 +212,7 @@ class TestAuthSession(unittest.TestCase): query = self.queries[2] self.assertEqual(query.tables, ['sessions']) self.assertEqual(query.joins, None) - self.assertEqual(query.clauses, ['exclusive = TRUE', 'expired = FALSE', + self.assertEqual(query.clauses, ['closed = FALSE', 'exclusive = TRUE', 'user_id=%(user_id)s']) self.assertEqual(query.columns, ['id']) @@ -460,7 +462,7 @@ class TestAuthSession(unittest.TestCase): self.assertEqual(update.table, 'sessions') self.assertEqual(update.values, {'session_id': 111, 'master': 123}) self.assertEqual(update.clauses, ['id = %(session_id)i', 'master = %(master)i']) - self.assertEqual(update.data, {'expired': True, 'exclusive': None}) + self.assertEqual(update.data, {'expired': True, 'exclusive': None, 'closed': True}) self.assertEqual(update.rawdata, {}) def test_makeExclusive_not_master(self): @@ -513,7 +515,7 @@ class TestAuthSession(unittest.TestCase): query = self.queries[4] self.assertEqual(query.tables, ['sessions']) self.assertEqual(query.joins, None) - self.assertEqual(query.clauses, ['exclusive = TRUE', 'expired = FALSE', + self.assertEqual(query.clauses, ['closed = FALSE', 'exclusive = TRUE', 'user_id=%(user_id)s']) self.assertEqual(query.columns, ['id']) self.assertEqual(query.values, {'user_id': 1}) @@ -525,7 +527,7 @@ class TestAuthSession(unittest.TestCase): self.assertEqual(update.table, 'sessions') self.assertEqual(update.values, {'excl_id': 123}) self.assertEqual(update.clauses, ['id=%(excl_id)s']) - self.assertEqual(update.data, {'expired': True, 'exclusive': None}) + self.assertEqual(update.data, {'expired': True, 'exclusive': None, 'closed': True}) self.assertEqual(update.rawdata, {}) update = self.updates[3] diff --git a/tests/test_lib/test_gssapi.py b/tests/test_lib/test_gssapi.py index 1c7d918..6249221 100644 --- a/tests/test_lib/test_gssapi.py +++ b/tests/test_lib/test_gssapi.py @@ -27,7 +27,7 @@ class TestGSSAPI(unittest.TestCase): old_environ = dict(**os.environ) self.session.gssapi_login() self.session._callMethod.assert_called_with( - 'sslLogin', [], {'proxyuser': None, 'session_key': None}, retry=False) + 'sslLogin', [], {'proxyuser': None}, retry=False) self.assertEqual(old_environ, dict(**os.environ)) @mock.patch('koji.reqgssapi.HTTPKerberosAuth') @@ -47,7 +47,7 @@ class TestGSSAPI(unittest.TestCase): koji.reqgssapi.__version__ = accepted_version rv = self.session.gssapi_login(principal, keytab, ccache) self.session._callMethod.assert_called_with( - 'sslLogin', [], {'proxyuser': None, 'session_key': None}, retry=False) + 'sslLogin', [], {'proxyuser': None}, retry=False) self.assertEqual(old_environ, dict(**os.environ)) self.assertTrue(rv) self.session._callMethod.reset_mock() @@ -84,7 +84,7 @@ class TestGSSAPI(unittest.TestCase): with self.assertRaises(koji.GSSAPIAuthError): self.session.gssapi_login() self.session._callMethod.assert_called_with( - 'sslLogin', [], {'proxyuser': None, 'session_key': None}, retry=False) + 'sslLogin', [], {'proxyuser': None}, retry=False) self.assertEqual(old_environ, dict(**os.environ)) def test_gssapi_login_http(self): From 42c80e2167a909afb1fafca60915f3cac544d1d0 Mon Sep 17 00:00:00 2001 From: Tomas Kopecek Date: Jan 25 2023 14:24:19 +0000 Subject: [PATCH 15/16] remove f-strings for py2 compatibility --- diff --git a/koji/auth.py b/koji/auth.py index 2189592..d3406a3 100644 --- a/koji/auth.py +++ b/koji/auth.py @@ -140,18 +140,18 @@ class Session(object): # check for expiration if session_data['expired']: if getattr(context, 'method') not in AUTH_METHODS: - raise koji.AuthExpired(f'session "{self.id}" has expired') + raise koji.AuthExpired('session "%s" has expired' % self.id) # check for callnum sanity if callnum is not None: try: callnum = int(callnum) except (ValueError, TypeError): - raise koji.AuthError(f"Invalid callnum: {callnum!r}") + raise koji.AuthError("Invalid callnum: %r" % callnum) lastcall = session_data['callnum'] if lastcall is not None: if lastcall > callnum: - raise koji.SequenceError(f"{lastcall} > {callnum} (session {self.id})") + raise koji.SequenceError("%s > %s (session %s)" % (lastcall, callnum, self.id)) elif lastcall == callnum: # Some explanation: # This function is one of the few that performs its own commit. @@ -164,8 +164,8 @@ class Session(object): method = getattr(context, 'method', 'UNKNOWN') if method not in RetryWhitelist: raise koji.RetryError( - f"unable to retry call {callnum} " - f"(method {method}) for session {self.id}") + "unable to retry call %s (method %s) for session %s" % + (callnum, method, self.id)) if session_data['expired']: return From 40780155a228326c9b950b71a1509003b56bead8 Mon Sep 17 00:00:00 2001 From: Tomas Kopecek Date: Jan 25 2023 14:24:19 +0000 Subject: [PATCH 16/16] remove staticmethod due to py2.7 compatibility --- diff --git a/koji/__init__.py b/koji/__init__.py index 5dbd853..70354b4 100644 --- a/koji/__init__.py +++ b/koji/__init__.py @@ -2911,7 +2911,6 @@ class ClientSession(object): self.logged_in = False auth_method(*args, **kwargs) - @staticmethod def renew_expired_session(func): """Decorator to renew expirated session or subsession.""" def _renew_expired_session(self, *args, **kwargs):