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 permission checks for non admin users #2366

Merged
merged 3 commits into from Jun 17, 2016

Conversation

Projects
None yet
2 participants
@edmundoa
Member

edmundoa commented Jun 16, 2016

Some components were using wrong permission values in checks, so non-admin users could never get to do those actions.

Original issue: #2358

The changes should also be merged into 2.0, I already checked they apply cleanly.

@edmundoa edmundoa added this to the 2.0.3 milestone Jun 16, 2016

@dennisoelkers dennisoelkers self-assigned this Jun 16, 2016

Fix permission checks for non admin users
Some components were using wrong permission values, so non-admin users
could never get to do those actions.

Refs #2358

@edmundoa edmundoa force-pushed the issue-2358 branch from b958d7b to c2fba39 Jun 16, 2016

@dennisoelkers

This comment has been minimized.

Member

dennisoelkers commented Jun 16, 2016

Could you please replace the {this.isPermitted(...) && pattern with usages of the <IfPermitted /> component? Makes the code much more readable and maintainable imo.

@edmundoa

This comment has been minimized.

Member

edmundoa commented Jun 16, 2016

Sure thing, I was looking so much into writing the right permissions that I didn't even look at that 👍

edmundoa added some commits Jun 16, 2016

Fix another couple of broken permissions
This time they were using the `IfPermitted` component
Replace isPermitted with IfPermitted
Use IfPermitted in previously changed files when possible.
@edmundoa

This comment has been minimized.

Member

edmundoa commented Jun 16, 2016

I have replaced most of the isPermitted in touched files with IfPermitted. I found some issues doing so in the navigation bar dropdown, as IfPermitted is swallowing additional props that react-bootstrap is giving to it.

We could clone the children elements given to IfPermitted to pass the props down, but I don't feel confident on doing so if the changes are going to 2.0.3, as we don't know if they affect other components or have any performance impact. I'm willing to make that change in master afterwards, as we can see the impact in a longer term. How does that sound?

@dennisoelkers dennisoelkers merged commit bb5a1ab into master Jun 17, 2016

4 checks passed

ci-server-integration Jenkins build graylog2-server-integration-pr 995 has succeeded
Details
ci-web-linter Jenkins build graylog-pr-linter-check 481 has succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

dennisoelkers added a commit that referenced this pull request Jun 17, 2016

Fix permission checks for non admin users (#2366)
* Fix permission checks for non admin users

Some components were using wrong permission values, so non-admin users
could never get to do those actions.

Refs #2358

* Fix another couple of broken permissions

This time they were using the `IfPermitted` component

* Replace isPermitted with IfPermitted

Use IfPermitted in previously changed files when possible.

(cherry picked from commit bb5a1ab)

@dennisoelkers dennisoelkers deleted the issue-2358 branch Jun 17, 2016

edmundoa added a commit that referenced this pull request Jul 26, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment