Repository: traPtitech/NeoShowcase (go)

Issue:
Website path overlayを同一ownerでしかできないようにする
多分そういうユースケースしかない

Pull request:
allow website path overlay only if owner
resolves #490 

Patch:
diff --git a/pkg/domain/app.go b/pkg/domain/app.go
index 9f1e20de..ba5dcbf0 100644
--- a/pkg/domain/app.go
+++ b/pkg/domain/app.go
@@ -16,6 +16,7 @@ import (
 	"golang.org/x/exp/slices"
 	"golang.org/x/net/idna"
 
+	"github.com/traPtitech/neoshowcase/pkg/util/ds"
 	"github.com/traPtitech/neoshowcase/pkg/util/optional"
 )
 
@@ -200,7 +201,7 @@ type Application struct {
 	OwnerIDs []string
 }
 
-func (a *Application) Validate() error {
+func (a *Application) SelfValidate() error {
 	if a.Name == "" {
 		return errors.New("name is required")
 	}
@@ -224,6 +225,47 @@ func (a *Application) Validate() error {
 	return nil
 }
 
+func (a *Application) Validate(
+	ctx context.Context,
+	actor *User,
+	existing []*Application,
+	controller ControllerServiceClient,
+	domains AvailableDomainSlice,
+) (validateErr error, err error) {
+	if err = a.SelfValidate(); err != nil {
+		return err, nil
+	}
+
+	for _, website := range a.Websites {
+		if website.Authentication == AuthenticationTypeOff {
+			continue
+		}
+		available, err := controller.AuthAvailable(ctx, website.FQDN)
+		if err != nil {
+			return nil, errors.Wrap(err, "checking auth availability")
+		}
+		if !available {
+			return errors.Errorf("auth not available for domain %s", website.FQDN), nil
+		}
+	}
+
+	for _, website := range a.Websites {
+		if !domains.IsAvailable(website.FQDN) {
+			return errors.Errorf("domain %s not available", website.FQDN), nil
+		}
+	}
+
+	if a.WebsiteConflicts(existing, actor) {
+		return errors.New("website conflict"), nil
+	}
+
+	return nil, nil
+}
+
+func (a *Application) IsOwner(user *User) bool {
+	return user.Admin || lo.Contains(a.OwnerIDs, user.ID)
+}
+
 type Artifact struct {
 	ID        string
 	BuildID   string
@@ -396,6 +438,10 @@ func (r *Repository) Validate() error {
 	return nil
 }
 
+func (r *Repository) IsOwner(user *User) bool {
+	return user.Admin || lo.Contains(r.OwnerIDs, user.ID)
+}
+
 type RepositoryAuthMethod int
 
 const (
@@ -475,10 +521,11 @@ func (w *Website) Validate() error {
 }
 
 func (w *Website) pathComponents() []string {
+	// NOTE: empty PathPrefix must not exist
 	if w.PathPrefix == "/" {
 		return []string{}
 	}
-	return strings.Split(w.PathPrefix, "/")[1:]
+	return strings.Split(w.PathPrefix[1:], "/")
 }
 
 func (w *Website) pathContainedBy(target *Website) bool {
@@ -487,25 +534,40 @@ func (w *Website) pathContainedBy(target *Website) bool {
 	if len(this) < len(other) {
 		return false
 	}
-	for i := range other {
-		if this[i] != other[i] {
-			return false
-		}
+	return ds.Equals(this[:len(other)], other)
+}
+
+func (w *Website) Equals(target *Website) bool {
+	if w.FQDN != target.FQDN {
+		return false
+	}
+	if w.HTTPS != target.HTTPS {
+		return false
 	}
-	return true
+	return w.PathPrefix == target.PathPrefix
 }
 
-// ConflictsWith checks whether this website's path prefix is contained in the existing websites' path prefixes.
-func (w *Website) ConflictsWith(existing []*Website) bool {
-	for _, ex := range existing {
-		if w.FQDN != ex.FQDN {
-			continue
-		}
-		if w.HTTPS != ex.HTTPS {
-			continue
-		}
-		if w.pathContainedBy(ex) {
-			return true
+func (w *Website) conflictsWith(target *Website) bool {
+	if w.FQDN != target.FQDN {
+		return false
+	}
+	if w.HTTPS != target.HTTPS {
+		return false
+	}
+	return w.pathContainedBy(target) || target.pathContainedBy(w)
+}
+
+func (a *Application) WebsiteConflicts(existing []*Application, actor *User) bool {
+	for _, w := range a.Websites {
+		for _, ex := range existing {
+			for _, w2 := range ex.Websites {
+				if w.Equals(w2) {
+					return true
+				}
+				if w.conflictsWith(w2) && !ex.IsOwner(actor) {
+					return true
+				}
+			}
 		}
 	}
 	return false
diff --git a/pkg/interface/repository/application.go b/pkg/interface/repository/application.go
index 6f6b05fa..4d6fc9ea 100644
--- a/pkg/interface/repository/application.go
+++ b/pkg/interface/repository/application.go
@@ -9,7 +9,6 @@ import (
 	"github.com/samber/lo"
 	"github.com/volatiletech/sqlboiler/v4/boil"
 	"github.com/volatiletech/sqlboiler/v4/queries/qm"
-	"golang.org/x/exp/slices"
 
 	"github.com/traPtitech/neoshowcase/pkg/domain"
 	"github.com/traPtitech/neoshowcase/pkg/infrastructure/admindb/models"
@@ -121,7 +120,7 @@ func (r *applicationRepository) CreateApplication(ctx context.Context, app *doma
 		return fmt.Errorf("failed to create application config")
 	}
 
-	err = r.validateAndInsertWebsites(ctx, tx, ma, app.Websites)
+	err = r.setWebsites(ctx, tx, ma, app.Websites)
 	if err != nil {
 		return err
 	}
@@ -190,11 +189,7 @@ func (r *applicationRepository) UpdateApplication(ctx context.Context, id string
 		}
 	}
 	if args.Websites.Valid {
-		_, err = app.R.Websites.DeleteAll(ctx, tx)
-		if err != nil {
-			return errors.Wrap(err, "failed to delete all websites")
-		}
-		err = r.validateAndInsertWebsites(ctx, tx, app, args.Websites.V)
+		err = r.setWebsites(ctx, tx, app, args.Websites.V)
 		if err != nil {
 			return err
 		}
@@ -250,30 +245,17 @@ func (r *applicationRepository) DeleteApplication(ctx context.Context, id string
 	return nil
 }
 
-func (r *applicationRepository) validateAndInsertWebsites(ctx context.Context, ex boil.ContextExecutor, app *models.Application, websites []*domain.Website) error {
-	// Validate and insert from the most specific
-	slices.SortFunc(websites, func(a, b *domain.Website) bool { return len(b.PathPrefix) < len(a.PathPrefix) })
-	for _, website := range websites {
-		if err := r.validateAndInsertWebsite(ctx, ex, app, website); err != nil {
-			return err
-		}
-	}
-	return nil
-}
-
-func (r *applicationRepository) validateAndInsertWebsite(ctx context.Context, ex boil.ContextExecutor, app *models.Application, website *domain.Website) error {
-	websites, err := models.Websites(models.WebsiteWhere.FQDN.EQ(website.FQDN), qm.For("UPDATE")).All(ctx, ex)
+func (r *applicationRepository) setWebsites(ctx context.Context, ex boil.ContextExecutor, app *models.Application, websites []*domain.Website) error {
+	_, err := app.R.Websites.DeleteAll(ctx, ex)
 	if err != nil {
-		return errors.Wrap(err, "failed to get existing websites")
-	}
-	existing := lo.Map(websites, func(website *models.Website, i int) *domain.Website { return repoconvert.ToDomainWebsite(website) })
-	if website.ConflictsWith(existing) {
-		return errors.New("conflicts with existing websites")
+		return errors.Wrap(err, "failed to delete all websites")
 	}
-	mw := repoconvert.FromDomainWebsite(app.ID, website)
-	err = mw.Insert(ctx, ex, boil.Blacklist())
-	if err != nil {
-		return errors.Wrap(err, "failed to add website")
+	for _, w := range websites {
+		mw := repoconvert.FromDomainWebsite(app.ID, w)
+		err = mw.Insert(ctx, ex, boil.Blacklist())
+		if err != nil {
+			return errors.Wrap(err, "failed to insert website")
+		}
 	}
 	return nil
 }
diff --git a/pkg/usecase/api_app_service.go b/pkg/usecase/api_app_service.go
index 3f11bbfb..0dc02359 100644
--- a/pkg/usecase/api_app_service.go
+++ b/pkg/usecase/api_app_service.go
@@ -2,13 +2,13 @@ package usecase
 
 import (
 	"context"
-	"fmt"
 	"strconv"
 
 	"github.com/friendsofgo/errors"
 	log "github.com/sirupsen/logrus"
 
 	"github.com/traPtitech/neoshowcase/pkg/domain"
+	"github.com/traPtitech/neoshowcase/pkg/domain/web"
 	"github.com/traPtitech/neoshowcase/pkg/util/optional"
 	"github.com/traPtitech/neoshowcase/pkg/util/random"
 )
@@ -18,6 +18,7 @@ func (s *APIServerService) CreateApplication(ctx context.Context, app *domain.Ap
 	if err != nil {
 		return nil, err
 	}
+
 	// Only check for repository owner if repository is private;
 	// allow everyone to create application if repository is public
 	if repo.Auth.Valid {
@@ -27,32 +28,21 @@ func (s *APIServerService) CreateApplication(ctx context.Context, app *domain.Ap
 		}
 	}
 
-	if err = app.Validate(); err != nil {
-		return nil, newError(ErrorTypeBadRequest, "invalid application", err)
-	}
-	for _, website := range app.Websites {
-		if website.Authentication == domain.AuthenticationTypeOff {
-			continue
-		}
-		available, err := s.controller.AuthAvailable(ctx, website.FQDN)
-		if err != nil {
-			return nil, errors.Wrap(err, "checking auth availability")
-		}
-		if !available {
-			return nil, newError(ErrorTypeBadRequest, fmt.Sprintf("auth not available for domain %s", website.FQDN), err)
-		}
+	// Validate
+	existing, err := s.appRepo.GetApplications(ctx, domain.GetApplicationCondition{})
+	if err != nil {
+		return nil, errors.Wrap(err, "getting existing applications")
 	}
-
 	domains, err := s.adRepo.GetAvailableDomains(ctx)
 	if err != nil {
-		return nil, err
+		return nil, errors.Wrap(err, "getting available domains")
 	}
-	for _, website := range app.Websites {
-		if !domains.IsAvailable(website.FQDN) {
-			return nil, newError(ErrorTypeBadRequest, "domain not available", nil)
-		}
+	valErr, err := app.Validate(ctx, web.GetUser(ctx), existing, s.controller, domains)
+	if valErr != nil {
+		return nil, newError(ErrorTypeBadRequest, "invalid application", err)
 	}
 
+	// Create
 	err = s.appRepo.CreateApplication(ctx, app)
 	if err != nil {
 		return nil, err
@@ -170,20 +160,19 @@ func (s *APIServerService) UpdateApplication(ctx context.Context, id string, arg
 		return err
 	}
 	app.Apply(args)
-	if err = app.Validate(); err != nil {
-		return newError(ErrorTypeBadRequest, "invalid application", err)
+
+	// Validate
+	existing, err := s.appRepo.GetApplications(ctx, domain.GetApplicationCondition{})
+	if err != nil {
+		return errors.Wrap(err, "getting existing applications")
 	}
-	for _, website := range app.Websites {
-		if website.Authentication == domain.AuthenticationTypeOff {
-			continue
-		}
-		available, err := s.controller.AuthAvailable(ctx, website.FQDN)
-		if err != nil {
-			return errors.Wrap(err, "checking auth availability")
-		}
-		if !available {
-			return newError(ErrorTypeBadRequest, fmt.Sprintf("auth not available for domain %s", website.FQDN), err)
-		}
+	domains, err := s.adRepo.GetAvailableDomains(ctx)
+	if err != nil {
+		return errors.Wrap(err, "getting available domains")
+	}
+	valErr, err := app.Validate(ctx, web.GetUser(ctx), existing, s.controller, domains)
+	if valErr != nil {
+		return newError(ErrorTypeBadRequest, "invalid application", err)
 	}
 
 	return s.appRepo.UpdateApplication(ctx, id, args)
diff --git a/pkg/usecase/api_service.go b/pkg/usecase/api_service.go
index 7a8883f4..20492d15 100644
--- a/pkg/usecase/api_service.go
+++ b/pkg/usecase/api_service.go
@@ -4,7 +4,6 @@ import (
 	"context"
 
 	"github.com/friendsofgo/errors"
-	"github.com/samber/lo"
 
 	"github.com/traPtitech/neoshowcase/pkg/domain"
 	"github.com/traPtitech/neoshowcase/pkg/domain/web"
@@ -62,42 +61,33 @@ func NewAPIServerService(
 	}
 }
 
-func (s *APIServerService) isRepositoryOwner(ctx context.Context, id string) error {
+func (s *APIServerService) isRepositoryOwner(ctx context.Context, repoID string) error {
 	user := web.GetUser(ctx)
-	if user.Admin {
-		return nil
-	}
-	repo, err := s.gitRepo.GetRepository(ctx, id)
+	repo, err := s.gitRepo.GetRepository(ctx, repoID)
 	if err != nil {
 		return errors.Wrap(err, "failed to get repository")
 	}
-	if !lo.Contains(repo.OwnerIDs, user.ID) {
+	if !repo.IsOwner(user) {
 		return newError(ErrorTypeForbidden, "you do not have permission for this repository", nil)
 	}
 	return nil
 }
 
-func (s *APIServerService) isApplicationOwner(ctx context.Context, id string) error {
+func (s *APIServerService) isApplicationOwner(ctx context.Context, appID string) error {
 	user := web.GetUser(ctx)
-	if user.Admin {
-		return nil
-	}
-	app, err := s.appRepo.GetApplication(ctx, id)
+	app, err := s.appRepo.GetApplication(ctx, appID)
 	if err != nil {
 		return errors.Wrap(err, "failed to get application")
 	}
-	if !lo.Contains(app.OwnerIDs, user.ID) {
+	if !app.IsOwner(user) {
 		return newError(ErrorTypeForbidden, "you do not have permission for this application", nil)
 	}
 	return nil
 }
 
-func (s *APIServerService) isBuildOwner(ctx context.Context, id string) error {
+func (s *APIServerService) isBuildOwner(ctx context.Context, buildID string) error {
 	user := web.GetUser(ctx)
-	if user.Admin {
-		return nil
-	}
-	build, err := s.buildRepo.GetBuild(ctx, id)
+	build, err := s.buildRepo.GetBuild(ctx, buildID)
 	if err != nil {
 		return errors.Wrap(err, "failed to get build")
 	}
@@ -105,7 +95,7 @@ func (s *APIServerService) isBuildOwner(ctx context.Context, id string) error {
 	if err != nil {
 		return errors.Wrap(err, "failed to get application")
 	}
-	if !lo.Contains(app.OwnerIDs, user.ID) {
+	if !app.IsOwner(user) {
 		return newError(ErrorTypeForbidden, "you do not have permission for this application", nil)
 	}
 	return nil


Test patch:
diff --git a/pkg/domain/app_test.go b/pkg/domain/app_test.go
index 7876e739..f0bca43a 100644
--- a/pkg/domain/app_test.go
+++ b/pkg/domain/app_test.go
@@ -8,8 +8,6 @@ import (
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 
-	"github.com/samber/lo"
-
 	"github.com/traPtitech/neoshowcase/pkg/util/optional"
 )
 
@@ -161,7 +159,7 @@ func TestApplicationConfig_Validate(t *testing.T) {
 	}
 }
 
-func TestApplication_Validate(t *testing.T) {
+func TestApplication_SelfValidate(t *testing.T) {
 	runtimeValidConfig := ApplicationConfig{
 		BuildConfig: &BuildConfigRuntimeDockerfile{DockerfileName: "Dockerfile"},
 	}
@@ -247,7 +245,7 @@ func TestApplication_Validate(t *testing.T) {
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			err := tt.app.Validate()
+			err := tt.app.SelfValidate()
 			if tt.wantErr {
 				assert.Error(t, err)
 			} else {
@@ -717,33 +715,31 @@ func TestWebsite_pathComponents(t *testing.T) {
 	}
 }
 
-func TestWebsite_ConflictsWith(t *testing.T) {
+func TestWebsite_conflictsWith(t *testing.T) {
 	pathTests := []struct {
 		name     string
 		target   string
-		existing []string
+		existing string
 		want     bool
 	}{
-		{"ok1", "/", []string{}, false},
-		{"ok2", "/foo", []string{"/api", "/spa"}, false},
-		{"ok3", "/api/v2", []string{"/api/v1", "/spa"}, false},
-		{"ok4", "/api2", []string{"/api"}, false},
-		{"ok5", "/api", []string{"/api2"}, false},
-		{"ng1", "/", []string{"/"}, true},
-		{"ng2", "/api", []string{"/"}, true},
-		{"ng3", "/api/v2", []string{"/api"}, true},
-		{"ng4", "/api", []string{"/api"}, true},
+		{"ok1", "/foo", "/api", false},
+		{"ok2", "/foo", "/spa", false},
+		{"ok3", "/api/v2", "/api/v1", false},
+		{"ok4", "/api2", "/api", false},
+		{"ok5", "/api", "/api2", false},
+		{"ng1", "/", "/", true},
+		{"ng2", "/api", "/", true},
+		{"ng3", "/", "/api", true},
+		{"ng4", "/api/v2", "/api", true},
+		{"ng5", "/api", "/api/v2", true},
+		{"ng6", "/api", "/api", true},
 	}
 	for _, tt := range pathTests {
 		t.Run("path "+tt.name, func(t *testing.T) {
-			w := &Website{
-				PathPrefix: tt.target,
-			}
-			existingWebsites := lo.Map(tt.existing, func(ex string, i int) *Website {
-				return &Website{PathPrefix: ex}
-			})
-			if got := w.ConflictsWith(existingWebsites); got != tt.want {
-				t.Errorf("ConflictsWith() = %v, want %v", got, tt.want)
+			w := &Website{PathPrefix: tt.target}
+			target := &Website{PathPrefix: tt.existing}
+			if got := w.conflictsWith(target); got != tt.want {
+				t.Errorf("conflictsWith() = %v, want %v", got, tt.want)
 			}
 		})
 	}
@@ -751,33 +747,139 @@ func TestWebsite_ConflictsWith(t *testing.T) {
 	fullTests := []struct {
 		name     string
 		target   *Website
-		existing []*Website
+		existing *Website
 		want     bool
 	}{
 		{
 			name:     "ng if same scheme",
 			target:   &Website{PathPrefix: "/", HTTPS: false},
-			existing: []*Website{{PathPrefix: "/", HTTPS: false}},
+			existing: &Website{PathPrefix: "/", HTTPS: false},
 			want:     true,
 		},
 		{
 			name:     "ok if different scheme",
 			target:   &Website{PathPrefix: "/", HTTPS: true},
-			existing: []*Website{{PathPrefix: "/", HTTPS: false}},
+			existing: &Website{PathPrefix: "/", HTTPS: false},
 			want:     false,
 		},
 		{
 			name:     "ok if different fqdn",
 			target:   &Website{FQDN: "google.com", PathPrefix: "/", HTTPS: false},
-			existing: []*Website{{FQDN: "yahoo.com", PathPrefix: "/", HTTPS: false}},
+			existing: &Website{FQDN: "yahoo.com", PathPrefix: "/", HTTPS: false},
 			want:     false,
 		},
 	}
 	for _, tt := range fullTests {
 		t.Run(tt.name, func(t *testing.T) {
-			if got := tt.target.ConflictsWith(tt.existing); got != tt.want {
-				t.Errorf("ConflictsWith() = %v, want %v", got, tt.want)
+			if got := tt.target.conflictsWith(tt.existing); got != tt.want {
+				t.Errorf("conflictsWith() = %v, want %v", got, tt.want)
 			}
 		})
 	}
 }
+
+func TestApplication_WebsiteConflicts(t *testing.T) {
+	u1 := &User{ID: "user1"}
+	u2 := &User{ID: "user2"}
+	u3 := &User{ID: "user3"}
+	admin := &User{ID: "user4", Admin: true}
+	existing := &Application{
+		Websites: []*Website{{
+			FQDN:       "bar.trap.games",
+			PathPrefix: "/",
+		}},
+		OwnerIDs: []string{u2.ID, u3.ID},
+	}
+	tests := []struct {
+		name     string
+		target   *Application
+		existing *Application
+		actor    *User
+		want     bool
+	}{
+		{
+			name: "ok (different fqdn, no conflict)",
+			target: &Application{
+				Websites: []*Website{{
+					FQDN:       "foo.trap.games",
+					PathPrefix: "/api",
+				}},
+				OwnerIDs: []string{u1.ID, u3.ID},
+			},
+			existing: existing,
+			actor:    u1,
+			want:     false,
+		},
+		{
+			name: "ng (conflict, no ownership of the other)",
+			target: &Application{
+				Websites: []*Website{{
+					FQDN:       "bar.trap.games",
+					PathPrefix: "/api",
+				}},
+				OwnerIDs: []string{u1.ID, u3.ID},
+			},
+			existing: existing,
+			actor:    u1,
+			want:     true,
+		},
+		{
+			name: "ng (conflict, owner of the other, but same website)",
+			target: &Application{
+				Websites: []*Website{{
+					FQDN:       "bar.trap.games",
+					PathPrefix: "/",
+				}},
+				OwnerIDs: []string{u1.ID, u3.ID},
+			},
+			existing: existing,
+			actor:    u3,
+			want:     true,
+		},
+		{
+			name: "ng (conflict, actor is admin, but same website)",
+			target: &Application{
+				Websites: []*Website{{
+					FQDN:       "bar.trap.games",
+					PathPrefix: "/",
+				}},
+				OwnerIDs: []string{u1.ID, u3.ID},
+			},
+			existing: existing,
+			actor:    admin,
+			want:     true,
+		},
+		{
+			name: "ok (conflict, but owner of the other)",
+			target: &Application{
+				Websites: []*Website{{
+					FQDN:       "bar.trap.games",
+					PathPrefix: "/api",
+				}},
+				OwnerIDs: []string{u1.ID, u3.ID},
+			},
+			existing: existing,
+			actor:    u3,
+			want:     false,
+		},
+		{
+			name: "ok (conflict, but actor is admin)",
+			target: &Application{
+				Websites: []*Website{{
+					FQDN:       "bar.trap.games",
+					PathPrefix: "/api",
+				}},
+				OwnerIDs: []string{u1.ID, u3.ID},
+			},
+			existing: existing,
+			actor:    admin,
+			want:     false,
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got := tt.target.WebsiteConflicts([]*Application{tt.existing}, tt.actor)
+			assert.Equal(t, tt.want, got)
+		})
+	}
+}