From 00e79300e30a01a7bd9c1a9bc34f825a1821fe5f Mon Sep 17 00:00:00 2001 From: Dave Lasley Date: Mon, 6 Nov 2017 08:39:31 -0800 Subject: [PATCH] [IMP] auth_session_timeout: Deprecate backwards compat + improve * Deprecate backwards compatibility methods that were retained during v9 rework * Upgrade API and rename a few things for PEP-8 * Switch to HttpCase for tests * Switch to isolated build --- .travis.yml | 6 +- .../models/ir_config_parameter.py | 51 ++++++------- auth_session_timeout/models/res_users.py | 71 +++++++++--------- .../tests/test_ir_config_parameter.py | 37 ++++++---- auth_session_timeout/tests/test_res_users.py | 73 ++++++------------- base_external_system/tests/common.py | 4 +- 6 files changed, 108 insertions(+), 134 deletions(-) diff --git a/.travis.yml b/.travis.yml index ce704dbad..19a5a9f9e 100644 --- a/.travis.yml +++ b/.travis.yml @@ -24,10 +24,12 @@ env: matrix: - LINT_CHECK="1" - TRANSIFEX="1" - - TESTS="1" ODOO_REPO="odoo/odoo" EXCLUDE="database_cleanup" - - TESTS="1" ODOO_REPO="OCA/OCB" EXCLUDE="database_cleanup" + - TESTS="1" ODOO_REPO="odoo/odoo" EXCLUDE="database_cleanup,auth_session_timeout" + - TESTS="1" ODOO_REPO="OCA/OCB" EXCLUDE="database_cleanup,auth_session_timeout" - TESTS="1" ODOO_REPO="odoo/odoo" INCLUDE="database_cleanup" - TESTS="1" ODOO_REPO="OCA/OCB" INCLUDE="database_cleanup" + - TESTS="1" ODOO_REPO="odoo/odoo" INCLUDE="auth_session_timeout" + - TESTS="1" ODOO_REPO="OCA/OCB" INCLUDE="auth_session_timeout" virtualenv: system_site_packages: true diff --git a/auth_session_timeout/models/ir_config_parameter.py b/auth_session_timeout/models/ir_config_parameter.py index bcc8022a3..ea6854942 100644 --- a/auth_session_timeout/models/ir_config_parameter.py +++ b/auth_session_timeout/models/ir_config_parameter.py @@ -1,9 +1,8 @@ # -*- coding: utf-8 -*- # (c) 2015 ACSONE SA/NV, Dhinesh D - # License AGPL-3.0 or later (http://www.gnu.org/licenses/agpl.html). -from odoo import models, api, tools, SUPERUSER_ID +from odoo import api, models, tools DELAY_KEY = 'inactive_session_time_out_delay' IGNORED_PATH_KEY = 'inactive_session_time_out_ignored_url' @@ -12,34 +11,30 @@ IGNORED_PATH_KEY = 'inactive_session_time_out_ignored_url' class IrConfigParameter(models.Model): _inherit = 'ir.config_parameter' - @tools.ormcache('db') - def get_session_parameters(self, db): - param_model = self.pool['ir.config_parameter'] - cr = self.pool.cursor() - delay = False - urls = [] - try: - delay = int(param_model.get_param( - cr, SUPERUSER_ID, DELAY_KEY, 7200)) - urls = param_model.get_param( - cr, SUPERUSER_ID, IGNORED_PATH_KEY, '').split(',') - finally: - cr.close() - return delay, urls - + @api.model + @tools.ormcache('self.env.cr.dbname') def _auth_timeout_get_parameter_delay(self): - delay, urls = self.get_session_parameters(self.pool.db_name) - return delay - - def _auth_timeout_get_parameter_ignoredurls(self): - delay, urls = self.get_session_parameters(self.pool.db_name) - return urls + return int( + self.env['ir.config_parameter'].sudo().get_param( + DELAY_KEY, 7200, + ) + ) + + @api.model + @tools.ormcache('self.env.cr.dbname') + def _auth_timeout_get_parameter_ignored_urls(self): + urls = self.env['ir.config_parameter'].sudo().get_param( + IGNORED_PATH_KEY, '', + ) + return urls.split(',') @api.multi - def write(self, vals, context=None): + def write(self, vals): res = super(IrConfigParameter, self).write(vals) - if self.key == DELAY_KEY: - self.get_session_parameters.clear_cache(self) - elif self.key == IGNORED_PATH_KEY: - self.get_session_parameters.clear_cache(self) + self._auth_timeout_get_parameter_delay.clear_cache( + self.filtered(lambda r: r.key == DELAY_KEY), + ) + self._auth_timeout_get_parameter_ignored_urls.clear_cache( + self.filtered(lambda r: r.key == IGNORED_PATH_KEY), + ) return res diff --git a/auth_session_timeout/models/res_users.py b/auth_session_timeout/models/res_users.py index c916a6d1f..00b0833cc 100644 --- a/auth_session_timeout/models/res_users.py +++ b/auth_session_timeout/models/res_users.py @@ -5,14 +5,11 @@ import logging -from odoo import models - -from odoo.http import root -from odoo.http import request - -from os import utime from os.path import getmtime from time import time +from os import utime + +from odoo import api, http, models _logger = logging.getLogger(__name__) @@ -20,23 +17,27 @@ _logger = logging.getLogger(__name__) class ResUsers(models.Model): _inherit = 'res.users' - def _auth_timeout_ignoredurls_get(self): + @api.model_cr_context + def _auth_timeout_get_ignored_urls(self): """Pluggable method for calculating ignored urls Defaults to stored config param """ - param_model = self.pool['ir.config_parameter'] - return param_model._auth_timeout_get_parameter_ignoredurls() + params = self.env['ir.config_parameter'] + return params._auth_timeout_get_parameter_ignored_urls() + @api.model_cr_context def _auth_timeout_deadline_calculate(self): """Pluggable method for calculating timeout deadline - Defaults to current time minus delay using delay stored as config param + Defaults to current time minus delay using delay stored as config + param. """ - param_model = self.pool['ir.config_parameter'] - delay = param_model._auth_timeout_get_parameter_delay() - if delay is False or delay <= 0: + params = self.env['ir.config_parameter'] + delay = params._auth_timeout_get_parameter_delay() + if delay <= 0: return False return time() - delay + @api.model_cr_context def _auth_timeout_session_terminate(self, session): """Pluggable method for terminating a timed-out session @@ -52,11 +53,14 @@ class ResUsers(models.Model): session.logout(keep_db=True) return True + @api.model_cr_context def _auth_timeout_check(self): - if not request: + """Perform session timeout validation and expire if needed.""" + + if not http.request: return - session = request.session + session = http.request.session # Calculate deadline deadline = self._auth_timeout_deadline_calculate() @@ -64,15 +68,15 @@ class ResUsers(models.Model): # Check if past deadline expired = False if deadline is not False: - path = root.session_store.get_session_filename(session.sid) + path = http.root.session_store.get_session_filename(session.sid) try: expired = getmtime(path) < deadline except OSError as e: - _logger.warning( - 'Exception reading session file modified time: %s' - % e + _logger.exception( + 'Exception reading session file modified time.', ) - pass + # Force expire the session. Will be resolved with new session. + expired = True # Try to terminate the session terminated = False @@ -84,27 +88,22 @@ class ResUsers(models.Model): return # Else, conditionally update session modified and access times - ignoredurls = self._auth_timeout_ignoredurls_get() + ignored_urls = self._auth_timeout_get_ignored_urls() - if request.httprequest.path not in ignoredurls: + if http.request.http.request.path not in ignored_urls: if 'path' not in locals(): - path = root.session_store.get_session_filename(session.sid) + path = http.root.session_store.get_session_filename( + session.sid, + ) try: utime(path, None) except OSError as e: - _logger.warning( - 'Exception updating session file access/modified times: %s' - % e + _logger.exception( + 'Exception updating session file access/modified times.', ) - pass - - return - - def _check_session_validity(self, db, uid, passwd): - """Adaptor method for backward compatibility""" - return self._auth_timeout_check() - def check(self, db, uid, passwd): - res = super(ResUsers, self).check(db, uid, passwd) - self._check_session_validity(db, uid, passwd) + @classmethod + def check(cls, *args, **kwargs): + res = super(ResUsers, cls).check(*args, **kwargs) + http.request.env.user._auth_timeout_check() return res diff --git a/auth_session_timeout/tests/test_ir_config_parameter.py b/auth_session_timeout/tests/test_ir_config_parameter.py index 2b15d2b0d..e10e938d8 100644 --- a/auth_session_timeout/tests/test_ir_config_parameter.py +++ b/auth_session_timeout/tests/test_ir_config_parameter.py @@ -16,19 +16,13 @@ class TestIrConfigParameter(common.TransactionCase): self.delay = self.env.ref( 'auth_session_timeout.inactive_session_time_out_delay') - def test_check_session_params(self): - delay, urls = self.param_obj.get_session_parameters(self.db) - self.assertEqual(delay, int(self.delay.value)) - self.assertIsInstance(delay, int) - self.assertIsInstance(urls, list) - def test_check_session_param_delay(self): delay = self.param_obj._auth_timeout_get_parameter_delay() self.assertEqual(delay, int(self.delay.value)) self.assertIsInstance(delay, int) def test_check_session_param_urls(self): - urls = self.param_obj._auth_timeout_get_parameter_ignoredurls() + urls = self.param_obj._auth_timeout_get_parameter_ignored_urls() self.assertIsInstance(urls, list) @@ -43,7 +37,8 @@ class TestIrConfigParameterCaching(common.TransactionCase): def get_param(*args, **kwargs): test.get_param_called = True - return orig_get_param(args[3], args[4]) + return orig_get_param(*args[1:], **kwargs) + orig_get_param = self.param_obj.get_param self.param_obj._patch_method( 'get_param', @@ -53,19 +48,29 @@ class TestIrConfigParameterCaching(common.TransactionCase): super(TestIrConfigParameterCaching, self).tearDown() self.param_obj._revert_method('get_param') - def test_check_param_cache_working(self): + def test_auth_timeout_get_parameter_delay_cache(self): + """It should cache the parameter call.""" self.get_param_called = False - delay, urls = self.param_obj.get_session_parameters(self.db) + self.param_obj._auth_timeout_get_parameter_delay() self.assertTrue(self.get_param_called) - self.get_param_called = False - delay, urls = self.param_obj.get_session_parameters(self.db) - self.assertFalse(self.get_param_called) - def test_check_param_writes_clear_cache(self): + def test_auth_timeout_get_parameter_ignored_urls_cache(self): + """It should cache the parameter call.""" self.get_param_called = False - delay, urls = self.param_obj.get_session_parameters(self.db) + self.param_obj._auth_timeout_get_parameter_ignored_urls() self.assertTrue(self.get_param_called) + + def test_check_param_writes_clear_delay_cache(self): + self.param_obj._auth_timeout_get_parameter_delay() self.get_param_called = False self.param_obj.set_param('inactive_session_time_out_delay', 7201) - delay, urls = self.param_obj.get_session_parameters(self.db) + self.param_obj._auth_timeout_get_parameter_delay() + self.assertTrue(self.get_param_called) + + def test_check_param_writes_clear_ignore_url_cache(self): + self.param_obj._auth_timeout_get_parameter_ignored_urls() + self.get_param_called = False + self.param_obj.set_param('inactive_session_time_out_ignored_url', + 'example.com') + self.param_obj._auth_timeout_get_parameter_ignored_urls() self.assertTrue(self.get_param_called) diff --git a/auth_session_timeout/tests/test_res_users.py b/auth_session_timeout/tests/test_res_users.py index f824dcdce..ad601202e 100644 --- a/auth_session_timeout/tests/test_res_users.py +++ b/auth_session_timeout/tests/test_res_users.py @@ -1,17 +1,14 @@ # -*- coding: utf-8 -*- -# Copyright 2016 LasLabs Inc. +# Copyright 2016-2017 LasLabs Inc. # License AGPL-3.0 or later (http://www.gnu.org/licenses/agpl.html). import mock -from os import strerror -from errno import ENOENT from contextlib import contextmanager +from odoo.tools.misc import mute_logger from odoo.tests.common import TransactionCase -_package_path = 'odoo.addons.auth_session_timeout' - class EndTestException(Exception): """ It stops tests from continuing """ @@ -38,31 +35,23 @@ class TestResUsers(TransactionCase): ) as mocks: yield mocks - def _check_session_validity(self): - """ It wraps ``_check_session_validity`` for easier calling """ + def _auth_timeout_check(self, http_mock): + """ It wraps ``_auth_timeout_check`` for easier calling """ self.db = mock.MagicMock() self.uid = mock.MagicMock() self.passwd = mock.MagicMock() - return self.ResUsers._check_session_validity( - self.db, self.uid, self.passwd, - ) + self.path = '/this/is/a/test/path' + get_filename = http_mock.root.session_store.get_session_filename + get_filename.return_value = self.path + return self.ResUsers._auth_timeout_check() def test_session_validity_no_request(self): """ It should return immediately if no request """ with self._mock_assets() as assets: assets['http'].request = False - res = self._check_session_validity() + res = self._auth_timeout_check(assets['http']) self.assertFalse(res) - def test_session_validity_gets_params(self): - """ It should call ``get_session_parameters`` with db """ - with self._mock_assets() as assets: - get_params = assets['http'].request.env[''].get_session_parameters - get_params.side_effect = EndTestException - with self.assertRaises(EndTestException): - self._check_session_validity() - get_params.assert_called_once_with() - def test_session_validity_gets_session_file(self): """ It should call get the session file for the session id """ with self._mock_assets() as assets: @@ -71,7 +60,7 @@ class TestResUsers(TransactionCase): store = assets['http'].root.session_store store.get_session_filename.side_effect = EndTestException with self.assertRaises(EndTestException): - self._check_session_validity() + self._auth_timeout_check(assets['http']) store.get_session_filename.assert_called_once_with( assets['http'].request.session.sid, ) @@ -82,7 +71,7 @@ class TestResUsers(TransactionCase): get_params = assets['http'].request.env[''].get_session_parameters get_params.return_value = -9999, [] assets['getmtime'].return_value = 0 - self._check_session_validity() + self._auth_timeout_check(assets['http']) assets['http'].request.session.logout.assert_called_once_with( keep_db=True, ) @@ -92,44 +81,28 @@ class TestResUsers(TransactionCase): with self._mock_assets(['http', 'getmtime', 'utime']) as assets: get_params = assets['http'].request.env[''].get_session_parameters get_params.return_value = 9999, [] - self._check_session_validity() + self._auth_timeout_check(assets['http']) assets['utime'].assert_called_once_with( assets['http'].root.session_store.get_session_filename(), None, ) + @mute_logger('odoo.addons.auth_session_timeout.models.res_users') def test_session_validity_os_error_guard(self): """ It should properly guard from OSError & return """ with self._mock_assets(['http', 'utime', 'getmtime']) as assets: get_params = assets['http'].request.env[''].get_session_parameters get_params.return_value = 0, [] assets['getmtime'].side_effect = OSError - res = self._check_session_validity() + res = self._auth_timeout_check(assets['http']) self.assertFalse(res) - @mock.patch(_package_path + '.models.res_users.request') - @mock.patch(_package_path + '.models.res_users.root') - @mock.patch(_package_path + '.models.res_users.getmtime') - def test_on_timeout_session_loggedout(self, mock_getmtime, - mock_root, mock_request): - mock_getmtime.return_value = 0 - mock_request.session.uid = self.env.uid - mock_request.session.dbname = self.env.cr.dbname - mock_request.session.sid = 123 - mock_request.session.logout = mock.Mock() - self.resUsers._auth_timeout_check() - self.assertTrue(mock_request.session.logout.called) - - @mock.patch(_package_path + '.models.res_users.request') - @mock.patch(_package_path + '.models.res_users.root') - @mock.patch(_package_path + '.models.res_users.getmtime') - @mock.patch(_package_path + '.models.res_users.utime') - def test_sessionfile_io_exceptions_managed(self, mock_utime, mock_getmtime, - mock_root, mock_request): - mock_getmtime.side_effect = OSError( - ENOENT, strerror(ENOENT), 'non-existent-filename') - mock_request.session.uid = self.env.uid - mock_request.session.dbname = self.env.cr.dbname - mock_request.session.sid = 123 - self.resUsers._auth_timeout_check() - + def test_on_timeout_session_loggedout(self): + with self._mock_assets(['http', 'getmtime']) as assets: + assets['getmtime'].return_value = 0 + assets['http'].request.session.uid = self.env.uid + assets['http'].request.session.dbname = self.env.cr.dbname + assets['http'].request.session.sid = 123 + assets['http'].request.session.logout = mock.Mock() + self.ResUsers._auth_timeout_check() + self.assertTrue(assets['http'].request.session.logout.called) diff --git a/base_external_system/tests/common.py b/base_external_system/tests/common.py index 0c212359e..17cfebc6d 100644 --- a/base_external_system/tests/common.py +++ b/base_external_system/tests/common.py @@ -5,10 +5,10 @@ from contextlib import contextmanager from mock import MagicMock -from odoo.tests.common import TransactionCase +from odoo.tests.common import HttpCase -class Common(TransactionCase): +class Common(HttpCase): @contextmanager def _mock_method(self, method_name, method_obj=None):