Per-user login history report shows data from all users [#1691412]

If you navigate to a user's login history page at /user/%user/login_history, the report shows data from all users. It looks like login_history_menu() passes an argument to login_history_report_callback(), but that argument is not in the function signature, nor is it used to refine the query.

This may be a potential security issue if you allow users to "View own login history", since the user will then have access to potentially sensitive data from all users.

Comment	File	Size	Author
#6	login_history-1691412-6.patch	1.22 KB	star-szr
#6
#3	login_history-1691412-3.patch	846 bytes	star-szr
#3
#1	login_history_report_bug-1691412.patch	2.8 KB	a_thakur
#1
#1	Screenshot from 2012-08-01 13:30:47.png	96.47 KB	a_thakur
#1	Screenshot from 2012-08-01 13:30:59.png	62.54 KB	a_thakur

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

a_thakur CreditAttribution: a_thakur commented 1 August 2012 at 08:05

Status:

Active

» Needs review

File	Size
Screenshot from 2012-08-01 13:30:59.png	62.54 KB
Screenshot from 2012-08-01 13:30:47.png	96.47 KB
login_history_report_bug-1691412.patch	2.8 KB

The attached patch resolves the issue. Please review and test it. The attached screen shots shows the results.

Comment #2

greggles

he/him

English

Denver, Colorado, USA

CreditAttribution: greggles commented 1 August 2012 at 15:33

Looks good to me.

Comment #3

star-szr

he/him

English

CreditAttribution: star-szr commented 3 August 2012 at 13:43

File	Size
login_history-1691412-3.patch	846 bytes

Thanks for the patch @a_thakur. This patch fixes the user-specific reports, but unfortunately it also breaks the global login report.

Warning: Missing argument 1 for login_history_report_callback() in login_history_report_callback() (line 11 of login_history/includes/login_history.pages.inc).
Notice: Undefined variable: account in login_history_report_callback() (line 28 of login_history/includes/login_history.pages.inc).
Notice: Trying to get property of non-object in login_history_report_callback() (line 28 of login_history/includes/login_history.pages.inc).

Here's a new patch to fix this bug. This patch only adjusts the menu callback and leaves the menu path as is to be addressed in #1707200: Change user login history menu item to use a dash instead of an underscore. Feel free to open a new documentation issue for the @file docblocks, that way we can keep things a bit more separated and easier to review.

As with my other patches, #1691474: Convert files to unix line endings has been applied first.

Comment #4

a_thakur CreditAttribution: a_thakur commented 6 August 2012 at 11:18

Status:

Needs review

» Reviewed & tested by the community

Thanks for the revised patch Cottser, I had suspected that I had missed that particular setting which I believe had caused to error to appear when the login history report was viewed from "Report" menu item in the admin menu.

The patch works fine.