authz: Make root project/org no longer special #1092

JAORMX · 2023-10-03T12:52:11Z

This instead relies on keycloak claims to determine if a user is a member of staff
or not. Thus removing the "specialness" from the root project and organization.

Note that this is simply an intermediary step before we start removing the superadmin user entirely.

This instead relies on keycloak claims to determine if a user is a member of staff or not. Thus removing the "specialness" from the root project and organization.

evankanderson

Regardless of whether we remove superadmins entirely, I think making it a bool on the claims is an improvement.

For testing if nothing else...

evankanderson · 2023-10-03T16:38:14Z

database/migrations/000001_init.up.sql

@@ -307,20 +307,5 @@ CREATE TRIGGER update_policy_status
    EXECUTE PROCEDURE update_policy_status();

 -- Create default root organization and get id so we can create the root project


Where do we create the root project? Also, what does "root project" mean here? It's it just a name?

(The previous root org was special, but actually didn't normally have any useful groups / projects in it.)

It means nothing, I could just delete this actually. We could use a root project if we wanted the phantom root we talked about, but maybe we should just remove what we don't use.

evankanderson · 2023-10-03T16:41:06Z

internal/controlplane/handlers_authz.go

+
+func containsSuperadminRole(openIdToken openid.Token) bool {
+	if realmAccess, ok := openIdToken.Get("realm_access"); ok {
+		if realms, ok := realmAccess.(map[string]interface{}); ok {


Does it make sense to cast to map[string] [] string once here, and avoid the subsequent casts? (You can also use the default map lookup property then...)

This was just copy pasta, I guess we could do that

Added a suggestion so you can compare.

evankanderson · 2023-10-03T16:43:01Z

internal/controlplane/handlers_keys_test.go

@@ -48,10 +49,11 @@ func TestKeysHandler(t *testing.T) {

 	ctx := auth.WithPermissionsContext(context.Background(), auth.UserPermissions{
 		UserId:         1,
-		OrganizationId: rootOrganization,
-		ProjectIds:     []uuid.UUID{rootProject},
+		OrganizationId: orgID,


Do you need these, or just the super admin claim?

I just wanted it to work, some of these will probably be deleted later.

evankanderson · 2023-10-03T16:43:46Z

internal/controlplane/handlers_keys_test.go

-		ProjectIds:     []uuid.UUID{rootProject},
+		OrganizationId: orgID,
+		ProjectIds:     []uuid.UUID{projectID},
+		IsStaff:        true, // TODO: remove this
 		Roles: []auth.RoleInfo{


Do we need these roles, either?

I'd like to remove this structure in the same PR that would remove the roles db table

Sounds good, though I was going to suggest refactoring to have a func testSuperAdmin() auth.UserPermissions method to centralize this throughout all the tests. We might also have func testOrgAdmin(org string) and func testNormalUser(project string) methods to encapsulate this.

evankanderson · 2023-10-03T16:48:05Z

internal/controlplane/handlers_user.go

-		if err != nil {
-			return nil, status.Errorf(codes.Internal, "failed to create default organization records: %s", err)
-		}
+	// otherwise self-enroll user, by creating a new org and project and making the user an admin of those


Does "otherwise" make sense here?

Gotta remove this old comment

evankanderson

Reflecting further, it actually seems like it might be a good pattern if superadmins didn't actually have organizations -- it would discourage casual use of the super-powered token (and presumably reduce the risk of losing one).

evankanderson · 2023-10-03T19:21:36Z

internal/controlplane/handlers_authz.go

+
+func containsSuperadminRole(openIdToken openid.Token) bool {
+	if realmAccess, ok := openIdToken.Get("realm_access"); ok {
+		if realms, ok := realmAccess.(map[string]interface{}); ok {


Added a suggestion so you can compare.

evankanderson · 2023-10-03T19:23:13Z

internal/controlplane/handlers_authz.go

+		if realms, ok := realmAccess.(map[string]interface{}); ok {
+			if roles, ok := realms["roles"]; ok {
+				if userRoles, ok := roles.([]interface{}); ok {
+					if slices.Contains(userRoles, "superadmin") {
+						return true
+					}
+				}
+			}


Suggested change

if realms, ok := realmAccess.(map[string]interface{}); ok {

if roles, ok := realms["roles"]; ok {

if userRoles, ok := roles.([]interface{}); ok {

if slices.Contains(userRoles, "superadmin") {

return true

}

}

}

if realms, ok := realmAccess.(map[string][]string); ok {

return slices.Contains(realms["roles"], "superadmin")

For some reason, the proposal didn't quite work and results in superuser calls always getting permission denied. I'm not gonna spend too much time on optimizing this as we hope to get rid of this in the near future.

evankanderson · 2023-10-03T19:26:03Z

internal/controlplane/handlers_keys_test.go

-		ProjectIds:     []uuid.UUID{rootProject},
+		OrganizationId: orgID,
+		ProjectIds:     []uuid.UUID{projectID},
+		IsStaff:        true, // TODO: remove this
 		Roles: []auth.RoleInfo{


Sounds good, though I was going to suggest refactoring to have a func testSuperAdmin() auth.UserPermissions method to centralize this throughout all the tests. We might also have func testOrgAdmin(org string) and func testNormalUser(project string) methods to encapsulate this.

JAORMX changed the title ~~authz: Make root project/org no longer special~~ WIP: authz: Make root project/org no longer special Oct 3, 2023

JAORMX force-pushed the staff-user branch from 0d8bc45 to 9243e3f Compare October 3, 2023 13:58

JAORMX requested review from eleftherias and lukehinds October 3, 2023 13:58

authz: Make root project/org no longer special

42c755f

This instead relies on keycloak claims to determine if a user is a member of staff or not. Thus removing the "specialness" from the root project and organization.

JAORMX force-pushed the staff-user branch from 9243e3f to 42c755f Compare October 3, 2023 14:00

JAORMX requested a review from evankanderson October 3, 2023 14:00

JAORMX changed the title ~~WIP: authz: Make root project/org no longer special~~ authz: Make root project/org no longer special Oct 3, 2023

evankanderson reviewed Oct 3, 2023

View reviewed changes

evankanderson approved these changes Oct 3, 2023

View reviewed changes

JAORMX merged commit 84f4f41 into main Oct 4, 2023
12 checks passed

JAORMX deleted the staff-user branch October 4, 2023 06:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

authz: Make root project/org no longer special #1092

authz: Make root project/org no longer special #1092

JAORMX commented Oct 3, 2023 •

edited

Loading

evankanderson left a comment •

edited

Loading

evankanderson Oct 3, 2023

JAORMX Oct 3, 2023

evankanderson Oct 3, 2023

JAORMX Oct 3, 2023

evankanderson Oct 3, 2023

evankanderson Oct 3, 2023

JAORMX Oct 3, 2023

JAORMX Oct 3, 2023

evankanderson Oct 3, 2023

JAORMX Oct 3, 2023

evankanderson Oct 3, 2023

evankanderson Oct 3, 2023

JAORMX Oct 3, 2023

evankanderson left a comment

evankanderson Oct 3, 2023

evankanderson Oct 3, 2023

JAORMX Oct 4, 2023

evankanderson Oct 3, 2023

		@@ -307,20 +307,5 @@ CREATE TRIGGER update_policy_status
		EXECUTE PROCEDURE update_policy_status();

		-- Create default root organization and get id so we can create the root project

authz: Make root project/org no longer special #1092

authz: Make root project/org no longer special #1092

Conversation

JAORMX commented Oct 3, 2023 • edited Loading

evankanderson left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evankanderson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JAORMX commented Oct 3, 2023 •

edited

Loading

evankanderson left a comment •

edited

Loading