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

Fixed : Even though you activate the user who gets inactivated by fai… #8528

Merged
merged 1 commit into from Nov 12, 2020

Conversation

arielkamoyedji
Copy link
Contributor

@arielkamoyedji arielkamoyedji commented Apr 9, 2020

This pull request is to fix issue #8449.

In UserLoginAttempts table, utcDate field type is timestamp.
{ $after = Carbon::now('UTC')->subSeconds($duration); } returns a Datetime value which has to be converted into a timestamp value before being passed on to the following where clause:
{ $qb->where('a.utcDate > :after')->setParameter('after', $after->getTimestamp()); }
Otherwise, it would come down to comparing a Datetime value with a timestamp value which returns uncertain results.

Once a user failed to login several times and has been deactivated, then reactivated, next times that user wants to log back in, all past failed login attempts are fetched from the database instead of those that occured within the duration specified in concrete.user.deactivation.duration and that's why after a single login attempt failure again, the user will be deactivated.
This code line { return max(0, $allowed - $attempts); } in function remainingAttempts() would then almost always return 0 since $allowed < $attempts which is a malfunction.

However even after adding this modification to the code, in case a reactivated user wants to log back in within concrete.user.deactivation.duration, they will still be deactivated after a single login attempt failure since previous login attempt failures are not cleared from the database, which is a normal behaviour of the system in my opinion. But whether past login attempt failures should be deleted from the database or not after a user has been deactivated and then reactivated is up to the core team.

…ling to log in, the login count is not cleared.
@aembler
Copy link
Member

aembler commented Apr 9, 2020

Does this make sense to you @KorvinSzanto ?

@stale
Copy link

stale bot commented Nov 8, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions!

@stale stale bot added the Stale Issues that have been inactive for more than 180 days and will soon be closed label Nov 8, 2020
@mlocati
Copy link
Contributor

mlocati commented Nov 8, 2020

This is valid for v8 and v9.
I'd also change $qb->where('a.utcDate < :before')->setParameter('before', $before); to $qb->where('a.utcDate < :before')->setParameter('before', $before->getTimestamp()); in the same file.

@stale stale bot removed the Stale Issues that have been inactive for more than 180 days and will soon be closed label Nov 8, 2020
@aembler aembler merged commit 78d0e0c into concretecms:develop Nov 12, 2020
@arielkamoyedji arielkamoyedji deleted the fix/#8449 branch November 12, 2020 23:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants