[#20521] - Merge changes from joomla security issue #70
- Closed
- 13 May 2019
- Medium
- Build: staging
- # 20521
- Diff
- tristanbailey:staging
- Success Hound No violations found. Woof! Details
- Failure continuous-integration/drone/pr the build failed Details
- Success continuous-integration/travis-ci/pr The Travis CI build passed Details
- Success continuous-integration/appveyor/pr AppVeyor build succeeded Details
User tests: Successful: Unsuccessful:
Pull Request for Issue # . (Issue #70 on security)
Summary of Changes
(This is a PR I worked on when the JaB 2018 sprint was on, so is not my original request, and I do not have access to Security repo, I have improved it)
Adds wordpress style button when editing an user allowing to log all instances of that account out by hitting the magic button.
Questions on whether we should hide it for the existing logged in user or log out for all instances except the existing one (and if so how?)
Testing Instructions
- Create a second administration user.
- Log in as the second user in one browser, and in another browser on same or another computer with same user.
- Click to edit that user in one browser, and click the log out button at bottom of edit user tab.
Expected result
When browser refreshes clicking any admin link both browsers should be logged out.
(in issue #70 this would log out the current user not the one for the edit page)
Actual result
.
Documentation Changes Required
Would need to explain that you can log out all instances of the user_id logged in.
| Status | New | ⇒ | Pending |
joomla-cms-bot
- | Category | ⇒ | Administration com_users Language & Strings JavaScript |
BUT This all is a securing issue I think, as "var user_id" is just in global namespace in JS, so at JS console I could write the id for another user, that I do not have permission to edit:
What about checking for core.edit on the current user?
JFactory::getUser()->authorise('core.edit', 'com_users');We should also implement the token check ;)
| Category | Administration com_users Language & Strings JavaScript | ⇒ | Administration com_users JavaScript |
@tristanbailey whats the state of this Pull Request?
Hi,
Not looked at it in a long time, my memory I checked, but I didnt have the
security release to work on testing. Following that a vague memory it
needed something else that required a version change or something not ready
in another area.
Sorry to not have a better memory, I was only on it for one day on a
sprint.
On Sun, 12 May 2019 at 08:18, Franz Wohlkönig notifications@github.com
wrote:
@tristanbailey https://github.com/tristanbailey whats the state of this
Pull Request?—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#20521 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AABQ3SNNHZJBA2N6542NMHTPU7ACRANCNFSM4FA5PAHQ
.
--
Holdingbay
Manufacturers and B2B, we offer Web Development and sales lead growth.
site: https://holdingbay.co.uk
skype: tristanbailey
Host: Cliff Notes Podcast https://holdingbay.co.uk/cliff-notes/
one more PR for @HLeithner to decide.
This PR will not get into j3, maybe we can add it into j4 but at the moment we my not have all metadata to perform the delete action.
I'm closing this for now, if you are interested to rebase this for J4 please talk to @SniperSister
| Status | Pending | ⇒ | Closed |
| Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2019-05-13 07:51:40 |
| Closed_By | ⇒ | HLeithner | |
| Labels |
Removed:
J3 Issue
|
||
| Category | Administration com_users JavaScript | ⇒ | Administration com_users Language & Strings JavaScript |
Note here for @zero-24 who I was doing this PR for.
I have this feedback on this ticket.
a) the old 2016 issue #70 merge diff still works with 3.8.x staging branch.
b) the code did not work as required. It passed the current logged in user not the one you are editing, so you always log yourself out.
It was using JFactory::getUser()->id in this file:
/administrator/components/com_users/views/user/tmpl/edit_session.php
c) I fixed this with replacing to JFactory::getApplication()->input->get('id', 0, 'int')
So code looks to work.
BUT This all is a securing issue I think, as "var user_id" is just in global namespace in JS, so at JS console I could write the id for another user, that I do not have permission to edit:
And then click button on the page and this logged out another id.
Maybe there could be more checks in User->destroyUsersSessions() but should be considered by other people first?