Skip to content

Commit

Permalink
Store current user in SQLAlchemy session so activity hook can get it
Browse files Browse the repository at this point in the history
Up until now, the activity stream session extension used the revision
property of the session to get the user performing the action. With the
revisions gone we need to find an alternative. The
`session.revision.user` property was set after a revision was explicitly
created in an action, and its `author`  property set (by way of [1]):

    rev = model.repo.new_revision()
    rev.author = user

So it makes sense to attached the user performing an action when the
actions happen. But rather than have to call it explicitly on each
action that has activities as above, this change adds a unique function
in ckan.logic.get_action that uses the value in the `context` object.
These can be provided by users but most of the time they are
prepolutated with the following defaults: model, model.Session and
c.user.

The main drawback of this is that if extensions import and call core
action functions directly activites will get registered as a Non logged
in user. This is discouraged but it was quite common before chained
actions were introduced (and is still used now). To support this
situation we should add the call to set the user in the session in all
actions that have activities, like it was done before with the
revisions.

[1] https://github.com/ckan/ckan/blob/42596a014a713f9787d4ee9167f4c51afaaefaa6/ckan/model/__init__.py#L436
  • Loading branch information
amercader committed Oct 4, 2019
1 parent 9b8b2a3 commit 8c02842
Show file tree
Hide file tree
Showing 2 changed files with 36 additions and 8 deletions.
13 changes: 5 additions & 8 deletions ckan/lib/activity_streams_session_extension.py
Expand Up @@ -44,14 +44,11 @@ def before_commit(self, session):
except AttributeError:
# session had no _object_cache; skipping this commit
return

user = 'DUNNO' # TODO!!
if user:
user_id = user
else:
# If the user is not logged in then revision.user is None and
# revision.author is their IP address. Just log them as 'not logged
# in' rather than logging their IP address.
try:
user_id = session.user.id
except AttributeError:
# If the user is not logged in then session.user is None or not set.
# Just log them as 'Not logged in' rather than logging their IP address.
user_id = 'not logged in'

# The top-level objects that we will append to the activity table. The
Expand Down
31 changes: 31 additions & 0 deletions ckan/logic/__init__.py
Expand Up @@ -6,6 +6,7 @@
import sys
from collections import defaultdict

import sqlalchemy
import formencode.validators
from six import string_types, text_type

Expand Down Expand Up @@ -238,6 +239,32 @@ def _prepopulate_context(context):
return context


def _set_user_in_session(context):
'''
Attaches the user performing an action to the SQLAlchemy session
This is needed so the Activity Streams before_commit hook in
``ckan/lib/activity_streams_session_extension.py`` can record the user
that performed the activity (which used to be done using revisions).
This assumes model and session properties present in the context, ie
should only be run after ``_prepopulate_context()`` has been executed.
If present the user object is attached both the Session class and an
instance of it, otherwise the hook won't get the expected attribute.
'''
user_name = context.get('user')
if user_name:
model = context['model']
session = context['session']
user_obj = session.query(
model.User).filter_by(name=user_name).one_or_none()
setattr(session, 'user', user_obj)
if isinstance(session, sqlalchemy.orm.scoping.ScopedSession):
session_instance = session()
setattr(session_instance, 'user', user_obj)


def check_access(action, context, data_dict=None):
'''Calls the authorization function for the provided action
Expand Down Expand Up @@ -452,6 +479,10 @@ def wrapped(context=None, data_dict=None, **kw):

context = _prepopulate_context(context)

# Attach the user (if any) to the SQLAlchemy session so it can
# be used in the activities before_commit hook
_set_user_in_session(context)

# Auth Auditing - checks that the action function did call
# check_access (unless there is no accompanying auth function).
# We push the action name and id onto the __auth_audit stack
Expand Down

1 comment on commit 8c02842

@davidread
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❤️

Please sign in to comment.