Module auth_session_timeout: Pluggability by jmorgannz · Pull Request #887 · OCA/server-tools

jmorgannz · 2017-07-05T15:25:01Z

Refactor to allow other modules to inherit and augment or override the following:
- Session expiry time (deadline) calculation
- Ignored URLs
- Final session expiry (with possibility to late-abort)
Re-ordered functionality to remove unnecessary work, as this code is called very often.
Do not expire a session if delay gets set to zero (or unset / false)

--------------------------- * Refactor to allow other modules to inherit and augment or override the following: ** Session expiry time (deadline) calculation ** Ignored URLs ** Final session expiry (with possibility to late-abort) * Re-ordered functionality to remove unnecessary work, as this code is called very often. * Do not expire a session if delay gets set to zero (or unset / false)

oca-clabot · 2017-07-05T15:25:04Z

Hey @jmorgannz, thank you for your Pull Request.

It looks like some users haven't signed our Contributor License Agreement, yet.
You can read and sign our full Contributor License Agreement here: http://odoo-community.org/page/website.cla
Here is a list of the users:

@jmorgannz (login unknown in OCA database)

Appreciation of efforts,
OCA CLAbot

jmorgannz · 2017-07-05T15:26:59Z

I'm new at this (contributing); guess I might need to read up on the CLA.
The reason I did this is that I needed to add the ability to exempt certain users from the expiry functionality.

I didn't want to re-implement the entire thing, but the OCA copy was not easily pluggable.
I did this, then made my own module that adds the exempt-by-user functionality.

I am using this in production.

lasley

Thanks for the contribution @jmorgannz!

Information on the CLA is here. Let us know if you have questions.

I added some comments inline after reviewing. Also, it would be nice to see some tests for your added logic - we're dropping in coverage here.

lasley · 2017-07-05T19:25:46Z

@@ -1,5 +1,5 @@
 # -*- coding: utf-8 -*-
-# (c) 2015 ACSONE SA/NV, Dhinesh D
+# (c) 2015 ACSONE SA/NV, Dhinesh D, Jesse Morgan


Please do not append to others' copyrights. Also, you did not change this file at all, so your claim to copyright on any aspect of it would be invalid.

Regarding the copyright headers & what happened here - we were significantly more lax on what happened with these headers previously. If you look at the history of this file, you'll see that Dhinesh D's copyright is actually invalid here too & I would argue it is invalid for the entire module. That argument isn't something to make for history though, but it's most certainly something to enforce going forwards.

Typically if you're going to claim copyright on a file in the header, you need to have written the whole thing. Otherwise, Git line-by-line attribution is an implicit copyright on things you do actually change.

Thanks for the heads-up.
I did in fact only change the copyright headers on the files I modified to begin with, but when I was 'polishing it up' to submit as a pull request I went back and added to the other files.

I guess the thought was that the enter module would be subject to copyright rather than each file within it.

No matter, I will change it :)

lasley · 2017-07-05T19:27:56Z


    @tools.ormcache(skiparg=0)
-    def get_session_parameters(self, db):
+    def _auth_timeout_get_parameter_delay(self, db):


This is changing the public external facing interface of a module that is already in production, and therefore is against a stable release policy.

Is the rename of this method actually necessary? If so, we'll need to provide a get_session_parameters method to use as an adaptation layer for old code.

I was trying to be granular here.
With the new structure in ResUser each param is required in different places, so I split get_session_parameters in two.

Alternatively I could have just called get_session_parameters in all the now required places, using the parameter returned that I need and discarding the other; however I was feeling the need to be frugal given that from what I can see these methods are running in a fairly hot code-path.

The other thing that occurs to me now is that this code now opens more cursors than before.
Technically this new code would open and close a cursor for each parameter read, whereas they were both read in a single cursor before.

Am I barking up the wrong tree here?
Should I be being less frugal and instead trust the ORM layer to mitigate any repeat work ?

I 100% agree with your design choice. The solution here is to basically just create a get_session_parameters method that returns the output of the two new methods in a compatible tuple

EDIT: I'll comment inline about the cursors

lasley · 2017-07-05T19:30:52Z

        finally:
            cr.close()
-        return delay, urls
+        return urls


This is changing the external facing API. The adapter method would need to return compatibly

