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

Remove "allow all users" ability #2721

Closed
vito opened this issue Oct 23, 2018 · 2 comments

Comments

@vito
Copy link
Member

@vito vito commented Oct 23, 2018

What challenge are you facing?

It's currently possible to configure --main-team-allow-all-users (and --allow-all-users on set-team), which will do what it says on the tin: allow literally everyone that can log in to be a member of the team. This is almost certainly a bad idea in production scenarios, and we really don't have a strong need for it during development either, since we can just list the test user.

What would make this better?

Let's just remove it. It broke with #2670 anyway.

Are you interested in implementing this yourself?

Nah, y'all do it.

pivotal-jwinters added a commit that referenced this issue Oct 24, 2018
#2721

Signed-off-by: Divya Dadlani <ddadlani@pivotal.io>
@xtremerui

This comment has been minimized.

Copy link
Contributor

@xtremerui xtremerui commented Nov 28, 2018

We need to consider migration of teams table since if someone config a team with --allow-all-users, the auth would be
{"owner":{"groups":[],"users":[]}} after the migration ran as in

BEGIN;
UPDATE teams SET auth=json_build_object('owner', auth::json);
COMMIT;

With this change, it is impossible to config a team to allow all users access anymore. Then this value would be invalid.

Solutions could be:

  1. Copy the auth config from main team so admin users in main team could have access to this specific team at least. But problem is, if main team was configured with --allow-all-users, the auth would be also invalid.
  2. Fail the migration by forcing user to reconfigure the team to has auth config without --allow-all-users
@vito

This comment has been minimized.

Copy link
Member Author

@vito vito commented Nov 28, 2018

@xtremerui maybe we should just permit existing configurations to work as they did before (but still prohibit configuring it), and fix the behavior which was broken with #2670?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.