# HG changeset patch
# User Sylvain Thénault <sylvain.thenault@logilab.fr>
# Date 1465219609 -7200
# Node ID b48020a80dc3666401235319c89ca6df55e3fd29
# Parent  9ea50837bc58525088b86936997f9f28393e1012
Store user groups and properties as session data

* stop retrieving them systematically, only when need,

* reimplement session synchronization hooks with some cleanups along the way,

* cleanup call to set language: not needed from the base request nor from the
  server side, only for the web request (on the server side, language is
  necessary only for notification and such code should set it explicitly).

There is still a XXX remaining about one can only "enter" a connection once and
this is a problem in some cases. IMO, this restriction could be removed.

Closes #13500113.

diff -r 9ea50837bc58 -r b48020a80dc3 cubicweb/devtools/repotest.py
--- a/cubicweb/devtools/repotest.py	Wed Oct 05 10:17:39 2016 +0200
+++ b/cubicweb/devtools/repotest.py	Mon Jun 06 15:26:49 2016 +0200
@@ -252,10 +252,10 @@
         """lightweight session using the current user with hi-jacked groups"""
         # use self.session.user.eid to get correct owned_by relation, unless explicit eid
         with self.session.new_cnx() as cnx:
-            u = self.repo._build_user(cnx, self.session.user.eid)
-            u._groups = set(groups)
-            s = Session(u, self.repo)
-            return s
+            user_eid = self.session.user.eid
+            session = Session(self.repo._build_user(cnx, user_eid), self.repo)
+            session.data['%s-groups' % user_eid] = set(groups)
+            return session
 
     def qexecute(self, rql, args=None, build_descr=True):
         with self.session.new_cnx() as cnx:
diff -r 9ea50837bc58 -r b48020a80dc3 cubicweb/devtools/testlib.py
--- a/cubicweb/devtools/testlib.py	Wed Oct 05 10:17:39 2016 +0200
+++ b/cubicweb/devtools/testlib.py	Mon Jun 06 15:26:49 2016 +0200
@@ -890,8 +890,9 @@
 
     def assertAuthSuccess(self, req, origsession, nbsessions=1):
         session = self.app.get_session(req)
-        cnx = repoapi.Connection(session)
-        req.set_cnx(cnx)
+        cnx = session.new_cnx()
+        with cnx:
+            req.set_cnx(cnx)
         self.assertEqual(len(self.open_sessions), nbsessions, self.open_sessions)
         self.assertEqual(session.login, origsession.login)
         self.assertEqual(session.anonymous_session, False)
diff -r 9ea50837bc58 -r b48020a80dc3 cubicweb/entities/authobjs.py
--- a/cubicweb/entities/authobjs.py	Wed Oct 05 10:17:39 2016 +0200
+++ b/cubicweb/entities/authobjs.py	Mon Jun 06 15:26:49 2016 +0200
@@ -26,6 +26,11 @@
 from cubicweb import Unauthorized
 from cubicweb.entities import AnyEntity, fetch_config
 
+
+def user_session_cache_key(user_eid, data_name):
+    return '{0}-{1}'.format(user_eid, data_name)
+
+
 class CWGroup(AnyEntity):
     __regid__ = 'CWGroup'
     fetch_attrs, cw_fetch_order = fetch_config(['name'])
@@ -54,34 +59,32 @@
     AUTHENTICABLE_STATES = ('activated',)
 
     # low level utilities #####################################################
-    def __init__(self, *args, **kwargs):
-        groups = kwargs.pop('groups', None)
-        properties = kwargs.pop('properties', None)
-        super(CWUser, self).__init__(*args, **kwargs)
-        if groups is not None:
-            self._groups = groups
-        if properties is not None:
-            self._properties = properties
 
     @property
     def groups(self):
+        key = user_session_cache_key(self.eid, 'groups')
         try:
-            return self._groups
-        except AttributeError:
-            self._groups = set(g.name for g in self.in_group)
-            return self._groups
+            return self._cw.data[key]
+        except KeyError:
+            with self._cw.security_enabled(read=False):
+                groups = set(group for group, in self._cw.execute(
+                    'Any GN WHERE U in_group G, G name GN, U eid %(userid)s',
+                    {'userid': self.eid}))
+            self._cw.data[key] = groups
+            return groups
 
     @property
     def properties(self):
+        key = user_session_cache_key(self.eid, 'properties')
         try:
-            return self._properties
-        except AttributeError:
-            self._properties = dict(
-                self._cw.execute(
+            return self._cw.data[key]
+        except KeyError:
+            with self._cw.security_enabled(read=False):
+                properties = dict(self._cw.execute(
                     'Any K, V WHERE P for_user U, U eid %(userid)s, '
-                    'P pkey K, P value V',
-                    {'userid': self.eid}))
-            return self._properties
+                    'P pkey K, P value V', {'userid': self.eid}))
+            self._cw.data[key] = properties
+            return properties
 
     def prefered_language(self, language=None):
         """return language used by this user, if explicitly defined (eg not
diff -r 9ea50837bc58 -r b48020a80dc3 cubicweb/hooks/syncsession.py
--- a/cubicweb/hooks/syncsession.py	Wed Oct 05 10:17:39 2016 +0200
+++ b/cubicweb/hooks/syncsession.py	Mon Jun 06 15:26:49 2016 +0200
@@ -23,6 +23,7 @@
 from cubicweb import UnknownProperty, BadConnectionId, validation_error
 from cubicweb.predicates import is_instance
 from cubicweb.server import hook
+from cubicweb.entities.authobjs import user_session_cache_key
 
 
 def get_user_sessions(repo, ueid):
@@ -31,6 +32,27 @@
             yield session
 
 
+class CachedValueMixin(object):
+    """Mixin class providing methods to retrieve some value, specified through
+    `value_name` attribute, in session data.
+    """
+    value_name = None
+    session = None  # make pylint happy
+
+    @property
+    def cached_value(self):
+        """Return cached value for the user, or None"""
+        key = user_session_cache_key(self.session.user.eid, self.value_name)
+        return self.session.data.get(key, None)
+
+    def update_cached_value(self, value):
+        """Update cached value for the user (modifying the set returned by cached_value may not be
+        necessary depending on session data implementation, e.g. redis)
+        """
+        key = user_session_cache_key(self.session.user.eid, self.value_name)
+        self.session.data[key] = value
+
+
 class SyncSessionHook(hook.Hook):
     __abstract__ = True
     category = 'syncsession'
@@ -38,18 +60,18 @@
 
 # user/groups synchronisation #################################################
 
-class _GroupOperation(hook.Operation):
-    """base class for group operation"""
-    cnxuser = None # make pylint happy
+class _GroupOperation(CachedValueMixin, hook.Operation):
+    """Base class for group operation"""
+    value_name = 'groups'
 
     def __init__(self, cnx, *args, **kwargs):
-        """override to get the group name before actual groups manipulation:
+        """Override to get the group name before actual groups manipulation
 
         we may temporarily loose right access during a commit event, so
         no query should be emitted while comitting
         """
         rql = 'Any N WHERE G eid %(x)s, G name N'
-        result = cnx.execute(rql, {'x': kwargs['geid']}, build_descr=False)
+        result = cnx.execute(rql, {'x': kwargs['group_eid']}, build_descr=False)
         hook.Operation.__init__(self, cnx, *args, **kwargs)
         self.group = result[0][0]
 
@@ -58,25 +80,20 @@
     """Synchronize user when a in_group relation has been deleted"""
 
     def postcommit_event(self):
-        """the observed connections set has been commited"""
-        groups = self.cnxuser.groups
-        try:
-            groups.remove(self.group)
-        except KeyError:
-            self.error('user %s not in group %s',  self.cnxuser, self.group)
+        cached_groups = self.cached_value
+        if cached_groups is not None:
+            cached_groups.remove(self.group)
+            self.update_cached_value(cached_groups)
 
 
 class _AddGroupOp(_GroupOperation):
     """Synchronize user when a in_group relation has been added"""
 
     def postcommit_event(self):
