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

Fix bug of event search not returning writable events for ROLE_ADMIN #1134

Merged
merged 6 commits into from
Mar 5, 2024

Conversation

LukasKalbertodt
Copy link
Member

See commits.

Previously, users with ROLE_ADMIN did only see listed videos. This is
a bug caused by using the `Option<Filter>` in a `Filter::Or`. Using
`Option` to omit the filter only makes sense inside `Filter::And`. Now
admins can see all events always as it should have been.
I fixed a bug a few commits ago and this is logic that got us our
first security vulnerability. So it's probably a good idea to test that.
@LukasKalbertodt LukasKalbertodt added the changelog:user User facing changes label Mar 5, 2024
@github-actions github-actions bot temporarily deployed to test-deployment-pr1134 March 5, 2024 15:20 Destroyed
@@ -110,5 +110,40 @@ test("Read access is checked", async ({ page, standardData, activeSearchIndex, b
});
});

for (const username of ["jose", "admin"] as const) {
test(`Video search finds listed & writable (for ${username})`, async ({
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if this should be in a separate file. I know I haven't set the best example for this, but I think it is generally encouraged to split these tests up into smaller packages and "domains", so to speak. Right now this mixes the general search (for the search page) with the searchableSelect things used for adding/editing content.

I would probably rename this file to searchPage.spec and do the unrelated tests either in blocks.spec or rather in a new file like searchableSelect.spec in the long run. So no need to change it in this PR (and maybe you disagree - this is something we can discuss at a later point).

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah that's a good point. Probably move it to its own file. Also because it can run different files in parallel.

frontend/src/i18n/locales/en.yaml Show resolved Hide resolved
@owi92 owi92 merged commit 2ddd3ca into elan-ev:master Mar 5, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog:user User facing changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants