741 permissions refactoring

[SofiaSazonova]

Feature or Bugfix

• Refactoring

Detail

• core.permissions refactoring
• separate Service and Repository logic
• separate TenantPermissions, Resource policy-Permissions and Permissions
• move decorators to separate folder

Relates

• 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)?N/A

• Does this PR introduce any functionality or component that requires authorization? N/A

• Are you using or adding any cryptographic features? N/A

• Are you introducing any new policies/roles/users? N/A

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

[dlpzx]

Regarding the code layout for the permissions module, here are some suggestions:

• constants/permissions are basically the core_permissions = environment_permissions, group_permissions, organization_permissions. I think we should include them not as constants but as a part of services like we do in the module (e.g. dataall/modules/dashboards/services/dashboard_permissions.py). We can do this in the permissions module permissions/services/core_permissions.py or in each of the modules: environment/services/environment_permissions.py ....
• A second idea (in which I am less opinionated), is to include the dataall/core/permissions/decorators/permission_checker.py not in a new layer, but in the services layer. Just to keep it simple and not include new layers/components in the design

[dlpzx]

Halfway reviewed!

[dlpzx]

Input validation should happen in api/resolver

[dlpzx]

This one is not fixed (line 42 and 43)

[petrkalos]

As a general comment, I'd split this in at least 2 PRs (maybe a third isn't possible)

1. Automatic rename
2. Move files around
3. Functional changes

By doing so I wouldn't even review n1 and n2

[SofiaSazonova]

@petrkalos sorry, this one is already very mixed(
I will try to do it in next PRs

[SofiaSazonova]

@dlpzx

1. i moved permissions.py from constants to services and renamed to core_permissions
2. decorators are now the part of services

[dlpzx]

How is this operation handled now? Is there any impact for newly deployed data.all infra?

[SofiaSazonova]

I moved the definition to the new class TenantPolicyService, as it seems to belong there.

This function is called in backend/api_handler.py and backend/local_graphql_server.py as it was before.

[dlpzx]

Hi @SofiaSazonova thanks for updating the PR. How have you tested that everything works as expected?

This PR is pretty packed, but I think there are 2 improvements that we should implement once it is merged:

1. separate core_permissions into environment_permissions , organization_permissions... and include them as part of their core module
2. improve the validator of the resolvers. Right now there are a bunch of functions that I think we could condense in less code. This affects not only these modules in core but also other modules. I was thinking of some utility in utils that validates input parameters in a more abstract way. What are your thoughts?

[SofiaSazonova]

@dlpzx

1. Ok)
2. I thought of some changes to more abstract checks, but I'd rather implement it after other refactoring is done, as a separate PR and apply it to all modules as well.

[dlpzx]

It looks good, the only thing is the part of testing. Could you include how have you tested this PR?

[SofiaSazonova]

@dlpzx please have a look. The last commit is better to be reviewed separately as there are a lot of moving of files there

[dlpzx]

We should place the permissions of a module inside the module itself as we do in any other module. For example, take notebooks, we define notebook_permissions in modules/notebooks/service/notebooks_permissions.py. In the same way environment permissions should be defined in core/environments/service/environment_permissions.py And the same applies to other core modules in this last commit

[dlpzx]

Check comment

[SofiaSazonova]

Performed checks

1. User1 from Group1

• creates Env
• creates Dataset
• uploads csv with 1 table and starts crawler
• crawler got 1 table
• queries this table via Worksheet
• invites User2 from Group2 to Env

2. User2 from Group2

• can see Env and data|worksheets in Catalog
• can create consumption role in environment
• can request share of worksheet
• can query the shared worksheet
• can request share of dataset for the selected consumption role
• can query dataset in Athena with the correct Workgroup

[dlpzx]

Morning @SofiaSazonova! Thanks for adding the tests :) :)
One last comment, I suggested moving the permissions to their own core modules, but the permissions seem to be all in the permissions module:

Instead of core/permissions/service/environment_permissions.py ----> core/environments/service/environment_permissions.py, wdyt?

[SofiaSazonova]

@dlpzx Organizations, groups and environments permissions can be moved to its own module, but tenants, resources and networks not, so IMHO it will be confusing: half of permissions here, half there.

[dlpzx]

@dlpzx Organizations, groups and environments permissions can be moved to its own module, but tenants, resources and networks not, so IMHO it will be confusing: half of permissions here, half there.

So the thing is that if I am developing something related to environments I would want to modify code in the environments module only. The only 2 permissions that are not part of a core/module are resources_permissions and tenant_permissions because network_permissions are used in the core/vpc module.

resources_permissions does not define any new permission, it defines the set of permissions RESOURCES_ALL. In the same way we should move some of the tenant permissions to their respective modules (e.g. MANAGE_ORGANIZATIONS to organizations) and tenant_permissions would define the TENANT_ALL only.

[dlpzx]

Looks good. Approving, @SofiaSazonova clarified offline that moving permissions around was causing circular dependencies. If we want to "modularize" core modules further we can do it in a separate PR