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

Allowing non-admin users to create dashboards. #4155

Merged
merged 14 commits into from Oct 10, 2017

Conversation

Projects
None yet
3 participants
@dennisoelkers
Member

dennisoelkers commented Sep 15, 2017

Description

Motivation and Context

Before this change, non-admin users possessing the dashboards:create permission were able to create new dashboards, but were never able to access them if they do not have the global dashboards:read permission (which is not desired in many cases, because it allows users to access all dashboards).

With this change, users creating a dashboard which lack the permission to read/edit that dashboard get the required permissions assigned dynamically. Upon dashboard deletion the user list is traversed and permissions for this specific dashboard are revoked.

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)
  • 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.
  • All new and existing tests passed.

@dennisoelkers dennisoelkers added this to the 2.4.0 milestone Sep 15, 2017

@bernd bernd self-assigned this Sep 28, 2017

@bernd

This comment has been minimized.

Member

bernd commented Sep 28, 2017

@dennisoelkers I am getting the following error when navigating to the dashboards page.

TypeError: this.state.dashboards is undefined
Stack trace:
render@webpack:///./src/components/dashboard/DashboardListPage.jsx?:79:9
_renderValidatedComponentWithoutOwnerOrContext/renderedElement<@http://localhost:8080/vendor.js:28541:16
measureLifeCyclePerf@http://localhost:8080/vendor.js:27821:12
_renderValidatedComponentWithoutOwnerOrContext@http://localhost:8080/vendor.js:28540:25
_renderValidatedComponent@http://localhost:8080/vendor.js:28567:27
performInitialMount@http://localhost:8080/vendor.js:28107:25
mountComponent@http://localhost:8080/vendor.js:28003:16
mountComponent@http://localhost:8080/vendor.js:7008:18
performInitialMount@http://localhost:8080/vendor.js:28116:18
mountComponent@http://localhost:8080/vendor.js:28003:16
mountComponent@http://localhost:8080/vendor.js:7008:18
performInitialMount@http://localhost:8080/vendor.js:28116:18
mountComponent@http://localhost:8080/vendor.js:28003:16
mountComponent@http://localhost:8080/vendor.js:7008:18
_updateRenderedComponent@http://localhost:8080/vendor.js:28510:24
_performComponentUpdate@http://localhost:8080/vendor.js:28469:5
updateComponent@http://localhost:8080/vendor.js:28390:7
performUpdateIfNecessary@http://localhost:8080/vendor.js:28306:7
performUpdateIfNecessary@http://localhost:8080/vendor.js:7119:5
runBatchedUpdates@http://localhost:8080/vendor.js:5730:5
perform@http://localhost:8080/vendor.js:8787:13
perform@http://localhost:8080/vendor.js:8787:13
perform@http://localhost:8080/vendor.js:5669:12
flushBatchedUpdates@http://localhost:8080/vendor.js:5752:7
closeAll@http://localhost:8080/vendor.js:8853:11
perform@http://localhost:8080/vendor.js:8800:11
batchedUpdates@http://localhost:8080/vendor.js:31760:14
enqueueUpdate@http://localhost:8080/vendor.js:5780:5
enqueueUpdate@http://localhost:8080/vendor.js:10103:3
enqueueSetState@http://localhost:8080/vendor.js:10297:5
__vendor</</ReactComponent.prototype.setState@http://localhost:8080/vendor.js:68980:3
module.exports/desc.componentDidMount/<@webpack:///./node_modules/react-proxy-loader/mixinReactProxy.js?:20:6
loadComponent/<@webpack:///./src/pages/DashboardsPage.jsx?:8:18
run@webpack:///./node_modules/core-js/modules/es6.promise.js?:66:22
notify/<@webpack:///./node_modules/core-js/modules/es6.promise.js?:79:30

@dennisoelkers dennisoelkers force-pushed the issue-4137-master branch from d0dd71e to cba7292 Sep 29, 2017

@dennisoelkers

This comment has been minimized.

Member

dennisoelkers commented Sep 29, 2017

I cannot reproduce the problem. Did you restart the webpack-dev-server process before trying it? It is necessary when just switching branches, because it keeps a stale reference to DashboardsStore.ts, which was deleted.

@bernd

This comment has been minimized.

Member

bernd commented Sep 29, 2017

