feat(auto-onboarding): Allow auto-onboarding on organizations #951

javirln · 2024-06-13T16:40:49Z

This patch introduces a new configuration option that enables a Chainloop instance to automatically onboard new users into predefined organizations with specific roles.

For example:

# config.devel.yaml

# Previous existing configuration

onboarding:
  - name: "read-only-demo"
    role: "viewer"
  - name: "read-write-demo"
    role: "owner"

With this configuration, new and existing users will be automatically onboarded into the read-only-demo and chainloop organizations with the roles of viewer and owner, respectively. When users log in to Chainloop for the first time, the server will:

Find or create the specified organizations
Create memberships for users in these organizations if they do not already exist. Existing memberships will not be modified.

Refs #942

Signed-off-by: Javier Rodriguez <javier@chainloop.dev>

migmartri

More feedback to come

app/controlplane/internal/conf/controlplane/config/v1/conf.proto

Signed-off-by: Javier Rodriguez <javier@chainloop.dev>

app/controlplane/internal/service/auth.go

migmartri

Some comments on code structuring and how to use what we already have.

Thanks!

Makefile

app/controlplane/configs/config.devel.yaml

app/controlplane/internal/conf/controlplane/config/v1/conf.proto

app/controlplane/internal/service/auth.go

app/controlplane/pkg/biz/organization.go

migmartri

requesting changes per our conversation offline, thanks

Signed-off-by: Javier Rodriguez <javier@chainloop.dev>

javirln · 2024-06-17T06:58:17Z

All changes addressed:

Moved logic to biz
Moved all integration tests to organization_integration_tests
Example of configuration the sample config
Created an alternative method of creating users otherwise integration tests won't make sense. Another option was to use a repo on the integration tests but, not super happy about it.

migmartri

Thanks Javi, looks good in general!

app/controlplane/internal/conf/controlplane/config/v1/conf.proto

app/controlplane/internal/service/user.go

app/controlplane/pkg/biz/organization.go

migmartri · 2024-06-17T07:35:08Z

app/controlplane/pkg/biz/organization.go

+// If the organization does not exist, it creates it.
+func (uc *OrganizationUseCase) ensureOrganizationExists(ctx context.Context, spec *conf.OnboardingSpec) (*Organization, error) {
+	// Ensure the organization exists or create it if it doesn't
+	org, err := uc.FindByName(ctx, spec.GetName())


why are you using the use-case here instead of the existing underlying repo? I'd say that this private method should use the private repo instead. It will also make error handling easier.

Removed the method and used the repos instead.

app/controlplane/pkg/biz/organization.go

app/controlplane/pkg/biz/organization_integration_test.go

migmartri · 2024-06-17T07:47:27Z

app/controlplane/pkg/biz/user.go

 	return uc.userRepo.Delete(ctx, userUUID)
 }

