-
Notifications
You must be signed in to change notification settings - Fork 111
feat(entities): Implement ENTITY_ORGANIZATION for Github Providers #6356
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
base: main
Are you sure you want to change the base?
Changes from 3 commits
56cbde2
26ed1fc
4000a99
3b5eb14
cae8441
bda676f
34c66c2
ab5e202
9a4b2be
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,86 @@ | ||
| // SPDX-FileCopyrightText: Copyright 2026 The Minder Authors | ||
| // SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| package app | ||
|
|
||
| import ( | ||
| "context" | ||
| "database/sql" | ||
| "encoding/json" | ||
| "errors" | ||
| "fmt" | ||
| "strings" | ||
|
|
||
| "github.com/rs/zerolog" | ||
|
|
||
| "github.com/mindersec/minder/internal/db" | ||
| "github.com/mindersec/minder/pkg/entities/properties" | ||
| ) | ||
|
|
||
| func backfillOrganizations(ctx context.Context, store db.Store) error { | ||
| l := zerolog.Ctx(ctx) | ||
| l.Info().Msg("Starting backfill for Organization entities...") | ||
|
|
||
| provs, err := store.GlobalListProvidersByClass(ctx, db.ProviderClassGithubApp) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to list providers: %w", err) | ||
| } | ||
|
|
||
| count := 0 | ||
|
|
||
| for _, prov := range provs { | ||
| login := strings.TrimPrefix(prov.Name, string(db.ProviderClassGithubApp)+"-") | ||
|
Jaydeep869 marked this conversation as resolved.
Outdated
|
||
|
|
||
| _, err = db.WithTransaction(store, func(qtx db.ExtendQuerier) (any, error) { | ||
| // Check if organization entity already exists | ||
| _, err := qtx.GetEntityByName(ctx, db.GetEntityByNameParams{ | ||
| EntityType: db.EntitiesOrganization, | ||
| Name: login, | ||
| ProviderID: prov.ID, | ||
| ProjectID: prov.ProjectID, | ||
| }) | ||
|
|
||
| if err == nil { | ||
| return nil, nil // already exists | ||
| } else if !errors.Is(err, sql.ErrNoRows) { | ||
| return nil, err | ||
| } | ||
|
|
||
| // Entity doesn't exist, create it | ||
| ent, err := qtx.CreateEntity(ctx, db.CreateEntityParams{ | ||
| EntityType: db.EntitiesOrganization, | ||
| Name: login, | ||
| ProviderID: prov.ID, | ||
| ProjectID: prov.ProjectID, | ||
| }) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| // Set the default property (login name) | ||
| propVal := map[string]any{ | ||
| "minder.internal.type": "string", | ||
| "minder.internal.value": login, | ||
| } | ||
| propBytes, _ := json.Marshal(propVal) | ||
|
|
||
| _, err = qtx.UpsertProperty(ctx, db.UpsertPropertyParams{ | ||
| EntityID: ent.ID, | ||
| Key: properties.PropertyName, | ||
| Value: propBytes, | ||
| }) | ||
|
|
||
| if err == nil { | ||
| count++ | ||
| } | ||
| return nil, err | ||
| }) | ||
|
|
||
| if err != nil { | ||
| l.Error().Err(err).Str("provider", prov.ID.String()).Msg("Failed to backfill organization for provider") | ||
| } | ||
| } | ||
|
|
||
| l.Info().Int("count", count).Msg("Completed backfill for Organization entities") | ||
| return nil | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,4 @@ | ||
| -- SPDX-FileCopyrightText: Copyright 2026 The Minder Authors | ||
| -- SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| -- Postgres doesn't easily drop enum values, down migration is a no-op |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,4 @@ | ||
| -- SPDX-FileCopyrightText: Copyright 2026 The Minder Authors | ||
| -- SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| ALTER TYPE entities ADD VALUE 'organization'; |
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -436,18 +436,14 @@ func (s *Server) processAppCallback(ctx context.Context, w http.ResponseWriter, | |
|
|
||
| logger.BusinessRecord(ctx).Project = stateData.ProjectID | ||
|
|
||
| var confErr providers.ErrProviderInvalidConfig | ||
| _, err = s.ghProviders.CreateGitHubAppProvider(ctx, *token, stateData, installationID, state) | ||
| dbProv, err := s.ghProviders.CreateGitHubAppProvider(ctx, *token, stateData, installationID, state) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since the only time the return value from
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Rather than returning a bunch of state which needs to be passed along to |
||
| if err != nil { | ||
| if errors.As(err, &confErr) { | ||
| return newHttpError(http.StatusBadRequest, "Invalid provider config").SetContents( | ||
| "The provider configuration is invalid: %s", confErr.Details) | ||
| } | ||
| if errors.Is(err, service.ErrInvalidTokenIdentity) { | ||
| return newHttpError(http.StatusForbidden, "User token mismatch").SetContents( | ||
| "The provided login token was associated with a different GitHub user.") | ||
| } | ||
| return fmt.Errorf("error creating GitHub App provider: %w", err) | ||
| return handleProviderCreationError(err) | ||
| } | ||
|
|
||
| if dbProv != nil { | ||
| login := strings.TrimPrefix(dbProv.Name, string(db.ProviderClassGithubApp)+"-") | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| s.publishOrganizationEntityEvent(ctx, dbProv.ID, dbProv.ProjectID, login) | ||
| } | ||
|
|
||
| if stateData.RedirectUrl.Valid || stateData.EncryptedRedirect.Valid { | ||
|
|
@@ -541,7 +537,17 @@ func (s *Server) handleAppInstallWithoutInvite(ctx context.Context, token *oauth | |
| } | ||
|
|
||
| _, err = db.WithTransaction(s.store, func(qtx db.ExtendQuerier) (*db.Project, error) { | ||
| return s.ghProviders.CreateGitHubAppWithoutInvitation(ctx, qtx, *userID, installationID) | ||
| proj, dbProv, err := s.ghProviders.CreateGitHubAppWithoutInvitation(ctx, qtx, *userID, installationID) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| if dbProv != nil && proj != nil { | ||
| login := strings.TrimPrefix(dbProv.Name, string(db.ProviderClassGithubApp)+"-") | ||
| // It is generally safe to publish an event from within a transaction, as long | ||
| // as the event handler evaluates the state matching later. | ||
| s.publishOrganizationEntityEvent(ctx, dbProv.ID, proj.ID, login) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It occurs to me that the |
||
| } | ||
| return proj, nil | ||
| }) | ||
| return err | ||
| } | ||
|
|
@@ -787,3 +793,16 @@ func (s *Server) decryptRedirect(stateData *db.GetProjectIDBySessionStateRow) (* | |
| } | ||
| return parsedURL, nil | ||
| } | ||
|
|
||
| func handleProviderCreationError(err error) error { | ||
| var confErr providers.ErrProviderInvalidConfig | ||
| if errors.As(err, &confErr) { | ||
| return newHttpError(http.StatusBadRequest, "Invalid provider config").SetContents( | ||
| "The provider configuration is invalid: %s", confErr.Details) | ||
| } | ||
| if errors.Is(err, service.ErrInvalidTokenIdentity) { | ||
| return newHttpError(http.StatusForbidden, "User token mismatch").SetContents( | ||
| "The provided login token was associated with a different GitHub user.") | ||
| } | ||
| return fmt.Errorf("error creating GitHub App provider: %w", err) | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -427,7 +427,7 @@ func (s *Server) getRuleEvalStatus( | |
| repoPath = fmt.Sprintf("%s/%s", prRepoOwner, prRepoName) | ||
| } | ||
| case db.EntitiesBuildEnvironment, db.EntitiesRelease, db.EntitiesPipelineRun, | ||
| db.EntitiesTaskRun, db.EntitiesBuild: | ||
| db.EntitiesTaskRun, db.EntitiesBuild, db.EntitiesOrganization: | ||
|
Comment on lines
429
to
+430
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is probably okay for now, but it seems like alerts will have incorrect URLs (and log errors) until we fix it. It can be a good idea to file an issue for this if we merge this PR, to track the need to update it later. |
||
| zerolog.Ctx(ctx).Warn().Msgf("attempting to set alerts for unsupported entity type: %v", dbRuleEvalStat.EntityType) | ||
| default: | ||
| zerolog.Ctx(ctx).Error().Msgf("unknown entity type: %v", dbRuleEvalStat.EntityType) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,6 +12,7 @@ import ( | |
| "net/http" | ||
| "path" | ||
| "strconv" | ||
| "strings" | ||
|
|
||
| "github.com/google/uuid" | ||
| gauth "github.com/grpc-ecosystem/go-grpc-middleware/v2/interceptors/auth" | ||
|
|
@@ -134,11 +135,15 @@ func (s *Server) claimGitHubInstalls(ctx context.Context, qtx db.ExtendQuerier) | |
|
|
||
| for _, i := range installs { | ||
| // TODO: if we can get an GitHub auth token for the user, we can do the rest with CreateGitHubAppWithoutInvitation | ||
| proj, err := s.ghProviders.CreateGitHubAppWithoutInvitation(ctx, qtx, userID, i.AppInstallationID) | ||
| proj, dbProv, err := s.ghProviders.CreateGitHubAppWithoutInvitation(ctx, qtx, userID, i.AppInstallationID) | ||
| if err != nil { | ||
| zerolog.Ctx(ctx).Error().Err(err).Int64("org_id", i.OrganizationID).Msg("failed to create GitHub app at first login") | ||
| continue | ||
| } | ||
| if dbProv != nil { | ||
| login := strings.TrimPrefix(dbProv.Name, string(db.ProviderClassGithubApp)+"-") | ||
| s.publishOrganizationEntityEvent(ctx, dbProv.ID, proj.ID, login) | ||
| } | ||
|
Comment on lines
+142
to
+144
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Again, let's try to centralize this pattern in the GitHub provider. |
||
| if proj != nil { | ||
| userProjects = append(userProjects, proj) | ||
| } | ||
|
|
||
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -255,7 +255,7 @@ func (e *EEA) buildEntityWrapper( | |
| case db.EntitiesPullRequest: | ||
| return e.buildPullRequestInfoWrapper(ctx, entityID, projID) | ||
| case db.EntitiesBuildEnvironment, db.EntitiesRelease, | ||
| db.EntitiesPipelineRun, db.EntitiesTaskRun, db.EntitiesBuild: | ||
| db.EntitiesPipelineRun, db.EntitiesTaskRun, db.EntitiesBuild, db.EntitiesOrganization: | ||
| return nil, fmt.Errorf("entity type %q not yet supported", entity) | ||
|
Comment on lines
+258
to
260
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think leaving this this way will prevent us from evaluating policy on organizations (but I'd need to test to be sure). Definitely something to file a follow-up issue on if we leave like this. |
||
| default: | ||
| return nil, fmt.Errorf("unknown entity type: %q", entity) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,30 @@ | ||
| // SPDX-FileCopyrightText: Copyright 2026 The Minder Authors | ||
| // SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| package validators | ||
|
|
||
| import ( | ||
| "context" | ||
|
|
||
| "github.com/google/uuid" | ||
|
|
||
| "github.com/mindersec/minder/pkg/entities/properties" | ||
| ) | ||
|
|
||
| // OrganizationValidator validates organization entity creation | ||
| type OrganizationValidator struct{} | ||
|
|
||
| // NewOrganizationValidator creates a new OrganizationValidator | ||
| func NewOrganizationValidator() *OrganizationValidator { | ||
| return &OrganizationValidator{} | ||
| } | ||
|
|
||
| // Validate checks if an organization entity can be created | ||
| func (*OrganizationValidator) Validate( | ||
| _ context.Context, | ||
| _ *properties.Properties, | ||
| _ uuid.UUID, | ||
| ) error { | ||
| // For now, any organization properties that make it this far are valid | ||
| return nil | ||
| } | ||
|
Jaydeep869 marked this conversation as resolved.
Outdated
|
||
Uh oh!
There was an error while loading. Please reload this page.