Skip to content
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

Merged
merged 2 commits into from
Dec 3, 2024

Conversation

dlpzx
Copy link
Contributor

@dlpzx dlpzx commented Nov 21, 2024

Feature or Bugfix

  • Refactoring
  • Feature

Detail

  • 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

Testing:

  • CICD deployment completes
  • Add permissions to getTrustedAccount API
    • in environment creation form view we can get the trusted account
  • Remove usage of central account in administrator view dashboard tab
    • admin view renders without issue
  • refactor environment Service functions to use decorator for resource policies
    • enable_subscriptions with unauthorized user = unauthorized
    • enable_subscriptions with authorized user = success
    • disable_subscriptions with unauthorized user = unauthorized
    • disable_subscriptions with authorized user = success
    • get environment assume role url with unauthorized user = unauthorized -- it throws error of user does not belong to group
    • get environment assume role url with authorized user = success
    • get environment access token with unauthorized user = unauthorized
    • 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
    • Now we get an Unauthorized error message when LINK_ENVIRONMENT permissions are missing before hitting the create Environment button

Relates

Security

Please answer the questions below briefly where applicable, or write N/A. Based on
OWASP 10.

  • 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.

@dlpzx dlpzx marked this pull request as draft November 21, 2024 15:23
@dlpzx dlpzx force-pushed the fix/add-more-checks-clean-permissions branch from 8305aa4 to f7ce0c4 Compare November 21, 2024 16:01
@dlpzx dlpzx force-pushed the fix/add-more-checks-clean-permissions branch from f7ce0c4 to 1e0d54f Compare November 22, 2024 09:34
@dlpzx dlpzx marked this pull request as ready for review November 22, 2024 10:58
Comment on lines 22 to 24
createQuicksightDataSourceSet,
getMonitoringDashboardId,
getMonitoringVPCConnectionId,
Copy link
Contributor

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

Copy link
Contributor

@noah-paige noah-paige left a 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

Copy link
Contributor

@noah-paige noah-paige left a 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 that getMonitoringDashboardId, 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

@dlpzx
Copy link
Contributor Author

dlpzx commented Nov 27, 2024

Thanks @noah-paige, I will remove (1) and fix (2) in a separate PR. This one is getting out of hands :)

@dlpzx
Copy link
Contributor Author

dlpzx commented Nov 28, 2024

@noah-paige Done! I removed the unused getPlatformAuthorSession

@dlpzx dlpzx merged commit 32d02ad into main Dec 3, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants