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.

Sign up for GitHub

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

Jump to bottom

Add simple peak detection #152

Merged
merged 1 commit into from
Sep 11, 2019
Merged

Add simple peak detection #152

merged 1 commit into from
Sep 11, 2019

Conversation

ChristophWurst
Copy link
Member

@ChristophWurst ChristophWurst commented Sep 10, 2019 •
edited
Loading

Fixes #115

@ChristophWurst ChristophWurst added enhancement New feature or request 3. to review labels Sep 10, 2019
@ChristophWurst ChristophWurst added this to the next milestone Sep 10, 2019
@ChristophWurst ChristophWurst self-assigned this Sep 10, 2019
rullzer
rullzer reviewed Sep 10, 2019
View reviewed changes
lib/Service/LoginClassifier.php
// Have we sent more than three alerts in the last hour
$lastHour = count($this->mapper->findRecentByUid($uid, $now - 60 * 60));
if ($lastHour >= 3) {
$this->logger->warning("Suspicious login peak detected: $uid received $lastTwoDays alerts in the last hour");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$lastTwoDays seems incorrect

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
$this->logger->warning("Suspicious login peak detected: $uid received $lastTwoDays alerts in the last hour");
$this->logger->warning("Suspicious login peak detected: $uid received $lastHour alerts in the last hour");

rullzer
rullzer requested changes Sep 10, 2019
View reviewed changes
Copy link
Member

@rullzer rullzer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks good but the counting of the notifications is not right I think :P

@ChristophWurst
Add a simple peak detection
8d3b253 
Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>
@ChristophWurst
Copy link
Member Author

Please have another look.

  • Removed the comments as they are prone to get out of sync with the code/notification
  • Changed the comparison to > as opposed to >= as the current alert is included
  • Reduced to 10 alerts per two days window max

@ChristophWurst ChristophWurst merged commit a3f08da into master Sep 11, 2019
@ChristophWurst ChristophWurst deleted the feature/simple-peak-detection branch September 11, 2019 06:48
@ChristophWurst ChristophWurst modified the milestones: 2.1.0, 2.2.0 Sep 11, 2019
@ChristophWurst ChristophWurst mentioned this pull request Aug 10, 2020
Closed
joshtrichards added a commit that referenced this pull request May 3, 2024
@joshtrichards
fix(LoginClassifier): Adjust log levels / reduce logging noise
e09dd0b 
While #152 addressed rate limiting notifications to users, we still have some logging outside of there that isn't covered.

* Adjusts log levels (Warning -> a mixture of Debug and Info) of the two main messages that aren't rate limited
* Adjusts log levels of the already rate limited messages to Info level (from Warning) to be more consistent with Brute Force Protection log levels

This cuts down on the log noise from "Detected a login from a suspicious login..." 

All logs remain available at the appropriate log levels if desired.

Signed-off-by: Josh <josh.t.richards@gmail.com>
@joshtrichards joshtrichards mentioned this pull request May 3, 2024
Merged
AndyScherzinger pushed a commit that referenced this pull request Jul 2, 2024
@joshtrichards @AndyScherzinger
fix(LoginClassifier): Adjust log levels / reduce logging noise
25c266f 
While #152 addressed rate limiting notifications to users, we still have some logging outside of there that isn't covered.

* Adjusts log levels (Warning -> a mixture of Debug and Info) of the two main messages that aren't rate limited
* Adjusts log levels of the already rate limited messages to Info level (from Warning) to be more consistent with Brute Force Protection log levels

This cuts down on the log noise from "Detected a login from a suspicious login..." 

All logs remain available at the appropriate log levels if desired.

Signed-off-by: Josh <josh.t.richards@gmail.com>
AndyScherzinger pushed a commit that referenced this pull request Jul 2, 2024
@joshtrichards @AndyScherzinger
fix(LoginClassifier): Adjust log levels / reduce logging noise
d631fae 
While #152 addressed rate limiting notifications to users, we still have some logging outside of there that isn't covered.

* Adjusts log levels (Warning -> a mixture of Debug and Info) of the two main messages that aren't rate limited
* Adjusts log levels of the already rate limited messages to Info level (from Warning) to be more consistent with Brute Force Protection log levels

This cuts down on the log noise from "Detected a login from a suspicious login..." 

All logs remain available at the appropriate log levels if desired.

Signed-off-by: Josh <josh.t.richards@gmail.com>
ChristophWurst pushed a commit that referenced this pull request Sep 30, 2024
@joshtrichards @ChristophWurst
fix(LoginClassifier): Adjust log levels / reduce logging noise
6094ae6 
While #152 addressed rate limiting notifications to users, we still have some logging outside of there that isn't covered.

* Adjusts log levels (Warning -> a mixture of Debug and Info) of the two main messages that aren't rate limited
* Adjusts log levels of the already rate limited messages to Info level (from Warning) to be more consistent with Brute Force Protection log levels

This cuts down on the log noise from "Detected a login from a suspicious login..." 

All logs remain available at the appropriate log levels if desired.

Signed-off-by: Josh <josh.t.richards@gmail.com>
backportbot bot pushed a commit that referenced this pull request Sep 30, 2024
@joshtrichards @backportbot
fix(LoginClassifier): Adjust log levels / reduce logging noise
f678bb4 
While #152 addressed rate limiting notifications to users, we still have some logging outside of there that isn't covered.

* Adjusts log levels (Warning -> a mixture of Debug and Info) of the two main messages that aren't rate limited
* Adjusts log levels of the already rate limited messages to Info level (from Warning) to be more consistent with Brute Force Protection log levels

This cuts down on the log noise from "Detected a login from a suspicious login..." 

All logs remain available at the appropriate log levels if desired.

Signed-off-by: Josh <josh.t.richards@gmail.com>
backportbot bot pushed a commit that referenced this pull request Sep 30, 2024
@joshtrichards @backportbot
fix(LoginClassifier): Adjust log levels / reduce logging noise
1e1b5c6 
While #152 addressed rate limiting notifications to users, we still have some logging outside of there that isn't covered.

* Adjusts log levels (Warning -> a mixture of Debug and Info) of the two main messages that aren't rate limited
* Adjusts log levels of the already rate limited messages to Info level (from Warning) to be more consistent with Brute Force Protection log levels

This cuts down on the log noise from "Detected a login from a suspicious login..." 

All logs remain available at the appropriate log levels if desired.

Signed-off-by: Josh <josh.t.richards@gmail.com>
backportbot bot pushed a commit that referenced this pull request Sep 30, 2024
@joshtrichards @backportbot
fix(LoginClassifier): Adjust log levels / reduce logging noise
0685157 
While #152 addressed rate limiting notifications to users, we still have some logging outside of there that isn't covered.

* Adjusts log levels (Warning -> a mixture of Debug and Info) of the two main messages that aren't rate limited
* Adjusts log levels of the already rate limited messages to Info level (from Warning) to be more consistent with Brute Force Protection log levels

This cuts down on the log noise from "Detected a login from a suspicious login..." 

All logs remain available at the appropriate log levels if desired.

Signed-off-by: Josh <josh.t.richards@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rullzer rullzer rullzer approved these changes

Assignees

@ChristophWurst ChristophWurst

Labels
3. to review enhancement New feature or request
Projects
None yet
Milestone
2.2.0
Development

Successfully merging this pull request may close these issues.

thausends over thausends messeseges for suspicios_login
2 participants
@ChristophWurst @rullzer