Skip to content

Commit f17706c

Browse files
agaudreaultagaudreault
authored
fix(server): validate new project on update (argoproj#23970) (argoproj#23973)
Signed-off-by: Alexandre Gaudreault <[email protected]>
1 parent bb8f9b3 commit f17706c

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
@@ -1317,6 +1317,12 @@ func (s *Server) validateAndNormalizeApp(ctx context.Context, app *v1alpha1.Appl
13171317
if err := s.enf.EnforceErr(ctx.Value("claims"), rbac.ResourceApplications, rbac.ActionUpdate, currApp.RBACName(s.ns)); err != nil {
13181318
return err
13191319
}
1320+
// Validate that the new project exists and the application is allowed to use it
1321+
newProj, err := s.getAppProject(ctx, app, log.WithFields(applog.GetAppLogFields(app)))
1322+
if err != nil {
1323+
return err
1324+
}
1325+
proj = newProj
13201326
}
13211327

13221328
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
@@ -1608,14 +1608,130 @@ func TestCreateAppUpsert(t *testing.T) {
16081608
}
16091609

16101610
func TestUpdateApp(t *testing.T) {
1611-
testApp := newTestApp()
1612-
appServer := newTestAppServer(t, testApp)
1613-
testApp.Spec.Project = ""
1614-
app, err := appServer.Update(t.Context(), &application.ApplicationUpdateRequest{
1615-
Application: testApp,
1611+
t.Parallel()
1612+
t.Run("Same spec", func(t *testing.T) {
1613+
t.Parallel()
1614+
testApp := newTestApp()
1615+
appServer := newTestAppServer(t, testApp)
1616+
testApp.Spec.Project = ""
1617+
app, err := appServer.Update(t.Context(), &application.ApplicationUpdateRequest{
1618+
Application: testApp,
1619+
})
1620+
require.NoError(t, err)
1621+
assert.Equal(t, "default", app.Spec.Project)
1622+
})
1623+
t.Run("Invalid existing app can be updated", func(t *testing.T) {
1624+
t.Parallel()
1625+
testApp := newTestApp()
1626+
testApp.Spec.Destination.Server = "https://invalid-cluster"
1627+
appServer := newTestAppServer(t, testApp)
1628+
1629+
updateApp := newTestAppWithDestName()
1630+
updateApp.TypeMeta = testApp.TypeMeta
1631+
updateApp.Spec.Source.Name = "updated"
1632+
app, err := appServer.Update(t.Context(), &application.ApplicationUpdateRequest{
1633+
Application: updateApp,
1634+
})
1635+
require.NoError(t, err)
1636+
require.NotNil(t, app)
1637+
assert.Equal(t, "updated", app.Spec.Source.Name)
1638+
})
1639+
t.Run("Can update application project from invalid", func(t *testing.T) {
1640+
t.Parallel()
1641+
testApp := newTestApp()
1642+
restrictedProj := &v1alpha1.AppProject{
1643+
ObjectMeta: metav1.ObjectMeta{Name: "restricted-proj", Namespace: "default"},
1644+
Spec: v1alpha1.AppProjectSpec{
1645+
SourceRepos: []string{"not-your-repo"},
1646+
Destinations: []v1alpha1.ApplicationDestination{{Server: "*", Namespace: "not-your-namespace"}},
1647+
},
1648+
}
1649+
testApp.Spec.Project = restrictedProj.Name
1650+
appServer := newTestAppServer(t, testApp, restrictedProj)
1651+
1652+
updateApp := newTestAppWithDestName()
1653+
updateApp.TypeMeta = testApp.TypeMeta
1654+
updateApp.Spec.Project = "my-proj"
1655+
app, err := appServer.Update(t.Context(), &application.ApplicationUpdateRequest{
1656+
Application: updateApp,
1657+
})
1658+
require.NoError(t, err)
1659+
require.NotNil(t, app)
1660+
assert.Equal(t, "my-proj", app.Spec.Project)
1661+
})
1662+
t.Run("Cannot update application project to invalid", func(t *testing.T) {
1663+
t.Parallel()
1664+
testApp := newTestApp()
1665+
restrictedProj := &v1alpha1.AppProject{
1666+
ObjectMeta: metav1.ObjectMeta{Name: "restricted-proj", Namespace: "default"},
1667+
Spec: v1alpha1.AppProjectSpec{
1668+
SourceRepos: []string{"not-your-repo"},
1669+
Destinations: []v1alpha1.ApplicationDestination{{Server: "*", Namespace: "not-your-namespace"}},
1670+
},
1671+
}
1672+
appServer := newTestAppServer(t, testApp, restrictedProj)
1673+
1674+
updateApp := newTestAppWithDestName()
1675+
updateApp.TypeMeta = testApp.TypeMeta
1676+
updateApp.Spec.Project = restrictedProj.Name
1677+
_, err := appServer.Update(t.Context(), &application.ApplicationUpdateRequest{
1678+
Application: updateApp,
1679+
})
1680+
require.Error(t, err)
1681+
require.ErrorContains(t, err, "application repo https://github.com/argoproj/argocd-example-apps.git is not permitted in project 'restricted-proj'")
1682+
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'")
1683+
})
1684+
t.Run("Cannot update application project to inexisting", func(t *testing.T) {
1685+
t.Parallel()
1686+
testApp := newTestApp()
1687+
appServer := newTestAppServer(t, testApp)
1688+
1689+
updateApp := newTestAppWithDestName()
1690+
updateApp.TypeMeta = testApp.TypeMeta
1691+
updateApp.Spec.Project = "i-do-not-exist"
1692+
_, err := appServer.Update(t.Context(), &application.ApplicationUpdateRequest{
1693+
Application: updateApp,
1694+
})
1695+
require.Error(t, err)
1696+
require.ErrorContains(t, err, "app is not allowed in project \"i-do-not-exist\", or the project does not exist")
1697+
})
1698+
t.Run("Can update application project with project argument", func(t *testing.T) {
1699+
t.Parallel()
1700+
testApp := newTestApp()
1701+
appServer := newTestAppServer(t, testApp)
1702+
1703+
updateApp := newTestAppWithDestName()
1704+
updateApp.TypeMeta = testApp.TypeMeta
1705+
updateApp.Spec.Project = "my-proj"
1706+
app, err := appServer.Update(t.Context(), &application.ApplicationUpdateRequest{
1707+
Application: updateApp,
1708+
Project: ptr.To("default"),
1709+
})
1710+
require.NoError(t, err)
1711+
require.NotNil(t, app)
1712+
assert.Equal(t, "my-proj", app.Spec.Project)
1713+
})
1714+
t.Run("Existing label and annotations are replaced", func(t *testing.T) {
1715+
t.Parallel()
1716+
testApp := newTestApp()
1717+
testApp.Annotations = map[string]string{"test": "test-value", "update": "old"}
1718+
testApp.Labels = map[string]string{"test": "test-value", "update": "old"}
1719+
appServer := newTestAppServer(t, testApp)
1720+
1721+
updateApp := newTestAppWithDestName()
1722+
updateApp.TypeMeta = testApp.TypeMeta
1723+
updateApp.Annotations = map[string]string{"update": "new"}
1724+
updateApp.Labels = map[string]string{"update": "new"}
1725+
app, err := appServer.Update(t.Context(), &application.ApplicationUpdateRequest{
1726+
Application: updateApp,
1727+
})
1728+
require.NoError(t, err)
1729+
require.NotNil(t, app)
1730+
assert.Len(t, app.Annotations, 1)
1731+
assert.Equal(t, "new", app.GetAnnotations()["update"])
1732+
assert.Len(t, app.Labels, 1)
1733+
assert.Equal(t, "new", app.GetLabels()["update"])
16161734
})
1617-
require.NoError(t, err)
1618-
assert.Equal(t, "default", app.Spec.Project)
16191735
}
16201736

16211737
func TestUpdateAppSpec(t *testing.T) {

‎util/argo/argo.go‎

Lines changed: 17 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -589,7 +589,7 @@ func ValidatePermissions(ctx context.Context, spec *argoappv1.ApplicationSpec, p
589589
if !proj.IsSourcePermitted(spec.SourceHydrator.GetDrySource()) {
590590
conditions = append(conditions, argoappv1.ApplicationCondition{
591591
Type: argoappv1.ApplicationConditionInvalidSpecError,
592-
Message: fmt.Sprintf("application repo %s is not permitted in project '%s'", spec.GetSource().RepoURL, spec.Project),
592+
Message: fmt.Sprintf("application repo %s is not permitted in project '%s'", spec.SourceHydrator.GetDrySource().RepoURL, proj.Name),
593593
})
594594
}
595595
case spec.HasMultipleSources():
@@ -603,7 +603,7 @@ func ValidatePermissions(ctx context.Context, spec *argoappv1.ApplicationSpec, p
603603
if !proj.IsSourcePermitted(source) {
604604
conditions = append(conditions, argoappv1.ApplicationCondition{
605605
Type: argoappv1.ApplicationConditionInvalidSpecError,
606-
Message: fmt.Sprintf("application repo %s is not permitted in project '%s'", source.RepoURL, spec.Project),
606+
Message: fmt.Sprintf("application repo %s is not permitted in project '%s'", source.RepoURL, proj.Name),
607607
})
608608
}
609609
}
@@ -616,7 +616,7 @@ func ValidatePermissions(ctx context.Context, spec *argoappv1.ApplicationSpec, p
616616
if !proj.IsSourcePermitted(spec.GetSource()) {
617617
conditions = append(conditions, argoappv1.ApplicationCondition{
618618
Type: argoappv1.ApplicationConditionInvalidSpecError,
619-
Message: fmt.Sprintf("application repo %s is not permitted in project '%s'", spec.GetSource().RepoURL, spec.Project),
619+
Message: fmt.Sprintf("application repo %s is not permitted in project '%s'", spec.GetSource().RepoURL, proj.Name),
620620
})
621621
}
622622
}
@@ -629,22 +629,21 @@ func ValidatePermissions(ctx context.Context, spec *argoappv1.ApplicationSpec, p
629629
})
630630
return conditions, nil
631631
}
632-
633-
if destCluster.Server != "" {
634-
permitted, err := proj.IsDestinationPermitted(destCluster, spec.Destination.Namespace, func(project string) ([]*argoappv1.Cluster, error) {
635-
return db.GetProjectClusters(ctx, project)
636-
})
637-
if err != nil {
638-
return nil, err
639-
}
640-
if !permitted {
641-
conditions = append(conditions, argoappv1.ApplicationCondition{
642-
Type: argoappv1.ApplicationConditionInvalidSpecError,
643-
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),
644-
})
632+
permitted, err := proj.IsDestinationPermitted(destCluster, spec.Destination.Namespace, func(project string) ([]*argoappv1.Cluster, error) {
633+
return db.GetProjectClusters(ctx, project)
634+
})
635+
if err != nil {
636+
return nil, err
637+
}
638+
if !permitted {
639+
server := destCluster.Server
640+
if spec.Destination.Name != "" {
641+
server = destCluster.Name
645642
}
646-
} else if destCluster.Server == "" {
647-
conditions = append(conditions, argoappv1.ApplicationCondition{Type: argoappv1.ApplicationConditionInvalidSpecError, Message: ErrDestinationMissing})
643+
conditions = append(conditions, argoappv1.ApplicationCondition{
644+
Type: argoappv1.ApplicationConditionInvalidSpecError,
645+
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),
646+
})
648647
}
649648
return conditions, nil
650649
}

0 commit comments

Comments
 (0)