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

Create org, group, provider and role per user #1059

Merged
merged 1 commit into from
Oct 2, 2023

Conversation

eleftherias
Copy link
Contributor

Ref #691

When a new user logs in, we can create an org, group, provider and admin role for them.
This gives them permission to perform all operations within their own org.

@@ -132,6 +84,65 @@ func (s *Server) CreateOrganization(ctx context.Context,
return response, nil
}

// CreateDefaultRecordsForOrg creates the default records, such as groups, roles and provider for the organization
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I want to extract the common code for creating default records for an organization, since it's used in multiple places. I was between keeping it in this file because it relates to organizations, or moving it to common.go.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no preference, but it makes sense to move it.

// if the token has superadmin access to the realm, then make give them a superadmin role in the DB
userOrg = 1
userGroup = 1
userRoles = append(userRoles, 1)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is a little confusing (and maybe we just had a funky behavior from before)... So, if the realm_access claim is in the token (which we check via the containsSuperadminRole function), we overwrite the user and org because those magic numbers pertain to the superadmin role in the DB?

If we were to fix this behavior, would it make sense instead to just do RBAC checks for the superadmin role when trying to execute an action instead of relying on magic numbers?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In other words if the realm_access claim contains the role superadmin, then we don't create a new org, group & role for the user, but instead add them to the existing superadmin org, group & role.

would it make sense instead to just do RBAC checks for the superadmin role when trying to execute an action

My concern with this is that it would give us 2 sources of truth for RBAC. We would be checking the orgs, groups, roles in the DB and then also checkin the claims in the token. By assigning the superadmin DB role on user creation, we keep all RBAC in the DB and use the JWT only for authentication purposes.

Related, I've explored moving RBAC fully to the IdP but it would tightly couple us to the specific IdP implementation (in this case Keycloak), and I think it's too early to commit to that.

@eleftherias eleftherias force-pushed the self-enroll branch 2 times, most recently from 0a67ba7 to 5fc0f48 Compare October 2, 2023 09:59
@JAORMX
Copy link
Contributor

JAORMX commented Oct 2, 2023

LGTM, shipit! 🚢

@eleftherias eleftherias merged commit 745c119 into mindersec:main Oct 2, 2023
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants