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

auth: changes of managing roles and users #7524

Merged
merged 4 commits into from
Mar 23, 2017

Conversation

mitake
Copy link
Contributor

@mitake mitake commented Mar 17, 2017

  • The 1st commit adds a new flag --revoke to etcdctl role delete. If the flag is passed, the role will be revoked from all users. The 2nd commit adds an e2e test case for this.
  • The 3rd commit forbids deleting the role root and revoking the role from the user root if auth is enabled. The 4th commit adds an e2e test case for this.

@heyitsanthony
Copy link
Contributor

Is there a reason why roles should only be optionally revoked if deleted? UserGrantRole will reject non-existent roles, but letting users retain deleted roles defeats that check. Seems inconsistent?

@mitake
Copy link
Contributor Author

mitake commented Mar 17, 2017

@heyitsanthony yes... revoking the deleted roles unconditionally would be reasonable. I couldn't find or recall the discussion behind the current behavior, but it would have some reasons. If there's no motivations of keeping it, I agree with changing the behavior of the revoking. /cc @xiang90

@codecov-io
Copy link

codecov-io commented Mar 17, 2017

Codecov Report

❗ No coverage uploaded for pull request base (master@049ca87). Click here to learn what that means.
The diff coverage is 52.17%.

@@            Coverage Diff            @@
##             master    #7524   +/-   ##
=========================================
  Coverage          ?   70.28%           
=========================================
  Files             ?      324           
  Lines             ?    26387           
  Branches          ?        0           
=========================================
  Hits              ?    18545           
  Misses            ?     6370           
  Partials          ?     1472
Impacted Files Coverage Δ
etcdserver/api/v3rpc/rpctypes/error.go 100% <ø> (ø)
etcdserver/api/v3rpc/util.go 52.77% <0%> (ø)
auth/store.go 82.38% <57.14%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 049ca87...8d0d942. Read the comment docs.

This commit resolves a TODO of auth store:
Current scheme of role deletion allows existing users to have the
deleted roles. Assume a case like below:
create a role r1
create a user u1 and grant r1 to u1
delete r1

After this sequence, u1 is still granted the role r1. So if admin
create a new role with the name r1, The new r1 is automatically
granted u1. In some cases, it would be confusing. So we need to
revoke the deleted role from all users.
If auth is enabled,
1. deleting the user root
2. revoking the role root from the user root
must not be allowed. This commit forbids them.
@mitake
Copy link
Contributor Author

mitake commented Mar 23, 2017

@heyitsanthony I changed the default behaviour of the role deletion. Now it revokes the role from all users unconditionally. PTAL.

@heyitsanthony
Copy link
Contributor

lgtm. Thanks! Defer to @xiang90 on the new behavior. The old behavior seemed more like a bug than a desirable use case...

@xiang90
Copy link
Contributor

xiang90 commented Mar 23, 2017

lgtm

@xiang90 xiang90 merged commit 54928f5 into etcd-io:master Mar 23, 2017
@mitake mitake deleted the del-and-revoke-role branch March 24, 2017 00:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants