-
Notifications
You must be signed in to change notification settings - Fork 176
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
New Notion Detections #896
Conversation
DO NOT MERGE!We should wait until we release Notion Log source before merging this branch |
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.
Nice stuff, just had a couple of comments
if get_string_set(cache_key): | ||
# User has logged in from this location recently. Let's refresh this login key. | ||
set_key_expiration(cache_key, time.time() + DEFAULT_CACHE_PERIOD) | ||
return False # No need to alert - user has logged in from here before. | ||
|
||
# Else, his is a location the user hasn't recently used | ||
put_string_set(cache_key, ["arbitrary value"]) | ||
set_key_expiration(cache_key, time.time() + DEFAULT_CACHE_PERIOD) |
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.
is this supposed to be doing a location check of some sort? as defined when a user logs in for the first time it will trigger an alert .... this could be noisy once folks first turn this on
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.
Hmm yeah... I had added the location to the cache key so we can track TTL for each location separately, but it doesn't allow us to easily determine if there are any saved locations for a particular user. Perhaps we should have another cached string-set with the names of user ids we've seen before? Then if we don't see a previous login, we can reference that string set to determine if this is the first recorded login from that user.
An alternative is to move to a dictionary model instead of a string set, and store locations as keys and the last login time as the value. Then we can manually check if a location has expired, and we'd know if we didn't find a cached dictionary, then this is the first login for the user.
Any thoughts on a preferred approach?
Combine TTL assignment and put_string_set to one call Co-authored-by: darwayne <[email protected]>
Force TTL check when checking for past logins Co-authored-by: darwayne <[email protected]>
Changed the default assignment of LOGIN_TS to the module scope.
Using dictionaries instead of string sets means we can detect if this is a user's first login, and if so, then not alert. Makes the rule less noisy when turned on for the first time.
Based on feedback from Notion about what the page permission events mean, we adjusted the names/IDs of the related rules. Also added some unit tests and tweaked the titles/alert_contexts.
The SSO change is applied globally, not to a single workspace. The workspace ID in the event payload informs the user which workspace the action was performed from, not which workspace it applies to. (Based on a conversation with Notion.)
For consistency, I ordered the YAML keys according to the format we output when exporting rules from the UI.
Notion.LoginFromNewLocation would cause an error if there wasn't any location enrichment - now it will return False if that's the case.
Removed the impossible travel page view rule, and the login from blocked IP rule. Both of these are noisy unless tuned, so we want to avoind them being enabled when turning on the whole pack.
Background
With the release of Panther's notion integration, we also want to release a set of managed detections for basic coverage.
Changes
Testing