[MIG] auth_session_timeout: Migration to 10.0 by chienandalu · Pull Request #875 · OCA/server-tools

chienandalu · 2017-06-26T13:13:14Z

Auth Session Timeout

This module was written to be able to kill (logout) all inactive sessions since
a given delay. On each request the server checks if the session is yet valid
regarding the expiration delay. If not a clean logout is operated.

cc @Tecnativa

…r informations in README.rst - Import from same package on same line - Use CamelCase for class names

yajo

Code needs a small refactor to make it more maintaiable 😉

yajo · 2017-06-27T07:09:46Z

+    _inherit = 'ir.config_parameter'
+
+    @api.model
+    @tools.ormcache(skiparg=0)


Don't use skiparg, it's kept for backwards compatibility, but the new and preferred way of caching is by argument names.

In any case, IMHO this whole file is not needed. You should just use Odoo's native get_param method, which already has caching enabled.

yajo · 2017-06-27T07:12:53Z

+        with registry(db).cursor() as cr:
+            env = Environment(cr, SUPERUSER_ID, {})
+            param_obj = env['ir.config_parameter']
+            delay, urls = param_obj.get_session_parameters(db)


Again, you'll have to change this for 2 calls to ir.config_parameter.get_param if you delete ir_config_parameter.py file as suggested above.

yajo · 2017-06-27T07:15:49Z

+    @classmethod
+    def check(cls, db, uid, passwd):
+        res = super(ResUsers, cls).check(db, uid, passwd)
+        cls._check_session_validity(db, uid, passwd)


Why not just move all the code from _check_session_validity into this method? After all this one does nothing more...

Or even better, why not move these checks into check_credentials? That one is a normal method (not a @classmethod) and has a working self.env to work with.

chienandalu · 2017-06-27T08:54:44Z

@yajo Look at it now. Some notes:

Seems that _check_session_validity is not going to be called allways. So I took a halfway solution.
When the session expires, an exception raises. Do you think is convenient to catch it and explain to the user to reload browser and login again?
Besides no update is set in default data, every time I update the module the parameters are reset.

pedrobaeza · 2017-06-28T17:38:08Z

+    'author': "ACSONE SA/NV, "
+              "Tecnativa, "
+              "Odoo Community Association (OCA)",
+    'website': 'https://www.tecnativa.com',


Let original acsone URL.

pedrobaeza · 2017-06-28T17:38:57Z

+<?xml version='1.0' encoding='UTF-8' ?>
+<odoo>
+
+    <data noupdate="1">


Why 2 data noupdate? Put all in the first tag (<odoo noupdate="1">)

pedrobaeza · 2017-06-28T17:44:46Z

+            return
+        session = request.session
+        session_store = root.session_store
+        ipm = self.env['ir.config_parameter']


You have removed the other model and it was introduced for having a cache mechanism to not query so many times the DB, so please restore it as it was.

It was @yajo 's request, given that get_param has already a cached mechanism at its core: https://github.com/odoo/odoo/blob/10.0/odoo/addons/base/ir/ir_config_parameter.py#L53-L68

OK, I see. Then it's enough indeed. Maybe it would be good to throw a comment saying that this is cached, so no performance penalty...

pedrobaeza · 2017-06-28T17:53:31Z

+            if getmtime(path) < deadline:
+                if session.db and session.uid:
+                    session.logout(keep_db=True)
+            elif http.request.httprequest.path not in urls:


This is totally wrong (not your fault, as it was the same on v8), but the intended behavior for this is not achieved here:

This should be checked before doing anything. See that if the request has been inactive, you'll get logout anyway.

What you have to check is if one of the ignored path keys (variable urls) is not included in current http request, so the code should be something similar to:
if any([url in http.request.httprequest.path for x in urls]): return

chienandalu · 2017-06-29T09:58:47Z

@pedrobaeza Changes done

yajo · 2017-06-29T12:31:17Z

+        deadline = time() - delay
+        path = session_store.get_session_filename(session.sid)
+        try:
+            if any([url in http.request.httprequest.path for url in urls]):