-        """the observed connections set has been commited"""
-        groups = self.cnxuser.groups
-        if self.group in groups:
-            self.warning('user %s already in group %s', self.cnxuser,
-                         self.group)
-        else:
-            groups.add(self.group)
+        cached_groups = self.cached_value
+        if cached_groups is not None:
+            cached_groups.add(self.group)
+            self.update_cached_value(cached_groups)
 
 
 class SyncInGroupHook(SyncSessionHook):
@@ -91,66 +108,81 @@
         else:
             opcls = _AddGroupOp
         for session in get_user_sessions(self._cw.repo, self.eidfrom):
-            opcls(self._cw, cnxuser=session.user, geid=self.eidto)
+            opcls(self._cw, session=session, group_eid=self.eidto)
 
 
-class _DelUserOp(hook.Operation):
-    """close associated user's session when it is deleted"""
-    def __init__(self, cnx, sessionid):
-        self.sessionid = sessionid
-        hook.Operation.__init__(self, cnx)
+class _CloseSessionOp(hook.Operation):
+    """Close user's session when it has been deleted"""
 
     def postcommit_event(self):
         try:
-            self.cnx.repo.close(self.sessionid)
+            # remove cached groups for the user
+            key = user_session_cache_key(self.session.user.eid, 'groups')
+            self.session.data.pop(key, None)
+            self.session.repo.close(self.session.sessionid)
         except BadConnectionId:
             pass  # already closed
 
 
-class CloseDeletedUserSessionsHook(SyncSessionHook):
+class UserDeletedHook(SyncSessionHook):
+    """Watch deletion of user to close its opened session"""
     __regid__ = 'closession'
     __select__ = SyncSessionHook.__select__ & is_instance('CWUser')
     events = ('after_delete_entity',)
 
     def __call__(self):
         for session in get_user_sessions(self._cw.repo, self.entity.eid):
-            _DelUserOp(self._cw, session.sessionid)
+            _CloseSessionOp(self._cw, session=session)
 
 
 # CWProperty hooks #############################################################
 
-class _DelCWPropertyOp(hook.Operation):
-    """a user's custom properties has been deleted"""
-    cwpropdict = key = None # make pylint happy
 
-    def postcommit_event(self):
-        """the observed connections set has been commited"""
-        try:
-            del self.cwpropdict[self.key]
-        except KeyError:
-            self.error('%s has no associated value', self.key)
+class _UserPropertyOperation(CachedValueMixin, hook.Operation):
+    """Base class for property operation"""
+    value_name = 'properties'
+    key = None  # make pylint happy
 
 
-class _ChangeCWPropertyOp(hook.Operation):
-    """a user's custom properties has been added/changed"""
-    cwpropdict = key = value = None # make pylint happy
+class _ChangeUserCWPropertyOp(_UserPropertyOperation):
+    """Synchronize cached user's properties when one has been added/updated"""
+    value = None  # make pylint happy
 
     def postcommit_event(self):
-        """the observed connections set has been commited"""
-        self.cwpropdict[self.key] = self.value
+        cached_props = self.cached_value
+        if cached_props is not None:
+            cached_props[self.key] = self.value
+            self.update_cached_value(cached_props)
 
 
-class _AddCWPropertyOp(hook.Operation):
-    """a user's custom properties has been added/changed"""
-    cwprop = None # make pylint happy
+class _DelUserCWPropertyOp(_UserPropertyOperation):
+    """Synchronize cached user's properties when one has been deleted"""
 
     def postcommit_event(self):
-        """the observed connections set has been commited"""
+        cached_props = self.cached_value
+        if cached_props is not None:
+            cached_props.pop(self.key, None)
+            self.update_cached_value(cached_props)
+
+
+class _ChangeSiteWideCWPropertyOp(hook.Operation):
+    """Synchronize site wide properties when one has been added/updated"""
+    cwprop = None  # make pylint happy
+
+    def postcommit_event(self):
         cwprop = self.cwprop
         if not cwprop.for_user:
             self.cnx.vreg['propertyvalues'][cwprop.pkey] = \
                 self.cnx.vreg.typed_value(cwprop.pkey, cwprop.value)
