Skip to content

Commit cf3801c

Browse files
agaudreaultgithub-actions[bot]
agaudreault
authored and
github-actions[bot]
committed
fix(server): validate new project on update (#23970) (#23973)
Signed-off-by: Alexandre Gaudreault <[email protected]>
1 parent d1dbf20 commit cf3801c

File tree

3 files changed

+146
-25
lines changed

3 files changed

+146
-25
lines changed

‎server/application/application.go‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1288,6 +1288,12 @@ func (s *Server) validateAndNormalizeApp(ctx context.Context, app *v1alpha1.Appl
12881288
if err := s.enf.EnforceErr(ctx.Value("claims"), rbac.ResourceApplications, rbac.ActionUpdate, currApp.RBACName(s.ns)); err != nil {
12891289
return err
12901290
}
1291+
// Validate that the new project exists and the application is allowed to use it
1292+
newProj, err := s.getAppProject(ctx, app, log.WithFields(applog.GetAppLogFields(app)))
1293+
if err != nil {
1294+
return err
1295+
}
1296+
proj = newProj
12911297
}
12921298

12931299
if _, err := argo.GetDestinationCluster(ctx, app.Spec.Destination, s.db); err != nil {

‎server/application/application_test.go‎

Lines changed: 123 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1511,14 +1511,130 @@ func TestCreateAppWithOperation(t *testing.T) {
15111511
}
15121512

15131513
func TestUpdateApp(t *testing.T) {
1514-
testApp := newTestApp()
1515-
appServer := newTestAppServer(t, testApp)
1516-
testApp.Spec.Project = ""
1517-
app, err := appServer.Update(t.Context(), &application.ApplicationUpdateRequest{
1518-
Application: testApp,
1514+
t.Parallel()
1515+
t.Run("Same spec", func(t *testing.T) {
1516+
t.Parallel()
1517+
testApp := newTestApp()
1518+
appServer := newTestAppServer(t, testApp)
1519+
testApp.Spec.Project = ""
1520+
app, err := appServer.Update(t.Context(), &application.ApplicationUpdateRequest{
1521+
Application: testApp,
1522+
})
1523+
require.NoError(t, err)
1524+
assert.Equal(t, "default", app.Spec.Project)
1525+
})
1526+
t.Run("Invalid existing app can be updated", func(t *testing.T) {
1527+
t.Parallel()
1528+
testApp := newTestApp()
1529+
testApp.Spec.Destination.Server = "https://invalid-cluster"
1530+
appServer := newTestAppServer(t, testApp)
1531+
1532+
updateApp := newTestAppWithDestName()
1533+
updateApp.TypeMeta = testApp.TypeMeta
1534+
updateApp.Spec.Source.Name = "updated"
1535+
app, err := appServer.Update(t.Context(), &application.ApplicationUpdateRequest{
1536+
Application: updateApp,
1537+
})
1538+
require.NoError(t, err)
1539+
require.NotNil(t, app)
1540+
assert.Equal(t, "updated", app.Spec.Source.Name)
1541+
})
1542+
t.Run("Can update application project from invalid", func(t *testing.T) {
1543+
t.Parallel()
1544+
testApp := newTestApp()
1545+
restrictedProj := &v1alpha1.AppProject{
1546+
ObjectMeta: metav1.ObjectMeta{Name: "restricted-proj", Namespace: "default"},
1547+
Spec: v1alpha1.AppProjectSpec{
1548+
SourceRepos: []string{"not-your-repo"},
1549+
Destinations: []v1alpha1.ApplicationDestination{{Server: "*", Namespace: "not-your-namespace"}},
1550+
},
1551+
}
1552+
testApp.Spec.Project = restrictedProj.Name
1553+
appServer := newTestAppServer(t, testApp, restrictedProj)
1554+
1555+
updateApp := newTestAppWithDestName()
1556+
updateApp.TypeMeta = testApp.TypeMeta
1557+
updateApp.Spec.Project = "my-proj"
1558+
app, err := appServer.Update(t.Context(), &application.ApplicationUpdateRequest{
1559+
Application: updateApp,
1560+
})
1561+
require.NoError(t, err)
1562+
require.NotNil(t, app)
1563+
assert.Equal(t, "my-proj", app.Spec.Project)
1564+
})
1565+
t.Run("Cannot update application project to invalid", func(t *testing.T) {
1566+
t.Parallel()
1567+
testApp := newTestApp()
1568+
restrictedProj := &v1alpha1.AppProject{
1569+
ObjectMeta: metav1.ObjectMeta{Name: "restricted-proj", Namespace: "default"},
1570+
Spec: v1alpha1.AppProjectSpec{
1571+
SourceRepos: []string{"not-your-repo"},
1572+
Destinations: []v1alpha1.ApplicationDestination{{Server: "*", Namespace: "not-your-namespace"}},
1573+
},
1574+
}
1575+
appServer := newTestAppServer(t, testApp, restrictedProj)
1576+
1577+
updateApp := newTestAppWithDestName()
1578+
updateApp.TypeMeta = testApp.TypeMeta
1579+
updateApp.Spec.Project = restrictedProj.Name
1580+
_, err := appServer.Update(t.Context(), &application.ApplicationUpdateRequest{
1581+
Application: updateApp,
1582+
})
1583+
require.Error(t, err)
1584+
require.ErrorContains(t, err, "application repo https://github.com/argoproj/argocd-example-apps.git is not permitted in project 'restricted-proj'")
1585+
require.ErrorContains(t, err, "application destination server 'fake-cluster' and namespace 'fake-dest-ns' do not match any of the allowed destinations in project 'restricted-proj'")
1586+
})
1587+
t.Run("Cannot update application project to inexisting", func(t *testing.T) {
1588+
t.Parallel()
1589+
testApp := newTestApp()
1590+
appServer := newTestAppServer(t, testApp)
1591+
1592+
updateApp := newTestAppWithDestName()
1593+
updateApp.TypeMeta = testApp.TypeMeta
1594+
updateApp.Spec.Project = "i-do-not-exist"
1595+
_, err := appServer.Update(t.Context(), &application.ApplicationUpdateRequest{
1596+
Application: updateApp,
1597+
})
1598+
require.Error(t, err)
1599+
require.ErrorContains(t, err, "app is not allowed in project \"i-do-not-exist\", or the project does not exist")
1600+
})
1601+
t.Run("Can update application project with project argument", func(t *testing.T) {
1602+
t.Parallel()
1603+
testApp := newTestApp()
1604+
appServer := newTestAppServer(t, testApp)
1605+
1606+
updateApp := newTestAppWithDestName()
1607+
updateApp.TypeMeta = testApp.TypeMeta
1608+
updateApp.Spec.Project = "my-proj"
1609+
app, err := appServer.Update(t.Context(), &application.ApplicationUpdateRequest{
1610+
Application: updateApp,
1611+
Project: ptr.To("default"),
1612+
})
1613+
require.NoError(t, err)
1614+
require.NotNil(t, app)
1615+
assert.Equal(t, "my-proj", app.Spec.Project)
1616+
})
1617+
t.Run("Existing label and annotations are replaced", func(t *testing.T) {
1618+
t.Parallel()
1619+
testApp := newTestApp()
1620+
testApp.Annotations = map[string]string{"test": "test-value", "update": "old"}
1621+
testApp.Labels = map[string]string{"test": "test-value", "update": "old"}
1622+
appServer := newTestAppServer(t, testApp)
1623+
1624+
updateApp := newTestAppWithDestName()
1625+
updateApp.TypeMeta = testApp.TypeMeta
1626+
updateApp.Annotations = map[string]string{"update": "new"}
1627+
updateApp.Labels = map[string]string{"update": "new"}
1628+
app, err := appServer.Update(t.Context(), &application.ApplicationUpdateRequest{
1629+
Application: updateApp,
1630+
})
1631+
require.NoError(t, err)
1632+
require.NotNil(t, app)
1633+
assert.Len(t, app.Annotations, 1)
1634+
assert.Equal(t, "new", app.GetAnnotations()["update"])
1635+
assert.Len(t, app.Labels, 1)
1636+
assert.Equal(t, "new", app.GetLabels()["update"])
15191637
})
1520-
require.NoError(t, err)
1521-
assert.Equal(t, "default", app.Spec.Project)
15221638
}
15231639

15241640
func TestUpdateAppSpec(t *testing.T) {

‎util/argo/argo.go‎

Lines changed: 17 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -564,7 +564,7 @@ func ValidatePermissions(ctx context.Context, spec *argoappv1.ApplicationSpec, p
564564
if !proj.IsSourcePermitted(spec.SourceHydrator.GetDrySource()) {
565565
conditions = append(conditions, argoappv1.ApplicationCondition{
566566
Type: argoappv1.ApplicationConditionInvalidSpecError,
567-
Message: fmt.Sprintf("application repo %s is not permitted in project '%s'", spec.GetSource().RepoURL, spec.Project),
567+
Message: fmt.Sprintf("application repo %s is not permitted in project '%s'", spec.SourceHydrator.GetDrySource().RepoURL, proj.Name),
568568
})
569569
}
570570
case spec.HasMultipleSources():
@@ -578,7 +578,7 @@ func ValidatePermissions(ctx context.Context, spec *argoappv1.ApplicationSpec, p
578578
if !proj.IsSourcePermitted(source) {
579579
conditions = append(conditions, argoappv1.ApplicationCondition{
580580
Type: argoappv1.ApplicationConditionInvalidSpecError,
581-
Message: fmt.Sprintf("application repo %s is not permitted in project '%s'", source.RepoURL, spec.Project),
581+
Message: fmt.Sprintf("application repo %s is not permitted in project '%s'", source.RepoURL, proj.Name),
582582
})
583583
}
584584
}
@@ -591,7 +591,7 @@ func ValidatePermissions(ctx context.Context, spec *argoappv1.ApplicationSpec, p
591591
if !proj.IsSourcePermitted(spec.GetSource()) {
592592
conditions = append(conditions, argoappv1.ApplicationCondition{
593593
Type: argoappv1.ApplicationConditionInvalidSpecError,
594-
Message: fmt.Sprintf("application repo %s is not permitted in project '%s'", spec.GetSource().RepoURL, spec.Project),
594+
Message: fmt.Sprintf("application repo %s is not permitted in project '%s'", spec.GetSource().RepoURL, proj.Name),
595595
})
596596
}
597597
}
@@ -604,22 +604,21 @@ func ValidatePermissions(ctx context.Context, spec *argoappv1.ApplicationSpec, p
604604
})
605605
return conditions, nil
606606
}
607-
608-
if destCluster.Server != "" {
609-
permitted, err := proj.IsDestinationPermitted(destCluster, spec.Destination.Namespace, func(project string) ([]*argoappv1.Cluster, error) {
610-
return db.GetProjectClusters(ctx, project)
611-
})
612-
if err != nil {
613-
return nil, err
614-
}
615-
if !permitted {
616-
conditions = append(conditions, argoappv1.ApplicationCondition{
617-
Type: argoappv1.ApplicationConditionInvalidSpecError,
618-
Message: fmt.Sprintf("application destination server '%s' and namespace '%s' do not match any of the allowed destinations in project '%s'", spec.Destination.Server, spec.Destination.Namespace, spec.Project),
619-
})
607+
permitted, err := proj.IsDestinationPermitted(destCluster, spec.Destination.Namespace, func(project string) ([]*argoappv1.Cluster, error) {
608+
return db.GetProjectClusters(ctx, project)
609+
})
610+
if err != nil {
611+
return nil, err
612+
}
613+
if !permitted {
614+
server := destCluster.Server
615+
if spec.Destination.Name != "" {
616+
server = destCluster.Name
620617
}
621-
} else if destCluster.Server == "" {
622-
conditions = append(conditions, argoappv1.ApplicationCondition{Type: argoappv1.ApplicationConditionInvalidSpecError, Message: ErrDestinationMissing})
618+
conditions = append(conditions, argoappv1.ApplicationCondition{
619+
Type: argoappv1.ApplicationConditionInvalidSpecError,
620+
Message: fmt.Sprintf("application destination server '%s' and namespace '%s' do not match any of the allowed destinations in project '%s'", server, spec.Destination.Namespace, proj.Name),
621+
})
623622
}
624623
return conditions, nil
625624
}

0 commit comments

Comments
 (0)