Sorry, this is incorrect for 2 reasons:

Performance-wise, doing any([... for ...]) makes the generator be converted in a list and then iterated again under the any. You can save several iterations by just dropping the [] characters. Not critical here since the list will be short, but keep it in mind.

If you have /longpolling,/shop, then a request to /blog/longpolling-is-your-friend will give a false positive.

Instead of this, do:

if any(map(http.request.httprequest.path.startswith, urls)):

chienandalu · 2017-06-29T13:10:16Z

@yajo @pedrobaeza Changes done

pedrobaeza · 2017-06-30T07:50:37Z

+        self.ipm.set_param(DELAY_KEY, 1)
+        res = self.rus.authenticate(self.db, 'admin', 'admin', {})
+        time.sleep(1.1)
+        res = self.rus.browse(res).check(self.db, res, 'admin')


You are actually not checking here anything, and you also don't have any request. Better to put that this is a TODO and include in "Known issues" to finish tests. Or @yajo, do you have a quick suggestion for testing this?

To have full coverage, you can either mock the session or authenticate through an HttpCase.

Also, after this code (that checks you are OK), I'd try mocking also the method that checks if the session time is old to make it think it is, and then check that you are kicked off.

I've commited a test with HttpCase but I still don't get a request. Can you provide an example to use as a guideline?

Yes, you need to authenticate through an http request to get a working request (redundant, but that's it). Check this out: https://github.com/odoo/odoo/blob/10.0/odoo/tests/common.py#L270

@yajo I still can't see how to do such request 😕

Did you try calling that method?

Yes, it's what I tried in my last commit: https://github.com/Tecnativa/server-tools/blob/c32b5308088e736c2347965276235a6c1878287d/inactive_session_timeout/tests/test_inactive_session.py#L24

yajo · 2017-06-30T11:34:20Z

+        return
+
+    @classmethod
+    def check(cls, db, uid, passwd):


Do not override this method. Override check_credentials. Raise an AccessDenied if the session is expired.

No, the idea is to return to logout, disauthorizing future operations and returning to login

yajo · 2017-06-30T11:37:33Z

+        self.assertTrue(self.ipm.get_param(DELAY_KEY), 1)
+        self.authenticate('admin', 'admin')
+        time.sleep(1.1)
+        res = self.rus.browse(1).check(self.db, 1, 'admin')


Assert that this should raise a AccessDenied exception

pedrobaeza · 2017-07-05T15:29:14Z

I have just found that the module was renamed to auth_session_timeout in v9. We should rename it too.

pedrobaeza · 2017-07-05T15:29:40Z

See also #887

chienandalu · 2017-07-05T19:20:08Z

@pedrobaeza Implemented some of the good ideas of #887
7f38887

lasley · 2017-07-05T19:59:37Z

IMO we should complete #887 before a merge of this one just so we make sure we have a proper migration path

pedrobaeza · 2017-07-05T21:19:10Z

Well, if this one includes the changes of that PR, we assure migration path, isn't it?

lasley · 2017-07-05T21:26:43Z

This is true, however there were some PR review recommendations that I don't think were attended to here. Reviewing and modifying in two PRs seems like duplicating effort IMO (even though I'm totally guilty of submitting multiple active PRs against the same module, so kettle calling the pot black here).

Also, the commit wasn't cherry picked so we've lost attribution + I'm not entirely certain which aspects of #887 were retained because the commit is squashed into another logical one & the comment noted that only some of the good ideas were implemented.

pedrobaeza · 2017-07-05T21:30:27Z

OK, please try to speed up the other PR and we will wait for this.

chienandalu · 2017-07-06T07:17:17Z

Hi, @lasley. Maybe the confusion has been originated because I didn't realize in first place that there was an already migrated version on v9. So now they have diverged quite a bit. From the last PR, I took the idea of split some of functionalities into separate methods and adapted it to my existing code. Although I've got no objection in giving fair attribution. Anyway I'll wait until the PR is fully reviewed 😃 👍

pedrobaeza · 2017-09-06T10:44:44Z

Closing in favor of #580

lasley · 2017-09-06T14:21:58Z

