Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Correct username in plg_actionlog #24992

Conversation

Projects
None yet
8 participants
@tecpromotion
Copy link
Contributor

commented May 23, 2019

Summary of Changes

In the User Actions Log extension, the user is always used in the output. The exception is the log entries related to the cache. Here, the user name is always used instead of the user.

With the change now value username is used instead of value name.

Testing Instructions

Create a new user with
name = Super User
and
username = test

Expected result

User test deleted cache group all

Actual result

User Super User deleted cache group all

Documentation Changes Required

--

@brianteeman

This comment has been minimized.

Copy link
Contributor

commented May 23, 2019

Also see #24862

tecpromotion added some commits May 24, 2019

@degobbis

This comment has been minimized.

Copy link
Contributor

commented Jun 22, 2019

I have tested this item successfully on c3f1500

thx


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

@alikon

This comment has been minimized.

Copy link
Contributor

commented Jun 22, 2019

I have tested this item successfully on c3f1500


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

@zero-24 zero-24 added this to the Joomla 3.9.9 milestone Jun 22, 2019

@zero-24 zero-24 merged commit 4127c01 into joomla:staging Jun 22, 2019

5 checks passed

Hound No violations found. Woof!
JTracker/HumanTestResults Human Test Results: 2 Successful 0 Failed.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/drone/pr Build is passing
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@zero-24

This comment has been minimized.

Copy link
Contributor

commented Jun 22, 2019

Merging thanks.

@infograf768

This comment has been minimized.

Copy link
Member

commented Jun 22, 2019

The filter by user is still using name and not username
This PR implies changes to the logcreator field.

Without a modification here I get
Screen Shot 2019-06-22 at 09 16 48

Issue solved if modifying the query to use username and not name

// Construct the query
			$query = $db->getQuery(true)
				->select($db->quoteName('u.id', 'value'))
				->select($db->quoteName('u.username', 'text'))
				->from($db->quoteName('#__users', 'u'))
				->join('INNER', $db->quoteName('#__action_logs', 'c') . ' ON ' . $db->quoteName('c.user_id') . ' = ' . $db->quoteName('u.id'))
				->group($db->quoteName('u.id'))
				->group($db->quoteName('u.username'))
				->order($db->quoteName('u.username'));
@zero-24

This comment has been minimized.

Copy link
Contributor

commented Jun 22, 2019

@infograf768 can you send the required changes as PR?

@infograf768

This comment has been minimized.

Copy link
Member

commented Jun 22, 2019

TBH, I am not sure about the whole thing
Yes, what is logged should be consistent BUT imho it should be the name and not the username displayed in the logs.
The reason is that the username is also the Login Name and this could be a security breach.
What do you think?

@infograf768

This comment has been minimized.

Copy link
Member

commented Jun 22, 2019

This means that we have to do the reverse of that PR, i.e. use ->name everywhere and keep the filter as is.

@degobbis

This comment has been minimized.

Copy link
Contributor

commented Jun 22, 2019

I do not see a security breach here. If you can see the action logs, then you can also access the users component and see the usernames. The improvement is that the username is unique and therefore faster to allocate.

@infograf768

This comment has been minimized.

Copy link
Member

commented Jun 22, 2019

You are right that the name is not unique indeed (which is weird btw...).

Note: it is always the Name which is displayed in the Managers and Created by field (and its modal), which imho demonstrates that we should make the Name unique too...

@degobbis

This comment has been minimized.

Copy link
Contributor

commented Jun 22, 2019

I don't think the name should be unique.
For a clear identification we already have 2 values, username and e-mail address. That's enough, or not?

But if it causes such far-reaching changes, is it perhaps sufficient to write the username in parentheses after the names in the output?
What do you all mean?

infograf768 added a commit to infograf768/joomla-cms that referenced this pull request Jun 22, 2019

@infograf768

This comment has been minimized.

Copy link
Member

commented Jun 22, 2019

Please test
#25291

Going further would need more important changes and should imho be reserved to J4.

@infograf768

This comment has been minimized.

Copy link
Member

commented Jun 22, 2019

BTW, why keep using ->name in onUserAfterRemind($user)
It is not consistent imho.

zero-24 added a commit that referenced this pull request Jun 22, 2019

@HLeithner

This comment has been minimized.

Copy link
Member

commented Jun 22, 2019

username is unique, the name isn't. so imo it's better to use username

@brianteeman

This comment has been minimized.

Copy link
Contributor

commented Jun 22, 2019

we must not make "name" unique. That would be terribly restrictive. Even with my almost unique family name there are two "brian teeman"

@HLeithner

This comment has been minimized.

Copy link
Member

commented Jun 22, 2019

thats the reason that we stay with username instead using name.

@infograf768

This comment has been minimized.

Copy link
Member

commented Jun 23, 2019

Understood that. Tks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.