fix: enforce app create/update privileges when getting repo details #8558
Conversation
Codecov Report
@@ Coverage Diff @@
## master #8558 +/- ##
==========================================
+ Coverage 42.41% 42.60% +0.19%
==========================================
Files 176 176
Lines 22904 22941 +37
==========================================
+ Hits 9714 9774 +60
+ Misses 11804 11770 -34
- Partials 1386 1397 +11
Continue to review full report at Codecov.
|
jessesuen
force-pushed
the
fix-enforce-app-privileges
branch
from
565ed28
to
da70648
Compare
|
@alexmt I brought the ProjectLister into the repositories grpc server and it now verifies permitted sources when returning repo details. |
|
Just so we don't forget: as we discussed offlinethe same projection is required for |
jessesuen
force-pushed
the
fix-enforce-app-privileges
branch
2 times, most recently
from
874fd74
to
f5abc72
Compare
Signed-off-by: Jesse Suen <jesse@akuity.io>
Signed-off-by: Jesse Suen <jesse@akuity.io>
Signed-off-by: Jesse Suen <jesse@akuity.io>
jessesuen
force-pushed
the
fix-enforce-app-privileges
branch
from
f5abc72
to
a7cd31c
Compare
Signed-off-by: Jesse Suen <jesse@akuity.io>
jessesuen
force-pushed
the
fix-enforce-app-privileges
branch
from
a7cd31c
to
4624eb1
Compare
|
Is this a potentially breaking change in the API, when previously it was not required to specify both app name and app project? Can't we infer the project from the application's |
Yes, it is a breaking change. That said, since this is a UI-only endpoint, they would only need to refresh the page in their browser and that would fix the issue.
We can only do this if the app exists, but not when this endpoint is called during app creation, which is what we also need to fix. In the case where app already exists, I do have a check to make sure the supplied project in the request matches app.spec.project and returns permission denied if they mismatch. |
Signed-off-by: Jesse Suen <jesse@akuity.io>
I'm not quite sure if I go along the notation of UI only endpoints. It's in the public part of the API, potentially used by people who build their own UI for Argo CD (or use the endpoint otherwise from their clients). Anyhow, I think this change justifies the breakage. IMHO, we should point it out in the release notes that there has been a small breaking change without alternative to being breaking, and users consuming this API endpoint for whatever reason need to adapt their clients. |
| return nil, errPermissionDenied | ||
| } | ||
| // verify caller is not making a request with arbitrary source values which were not in our history | ||
| if !isSourceInHistory(app, *q.Source) { |
There was a problem hiding this comment.
Great catch. I totally forgot about sync history use case.
…rgoproj#8558) Signed-off-by: Jesse Suen <jesse@akuity.io>
…8558) Signed-off-by: Jesse Suen <jesse@akuity.io>
…8558) Signed-off-by: Jesse Suen <jesse@akuity.io>
…8558) Signed-off-by: Jesse Suen <jesse@akuity.io>
…rgoproj#8558) Signed-off-by: Jesse Suen <jesse@akuity.io> Signed-off-by: wojtekidd <wojtek.cichon@protonmail.com>
When getting repo details for an app, we will now require the user to specify both the project + app name for which it wants to create it or update. This ensures that we only return details about the repo if the user has permission to create/update the app.
Signed-off-by: Jesse Suen jesse@akuity.io