-        # if for_user is set, update is handled by a ChangeCWPropertyOp operation
+        # if for_user is set, update is handled by a ChangeUserCWPropertyOp operation
+
+
+class _DelSiteWideCWPropertyOp(hook.Operation):
+    """Synchronize site wide properties when one has been deleted"""
+    key = None  # make pylint happy
+
+    def postcommit_event(self):
+        self.cnx.vreg['propertyvalues'].pop(self.key, None)
 
 
 class AddCWPropertyHook(SyncSessionHook):
@@ -169,12 +201,11 @@
             msg = _('unknown property key %s')
             raise validation_error(self.entity, {('pkey', 'subject'): msg}, (key,))
         except ValueError as ex:
-            raise validation_error(self.entity,
-                                  {('value', 'subject'): str(ex)})
-        if not cnx.user.matching_groups('managers'):
+            raise validation_error(self.entity, {('value', 'subject'): str(ex)})
+        if cnx.user.matching_groups('managers'):
+            _ChangeSiteWideCWPropertyOp(cnx, cwprop=self.entity)
+        else:
             cnx.add_relation(self.entity.eid, 'for_user', cnx.user.eid)
-        else:
-            _AddCWPropertyOp(cnx, cwprop=self.entity)
 
 
 class UpdateCWPropertyHook(AddCWPropertyHook):
@@ -198,12 +229,9 @@
             raise validation_error(entity, {('value', 'subject'): str(ex)})
         if entity.for_user:
             for session in get_user_sessions(cnx.repo, entity.for_user[0].eid):
-                _ChangeCWPropertyOp(cnx, cwpropdict=session.user.properties,
-                                    key=key, value=value)
+                _ChangeUserCWPropertyOp(cnx, session=session, key=key, value=value)
         else:
-            # site wide properties
-            _ChangeCWPropertyOp(cnx, cwpropdict=cnx.vreg['propertyvalues'],
-                              key=key, value=value)
+            _ChangeSiteWideCWPropertyOp(cnx, cwprop=self.entity)
 
 
 class DeleteCWPropertyHook(AddCWPropertyHook):
@@ -217,8 +245,7 @@
                 # if for_user was set, delete already handled by hook on for_user deletion
                 break
         else:
-            _DelCWPropertyOp(cnx, cwpropdict=cnx.vreg['propertyvalues'],
-                             key=self.entity.pkey)
+            _DelSiteWideCWPropertyOp(cnx, key=self.entity.pkey)
 
 
 class AddForUserRelationHook(SyncSessionHook):
@@ -237,8 +264,7 @@
             msg = _("site-wide property can't be set for user")
             raise validation_error(eidfrom, {('for_user', 'subject'): msg})
         for session in get_user_sessions(cnx.repo, self.eidto):
-            _ChangeCWPropertyOp(cnx, cwpropdict=session.user.properties,
-                              key=key, value=value)
+            _ChangeUserCWPropertyOp(cnx, session=session, key=key, value=value)
 
 
 class DelForUserRelationHook(AddForUserRelationHook):
@@ -251,4 +277,4 @@
         cnx.transaction_data.setdefault('pendingrelations', []).append(
             (self.eidfrom, self.rtype, self.eidto))
         for session in get_user_sessions(cnx.repo, self.eidto):
