Problem/Motivation

Discovered as part of #3410312: Flood database backend ::isAllowed() should call ::ensureTableExists() and via #3377657: Add database query spans to otel traces.

When users log in, we clear previous flood attempts, however we do that whether there are any in the database to clear or not.

By checking Flood::isAllowed() with a limit of 1, we can check if the user has a clean slate, and if so skip clearing the flood attempts at all. This saves a DELETE query every user login, at the cost of an extra SELECT query if a user has already triggered a flood event for a failed login.

Steps to reproduce

Proposed resolution

Add an extra flag to form state if the user has a clean slate.

Adjust StandardPerformanceTest to reflect the lower number of database queries (!!!!).

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Before:

After:

CommentFileSizeAuthor
#5 Screenshot from 2023-12-21 22-54-45.png244.07 KBcatch
#5 Screenshot from 2023-12-21 22-53-24.png423.52 KBcatch

Issue fork drupal-3410419

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

catch created an issue. See original summary.

catch’s picture

catch
he/him
Primary language English
commented
Status: Active » Needs review

catch opened merge request !5929

catch’s picture

catch
he/him
Primary language English
commented

This is stacked on #3410312: Flood database backend ::isAllowed() should call ::ensureTableExists() due to the performance test changes, but the runtime code changes are independent.

catch’s picture

catch
he/him
Primary language English
commented
Issue summary: View changes
StatusFileSize
new Screenshot from 2023-12-21 22-53-24.png423.52 KB
new Screenshot from 2023-12-21 22-54-45.png244.07 KB

Adding screenshots from local performance test traces showing the DELETE query disappears.

heddn’s picture

heddn
Primary language English
Location Nicaragua
commented
Status: Needs review » Reviewed & tested by the community

Makes sense. LGTM.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Version: 11.x-dev » 10.2.x-dev
Status: Reviewed & tested by the community » Fixed

Also makes sense to me, can't really see this making a difference in the real world but if it can make tests slightly faster then all the better.

Committed and pushed 8e5acc674b to 11.x and 595dce06b7 to 10.2.x. Thanks!

  • longwave committed 595dce06 on 10.2.x 
    Issue #3410419 by catch: Only clear flood attempts when necessary during...

  • longwave committed 8e5acc67 on 11.x 
    Issue #3410419 by catch: Only clear flood attempts when necessary during...
catch’s picture

catch
he/him
Primary language English
commented

Found some more queries to get rid of :) #3410582: Optimize user logins by avoiding duplicate entity queries.

Just one database query is usually a millisecond or so, but the more we get rid of, the easier it is to see any other extraneous queries on top. I think I'm up to 3-4 now including two entity queries, which could end up 10ms or so off logins.

fabianx’s picture

fabianx commented

Looks good, but one typo nit in the docs:

bu -> but

(https://git.drupalcode.org/project/drupal/-/commit/8e5acc674b6394a83249f...)

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

staalex’s picture

staalex commented

Checking isAllowed() with threshold = 1 causes the logs to fill up with Flood control blocked login attempt for uid %uid from %ip messages for all users who try to log in from the second (or more, below threshold) attempt. This is because UserFloodSubscriber::blockedUser() is called on every FALSE result of isAllowed() calls. Should this be addressed in another issue?