@dennisoelkers Yep, I restarted the dev server and also removed all node modules and installed them again. I also tested with an artifact build and got a different issue. (tested it twice)

image

@bernd

This comment has been minimized.

Member

bernd commented Sep 29, 2017

I am also getting the following when loading a page. (even on the login page)

image

@bernd

This comment has been minimized.

Member

bernd commented Sep 29, 2017

And on another machine I get the following with a production build. Something is wrong... 😕

image

@bernd

This comment has been minimized.

Member

bernd commented Oct 2, 2017

@dennisoelkers I am getting the following error when navigating to "System/Authentication/Roles":

Invariant Violation: mergeIntoWithNoDuplicateKeys(): Tried to merge two objects with the same key: `dashboards`. This conflict may be due to a mixin; in particular, this may be caused by two getInitialState() or getDefaultProps() methods returning objects with clashing keys.
Stack trace:
invariant@http://localhost:8080/vendor.js:39546:15
mergeIntoWithNoDuplicateKeys@http://localhost:8080/vendor.js:69331:9
mergedResult@http://localhost:8080/vendor.js:69365:7
createClass/Constructor<@http://localhost:8080/vendor.js:69553:49
_constructComponentWithoutOwner/<@http://localhost:8080/vendor.js:27875:18
measureLifeCyclePerf@http://localhost:8080/vendor.js:27656:12
_constructComponentWithoutOwner@http://localhost:8080/vendor.js:27874:16
_constructComponent@http://localhost:8080/vendor.js:27865:14
mountComponent@http://localhost:8080/vendor.js:27768:16
mountComponent@http://localhost:8080/vendor.js:6985:18
mountChildren@http://localhost:8080/vendor.js:32283:28
_createInitialChildren@http://localhost:8080/vendor.js:29303:27
mountComponent@http://localhost:8080/vendor.js:29122:7
mountComponent@http://localhost:8080/vendor.js:6985:18
performInitialMount@http://localhost:8080/vendor.js:27951:18
mountComponent@http://localhost:8080/vendor.js:27838:16
mountComponent@http://localhost:8080/vendor.js:6985:18
mountChildren@http://localhost:8080/vendor.js:32283:28
_createInitialChildren@http://localhost:8080/vendor.js:29303:27
mountComponent@http://localhost:8080/vendor.js:29122:7
mountComponent@http://localhost:8080/vendor.js:6985:18
performInitialMount@http://localhost:8080/vendor.js:27951:18
mountComponent@http://localhost:8080/vendor.js:27838:16
mountComponent@http://localhost:8080/vendor.js:6985:18
performInitialMount@http://localhost:8080/vendor.js:27951:18
mountComponent@http://localhost:8080/vendor.js:27838:16
mountComponent@http://localhost:8080/vendor.js:6985:18
performInitialMount@http://localhost:8080/vendor.js:27951:18
mountComponent@http://localhost:8080/vendor.js:27838:16
mountComponent@http://localhost:8080/vendor.js:6985:18
_updateRenderedComponent@http://localhost:8080/vendor.js:28345:24
_performComponentUpdate@http://localhost:8080/vendor.js:28304:5
updateComponent@http://localhost:8080/vendor.js:28225:7
performUpdateIfNecessary@http://localhost:8080/vendor.js:28141:7
performUpdateIfNecessary@http://localhost:8080/vendor.js:7096:5
runBatchedUpdates@http://localhost:8080/vendor.js:5716:5
perform@http://localhost:8080/vendor.js:8748:13
perform@http://localhost:8080/vendor.js:8748:13
perform@http://localhost:8080/vendor.js:5655:12
flushBatchedUpdates@http://localhost:8080/vendor.js:5738:7
closeAll@http://localhost:8080/vendor.js:8814:11
perform@http://localhost:8080/vendor.js:8761:11
batchedUpdates@http://localhost:8080/vendor.js:31565:14
enqueueUpdate@http://localhost:8080/vendor.js:5766:5
enqueueUpdate@http://localhost:8080/vendor.js:10042:3
enqueueSetState@http://localhost:8080/vendor.js:10236:5
__vendor</</ReactComponent.prototype.setState@http://localhost:8080/vendor.js:68668:3
module.exports/desc.componentDidMount/<@webpack:///./node_modules/react-proxy-loader/mixinReactProxy.js?:20:6
loadComponent/<@webpack:///./src/pages/RolesPage.jsx?:8:18
run@webpack:///./node_modules/core-js/modules/es6.promise.js?:66:22
notify/<@webpack:///./node_modules/core-js/modules/es6.promise.js?:79:30
flush@webpack:///./node_modules/core-js/modules/_microtask.js?:18:9
@dennisoelkers

