fix(org): address Evan's review comments

Signed-off-by: jaydeep869 <jaydeeppokhariya2106@gmail.com>

Author: Jaydeep869

internal/controlplane/handlers_oauth.go

@@ -33,7 +33,6 @@ import (
 	"github.com/mindersec/minder/internal/logger"
 	"github.com/mindersec/minder/internal/providers"
 	"github.com/mindersec/minder/internal/providers/credentials"
-	"github.com/mindersec/minder/internal/providers/github"
 	"github.com/mindersec/minder/internal/providers/github/service"
 	"github.com/mindersec/minder/internal/providers/manager"
 	"github.com/mindersec/minder/internal/util"
@@ -443,8 +442,7 @@ func (s *Server) processAppCallback(ctx context.Context, w http.ResponseWriter,
 		}
 
 		if dbProv != nil {
-			login := github.GetGithubAppOwner(dbProv.Name)
-			s.publishOrganizationEntityEvent(ctx, dbProv.ID, dbProv.ProjectID, login)
+			s.publishOrganizationEntityEvent(ctx, dbProv.Provider.ID, dbProv.Provider.ProjectID, dbProv.InstallationOwner)
 		}
 
 		if stateData.RedirectUrl.Valid || stateData.EncryptedRedirect.Valid {
@@ -543,10 +541,9 @@ func (s *Server) handleAppInstallWithoutInvite(ctx context.Context, token *oauth
 			return nil, err
 		}
 		if dbProv != nil && proj != nil {
-			login := github.GetGithubAppOwner(dbProv.Name)
 			// 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)
+			s.publishOrganizationEntityEvent(ctx, dbProv.Provider.ID, proj.ID, dbProv.InstallationOwner)
 		}
 		return proj, nil
 	})

internal/controlplane/handlers_oauth_test.go

@@ -829,7 +829,7 @@ func TestHandleGitHubAppCallback(t *testing.T) {
 					}, nil)
 				service.EXPECT().
 					CreateGitHubAppProvider(gomock.Any(), gomock.Any(), gomock.Any(), installationID, gomock.Any()).
