-
Notifications
You must be signed in to change notification settings - Fork 81
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactor env permissions + modify getTrustAccount #1712
Conversation
8305aa4
to
f7ce0c4
Compare
f7ce0c4
to
1e0d54f
Compare
createQuicksightDataSourceSet, | ||
getMonitoringDashboardId, | ||
getMonitoringVPCConnectionId, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Existed before but all of the following use ParameterStore and try to assume into pivotRole in infra account to get SSM (pivot role DNE in infra account)
- createQuicksightDataSourceSet,
- getMonitoringDashboardId,
- getMonitoringVPCConnectionId,
- updateSSMParameter
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one comment on QS Monitoring feature - but refactoring looks good
Can you list testing you've done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-
(1) Do we want to clean up
getPlatformAuthorSession
- do not believe it is used anywhere now -
(2) I know not the primary focus of this PR but want to call out that when checking if the APIs used in
AdministratorDasbhoardViewer.js
I found thatgetMonitoringDashboardId
,getMonitoringVPCConnectionId
, +updateSSMParameter
all will throw exceptions since trying to assume pivotRole in the dataall infra account before fetching/updating an SSM Param- Should we fix in this PR? If not this PR lets open a new one with a fix
Thanks @noah-paige, I will remove (1) and fix (2) in a separate PR. This one is getting out of hands :) |
@noah-paige Done! I removed the unused getPlatformAuthorSession |
- Refactoring - Feature - Add permissions to getTrustedAccount API - Remove usage of central account in administrator view dashboard tab - refactor environment Service functions to use decorator for resource policies - Added LINK_ENVIRONMENT permissions instead of GET_ORGANIZATION to `get_pivot_role`, `get_external_id`, `get_pivot_role_template` - [X] CICD deployment completes - Add permissions to getTrustedAccount API - [X] in environment creation form view we can get the trusted account - Remove usage of central account in administrator view dashboard tab - [X] admin view renders without issue - refactor environment Service functions to use decorator for resource policies - [X] enable_subscriptions with unauthorized user = unauthorized - [X] enable_subscriptions with authorized user = success - [X] disable_subscriptions with unauthorized user = unauthorized - [X] disable_subscriptions with authorized user = success - [X] get environment assume role url with unauthorized user = unauthorized -- it throws error of user does not belong to group - [X] get environment assume role url with authorized user = success - [X] get environment access token with unauthorized user = unauthorized - [X] get environment access token with authorized user = success - Added LINK_ENVIRONMENT permissions instead of GET_ORGANIZATION to `get_pivot_role`, `get_external_id`, `get_pivot_role_template` - [X] Now we get an Unauthorized error message when LINK_ENVIRONMENT permissions are missing before hitting the create Environment button - <URL or Ticket> Please answer the questions below briefly where applicable, or write `N/A`. Based on [OWASP 10](https://owasp.org/Top10/en/). - Does this PR introduce or modify any input fields or queries - this includes fetching data from storage outside the application (e.g. a database, an S3 bucket)? - Is the input sanitized? - What precautions are you taking before deserializing the data you consume? - Is injection prevented by parametrizing queries? - Have you ensured no `eval` or similar functions are used? - Does this PR introduce any functionality or component that requires authorization? - How have you ensured it respects the existing AuthN/AuthZ mechanisms? - Are you logging failed auth attempts? - Are you using or adding any cryptographic features? - Do you use a standard proven implementations? - Are the used keys controlled by the customer? Where are they stored? - Are you introducing any new policies/roles/users? - Have you used the least-privilege principle? How? By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
Feature or Bugfix
Detail
get_pivot_role
,get_external_id
,get_pivot_role_template
Testing:
get_pivot_role
,get_external_id
,get_pivot_role_template
Relates
Security
Please answer the questions below briefly where applicable, or write
N/A
. Based onOWASP 10.
fetching data from storage outside the application (e.g. a database, an S3 bucket)?
eval
or similar functions are used?By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.