/graylog2-server

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

Migrate user permissions to roles #1389

Merged
merged 4 commits into from Sep 3, 2015

Conversation

Reviewers
No reviews
Assignees

@joschi joschi

Labels
ready-for-review
Projects
None yet
Milestone
  1.2.0
2 participants
@kroepke @joschi
@kroepke
Member

kroepke commented Sep 2, 2015

add periodical to migrate user's permissions to admin or reader roles during startup of the master

  • existing stream and dashboard permissions are preserved
  • existing other permissions set via the API are preserved
  • RestPermissions.readerPermissions are migrated to reader role
  • '*' is migrated to admin role

fixes #1382
@kroepke
add periodical to migrate user's permissions to admin or reader roles… 
… during startup of the master

 - existing stream and dashboard permissions are preserved
 - existing other permissions set via the API are preserved
 - RestPermissions.readerPermissions are migrated to reader role
 - '*' is migrated to admin role

fixes #1382
Loading status checks…
349fdd9

@kroepke kroepke added the ready-for-review label Sep 2, 2015

@kroepke kroepke added this to the 1.2.0 milestone Sep 2, 2015

@joschi joschi self-assigned this Sep 2, 2015

graylog2-server/src/main/java/org/graylog2/periodical/UserPermissionMigrationPeriodical.java Outdated
final Set<String> fixedPermissions = Sets.newHashSet();
final Set<String> fixedRoleIds = Sets.newHashSet(user.getRoleIds());
final HashSet<String> permissionSet = Sets.newHashSet(user.getPermissions());

This comment has been minimized.

Sign in to view
Copy link
@joschi

joschi Sep 2, 2015

Contributor

Minor nitpick: Why declare the variable as HashSet (implementation) instead of Set (interface)?

This comment has been minimized.

Sign in to view
Copy link
@kroepke

kroepke Sep 2, 2015

Member

because alt-cmd-v did it.
i'll change it (how can i make intellij make a better choice!?)
graylog2-server/src/main/java/org/graylog2/periodical/UserPermissionMigrationPeriodical.java
final String adminRoleId = roleService.getAdminRoleObjectId();
final String readerRoleId = roleService.getReaderRoleObjectId();
for (User user : users) {

This comment has been minimized.

Sign in to view
Copy link
@joschi

joschi Sep 2, 2015

Contributor

So the migration process will always run on start. Is there some easy way to just run it only once?

This comment has been minimized.

Sign in to view
Copy link
@joschi

joschi Sep 2, 2015

Contributor

For example you could store a simple boolean in MongoDB using ClusterConfigService.

This comment has been minimized.

Sign in to view
Copy link
@kroepke

kroepke Sep 2, 2015

Member

will do, even though i think it will not make much of a difference, because it won't touch users after they are migrated.
still might be confusing people on debug level

This comment has been minimized.

Sign in to view
Copy link
@joschi

joschi Sep 3, 2015

Contributor

✅
graylog2-server/src/main/java/org/graylog2/periodical/UserPermissionMigrationPeriodical.java Outdated
fixedRoleIds.add(readerRoleId);
}
// filter out the individual permissions to dashboards and streams
final ArrayList<String> dashboardStreamPermissions = Lists.newArrayList(

This comment has been minimized.

Sign in to view
Copy link
@joschi

joschi Sep 2, 2015

Contributor

Minor nitpick: Why declare the variable as ArrayList (implementation) instead of List (interface)?
graylog2-server/src/main/java/org/graylog2/periodical/UserPermissionMigrationPeriodical.java Outdated
public boolean apply(
String permission) {
return !basePermissions.contains(
permission) && !permission.equals(

This comment has been minimized.

Sign in to view
Copy link
@joschi

joschi Sep 2, 2015

Contributor

This formatting looks weird.
graylog2-server/src/main/java/org/graylog2/periodical/UserPermissionMigrationPeriodical.java Outdated
final UserPermissionMigrationState migrationState =
clusterConfigService.getOrDefault(UserPermissionMigrationState.class,
UserPermissionMigrationState.create(false));
if (migrationState.migrationDone()) {

This comment has been minimized.

Sign in to view
Copy link
@joschi

joschi Sep 2, 2015

Contributor

We could probably just move this into the startOnThisNode() method and return !migrationState.migrationDone(), which would just not run the periodical in clusters which have already been migrated.

This comment has been minimized.

Sign in to view
Copy link
@kroepke

kroepke Sep 2, 2015

Member

JA 👍
@joschi

This comment has been minimized.

Sign in to view
Contributor

joschi commented Sep 3, 2015

LGTM. 👍

joschi added a commit that referenced this pull request Sep 3, 2015

@joschi
Merge pull request #1389 from Graylog2/issue-1382 
Migrate pre-1.2.0 user permissions to roles
Loading status checks…
460a121

@joschi joschi merged commit 460a121 into 1.2 Sep 3, 2015
3 checks passed

3 checks passed

ci Jenkins build graylog2-server-integration-pr 210 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

@joschi joschi deleted the issue-1382 branch Sep 3, 2015

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