lasley · 2017-07-05T19:31:07Z

-            self.get_session_parameters.clear_cache(self)
+        if self.key == DELAY_KEY:
+            self._auth_timeout_get_parameter_delay.clear_cache(self)
+        if self.key == IGNORED_PATH_KEY:


lasley · 2017-07-05T19:32:29Z

    _inherit = 'res.users'

-    def _check_session_validity(self, db, uid, passwd):
+    def _auth_timeout_urls_get(self, session, db, uid):


It could be argued that you are also changing the interfaces here, but they are private so you're somewhat in the clear. I would still recommend an adapter method though- you don't want to be the reason that someone's code breaks in production.

lasley · 2017-07-05T19:33:22Z


-    def _check_session_validity(self, db, uid, passwd):
+    def _auth_timeout_urls_get(self, session, db, uid):
+        # Pluggable method for calculating ignored urls


Check out PEP-257, which describes Pythonic docblocks. This is more akin to a C docblock

lasley · 2017-07-05T19:33:51Z

+    def _auth_timeout_urls_get(self, session, db, uid):
+        # Pluggable method for calculating ignored urls
+        # Defaults to stored config param
+        param_obj = self.pool['ir.config_parameter']


self.env - Pool is old API. Please don't use

FYI, self.env is not defined in these methods.
Was able to use request.env, but I'd rather find out why self.env isn't working.

lasley · 2017-07-05T19:34:23Z

+    def _auth_timeout_deadline_calculate(self, session, db, uid):
+        # Pluggable method for calculating timeout deadline
+        # Defaults to current time minus delay using delay stored as session param
+        param_obj = self.pool['ir.config_parameter']


lasley · 2017-07-05T19:35:21Z

+            try:
+                expired = getmtime(path) < deadline
+            except OSError:
+                pass


Never pass silently. Please add a _logger.debug here at minimum

lasley · 2017-07-05T19:36:04Z

-        except OSError:
-            pass
+            except OSError:
+                pass


I know it's not your code, but you're already here & changing things. Never pass silently - _logger.debug

jmorgannz · 2017-07-06T11:02:35Z

@lasley Thanks for the input. Will investigate changes required!

lasley · 2017-07-07T01:15:25Z

-    def get_session_parameters(self, db):
+    def _auth_timeout_get_parameter_delay(self, db):
        param_model = self.pool['ir.config_parameter']
        cr = self.pool.cursor()


Sounds like you're going for a real fix, I ❤️ it and thank you! Let's do like this in both of the cursor areas:

with openerp.registry(self.env.cr.dbname).cursor() as cr: ...operations...

This will kill the try/finally.

Now, I do have a question regarding this & forgive if it's basic - I don't know this module. Why are we opening new cursors in these spots?

I wasn't sure why this was done originally, so followed the pattern in case it had good reason.

This is all complete conjecture, but:

Seems to me that the ResUser.check method could quite likely be called on requests that are at a stage that have not had their proper environment/context set up yet as part of request buildup/teardown.

I.e. Odoo should check a users validation credentials before opening them an active db cursor, etc.

Since this module hangs off ResUser.check, we get caught in that scenario.

The only scenario we are actually interested in though is (re)checks - we don't need to check for session expiry on the request that is logging a user into the system (but we do)

It would not surprise me if the manual cursor control was added as a workaround by the original developer(s) as they quickly realized that without them users could no longer log in!

I have taken the manual cursors out for now, and everything still appears to work correctly.

In order to use a manual cursor with the newer API, I will have to create a new env instance, from what I can see.

jmorgannz · 2017-07-08T03:50:30Z

Man, minimum line width of 79 in conjunction with strict line-continuation indent rules presents an interesting challenge.

jmorgannz · 2017-07-08T04:22:04Z

+        """
+        param_model = self.env['ir.config_parameter']
+        delay = int(param_model.sudo().get_param(DELAY_KEY, 7200))
+        urls = param_model.sudo().get_param(IGNORED_PATH_KEY, '').split(',')


Method no longer using the passed in db for database access

jmorgannz · 2017-07-09T18:57:03Z

+    @tools.ormcache('db')
    def get_session_parameters(self, db):
        param_model = self.pool['ir.config_parameter']
        cr = self.pool.cursor()
