-
Notifications
You must be signed in to change notification settings - Fork 585
[Rule Tuning] Microsoft Entra ID Exccessive Account Lockouts Detected #4851
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
Conversation
Rule: Tuning - GuidelinesThese guidelines serve as a reminder set of considerations when tuning an existing rule. Documentation and Context
Rule Metadata Checks
Testing and Validation
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
tests/test_all_rules.py
Outdated
created_str = rule.contents.metadata.creation_date | ||
updated_str = rule.contents.metadata.updated_date | ||
|
||
created = datetime.strptime(created_str, "%Y/%m/%d").date() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is a good approach at the moment. In the future we may want to prefer using an approach using dateutil
to parse the date as a more robust method.
tests/test_all_rules.py
Outdated
def test_dates_not_in_future(self): | ||
"""Ensure creation and updated dates are not in the future.""" | ||
invalid = [] | ||
today = datetime.now(timezone.utc).date() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This may not be a major concern, but for positive UTC offset timezones it is possible for this unit test to erroneously fail. For instance, we have team members in UTC+5. There will be certain times where the day will be head of UTC time for them.
E.g.
- UTC Time: January 1, 2023, 21:30 (9:30 PM)
- Local Time in UTC+5: January 2, 2023, 02:30 (2:30 AM)
- Local Time in UTC-4: January 1, 2023, 17:30 (5:30 PM)
One way to address this is to start today as defined at UTC+14, but there are other ways too.
today = datetime.now(timezone.utc).date() | |
# Define UTC+14 timezone | |
utc_plus_14 = timezone(timedelta(hours=14)) | |
# Get the current date in UTC+14 | |
today = datetime.now(utc_plus_14).date() |
IMO I'd probably rewrite this using git commit timestamp. I suspect it would be more reliable. @terrancedejesus if you can just remove the code changes and fix the rule and open an issue, we can try to tackle this similar to how we do in other repos. |
I think this makes sense as well 👍 Using this method, the localized date for the commit timestamp can be converted as needed. Also, the future check component of this PR is somewhat fungible. We are really looking for typos, with the obvious typo case being the created date in the future. A git commit timestamp approach also can check for typos on the date looking for if the rule file is new, does the creation/updated date match the commit date. If not, it is likely a typo, etc. |
Summary - What I changed
Fixes date in
Microsoft Entra ID Exccessive Account Lockouts Detected
.Checklist
bug
,enhancement
,schema
,maintenance
,Rule: New
,Rule: Deprecation
,Rule: Tuning
,Hunt: New
, orHunt: Tuning
so guidelines can be generatedmeta:rapid-merge
label if planning to merge within 24 hoursContributor checklist