-					Return(&db.Provider{}, nil)
+					Return(&ghService.GitHubProviderFacet{Provider: &db.Provider{}}, nil)
 			},
 			checkResponse: func(t *testing.T, resp httptest.ResponseRecorder) {
 				t.Helper()

internal/controlplane/handlers_user.go

@@ -26,7 +26,6 @@ import (
 	"github.com/mindersec/minder/internal/db"
 	"github.com/mindersec/minder/internal/logger"
 	"github.com/mindersec/minder/internal/projects"
-	"github.com/mindersec/minder/internal/providers/github"
 	"github.com/mindersec/minder/internal/util"
 	pb "github.com/mindersec/minder/pkg/api/protobuf/go/minder/v1"
 )
@@ -141,8 +140,7 @@ func (s *Server) claimGitHubInstalls(ctx context.Context, qtx db.ExtendQuerier)
 			continue
 		}
 		if dbProv != nil {
-			login := github.GetGithubAppOwner(dbProv.Name)
-			s.publishOrganizationEntityEvent(ctx, dbProv.ID, proj.ID, login)
+			s.publishOrganizationEntityEvent(ctx, dbProv.Provider.ID, proj.ID, dbProv.InstallationOwner)
 		}
 		if proj != nil {
 			userProjects = append(userProjects, proj)

internal/providers/github/properties/organization.go

@@ -93,7 +93,7 @@ func fetchOrganizationProperties(
 		}
 	}
 	if user.CreatedAt != nil {
-		result[properties.OrgPropertyCreatedAt] = user.GetCreatedAt().Time.Format(time.RFC3339)
+		result[properties.OrgPropertyCreatedAt] = user.GetCreatedAt().Format(time.RFC3339)
 	}
 	if user.Plan != nil {
 		result[properties.OrgPropertyPlanName] = user.GetPlan().GetName()

internal/providers/github/service/backfill.go

@@ -6,7 +6,6 @@ package service
 import (
 	"context"
 	"database/sql"
-	"encoding/json"
 	"errors"
 	"fmt"
 
@@ -59,16 +58,10 @@ func BackfillOrganizations(ctx context.Context, store db.Store) error {
 			}
 
 			// 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{
+			_, err = qtx.UpsertPropertyValueV1(ctx, db.UpsertPropertyValueV1Params{
 				EntityID: ent.ID,
 				Key:      properties.PropertyName,
-				Value:    propBytes,
+				Value:    login,
 			})
 
 			if err == nil {

internal/providers/github/service/mock/service.go

@@ -32,17 +32,23 @@ import (
 
 //go:generate go run go.uber.org/mock/mockgen -package mock_$GOPACKAGE -destination=./mock/$GOFILE -source=./$GOFILE
 
+// GitHubProviderFacet encapsulates the created db.Provider and additional GitHub specific properties
+type GitHubProviderFacet struct {
+	Provider          *db.Provider
+	InstallationOwner string
+}
+
 // GitHubProviderService encapsulates methods for creating and updating providers
 type GitHubProviderService interface {
 	// CreateGitHubAppProvider creates a GitHub App provider with an installation ID in a known project
 	CreateGitHubAppProvider(ctx context.Context, token oauth2.Token, stateData db.GetProjectIDBySessionStateRow,
-		installationID int64, state string) (*db.Provider, error)
+		installationID int64, state string) (*GitHubProviderFacet, error)
 	// CreateGitHubAppWithoutInvitation either creates a new project for the selected app, or stores
 	// the installation in preparation for creating a new project when the authorizing user logs in.
 	//
 	// Note that this function may return nil, nil if the installation user is not known to Minder.
 	CreateGitHubAppWithoutInvitation(ctx context.Context, qtx db.ExtendQuerier, userID int64,
-		installationID int64) (*db.Project, *db.Provider, error)
+		installationID int64) (*db.Project, *GitHubProviderFacet, error)
 	// ValidateGitHubInstallationId checks if the supplied GitHub token has access to the installation ID
 	ValidateGitHubInstallationId(ctx context.Context, token *oauth2.Token, installationID int64) error
 	// DeleteGitHubAppInstallation deletes the GitHub App installation and provider from the database.
@@ -106,13 +112,13 @@ func (p *ghProviderService) CreateGitHubAppProvider(
 	stateData db.GetProjectIDBySessionStateRow,
 	installationID int64,
 	state string,
-) (*db.Provider, error) {
+) (*GitHubProviderFacet, error) {
 	installationOwner, err := p.getInstallationOwner(ctx, installationID)
 	if err != nil {
 		return nil, err
 	}
 
-	return db.WithTransaction(p.store, func(qtx db.ExtendQuerier) (*db.Provider, error) {
+	return db.WithTransaction(p.store, func(qtx db.ExtendQuerier) (*GitHubProviderFacet, error) {
 		validateOwnership := func(ctx context.Context) error {
 			// Older enrollments may not have a RemoteUser stored; these should age out fairly quickly.
 			p.mt.AddTokenOpCount(ctx, "check", stateData.RemoteUser.Valid)
@@ -157,7 +163,10 @@ func (p *ghProviderService) CreateGitHubAppProvider(
 			},
 		)
 
-		return &provider, err
+		return &GitHubProviderFacet{
+			Provider:          &provider,
+			InstallationOwner: installationOwner.GetLogin(),
+		}, err
 	})
 }
 
@@ -170,7 +179,7 @@ func (p *ghProviderService) CreateGitHubAppWithoutInvitation(
 	qtx db.ExtendQuerier,
 	userID int64,
 	installationID int64,
-) (*db.Project, *db.Provider, error) {
+) (*db.Project, *GitHubProviderFacet, error) {
 	installationOwner, err := p.getInstallationOwner(ctx, installationID)
 	if err != nil {
 		return nil, nil, err
@@ -213,7 +222,10 @@ func (p *ghProviderService) CreateGitHubAppWithoutInvitation(
 
 	}
 
-	return project, &provider, err
+	return project, &GitHubProviderFacet{
+		Provider:          &provider,
+		InstallationOwner: installationOwner.GetLogin(),
+	}, err
 }
 
 // Internal shared implementation between CreateGitHubAppProvider and CreateGitHubAppWithoutInvitation.

internal/providers/github/service/service.go

@@ -271,15 +271,16 @@ func TestProviderService_CreateGitHubAppProvider(t *testing.T) {
 	require.NoError(t, err)
 	require.NotNil(t, dbProv)
 
-	require.Equal(t, dbProv.ProjectID, dbproj.ID)
-	require.Equal(t, dbProv.AuthFlows, clients.AppAuthorizationFlows)
-	require.Equal(t, dbProv.Implements, clients.AppImplements)
-	require.Equal(t, dbProv.Class, db.ProviderClassGithubApp)
-	require.Contains(t, dbProv.Name, db.ProviderClassGithubApp)
-	require.Contains(t, dbProv.Name, accountLogin)
+	require.Equal(t, dbProv.Provider.ProjectID, dbproj.ID)
+	require.Equal(t, dbProv.Provider.AuthFlows, clients.AppAuthorizationFlows)
+	require.Equal(t, dbProv.Provider.Implements, clients.AppImplements)
+	require.Equal(t, dbProv.Provider.Class, db.ProviderClassGithubApp)
+	require.Contains(t, dbProv.Provider.Name, db.ProviderClassGithubApp)
+	require.Contains(t, dbProv.Provider.Name, accountLogin)
+	require.Equal(t, accountLogin, dbProv.InstallationOwner)
 
 	dbInstall, err := mocks.fakeStore.GetInstallationIDByProviderID(context.Background(),
-		uuid.NullUUID{UUID: dbProv.ID, Valid: true},
+		uuid.NullUUID{UUID: dbProv.Provider.ID, Valid: true},
 	)
 	require.NoError(t, err)
 	require.Equal(t, dbInstall.AppInstallationID, int64(installationID))