Allow users to share sessions between apps

[mbabker]

Summary of Changes

Joomla's default behavior when generating session names is to use the class name for the running application class, however it also supports using a configuration value for a "static" session name, and when this value is set a user can set their Joomla install to share sessions between the site and admin apps (or really any app class based on JApplicationCms). Let's expose this via the global configuration.

Testing Instructions

With the patch applied, you'll see a new "Shared Sessions" config value under the session settings in the global configuration, defaulted to No (so off). When you set this to on, you will be logged out of the admin (as explained in the tooltip) and will need to log in again. Once logged in again, if you visit the frontend, you'll be authenticated. The same goes if you log into the frontend then try accessing the admin.

Unfortunately the success message is lost because it is enqueued to the active session that saves the configuration and the redirected request will start a new session under the new name. There isn't much that can be done here unfortunately without getting to a state where we pollute user sessions with data from another session.

Documentation Changes Required

If someone had already enabled this by setting a public $session_name = 'whatever'; var in their configuration.php file, they will need to add public $shared_session = '1'; to it as well. The latter var is needed to track the state of this option correctly and if it's not set you'll lose your shared session configuration.

[andrepereiradasilva]

didn't even know this existed!

[andrepereiradasilva]

I have tested this item ✅ successfully on 8440fd0

works as described

---

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/12068.}

[zero-24]

Looks cool. Just a quick question. On a site that comes from pre 3.7.0 when the new setting gots added to the configuration.php? Only on re save the config? Maybe i miss something (i need sleep ;)) and this is not a problem. It just come to my mind on reading this 👍 Thanks.

[zero-24]

Just a smal one please move the > to its own line ;)

[zero-24]

Can you please add the line now as we have a PR for the other definitions in that file: #12080

Thanks

[mbabker]

This is copy/pasted from another field in this file. Suggest making the CS
change to all of them at once as a separate PR instead of this being the
only one done this way.

On Sunday, September 18, 2016, zero-24 notifications@github.com wrote:

@zero-24 commented on this pull request.

In administrator/components/com_config/model/form/application.xml
#12068 (review)
:

@@ -759,6 +759,18 @@
size="6" />

  <field

•     name="shared_session"
  

•     type="radio"
  

•     class="btn-group btn-group-yesno"
  

•     default="0"
  

•     label="COM_CONFIG_FIELD_SHARED_SESSION_LABEL"
  

•     description="COM_CONFIG_FIELD_SHARED_SESSION_DESC"
  

•     filter="integer">
  

Just a smal one please move the > to its own line ;)

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#12068 (review),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAWfoX6EDdz5xQjDueM7Rr2OqdneMnAHks5qrYB9gaJpZM4J_9tC
.

[mbabker]

Nothing changes. The shared session config value is only used when
enabling/disabling the feature and the session name value isn't exposed via
the UI (this is the one that already exists in the code).

No need to change the config file for updates.

On Sunday, September 18, 2016, zero-24 notifications@github.com wrote:

Looks cool. Just a quick question. On a site that comes from pre 3.7.0
when the new setting gots added to the configuration.php? Only on re save
the config? Maybe i miss something (i need sleep ;)) and this is not a
problem. It just come to my mind on reading this 👍 Thanks.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#12068 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAWfoYfokOyTDwb1GyU7I6e2AUjmnjYbks5qrYA6gaJpZM4J_9tC
.

[zero-24]

Thanks it is on my list to fix next week :)

[brianteeman]

yay to adding more parameters that. I will remind you of this when you bitch at someone else adding a param ;)

[mbabker]

I hate the number of parameters we have with a passion but this is one case
where it can't be avoided.

On Sunday, September 18, 2016, Brian Teeman notifications@github.com
wrote:

yay to adding more parameters that. I will remind you of this when you
bitch at someone else adding a param ;)

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#12068 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAWfoX9U2zum4341cWtoxZFemSyyFuBLks5qrYTHgaJpZM4J_9tC
.

[zero-24]

That works but it looks like the last logged users module is confused now as it does not show the admin or the frontend login.

Or did i something wrong?

[mbabker]

Well with this it won't differentiate it. The session would be "owned" by whatever client the user logged into. In either case it's not affecting session functionality, just that module which is dependent on us logging the extra session metadata needlessly on every request for things like a list of logged in users.

[zero-24]

So we should just get rid of that module and the count at the bottom of the site? I guess this also affect the mod_whoisonline module?

Also i problem is that if you logout in the frontend (to test some things without beeing logged in) you got logged out from the backend too -> well this is expected i guess. ;)

[mbabker]

Also i problem is that if you logout in the frontend (to test some things without beeing logged in) you got logged out from the backend too -> well this is expected i guess. ;)

That's why it's a shared session 😉

[brianteeman]

For me this change breaks the existing functionality of displaying the logged in users. I am currently failing to see the benefit of this change in functionality.

---

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/12068.}

[mbabker]

Works fine here.

