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

Log all 'locked out' failures #859

Closed
PaulBender opened this issue Jul 3, 2014 · 32 comments
Closed

Log all 'locked out' failures #859

PaulBender opened this issue Jul 3, 2014 · 32 comments

Comments

@PaulBender
Copy link
Member

@PaulBender PaulBender commented Jul 3, 2014

If locked out of the Admin CP due to bad password, log these failures.

@PaulBender PaulBender added this to the 1.8 Beta 3 milestone Jul 3, 2014
@PaulBender PaulBender self-assigned this Jul 3, 2014
@euantorano
Copy link
Member

@euantorano euantorano commented Jul 3, 2014

THis should be an easy one, so I'll stake claim to it early 😆

@euantorano
Copy link
Member

@euantorano euantorano commented Jul 3, 2014

Started work in #861. Need to test locally.

@euantorano
Copy link
Member

@euantorano euantorano commented Jul 3, 2014

Ok, that seems to work for me locally. The log entry is pretty basic in the form as follows:

    Administrator login attempt for user ID 1 locked out from IP address ::1.
@euantorano euantorano added the feedback label Jul 6, 2014
@euantorano
Copy link
Member

@euantorano euantorano commented Jul 11, 2014

This should be done if somebody from SQA could test?

@JordanMussi
Copy link
Member

@JordanMussi JordanMussi commented Jul 13, 2014

image

Can I also suggest logging what user was logged into the frontend at that time as well as the admin account attempting to be logged into?

@euantorano
Copy link
Member

@euantorano euantorano commented Jul 13, 2014

That's odd. I'll have to test again, though it might require a mod to the function logging admin actions.

@Sama34
Copy link
Contributor

@Sama34 Sama34 commented Jul 13, 2014

4651dfb should fix the SQL error.

I suppose $mybb->user['uid'] will not be defined if the user is trying to log-in.

@euantorano simply pass the data to the log_admin_action() function and format it in the administrator logs page (:

@euantorano
Copy link
Member

@euantorano euantorano commented Jul 13, 2014

Nice one, cheers Omar. I'll look at it ASAP.

On 13 Jul 2014, at 21:08, Omar Gonzalez notifications@github.com wrote:

4651dfb should fix the SQL error.

I suppose $mybb->user['uid'] will not be defined if the user is trying to log-in.

@euantorano simply pass the data to the log_admin_action() function and format it in the administrator logs page (:


Reply to this email directly or view it on GitHub.

@euantorano
Copy link
Member

@euantorano euantorano commented Jul 14, 2014

@JordanMussi, the patch by @Sama34 fixes the issue. Could you please pull and test again? I've also simplified the log string rather than duplicating the IP info.

@JordanMussi
Copy link
Member

@JordanMussi JordanMussi commented Jul 16, 2014

Yeah it works...

My suggestions... 👅

  • The format for other logging strings is Edited user #2 (Test), Activated user #2 (Test) etc. so you should probably do: Administrator login attempt for user #{1} ({2}) locked out. (1 replaced with id and 2 with the username)
  • User attached to the log (for the username column of the logs table) should be of the logged in user in the frontend or none if it is a guest...
@JordanMussi JordanMussi removed the fixed label Jul 16, 2014
@euantorano
Copy link
Member

@euantorano euantorano commented Jul 16, 2014

Should already do #2. The username is fetched in the log_admin_action() function.

On 16 Jul 2014, at 21:07, Jordan Mussi notifications@github.com wrote:

Yeah it works...

My suggestions...

The format for other logging strings is Edited user #2 (Test), Activated user #2 (Test) etc. so you should probably do: Administrator login attempt for user #{1} ({2}) locked out. (1 replaced with id and 2 with the username)
User attached to the log (for the username column of the logs table) should be of the logged in user in the frontend or none if it is a guest...

Reply to this email directly or view it on GitHub.

@JordanMussi
Copy link
Member

@JordanMussi JordanMussi commented Jul 17, 2014

I don't fully follow... It doesn't look like you've done that in #861...

@euantorano
Copy link
Member

@euantorano euantorano commented Jul 17, 2014

If the user has a front-end login, it will show the username. If they aren't logged in on the front-end, it won't.

I'll capture a screenshot tonight.

@JordanMussi
Copy link
Member

@JordanMussi JordanMussi commented Jul 17, 2014

Really? Are you sure you've pushed that code?

@euantorano
Copy link
Member

@euantorano euantorano commented Jul 17, 2014

Pretty sure. Last I checked log_admin_action does it all.

On 17 Jul 2014, at 19:25, Jordan Mussi notifications@github.com wrote:

Really? Are you sure you've pushed that code?


Reply to this email directly or view it on GitHub.

@JordanMussi
Copy link
Member

@JordanMussi JordanMussi commented Jul 18, 2014

log_admin_action takes the currently logged in admin. Since it logs when there is no admin logged in there is nothing to capture...

@euantorano
Copy link
Member

@euantorano euantorano commented Jul 18, 2014

Are you sure? Line 30/31 of admin/inc/functions.php:

$log_entry = array(
    "uid" => (int)$mybb->user['uid'],

Surely that'll grab the current user ID? I'm sure my local install had a currently logged in user. I didn't get a chance to screenshot last night but will tonight.

@JordanMussi
Copy link
Member

@JordanMussi JordanMussi commented Jul 18, 2014

Yes but look at admin/index.php line 169
That replaces the $mybb->user with the user that is being attempted to be logged in to.

Also at line 302 $mybb->user doesn't seem to be set...

@euantorano
Copy link
Member

@euantorano euantorano commented Jul 18, 2014

Ah, in which case I'll have to create a temporary $front_end_user variable and assign it, but the actual display of the action will still be off :(

@euantorano
Copy link
Member

@euantorano euantorano commented Jul 18, 2014

To add the user details properly, I'm going to have to change quite a lot of stuff in admin/index.php. WHat does everybody else think? In the interests of moving on, I think it's probably passible as is. @mybb/developers...

@euantorano
Copy link
Member

@euantorano euantorano commented Jul 18, 2014

Also, this is how the logs currently look...

screen shot 2014-07-18 at 19 58 26

@PaulBender
Copy link
Member Author

@PaulBender PaulBender commented Jul 18, 2014

Couldn't you just use the user id of the user being locked out?

@Sama34
Copy link
Contributor

@Sama34 Sama34 commented Jul 19, 2014

Or use "Guest" as the username.

"euan" should be formatted in the first line. Don't we do this with other logs (formatting usernames in the details, if not then ignore me)?

@JordanMussi
Copy link
Member

@JordanMussi JordanMussi commented Jul 19, 2014

The username column should contain the frontend user attempting to login (not necessarily the admin account that is being entered into the admin login form) but if it requires a lot of effort I'm happy to skip.

@euantorano, you haven't committed the part where is also adds the username to the log language line.

@euantorano
Copy link
Member

@euantorano euantorano commented Jul 19, 2014

I'm pretty sure I did commit that Jordan. I'd say we call this complete for now in the interest of hitting Beta 3. I can always enhance it later.

@JordanMussi
Copy link
Member

@JordanMussi JordanMussi commented Jul 19, 2014

Damn, my bad. I didn't see a notification of the commit. 👅

@euantorano
Copy link
Member

@euantorano euantorano commented Jul 19, 2014

😉

@Sama34 Sama34 reopened this Jul 20, 2014
@Sama34
Copy link
Contributor

@Sama34 Sama34 commented Jul 20, 2014

I don't think this was meant to be closed?

@JordanMussi JordanMussi added fixed and removed feedback labels Jul 20, 2014
@DiogoParrinha
Copy link
Contributor

@DiogoParrinha DiogoParrinha commented Jul 21, 2014

@euantorano
Copy link
Member

@euantorano euantorano commented Jul 21, 2014

@PirataNervo See this: http://community.mybb.com/thread-155809-post-1087393.html#pid1087393

Pretty much sums up where I'm at with this.

DiogoParrinha pushed a commit that referenced this issue Jul 21, 2014
Starting work on logging admin lockouts for #859.
@DiogoParrinha
Copy link
Contributor

@DiogoParrinha DiogoParrinha commented Jul 21, 2014

Merged then. A new issue should be opened to correct that.

@euantorano
Copy link
Member

@euantorano euantorano commented Jul 21, 2014

Yes, can wait till 1.8.0.

@DiogoParrinha DiogoParrinha added feature and removed fixed labels Jul 21, 2014
DiogoParrinha pushed a commit that referenced this issue Jul 21, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants
You can’t perform that action at this time.