-
Notifications
You must be signed in to change notification settings - Fork 454
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
View events refactor #2539
View events refactor #2539
Conversation
Your preview environment pr-2539-bttf has been deployed. Preview environment endpoints are available at: |
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.
Looks great - simplified quite a bit.
Left one comment for consideration.
return res.json({ | ||
events: events.filter((event) => | ||
context.permissions.canViewEvent(event.data as NotificationEvent) | ||
), | ||
}); |
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.
Now that this endpoint is filtering the events based on the requester's permission level, it feels like we could loosen the permissions needed to view app.growthbook.io/events
.
I could imagine if a user is in the habit of clicking on a Slack notification to view an event, they might find value in being able to view all events (that they have permission to view).
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.
If we do go this route - we will want to conditionally render the export button as we can keep that to admins only.
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 can definitely see the value in seeing a filtered event log for just the things you have access to / care about. Might want to think some more about the best UX for that. I'm not sure sending people to Settings -> Logs
is the right solution.
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 looks great. I love the separation of concerns.
One TS remark to leverage types on checking permissions. I think this could be very useful when extending events coverage in the future.
environments: [], | ||
// The event contains the ip, userAgent, etc. of users | ||
// When marked as containing secrets, view access will be restricted to admins | ||
containsSecrets: true, |
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.
You could leverage the typing system for this:
type ProtectedEvent = {
containsSecrets: true,
...
}
type UnprotectedEvent = {
containsSecrets: false,
...
}
const unprotectedEventConsumer: (event: UnprotectedEvent) => ...
Then you can add intermediary functions to convert from ProtectedEvent
to either UnprotectedEvent
or a type that confirms that event permission has been checked.
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 like the idea of leaning on Typescript more. I might save this for a follow up since this PR is already getting quite big
Features and Changes
Today, only admins can view an audit log of events for their org. This makes sense for events like
user.login
- we don't want to expose the login ip/userAgent of everyone in the org to non-admins. But for other events likefeature.updated
, it's too restrictive - we want users to be able to click from a Slack notification and view the event details without being admins.This PR changes the logic for viewing event details. There are some events that contain sensitive info (like
user.login
) that we want to keep admin-only. But for everything else, we now use the event subject (e.g. the feature or experiment) to determine view access. If a user is able to view an experiment, they are now also able to view events about that experiment.projects
,tags
,environments
, andcontainsSecrets
viewEvents
/canViewEvents
withviewAuditLogs
/canViewAuditLogs
canViewEvent(eventPayload)
that checks read permission based on the new top-level event fields.Testing
readonly
roleadmin
role