+func (uc *UserUseCase) CreateByEmail(ctx context.Context, email string) (*User, error) {


I have mixed feelings of creating methods just for testing (same about findbyname), it feels upside down to me.

In fact I think in this case this is wrong and can be confusing, it's not trivial to know that createbyemail will not add auto-onboarding.

Personally, I've have passed an extra optional argument WithDisableAutoOnboarding or make it part of the the userUsecase struct`

This additional methods were to ease the testing indeed since I didn't want to use the repos on the integration tests but well, seems we can do it as well.

I've added an optional parameter to the method to disable the auto onboarding. Please note that I didn't create a regular Builder with all Its options because I do believe it's too verbose for just one configuration.

Signed-off-by: Javier Rodriguez <javier@chainloop.dev>

javirln

I've removed the methods FinByName and CreateByEmail, the dependencies on the private methods and reworked on the tests

app/controlplane/internal/conf/controlplane/config/v1/conf.proto

app/controlplane/pkg/biz/organization.go

javirln · 2024-06-17T09:57:14Z

app/controlplane/pkg/biz/organization.go

+// If the organization does not exist, it creates it.
+func (uc *OrganizationUseCase) ensureOrganizationExists(ctx context.Context, spec *conf.OnboardingSpec) (*Organization, error) {
+	// Ensure the organization exists or create it if it doesn't
+	org, err := uc.FindByName(ctx, spec.GetName())


Removed the method and used the repos instead.

app/controlplane/pkg/biz/organization.go

javirln · 2024-06-17T10:00:41Z

app/controlplane/pkg/biz/user.go

 	return uc.userRepo.Delete(ctx, userUUID)
 }

+func (uc *UserUseCase) CreateByEmail(ctx context.Context, email string) (*User, error) {


This additional methods were to ease the testing indeed since I didn't want to use the repos on the integration tests but well, seems we can do it as well.

I've added an optional parameter to the method to disable the auto onboarding. Please note that I didn't create a regular Builder with all Its options because I do believe it's too verbose for just one configuration.

migmartri

LGTM!

migmartri · 2024-06-17T11:04:37Z

app/controlplane/pkg/biz/organization.go

+
+// ensureOrganizationExists ensures that an organization specified by the onboarding configuration exists.
+// If the organization does not exist, it creates it.
+func (uc *OrganizationUseCase) ensureOrganizationExists(ctx context.Context, spec *conf.OnboardingSpec) (*Organization, error) {


Not a blocker but in general I prefer to make the arguments as scooped down as possible. For example instead of the whole spec, here I think you just need the name string, and in the other one you need the role.

👍 I agree. It would be simpler to just pass the name of the org.

I agree with you but in this case, we need the whole spec, the expected roles as well, so effectively what's being passed in the name of the orgs and the roles it should be applied.

jiparis · 2024-06-17T11:27:18Z

app/controlplane/pkg/biz/organization.go

+
+// ensureOrganizationExists ensures that an organization specified by the onboarding configuration exists.
+// If the organization does not exist, it creates it.
+func (uc *OrganizationUseCase) ensureOrganizationExists(ctx context.Context, spec *conf.OnboardingSpec) (*Organization, error) {


👍 I agree. It would be simpler to just pass the name of the org.

jiparis · 2024-06-17T11:32:03Z

app/controlplane/pkg/biz/user.go

+// FindOrCreateByEmail finds or creates a user by email. By default, it will auto-onboard the user
+// to the organizations defined in the configuration. If disableAutoOnboarding is set to true, it will
+// skip the auto-onboarding process.
+func (uc *UserUseCase) FindOrCreateByEmail(ctx context.Context, email string, disableAutoOnboarding ...bool) (*User, error) {


Why ...bool? How many bools are we expecting here?

It's an optional parameter otherwise we would have to add that parameter to all occurrences.

jiparis · 2024-06-17T11:32:51Z

app/controlplane/pkg/biz/organization.go

+	org, err := uc.orgRepo.FindByName(ctx, spec.GetName())
+	if err != nil {
+		return nil, fmt.Errorf("failed to find organization: %w", err)
+	} else if org == nil {


I might be wrong, but I remember we talked offline about about not creating orgs, and assume they existed previously.

As far as I remember if it was because of the possible collisions it was solved on the DB. In any case we can always come back and remove it.

Maybe when we have #945 done.

True, I'd do it in another smaller follow up PR

jiparis

Thanks! Just a couple of minor comments.

feat(auto-onboarding): Allow auto-onboarding on organizations

f83d340

Signed-off-by: Javier Rodriguez <javier@chainloop.dev>

javirln requested review from jiparis and migmartri June 13, 2024 16:40

migmartri reviewed Jun 14, 2024

View reviewed changes

app/controlplane/internal/conf/controlplane/config/v1/conf.proto Outdated Show resolved Hide resolved

Flatten onboarding configuration

c42e655

Signed-off-by: Javier Rodriguez <javier@chainloop.dev>

jiparis reviewed Jun 14, 2024

View reviewed changes

app/controlplane/internal/service/auth.go Outdated Show resolved Hide resolved

migmartri reviewed Jun 14, 2024

View reviewed changes

migmartri requested changes Jun 14, 2024

View reviewed changes

javirln marked this pull request as draft June 14, 2024 11:27

javirln self-assigned this Jun 16, 2024

Tackle feedback

2c0bfe1

Signed-off-by: Javier Rodriguez <javier@chainloop.dev>

javirln marked this pull request as ready for review June 17, 2024 06:49

migmartri reviewed Jun 17, 2024

View reviewed changes

javirln marked this pull request as draft June 17, 2024 09:30

Tackle feedback

856e745

Signed-off-by: Javier Rodriguez <javier@chainloop.dev>

javirln commented Jun 17, 2024

View reviewed changes

javirln marked this pull request as ready for review June 17, 2024 10:00

migmartri approved these changes Jun 17, 2024

View reviewed changes

jiparis reviewed Jun 17, 2024

View reviewed changes

jiparis approved these changes Jun 17, 2024

View reviewed changes

javirln merged commit 8c537cf into main Jun 17, 2024

javirln deleted the feat/942-auto-onboarding branch June 17, 2024 11:39

feat(auto-onboarding): Allow auto-onboarding on organizations #951

feat(auto-onboarding): Allow auto-onboarding on organizations #951

Uh oh!

Conversation

javirln commented Jun 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

migmartri left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

migmartri left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

migmartri left a comment

Choose a reason for hiding this comment

Uh oh!

javirln commented Jun 17, 2024

Uh oh!

migmartri left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

javirln Jun 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

javirln left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

javirln Jun 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

migmartri left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

javirln commented Jun 13, 2024 •

edited

Loading

javirln Jun 17, 2024 •

edited

Loading

javirln Jun 17, 2024 •

edited

Loading