I think you can remove this field, which also allows cleaning up a lot of cruft in userauth.go

Oh you are right. I think we can drop the CreateOrganization behavior entirely. I'll make a new PR that will look into dropping it.

should this also be part of the migration?

I thought about that too. What I didn't like about the migration was I can't tell if the INSERT INTO organizations ran with an insert, or without. So the followup of INSERT INTO groups cannot be dependent on if an org did not exist at the start of the migration or not.

Putting it in postFirstUser ensures that it is a new instance setup, so I know this is safe.

Although we do not allow dropping the everyone group, so it should be a safe INSERT INTO groups ... DO NOTHING.

I can move it to a migration if you feel that is better.

I'm just wary of all this side-effect stuff of creating a user.

I also kind of like the symmetry of creating an org and the corresponding everyone group together, since that's presumably what we'll do when we allow creating new organizations, since there is no similar "first user creation" event to tie it to.

I saw this comment after the approval merge, my mistake.

I can make another migration for the everyone group and do nothing on conflict.

Up to you

Ok, so a lot to unpack here.

1. I don't understand what this has to do with ensuring the default organization exists. Why is it necessary in this PR?

2. Why is it necessary to query AsSystemRestricted and then filter in this function? This kind of logic needs to live in the RBAC layer!

3. It looks like the logic is that if you can read the user's UserDataRBAC at global scope and are just a member of the organization, then you can see their roles in that organization. This is an example of shadow access control where we are computing a permission based on combinations of other permissions, and is going to be undocumented and hard to understand.

   The problem here is that we made UserDataRBAC a deployment-scoped resource, but some of the information is not actually global scope (organization roles). Fix the resource scoping issue, don't hack permissions out of other permissions.

I don't understand what this has to do with ensuring the default organization exists. Why is it necessary in this PR?

Yea, this came up as a result of this PR fixing up some unit tests that behavior changed. One of them was we used to insert the OrgMember() role explicitly. An oversight, as this query adds in implied roles automatically:

So GetAuthorizationUserRoles is the source of truth. This api call was just returning the []string field Roles, which is not correct. So this API call should be using GetAuthorizationUserRoles.

Why is it necessary to query AsSystemRestricted and then filter in this function? This kind of logic needs to live in the RBAC layer!

I can make the function no longer a system function, and filter in the dbauthz layer, but then all calls to this function get an added cost. Which happens on every single api request.

I think this is a system function as an optimization, since this is just a heavily used function.

It looks like the logic is that if you can read the user's UserDataRBAC at global scope and are just a member of the organization, then you can see their roles in that organization. This is an example of shadow access control where we are computing a permission based on combinations of other permissions, and is going to be undocumented and hard to understand.

It is. I can implement the filter in the dbauthz, but I didn't want to add the cost to everything. But I want to use the correct function for the source of truth for roles.

In general, our ability to relate perms is weak because the policy execution can only take 1 object as input at a time. So the relations are enforced outside of it.

For getting through this PR I could:

1. Just drop organization roles from this output. We don't use them anywhere since orgs are not really a concept atm. We can add another endpoint for listing their roles of a given org later.
2. Do this in dbauthz, and it would look something akin to this:

func (q *querier) GetAuthorizationUserRoles(ctx context.Context, userID uuid.UUID) (database.GetAuthorizationUserRolesRow, error) {
	// System context can read everything, so do not check any further permissions.
	systemCan := q.authorizeContext(ctx, rbac.ActionRead, rbac.ResourceSystem)
	if systemCan == nil {
		return q.db.GetAuthorizationUserRoles(ctx, userID)
	}

	// If the actor is a not a system, they might be able to read some of the roles.
	// Check if they can read the user.
	if err := q.authorizeContext(ctx, rbac.ActionRead,
		rbac.ResourceUserData.WithOwner(userID.String()).WithID(userID)); err != nil {
		return database.GetAuthorizationUserRolesRow{}, err
	}

	authorizationInfo, err := q.db.GetAuthorizationUserRoles(ctx, userID)
	if err != nil {
		return database.GetAuthorizationUserRolesRow{}, err
	}
	allowedRoles := make([]string, 0, len(authorizationInfo.Roles))

	// memberships is a map of orgID to whether the actor context can read the org membership.
	memberships := make(map[uuid.UUID]bool)
	for _, role := range authorizationInfo.Roles {
		if orgIDStr, ok := rbac.IsOrgRole(role); ok {
			orgID, err := uuid.Parse(orgIDStr)
			if err != nil {
				continue
			}

			// If we can read the org membership for this org, then we can read the role.
			// So include it.
			if _, ok := memberships[orgID]; !ok {
				canReadMem := q.authorizeContext(ctx, rbac.ActionRead, database.OrganizationMember{
					UserID:         userID,
					OrganizationID: orgID,
				})
				memberships[orgID] = canReadMem == nil
			}

			if memberships[orgID] {
				allowedRoles = append(allowedRoles, role)
			}
		} else {
			allowedRoles = append(allowedRoles, role)
		}
	}
	
	// TODO: authorizationInfo.Groups should also be filtered?
	authorizationInfo.Roles = allowedRoles
	return authorizationInfo, nil
}

Option 3, and the one I might just do. Is just not include the implied roles in the api response. The previous behavior was incorrectly omitting these, so this PR does not need to resolve this behavior.

I made an issue to fix this later: #12427

In general, our ability to relate perms is weak because the policy execution can only take 1 object as input at a time. So the relations are enforced outside of it.

It's actually the relations themselves that I also have a problem with. We've got this global-scoped permission: read UserDataRBAC that is granting access to org-scoped data via a tensor product with org membership. That makes it impossible to grant access just to the org-scoped data in a single org. If you have access to it in one org you have access to it in every org you are a member of.

It's just a confusing mess is you start allowing permissions to beget other permissions. Roles determine the permissions you have in RBAC. We need an org-scoped resource that represents the roles a user has in that org, and then to assign read permission on that resource to one or more roles in our system.

@spikecurtis Yes, this endpoint is incorrectly assuming UserDataRBAC should access the org roles. This is a mistake