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.

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

POC: Use stream categories and permissions for aggregation event definitions #20188

Closed

Conversation

kingzacko1
Copy link
Contributor

Adds a pseudo-SearchUser class EventProcessorSearchUser to use when adding streams to event processor searches. The class is used to respect stream permissions of the event definition owner when adding streams to event definitions with no streams assigned or with stream categories that need to be mapped to stream IDs.

/nocl just a POC for now

Description

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Refactoring (non-breaking change)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.

@danotorrey danotorrey self-requested a review August 16, 2024 21:23
Copy link
Contributor

@danotorrey danotorrey left a comment

Choose a reason for hiding this comment

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

@kingzacko1 This LGTM and tests successfully for both filter and aggregation events and with categories and standard streams. The approach looks good as far as I can see. I will do a little more testing with permissions on Monday morning.

I did not see any errors being logged about a missing header I think like you were mentioning. Are you still seeing those on your side?

@kingzacko1
Copy link
Contributor Author

@danotorrey did you create an event definition with a user other than admin? I think it needs to have ownership checks hitting the DB before those warnings pop up.

@danotorrey
Copy link
Contributor

@kingzacko1 I just tested with an event definition created by another user and still did not see the error. We can take a look after the high five if you have some time.

Base automatically changed from add-stream-category-filter to master August 26, 2024 16:51
@kingzacko1
Copy link
Contributor Author

I'm closing this for now. We will likely handle something like this in a more sweeping change to event definitions as a whole.

@kingzacko1 kingzacko1 closed this Oct 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants