? ? ? Success

User tests: Successful: Unsuccessful:

avatar mbabker
mbabker
18 Sep 2016

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.

avatar mbabker mbabker - open - 18 Sep 2016
avatar mbabker mbabker - change - 18 Sep 2016
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 18 Sep 2016
Category Administration Components Language & Strings Installation
avatar joomla-cms-bot joomla-cms-bot - change - 18 Sep 2016
Labels Added: ? ?
avatar joomla-cms-bot joomla-cms-bot - change - 18 Sep 2016
Labels Added: ? ?
avatar andrepereiradasilva
andrepereiradasilva - comment - 18 Sep 2016

didn't even know this existed!

avatar andrepereiradasilva andrepereiradasilva - test_item - 18 Sep 2016 - Tested successfully
avatar andrepereiradasilva
andrepereiradasilva - comment - 18 Sep 2016

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.

avatar zero-24 zero-24 - change - 18 Sep 2016
Labels Added: ?
avatar zero-24
zero-24 - comment - 18 Sep 2016

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.

avatar mbabker
mbabker - comment - 18 Sep 2016

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 (comment)
:

@@ -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 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAWfoX6EDdz5xQjDueM7Rr2OqdneMnAHks5qrYB9gaJpZM4J_9tC
.

avatar mbabker
mbabker - comment - 18 Sep 2016

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
.

avatar zero-24
zero-24 - comment - 18 Sep 2016

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

avatar brianteeman
brianteeman - comment - 18 Sep 2016

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

avatar mbabker
mbabker - comment - 18 Sep 2016

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
.

avatar zero-24
zero-24 - comment - 20 Sep 2016

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.
image

Or did i something wrong?

avatar mbabker
mbabker - comment - 20 Sep 2016

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.

avatar zero-24
zero-24 - comment - 20 Sep 2016

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. ;)

avatar mbabker
mbabker - comment - 20 Sep 2016

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 😉

avatar brianteeman
brianteeman - comment - 21 Sep 2016

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.

avatar mbabker
mbabker - comment - 21 Sep 2016

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.

avatar infograf768
infograf768 - comment - 21 Sep 2016

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

screen shot 2016-09-21 at 17 55 15

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

screen shot 2016-09-21 at 17 53 48
screen shot 2016-09-21 at 17 53 32

avatar infograf768
infograf768 - comment - 21 Sep 2016

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

screen shot 2016-09-21 at 18 04 30

avatar mbabker
mbabker - comment - 21 Sep 2016

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.

avatar andrepereiradasilva
andrepereiradasilva - comment - 21 Sep 2016

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)?

avatar brianteeman
brianteeman - comment - 21 Sep 2016

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

avatar andrepereiradasilva
andrepereiradasilva - comment - 21 Sep 2016

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:

avatar mbabker
mbabker - comment - 21 Sep 2016

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".

avatar brianteeman
brianteeman - comment - 21 Sep 2016

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

avatar mbabker
mbabker - comment - 21 Sep 2016

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.

avatar andrepereiradasilva
andrepereiradasilva - comment - 21 Sep 2016

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.

avatar infograf768
infograf768 - comment - 22 Sep 2016

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.

avatar mbabker
mbabker - comment - 2 Oct 2016

OK all the metadata junk is addressed now.

avatar andrepereiradasilva andrepereiradasilva - test_item - 2 Oct 2016 - Tested successfully
avatar andrepereiradasilva
andrepereiradasilva - comment - 2 Oct 2016

I have tested this item successfully on 8420cf9


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

avatar zero-24 zero-24 - test_item - 3 Oct 2016 - Tested successfully
avatar zero-24
zero-24 - comment - 3 Oct 2016

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.

avatar zero-24 zero-24 - change - 3 Oct 2016
Status Pending Ready to Commit
avatar zero-24
zero-24 - comment - 3 Oct 2016

RTC ❤️


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

avatar joomla-cms-bot joomla-cms-bot - change - 3 Oct 2016
Labels Added: ?
avatar joomla-cms-bot joomla-cms-bot - close - 3 Oct 2016
avatar rdeutz rdeutz - close - 3 Oct 2016
avatar rdeutz rdeutz - merge - 3 Oct 2016
avatar rdeutz rdeutz - change - 3 Oct 2016
Status Ready to Commit Fixed in Code Base
Closed_Date 0000-00-00 00:00:00 2016-10-03 17:12:37
Closed_By rdeutz
avatar izharaazmi
izharaazmi - comment - 5 Oct 2016

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.

avatar joomla-cms-bot joomla-cms-bot - change - 5 Oct 2016
Labels Removed: ?
avatar mbabker
mbabker - comment - 5 Oct 2016

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.

avatar mbabker
mbabker - comment - 5 Oct 2016

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.

avatar izharaazmi
izharaazmi - comment - 6 Oct 2016

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.

Add a Comment

Login with GitHub to post a comment