The benefit to this change is it does not require users to separately log in to both the frontend and backend. That's really it. It's a function that is already supported by the Joomla code base, this PR simply exposes the existing support for it to the global configuration.

• https://github.com/joomla/joomla-cms/blob/3.6.2/libraries/cms/application/cms.php#L122-L126 sets a default value to the session_name config variable in all application constructors inheriting from JApplicationCms (so all three of our web apps) to the current app name if something else hasn't been defined
• https://github.com/joomla/joomla-cms/blob/3.6.2/libraries/cms/application/cms.php#L712-L722 creates a hash of that configuration value (defaulting to the class name if something hasn't been set by that point) and forwards it into the session API as the internal session name
• What "forces" our apps to have separate sessions is the value of this session_name parameter; when it is the same value your sessions can be shared across applications, when it's different PHP handles the sessions as separate entities (AKA our existing behavior)

[infograf768]

I guess we need changes, if the param is set, to the 2 who's online modules and status module:

Also, here, even after deleting the session and re-login, I get multiple results for the same user:

[infograf768]

Looks like the sessions are not deleted from the db:
Logged in and out a few times and I get:

[mbabker]

A lot of this is simply because all of the extra metadata handling is designed to only support the notion of being logged in to either the site or the admin. The who's online module as an example only queries against the client ID, which is set by our metadata handler and isn't part of the session; as is the tracking is only good for whatever client you logged in to (so if you log into admin then visit site, the frontend online module won't have a clue you're logged in and authenticated; this is not a bug by any measure because sessions by design aren't limited to a "client" and this distinction is only made at the metadata tracking level which is not a part of the session at all but we store it in that table).

Session deletion is not touched by this patch at all. The session handler has no notion of this client tracking and simply deletes whatever session ID it was told to. Our "cleanup" routines (which shouldn't even run when the database handler is active) purge everything that should have expired based on the active configuration. Neither of those are touched here and frankly if that stops working simply by activating this then Joomla's sessions are even more borked than I realized.

[andrepereiradasilva]

humm ... when using shared session:

• if i log in the site i get client_id = 0 in the database
• if i log in the admin i get client_id = 1 in the database

maybe with shared sessions this should be null (or empty)?

[brianteeman]

I just don't see the benefits.i am only seeing the things that no longer work as expected

[andrepereiradasilva]

I just don't see the benefits.i am only seeing the things that no longer work as expected

i disagree. i see the benifits of having the same session in site and admin. much more easy to use not having to login in site and admin.

and things works as expected.
as michael said it, this feature always exists in joomla, it's not just a parameter in global config.
if you active this in a global config setting in 3.6.2 you will get the same problems.

---

Michael, there are several places there are DB queries to the session client_id:

• https://github.com/joomla/joomla-cms/blob/staging/libraries/cms/application/cms.php#L185
• https://github.com/joomla/joomla-cms/blob/staging/libraries/legacy/application/application.php#L1054
• https://github.com/joomla/joomla-cms/blob/staging/administrator/modules/mod_status/mod_status.php#L28
• https://github.com/joomla/joomla-cms/blob/staging/administrator/modules/mod_status/mod_status.php#L57
• https://github.com/joomla/joomla-cms/blob/staging/administrator/modules/mod_logged/helper.php#L32
• https://github.com/joomla/joomla-cms/blob/staging/modules/mod_whosonline/helper.php#L35
• https://github.com/joomla/joomla-cms/blob/staging/modules/mod_whosonline/helper.php#L87
• https://github.com/joomla/joomla-cms/blob/staging/plugins/user/joomla/joomla.php#L290

[mbabker]

If I'm going to keep getting brick walled I'm not going to pursue this further.

So here's the issue. Joomla is written in a very specific way that very statically (and borderline asininely) separates "church and state" (or frontend and backend). Some folks have commented that they would like a workflow where you can log in once and be logged in everywhere. The code already supports this feature, I'm exposing it via the UI with an extra tracking parameter so we don't have to instruct the user to create a properly dynamic name. Those who might already be using this feature all have the same issues being thrown up here regarding the extra metadata tracking features not working "as expected". The fix for that is to either set invalid data to the client ID (it should directly map to an ID retrievable via JApplicationHelper::getClientInfo()) or go through the code adding conditionals against the global configuration value to "trick" existing features to working the exact same way. The other option is to remove the feature and leave Joomla working in a very specific way matching a very specific vision of "key players".

[brianteeman]

I am fine with the change. I just don't want to deal with the support issues of the referenced things not working as the user expects.if they can be resolved then great

[mbabker]

Regarding each of those linked queries:

• First two are related to the session metadata tracking using the conventional "one session per user per app" logic; we would have to decide how to handle "multi-client" tracking for the ID either by nulling it or using an arbitrary value (this ID should be treated as a foreign key mapping directly to a known application client)
• Admin mod_status queries are to count the number of authenticated users per application; with this feature enabled there is not a distinction per application as the user is logged in globally
• Admin mod_logged uses the client ID to decide whether the X icon to force end a session (which actually doesn't work completely IIRC) should be displayed; it only shows for site users
• Frontend mod_whosonline queries are to count the guests and authenticated users for the site application; when enabled there's no distinction on applications
• The user plugin's query deletes records if force logout is enabled; this one's pretty FUBAR too because it doesn't actually end the user's session, just deletes the metadata record and it's filtered per client

So if we want this metadata stuff to keep working (I still say we need a killswitch for it for those who have a desire to not log it and require the extra database query(s) per request), basically we need to decide how to track the metadata.

[andrepereiradasilva]

First two are related to the session metadata tracking using the conventional "one session per user per app" logic; we would have to decide how to handle "multi-client" tracking for the ID either by nulling it or using an arbitrary value (this ID should be treated as a foreign key mapping directly to a known application client)

I would go for NULL. since there is no specific client id.

Admin mod_status queries are to count the number of authenticated users per application; with this feature enabled there is not a distinction per application as the user is logged in globally

We could check if the app config shared session parameter is setted and them show just "Users", and show "Site"/"Admin" (like now) otherwise

Admin mod_logged uses the client ID to decide whether the X icon to force end a session (which actually doesn't work completely IIRC) should be displayed; it only shows for site users

So here we don't need to do anything, because in shared session context we can't X a user that is logged in site and admin.
Or we make it so that you can X any user.

Frontend mod_whosonline queries are to count the guests and authenticated users for the site application; when enabled there's no distinction on applications

we could query client_id 0 or NULL

The user plugin's query deletes records if force logout is enabled; this one's pretty FUBAR too because it doesn't actually end the user's session, just deletes the metadata record and it's filtered per client

not sure about this one.

So if we want this metadata stuff to keep working (I still say we need a killswitch for it for those who have a desire to not log it and require the extra database query(s) per request), basically we need to decide how to track the metadata.

I would go for client_id NULL in shared_session. since there is no specific client id for the user session.

[infograf768]

Neither of those are touched here and frankly if that stops working simply by activating this then Joomla's sessions are even more borked than I realized.

When I logged in this morning, the former entries in the session table were correctly deleted.

[mbabker]

OK all the metadata junk is addressed now.

[andrepereiradasilva]

I have tested this item ✅ successfully on 8420cf9

---

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/12068.}

[zero-24]

I have tested this item ✅ successfully on 8420cf9

Thanks @mbabker 👍

---

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/12068.}

[zero-24]

RTC ❤️

---

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/12068.}

[izharaazmi]

Shared sessions are usually asked for by many clients. I remember at least 6 active clients of mine using session_name setting and maybe few more that I don't remember.

There are few side-effects of having this feature ON that we should be aware of.

The userState will also be shared being part of the session data. This means com_content.article.edit.data or com_content.articles.list.limit etc all will be passed/modified between the clients. That means edit forms, list paginations, list filters etc will be shared as well. If we are all okay with these and similar behaviour then it is okay to go with this.

What if we used session_name variable itself instead of an additional new parameter for shared session? That would have been more B/C. We won't even need to modify the session table's client_id NOT NULL setting.

On the Configuration UI we can still show the same thing as you are showing but internally we could set something like session_name = 'shared' and session_name = '' respectively for ON and OFF.

PS: Though it is not a good idea, but we can prevent the user from logging out after changing this setting by setting the shared session_name to administrator when ON.

[mbabker]

What if we used session_name variable itself instead of an additional new parameter for shared session?

Explained on your PR why I went the way I did. I would not give the user the ability to set the value themselves unless they know what they're doing (they can edit the configuration.php file themselves if so). As this is used in part for generating data related to the session, it should be a random value.

The database changes are required as long as we have this crap metadata stuff. Otherwise we are knowingly inserting invalid data and that is unacceptable. Additionally with the parameter changed the filters need to account for this because sessions generated under the old parameter value will still exist in the database and those have become invalidated and will remain until the regular garbage collection routines are run.

[mbabker]

There are few side-effects of having this feature ON that we should be aware of.

The userState will also be shared being part of the session data. This means com_content.article.edit.data or com_content.articles.list.limit etc all will be passed/modified between the clients. That means edit forms, list paginations, list filters etc will be shared as well. If we are all okay with these and similar behaviour then it is okay to go with this.

Missed responding to this part earlier.

First, these are issues that already exist if you were using the parameter. This PR fixes some of the things related to user facing features, but not some of the backend handling.

Second, fixing some of this (i.e. the state variable names) constitutes a B/C break and would need to go into the 4.0 effort (you'd need to differentiate these things by client or some other unique identifier to ensure that com_content frontend doesn't mess with com_content backend).

As for the edit stuff, that's only going to be an issue if you're trying to open the edit form for the same item in both apps. And again fixing it would be a B/C issue.

[izharaazmi]

Yes, I understand that this issue already exists and its nothing that this PR has caused. It was just a statutory warning to those who are going to or already using shared sessions 😉 .

And again fixing it would be a B/C issue.

Yes and No. Yes, if we do it as shared sessions. No, if we do it as shared login. But again its for another day, another PR.