feat(entities): Implement ENTITY_ORGANIZATION for Github Providers #6356
feat(entities): Implement ENTITY_ORGANIZATION for Github Providers #6356Jaydeep869 wants to merge 9 commits intomindersec:mainfrom
Conversation
Implements ENTITY_ORGANIZATION relying entirely on the new generic entity architecture to solve mindersec#5377 and unblock 2FA checks. Includes property fetcher, validator, and async Watermill organization auto-registration upon GitHub App installation. Also implements a backfill migration to synthesize missing organization associations for existing providers.
|
coverage: 60.566% (-0.01%) from 60.578% — Jaydeep869:feat/organization-entity-5377 into mindersec:main |
- Addresses exhaustive switch cases missed in initial entity setup - Fixes cyclomatic complexity warning in processAppCallback by extracting error handling - Fixes gh provider initialization panic by adding support for organization in RegisterEntity
evankanderson
left a comment
evankanderson
left a comment
There was a problem hiding this comment.
It looks like this sets up the lifecycle operations for (GitHub) organization entities, but it doesn't actually enable writing rules or profiles that enforce policies on organizations. I think that's fine, but it would be good to indicate the remaining work in the PR description as well as what is implemented.
Have you tested this end-to-end in a local minder instance? If not, I can try to do that in the next few days.
| EntityID: ent.ID, | ||
| Key: properties.PropertyName, | ||
| Value: propBytes, | ||
| }) |
There was a problem hiding this comment.
Can this use some common code with providers/github/properties/organization.go?
| } | ||
|
|
||
| if dbProv != nil { | ||
| login := strings.TrimPrefix(dbProv.Name, string(db.ProviderClassGithubApp)+"-") |
There was a problem hiding this comment.
Even if we don't end up adding a method to fetch the current organization name given the ID, we should at least centralize the name mangling in a function where we can add it later if needed. Ideally, that function would be a part of the GitHub provider.
|
|
||
| var confErr providers.ErrProviderInvalidConfig | ||
| _, err = s.ghProviders.CreateGitHubAppProvider(ctx, *token, stateData, installationID, state) | ||
| dbProv, err := s.ghProviders.CreateGitHubAppProvider(ctx, *token, stateData, installationID, state) |
There was a problem hiding this comment.
Since the only time the return value from CreateGitHubAppProvider is used is in a unit test, it should be easy to change that to return a provider facet or some other struct which can be used to resolve any provider properties we need. (Currently, it seems like mostly the org name, but possibly other data in the future.)
| result[properties.OrgPropertyAvatarURL] = *user.AvatarURL | ||
| } | ||
| if user.Company != nil { | ||
| result[properties.OrgPropertyCompany] = *user.Company |
There was a problem hiding this comment.
Company appears to be null for a large number of orgs; I'm not sure how it's normally set.
evankanderson
left a comment
evankanderson
left a comment
There was a problem hiding this comment.
I'm marking this PR as "request changes" (has 10+ pending comments) to help track which outstanding PRs need maintainer action vs contributor action.
Signed-off-by: jaydeep869 <jaydeeppokhariya2106@gmail.com>
|
Hi @evankanderson! Thanks for the insightful and deep review. I really appreciate it. I apologize for the massive delay in circling back to this. I’ve been incredibly busy knocking out my end semester exams. I have pushed up a fresh commit that tackles all of your feedback. |
No problem, I've had quite a backlog as well. |
|
I think you may need to rename your migration file, as we had another 000117 for adding a Rego version to the ruletypes table. |
Signed-off-by: jaydeep869 <jaydeeppokhariya2106@gmail.com>
Signed-off-by: jaydeep869 <jaydeeppokhariya2106@gmail.com>
Jaydeep869
force-pushed
the
feat/organization-entity-5377
branch
from
330e396 to
34c66c2
Compare
Signed-off-by: jaydeep869 <jaydeeppokhariya2106@gmail.com>
Signed-off-by: jaydeep869 <jaydeeppokhariya2106@gmail.com>
|
Hey @evankanderson, can you review it now |
3 participants
Description
This PR fully implements
ENTITY_ORGANIZATIONto act as a proper target for evaluation rules and effectively unblocks the 2FA checking mechanics highlighted in #3842.As per recent discussions with @evankanderson, rather than introducing custom endpoints or manual administration routines, this integration relies natively on the new generic entity properties architecture and auto registers organizations fully in the background directly after a user successfully installs the Github Provider app.
Key Changes
entitiesenum via a new migration (000117_organization_entity.up.sql) to include the'organization'value.ENTITY_ORGANIZATION = 9natively withinminder.proto.OrganizationFetcherwithingithub/propertiesthat queries Github's API to constructproperties.Propertiesmapping theis_userandavatar_urlfields correctly.OrganizationValidatorfor reliable validation logic and registeredENTITY_ORGANIZATIONinservice.go.CreateGitHubAppProviderto publish aMinderEventto theTopicQueueReconcileEntityAddWatermill queue synchronously upon provider registration. The control plane natively handles this via background reconcile pipelines.backfill_organizations.go) linked through themigrate upprocedure to automatically traverse all existing user/database providers and populate any orphanedorganizationlinks via transaction safety upon startup.*db.Provideras a third return value payload originating fromCreateGitHubAppWithoutInvitationand rebuilt all the testing mocks appropriately matching the expectation logic.Related Issues
Testing Performed
make buf,make sqlc,make mock).make test-silent), fixing broken mock signatures.make run-docker.