-            _DelCWPropertyOp(cnx, cwpropdict=session.user.properties, key=key)
+            _DelUserCWPropertyOp(cnx, session=session, key=key)
diff -r 9ea50837bc58 -r b48020a80dc3 cubicweb/req.py
--- a/cubicweb/req.py	Wed Oct 05 10:17:39 2016 +0200
+++ b/cubicweb/req.py	Mon Jun 06 15:26:49 2016 +0200
@@ -86,14 +86,10 @@
         connection too.
         """
         rset = self.eid_rset(orig_user.eid, 'CWUser')
-        user_cls = self.vreg['etypes'].etype_class('CWUser')
-        user = user_cls(self, rset, row=0, groups=orig_user.groups,
-                        properties=orig_user.properties)
-        user.cw_attr_cache['login'] = orig_user.login # cache login
+        user = self.vreg['etypes'].etype_class('CWUser')(self, rset, row=0)
+        user.cw_attr_cache['login'] = orig_user.login  # cache login
         self.user = user
         self.set_entity_cache(user)
-        self.set_language(user.prefered_language())
-
 
     def set_language(self, lang):
         """install i18n configuration for `lang` translation.
diff -r 9ea50837bc58 -r b48020a80dc3 cubicweb/server/migractions.py
--- a/cubicweb/server/migractions.py	Wed Oct 05 10:17:39 2016 +0200
+++ b/cubicweb/server/migractions.py	Mon Jun 06 15:26:49 2016 +0200
@@ -138,10 +138,12 @@
         while True:
             try:
                 self.cnx = repoapi.connect(self.repo, login, password=pwd)
-                if not 'managers' in self.cnx.user.groups:
-                    print('migration need an account in the managers group')
-                else:
-                    break
+                with self.cnx:  # needed to retrieve user's groups
+                    if 'managers' not in self.cnx.user.groups:
+                        print('migration need an account in the managers group')
+                    else:
+                        break
+                self.cnx._open = None  # XXX needed to reuse it later
             except AuthenticationError:
                 print('wrong user/password')
             except (KeyboardInterrupt, EOFError):
diff -r 9ea50837bc58 -r b48020a80dc3 cubicweb/server/repository.py
--- a/cubicweb/server/repository.py	Wed Oct 05 10:17:39 2016 +0200
+++ b/cubicweb/server/repository.py	Mon Jun 06 15:26:49 2016 +0200
@@ -480,13 +480,7 @@
         st.add_eid_restriction(st.get_variable('X'), 'x', 'Substitute')
         rset = cnx.execute(st.as_string(), {'x': eid})
         assert len(rset) == 1, rset
-        cwuser = rset.get_entity(0, 0)
-        # pylint: disable=W0104
-        # prefetch / cache cwuser's groups and properties. This is especially
-        # useful for internal sessions to avoid security insertions
-        cwuser.groups
-        cwuser.properties
-        return cwuser
+        return rset.get_entity(0, 0)
 
     # public (dbapi) interface ################################################
 
diff -r 9ea50837bc58 -r b48020a80dc3 cubicweb/server/session.py
--- a/cubicweb/server/session.py	Wed Oct 05 10:17:39 2016 +0200
+++ b/cubicweb/server/session.py	Mon Jun 06 15:26:49 2016 +0200
@@ -305,7 +305,6 @@
         # other session utility
         if session.user.login == '__internal_manager__':
             self.user = session.user
-            self.set_language(self.user.prefered_language())
         else:
             self._set_user(session.user)
 
diff -r 9ea50837bc58 -r b48020a80dc3 cubicweb/server/test/unittest_security.py
--- a/cubicweb/server/test/unittest_security.py	Wed Oct 05 10:17:39 2016 +0200
+++ b/cubicweb/server/test/unittest_security.py	Mon Jun 06 15:26:49 2016 +0200
@@ -113,6 +113,7 @@
 
     def test_not_relation_read_security(self):
         with self.new_access(u'iaminusersgrouponly').repo_cnx() as cnx:
+            cnx.user.groups  # fill the cache before screwing syntax_tree_search
             self.hijack_source_execute()
             cnx.execute('Any U WHERE NOT A todo_by U, A is Affaire')
             self.assertEqual(self.query[0][1].as_string(),
diff -r 9ea50837bc58 -r b48020a80dc3 cubicweb/web/application.py
--- a/cubicweb/web/application.py	Wed Oct 05 10:17:39 2016 +0200
+++ b/cubicweb/web/application.py	Mon Jun 06 15:26:49 2016 +0200
@@ -51,9 +51,9 @@
 def anonymized_request(req):
     orig_cnx = req.cnx
     anon_cnx = anonymous_cnx(orig_cnx.session.repo)
-    req.set_cnx(anon_cnx)
     try:
         with anon_cnx:
+            req.set_cnx(anon_cnx)
             yield req
     finally:
         req.set_cnx(orig_cnx)
@@ -262,9 +262,10 @@
         try:
             try:
                 session = self.get_session(req)
-                from  cubicweb import repoapi
-                cnx = repoapi.Connection(session)
-                req.set_cnx(cnx)
+                cnx = session.new_cnx()
+                with cnx:  # may need an open connection to access to e.g. properties
+                    req.set_cnx(cnx)
+                cnx._open = None  # XXX needed to reuse it a few line later :'(
             except AuthenticationError:
                 # Keep the dummy session set at initialisation.  such session will work to some
                 # extend but raise an AuthenticationError on any database access.
diff -r 9ea50837bc58 -r b48020a80dc3 cubicweb/web/test/unittest_application.py
--- a/cubicweb/web/test/unittest_application.py	Wed Oct 05 10:17:39 2016 +0200
+++ b/cubicweb/web/test/unittest_application.py	Mon Jun 06 15:26:49 2016 +0200
@@ -610,7 +610,9 @@
     def _test_auth_anon(self, req):
         asession = self.app.get_session(req)
         # important otherwise _reset_cookie will not use the right session
-        req.set_cnx(repoapi.Connection(asession))
+        cnx = asession.new_cnx()
+        with cnx:
+            req.set_cnx(cnx)
         self.assertEqual(len(self.open_sessions), 1)
         self.assertEqual(asession.login, 'anon')
         self.assertTrue(asession.anonymous_session)
@@ -619,8 +621,10 @@
     def _test_anon_auth_fail(self, req):
         self.assertEqual(1, len(self.open_sessions))
         session = self.app.get_session(req)
-        # important otherwise _reset_cookie will not use the right session
-        req.set_cnx(repoapi.Connection(session))
+        cnx = session.new_cnx()
+        with cnx:
+            # important otherwise _reset_cookie will not use the right session
+            req.set_cnx(cnx)
         self.assertEqual(req.message, 'authentication failure')
         self.assertEqual(req.session.anonymous_session, True)
         self.assertEqual(1, len(self.open_sessions))
diff -r 9ea50837bc58 -r b48020a80dc3 cubicweb/web/test/unittest_form.py
--- a/cubicweb/web/test/unittest_form.py	Wed Oct 05 10:17:39 2016 +0200
+++ b/cubicweb/web/test/unittest_form.py	Mon Jun 06 15:26:49 2016 +0200
@@ -48,6 +48,8 @@
             self.assertEqual(StringField().format(form), 'text/plain')
             req.cnx.execute('INSERT CWProperty X: X pkey "ui.default-text-format", X value "text/rest", X for_user U WHERE U login "admin"')
             req.cnx.commit()
+        with self.admin_access.web_request() as req:
+            form = FieldsForm(req, None)
             self.assertEqual(StringField().format(form), 'text/rest')
 
 
diff -r 9ea50837bc58 -r b48020a80dc3 cubicweb/web/test/unittest_formfields.py
--- a/cubicweb/web/test/unittest_formfields.py	Wed Oct 05 10:17:39 2016 +0200
+++ b/cubicweb/web/test/unittest_formfields.py	Mon Jun 06 15:26:49 2016 +0200
@@ -136,6 +136,8 @@
             req.cnx.create_entity('CWProperty', pkey=u"ui.default-text-format", value=u"text/rest",
                                   for_user=req.user.eid)
             req.cnx.commit()
+        with self.admin_access.web_request() as req:
+            form = EntityFieldsForm(req, entity=e)
             self.assertEqual(description_format_field.value(form), 'text/rest')
 
     def test_property_key_field(self):