This comment has been minimized.

Member

dennisoelkers commented Oct 4, 2017

Thanks! Addressed that in f06eff9.

@bernd bernd removed their assignment Oct 4, 2017

dennisoelkers added some commits Sep 13, 2017

Adding privileges to user for created dashboard if not admin.
Before this change, a user with sufficient privileges to create a
dashboard could create one which is immediately inaccessible to the user
because the user is missing read/edit privileges to it. This change
checks for the required privileges and adds them to this specific
dashboard if necessary.

(cherry picked from commit 0e41dc7)
Converting DashboardsStore to reflux store.
When adding dashboard permissions to non-admin users dynamically on
dashboard creation, the current user needs to be reloaded. With the old
DashboardsStore this lead to timing issue/race conditions that prevented
new dashboards to show up instantly in most cases.

This change is now converting the DashboardsStore to a Reflux store
similar3 to most other stores, which is also refreshing its state when
the CurrentUser store's state changes and triggers a reload of the
current user upon dashboard creation.

(cherry picked from commit f2a805d)
Adding license header.
(cherry picked from commit 2980f76)
Adding tests for DashboardsResource changes.
(cherry picked from commit b09123c)
Adding license header.
(cherry picked from commit 8f2c89e)
Removing store hydration call, retrieving current user is done async.
The call to `CurrentUserStore.get()` could return `undefined` in case
the `CurrentUserStore` has not finished updating yet. Retrieving the
current user's permission set is done anyway, when the
`CurrentUserStore` is updated through the `currentUserUpdated` function.
Until then, an empty permission set is assumed.

@joschi joschi self-assigned this Oct 10, 2017

joschi added some commits Oct 10, 2017

Fix permissions check in PermissionsMixin
According to https://shiro.apache.org/permissions.html, the permission "foo:bar:*" is identical to "foo:bar"
which wasn't previously taken into account.

@joschi joschi force-pushed the issue-4137-master branch from f06eff9 to 8fe6434 Oct 10, 2017

@joschi

joschi approved these changes Oct 10, 2017

@joschi joschi merged commit 969a993 into master Oct 10, 2017

5 checks passed

ci-web-linter Jenkins build graylog-pr-linter-check 1992 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
graylog-project/pr Jenkins build graylog-project-pr-snapshot 582 has succeeded
Details
license/cla Contributor License Agreement is signed.
Details

@wafflebot wafflebot bot removed the ready-for-review label Oct 10, 2017

@joschi joschi deleted the issue-4137-master branch Oct 10, 2017

joschi added a commit that referenced this pull request Oct 10, 2017

Allow normal users to create dashboards (#4155)
* Adding privileges to user for created dashboard if not admin.

Before this change, a user with sufficient privileges to create a
dashboard could create one which is immediately inaccessible to the user
because the user is missing read/edit privileges to it. This change
checks for the required privileges and adds them to this specific
dashboard if necessary.

* Converting DashboardsStore to reflux store.

When adding dashboard permissions to non-admin users dynamically on
dashboard creation, the current user needs to be reloaded. With the old
DashboardsStore this lead to timing issue/race conditions that prevented
new dashboards to show up instantly in most cases.

This change is now converting the DashboardsStore to a Reflux store
similar3 to most other stores, which is also refreshing its state when
the CurrentUser store's state changes and triggers a reload of the
current user upon dashboard creation.

* Removing store hydration call, retrieving current user is done async.

The call to `CurrentUserStore.get()` could return `undefined` in case
the `CurrentUserStore` has not finished updating yet. Retrieving the
current user's permission set is done anyway, when the
`CurrentUserStore` is updated through the `currentUserUpdated` function.
Until then, an empty permission set is assumed.

* Fix permissions check in PermissionsMixin

According to https://shiro.apache.org/permissions.html, the permission "foo:bar:*" is identical to "foo:bar" which wasn't previously taken into account.

(cherry picked from commit 969a993)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment