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

do not blindly override permission set of ldap users #2529

Merged
merged 2 commits into from Jul 26, 2016
Merged

do not blindly override permission set of ldap users #2529

merged 2 commits into from Jul 26, 2016

Conversation

@kroepke
Copy link
Member

@kroepke kroepke commented Jul 25, 2016

when fixing #2045 we introduced a bug clearing the permission set for synced ldap accounts

this only shows up when not using roles but the permission sets directly

fixes #2516

@kroepke kroepke added this to the 2.1.0 milestone Jul 25, 2016
// Do not store the dynamic user self edit permissions
permissions.removeAll(this.permissions.userSelfEditPermissions(getName()));
perms.removeAll(this.permissions.userSelfEditPermissions(getName()));

This comment has been minimized.

This comment has been minimized.

@kroepke

kroepke Jul 25, 2016
Author Member

touché

This comment has been minimized.

@kroepke

kroepke Jul 25, 2016
Author Member

@@ -250,7 +249,8 @@ private void updateFromLdap(User user, LdapEntry userEntry, LdapSettings ldapSet
translatedRoleIds.addAll(user.getRoleIds());
}
user.setRoleIds(translatedRoleIds);
user.setPermissions(Collections.emptyList());
// preserve the raw permissions (the ones without the synthetic self-edit permissions or the "*" admin one)
user.setPermissions(user.getPermissions());

This comment has been minimized.

@joschi joschi self-assigned this Jul 26, 2016
@joschi
Copy link
Contributor

@joschi joschi commented Jul 26, 2016

LGTM. 👍

@joschi joschi merged commit 6e46f68 into master Jul 26, 2016
4 checks passed
4 checks passed
@garybot2
ci-server-integration Jenkins build graylog2-server-integration-pr 1154 has succeeded
Details
@garybot2
ci-web-linter Jenkins build graylog-pr-linter-check 639 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-2516 branch Jul 26, 2016
@angelsedoy
Copy link

@angelsedoy angelsedoy commented Jul 28, 2016

I'm sorry, but this code does not work, or rather not quite correct. Now permissions are not overwritten, but are overwritten role, in particular the role of flies admin.

P.S. I forgot to add

  •    final List<String> perms = Lists.newArrayList(permissions);
    
    2 P. S. Did not help
@joschi
Copy link
Contributor

@joschi joschi commented Jul 28, 2016

@angelsedoy Thanks for your feedback. Please create a new issue for this at https://github.com/Graylog2/graylog2-server/issues/new

dennisoelkers added a commit that referenced this pull request Apr 3, 2017
In #2529, while fixing #2516, persisting all permissions of roles
assigned to a user was reintroduced (fixed in
35c9325 before). This happened due to
the introduction of a temporary variable that was not used effctively.

After this change, LDAP permissions are not modified blindly but the
permission set of a user still stays denormalized when modified and
saved.
dennisoelkers added a commit that referenced this pull request Apr 3, 2017
In #2529, while fixing #2516, persisting all permissions of roles
assigned to a user was reintroduced (fixed in
35c9325 before). This happened due to
the introduction of a temporary variable that was not used effctively.

After this change, LDAP permissions are not modified blindly but the
permission set of a user still stays denormalized when modified and
saved.

(cherry picked from commit 1f2aeb5)
kroepke added a commit that referenced this pull request Apr 3, 2017
In #2529, while fixing #2516, persisting all permissions of roles
assigned to a user was reintroduced (fixed in
35c9325 before). This happened due to
the introduction of a temporary variable that was not used effctively.

After this change, LDAP permissions are not modified blindly but the
permission set of a user still stays denormalized when modified and
saved.

(cherry picked from commit 1f2aeb5)
kroepke added a commit that referenced this pull request Apr 3, 2017
In #2529, while fixing #2516, persisting all permissions of roles
assigned to a user was reintroduced (fixed in
35c9325 before). This happened due to
the introduction of a temporary variable that was not used effctively.

After this change, LDAP permissions are not modified blindly but the
permission set of a user still stays denormalized when modified and
saved.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants