if a failed login attempt comes from an API request, mark it as from the API in the brute force log #17768
Conversation
… the API in the brute force log
| @@ -68,6 +71,10 @@ public function isEnabled() | |||
|
|
|||
| public function addFailedAttempt($ipAddress, $login = null) | |||
| { | |||
| if (empty($login) && Request::isRootRequestApiRequest()) { | |||
| $login = self::API_LOGIN_PLACEHOLDER; | |||
There was a problem hiding this comment.
would this cause any issue when there is an API call and it maybe searches for isUserLoginBlocked(null)? Could there be other side effects?
Maybe generally when login is null it's likely API anyway?
There was a problem hiding this comment.
would this cause any issue when there is an API call and it maybe searches for isUserLoginBlocked(null)? Could there be other side effects?
I don't think it's expected to search for isUserLoginBlocked(null). The query that searches for logins uses login = ? not login is NULL so I wouldn't expect that work anyway.
Maybe generally when login is null it's likely API anyway?
I think it could also be a tracker request?
There was a problem hiding this comment.
Possible re tracking request 👍 . Generally, I think it be better to add a column source or something similar just to prevent random side effects or bugs. If I see this right indeed isUserLoginBlocked is only called when there is a login so far. It just could also cause maybe side effects in the future as it may not be expected behaviour to have a login set where there is none and generally it's maybe not too important of an issue to introduce possible side effects in the future?
There was a problem hiding this comment.
A source column would probably be better. We could put the api method/module there as well. It wouldn't take too long to add, I'll look into it tomorrow.
There was a problem hiding this comment.
btw @diosmosis as the issue is not too high priority we could also just close the PR and work on it another day
There was a problem hiding this comment.
It won't take very long, I'll get to it when I have some time.
|
This issue is in "needs review" but there has been no activity for 7 days. ping @matomo-org/core-reviewers |
github-actions
bot
added
the
Stale
sgiehl
added
Pull Request WIP
|
If you don't want this PR to be closed automatically in 28 days then you need to assign the label 'Do not close'. |
github-actions
bot
added
the
Stale
|
not needed for now |
Description:
For debugging purposes it could be helpful to see what entries are from API requests (as opposed to login requests and tracker API requests).
Review