😆 did I seriously review a module that I also had an open PR for? whoops

Syncing from upstream OCA/server-tools (15.0)

oca-transbot and others added 8 commits June 26, 2017 15:11

OCA Transbot updated translations from Transifex

7fff6d3

Add a new module to logout inactive session after a configured delay

7c2ea93

Commit concerns: - Add AGPL-3 badge on README.rst - Insert Bug tracke…

e2d27ea

…r informations in README.rst - Import from same package on same line - Use CamelCase for class names

Add missing default oca icons

97a1a28

OCA Transbot updated translations from Transifex

ecc7f63

prefix versions with 8.0

b4b191e

invalid manifest

e15b6ee

[MIG] inactive_session_timeout: Migration to 10.0

a848e87

chienandalu force-pushed the 10.0-mig-inactive_session_timeout branch from 2d41a9a to a848e87 Compare June 26, 2017 13:15

yajo added the needs review label Jun 27, 2017

yajo added this to the 10.0 milestone Jun 27, 2017

yajo requested changes Jun 27, 2017

View reviewed changes

Rewrite methods

6d99db8

fix flake8

ec188d5

pedrobaeza requested changes Jun 28, 2017

View reviewed changes

Corrections

3cdb809

chienandalu force-pushed the 10.0-mig-inactive_session_timeout branch from b77a623 to 3cdb809 Compare June 29, 2017 09:22

yajo requested changes Jun 29, 2017

View reviewed changes

Improve and fix incorrect timeout

73a7f57

pedrobaeza approved these changes Jun 29, 2017

View reviewed changes

yajo approved these changes Jun 30, 2017

View reviewed changes

pedrobaeza reviewed Jun 30, 2017

View reviewed changes

Rewrite test

c32b530

yajo requested changes Jun 30, 2017

View reviewed changes

Split logic into methods and tests

7f38887

pedrobaeza mentioned this pull request Jul 12, 2017

Migrate module auth_session_timeout from 9.0 to 10.0 #897

Closed

chienandalu mentioned this pull request Jul 21, 2017

Module auth_session_timeout: Pluggability #887

Merged

chienandalu changed the title ~~[MIG] inactive_session_timeout: Migration to 10.0~~ [MIG] auth_session_timeout: Migration to 10.0 Aug 30, 2017

chienandalu added 2 commits August 30, 2017 17:21

fixup! Split logic into methods and tests

6cd9659

fixup! fixup! Split logic into methods and tests

4493419

pedrobaeza mentioned this pull request Sep 6, 2017

[MIG][10.0] auth_session_timeout #580

Merged

pedrobaeza closed this Sep 6, 2017

pedrobaeza deleted the 10.0-mig-inactive_session_timeout branch September 6, 2017 10:45

SiesslPhillip pushed a commit to grueneerde/OCA-server-tools that referenced this pull request Nov 20, 2024

Merge pull request OCA#875 from OCA/15.0

83f0d50

Syncing from upstream OCA/server-tools (15.0)

Uh oh!

Uh oh!

Conversation

chienandalu commented Jun 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Auth Session Timeout

Uh oh!

yajo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chienandalu commented Jun 27, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pedrobaeza Jun 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chienandalu commented Jun 29, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chienandalu commented Jun 29, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yajo Jun 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pedrobaeza commented Jul 5, 2017

Uh oh!

pedrobaeza commented Jul 5, 2017

Uh oh!

chienandalu commented Jul 5, 2017

Uh oh!

lasley commented Jul 5, 2017

Uh oh!

pedrobaeza commented Jul 5, 2017

Uh oh!

lasley commented Jul 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pedrobaeza commented Jul 5, 2017

Uh oh!

chienandalu commented Jul 6, 2017

Uh oh!

pedrobaeza commented Sep 6, 2017

Uh oh!

lasley commented Sep 6, 2017

chienandalu commented Jun 26, 2017 •

edited

Loading

pedrobaeza Jun 28, 2017 •

edited

Loading

yajo Jun 30, 2017 •

edited

Loading

lasley commented Jul 5, 2017 •

edited

Loading