feat: filter projectless events for normal users #7914
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.OpenSSF Scorecard
Scanned Manifest Files |
| @@ -390,3 +455,63 @@ test('should filter events by project using IS_ANY_OF', async () => { | |||
| total: 2, | |||
| }); | |||
| }); | |||
|
|
|||
| test('should not show user creation events for non-admins', async () => { | |||
There was a problem hiding this comment.
This tests the main logic.
| @@ -222,6 +229,14 @@ export default class EventService { | |||
| async getEventCreators() { | |||
| return this.eventStore.getEventCreators(); | |||
| } | |||
|
|
|||
| async getProjectFilterForNonAdmins(userId: number): Promise<IQueryParam[]> { | |||
| const isRootAdmin = await this.accessReadModel.isRootAdmin(userId); | |||
There was a problem hiding this comment.
This is the main logic. If you are not root admin, add db query filter to not include projects that have null as value.
There was a problem hiding this comment.
Oh, right. So effectively, this removes any non-project events for non-admins.
| if (isSingleParam) { | ||
| query.whereNot(param.field, param.values[0]); | ||
| } else { | ||
| query.whereNotIn(param.field, param.values); | ||
| } |
There was a problem hiding this comment.
Is this necessary? A list with a single list is still a lilt, right? So is not in [a] and is not in [a, b] should both be valid?
There was a problem hiding this comment.
Yes, it is because it is doing following sql query:
not in (x), and if you put in null for project , not in (null), then it will not work, because SQL things.
| await this.eventService.storeEvent( | ||
| new SegmentCreatedEvent({ | ||
| data: segment, | ||
| project: segment.project || 'no-project', | ||
| project: project, |
There was a problem hiding this comment.
Not sure why you're extracting the variable here. It's only used once, and I don't think this adds any more clarity?
But if you want to do it, we can at least avoid doubling up:
| project: project, | |
| project, |
Your call.
src/lib/types/events.ts
Outdated
There was a problem hiding this comment.
Good type clarification here. This is just making the types more accurate to how we actually use them, right?
| project: 'default', | ||
| createdBy: '[email protected]', | ||
| createdByUserId: TEST_USER_ID + 1, | ||
| ip: '127.0.0.1', | ||
| }); | ||
|
|
||
| await eventService.storeEvent({ | ||
| type: FEATURE_CREATED, | ||
| project: 'default', | ||
| createdBy: '[email protected]', |
There was a problem hiding this comment.
Interesting: did this test not work without the project? If so, why not?
There was a problem hiding this comment.
Actually since feature created always has project, wanted to make it legit.
| @@ -222,6 +229,14 @@ export default class EventService { | |||
| async getEventCreators() { | |||
| return this.eventStore.getEventCreators(); | |||
| } | |||
|
|
|||
| async getProjectFilterForNonAdmins(userId: number): Promise<IQueryParam[]> { | |||
| const isRootAdmin = await this.accessReadModel.isRootAdmin(userId); | |||
There was a problem hiding this comment.
Oh, right. So effectively, this removes any non-project events for non-admins.
| @@ -222,6 +229,14 @@ export default class EventService { | |||
| async getEventCreators() { | |||
| return this.eventStore.getEventCreators(); | |||
| } | |||
|
|
|||
| async getProjectFilterForNonAdmins(userId: number): Promise<IQueryParam[]> { | |||
There was a problem hiding this comment.
Small suggestion here: the naming was a little unclear to me at first, because it doesn't seem like we know whether the user is an admin or not at this point. If you read the method, you'll see that it's checked in there, though.
I'm not sure I have any better suggestions, but maybe getRoleBasedFilters would work? I ... dunno 🤷🏼
Now events that do not have project ( for example user creation, segment creation etc), will not be displayed to non root admins.