Refactoring env stack part4

[SofiaSazonova]

Feature or Bugfix

• Refactoring

Detail

• detangle StackRepository & StackService & EnvironmentService
• Move logic from resolvers and repositories to services
• Remove unused params

Relates

• Standardize core modules and cleanup dead code #741

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]

I think get_environmental_stack_by_uri is not needed. We can call get_stack_by_uri directly from get_and_describe_stack_in_env because the environment permissions check was done previously in the resolver get_stack when we call find_environment_by_uri, the decorator is already checking the GET_ENVIRONMENT permissions

[SofiaSazonova]

It's still needed for get_stack_logs, but here I replaced it with get_stack_by_uri
5ddf7b3

[dlpzx]

But if we think about it, checking GET_ENVIRONMENT permissions when we get ANY type of stack does not make much sense. We should check GET_ENVIRONMENT when we get the Environment stack, GET_DATASET for the Dataset stack and so on. But instead we check GET_ENVIRONMENT when we get the dataset, which means that a user could execute an API call to get other teams' infrastructure stack details if they are in the same environment.

To fix this we could use something similar to what is implemented in update_stack_by_target_uri or in list_key_value_tags

ResourcePolicyService.check_user_resource_permission(
                session=session,
                username=context.username,
                groups=context.groups,
                resource_uri=target_uri,
                permission_name=TargetType.get_resource_read_permission_name(target_type),
            )

[noah-paige]

1 - can we remove get_environmental_stack_by_uri since it is no longer used?

[noah-paige]

2 - Can we add targetUri and targetType as additional inputs in getStack and then add a permission check block similar to the one @dlpzx added above for get_and_describe_stack_in_env

• pass through targetUri and targetType to getStack API

• in get_and_describe_stack_in_env add:

            ResourcePolicyService.check_user_resource_permission(
                session=session,
                username=context.username,
                groups=context.groups,
                resource_uri=stack.targetUri,
                permission_name=TargetType.get_resource_read_permission_name(stack.targetType),
            )

• Update get_stack_by_uri to pass along session

[noah-paige]

This #2 above is a bigger change than originally anticipated - I have been looking into a bit and can open an enhancement PR after this one and can push this through if we clean up get_environmental_stack_by_uri and address type hints comment

[noah-paige]

Link to draft PR addressing permission checks described above - #1182

[petrkalos]

General comment about the refactoring...

• doesn't the existence of the resolvers.py become redundant? They are now just proxy calls so can't we just point the resolver (where the GraphQL query is defined) directly to the service?
• by creating the session within the business logic it's going to be more difficult to unit-test those methods because it will by harder to mock the results from the db. Right?

[SofiaSazonova]

Well, it's open question yet. At least it gives us asyncronous calls (like in get_logs)

[dlpzx]

The principle was that in resolvers we check the api input and in service we just execute business logic. In the PRs for refactoring we are adding validation in the service layer but not in the resolvers. I did not add any comment in these PRs, because we already talked about implementing a better validation mechanism. I opened an issue so that we do not forget about it

[noah-paige]

Tested creating Environments, describing env stacks, describing dataset stacks, etc.

All looks good!! (have one related PR regarding some additional stack clean up parts here but I think is not of immediate priority and to come after this PR)

[noah-paige]

approving!