data.all permission table has incorrect permissions after install

[zsaltys]

Describe the bug

After we installed data.all 2.3 we noticed that we cannot view any team permissions in Admin console and were getting a JS error. This is because the graphql endpoint was returning this:

{
  "data": {
    "listTenantPermissions": [
      {
        "name": "MANAGE_GROUPS",
        "description": "Manage teams",
        "__typename": "Permission"
      },
      {
        "name": "MANAGE_ENVIRONMENTS",
        "description": "Manage environments",
        "__typename": "Permission"
      },
      {
        "name": "MANAGE_ORGANIZATIONS",
        "description": "Manage organizations",
        "__typename": "Permission"
      },
      null,
      null,
      null
    ]
  }
}

Notice the nulls.

After we looked at the permission table it looked like this:

Notice the last 3 permissions. In our case they shouldn't be there. Once I deleted one of the permissions:

delete from DBNAME.permission WHERE "type"='TENANT' and "name" = 'MANAGE_SGMSTUDIO_USERS';

and reloaded the UI I was then able to list team permissions and the table now looks like this:

It seems there's a bug in L121 of dataall/backend/dataall/core/permissions/db/permission_repositories.py which doesn't correctly detect that permissions should be updated because it's based on count.

However I think the problem is even earlier and the db migrations should set the table into correct state on install and we shouldn't need to fix permissions during graphql api calls but I don't know why it was done this way.. I just can say it's unusual and it would make more sense if the db migrations would set the db into correct state on install.

How to Reproduce

*P.S. Please do not attach files as it's considered a security risk. Add code snippets directly in the message body as much as possible.*

Expected behavior

On fresh install of data.all 2.3 permission table should have correct state for tenant permissions.

Your project

No response

Screenshots

No response

OS

N/A

Python version

N/A

AWS data.all version

2.3

Additional context

No response

[noah-paige]

Hi @zsaltys - thank you for raising this issue. The above error of saving the initial set of Tenant Permissions is one that would only arise when doing an initial deploy of data.all with a particular subset of modules disabled in config.json.

Nonetheless, I do agree there are a number of improvements that we can do to better save permissions to the DB tables

1. Firstly, we do initialize the permissions in the table in both the DB Migration script (we have migration scripts that call init_permissions()) as well as in api_handler.py (here more as a safeguard - i.e. if DA Admin enabled 4 new modules need to init module-specific permissions somewhere)

• Maybe we can always call init_permissions() as part of the DB Migration CodeBuild (separate from pre-existing migration scripts) and remove entirely from api_handler.py (agreed that it feels out of place a bit here)

2. We need to change the ways we compare if the required permissions are already present in the table

• Instead of doing count checks on permissions - query to see if permission exists and if not insert it into the permissions table (this would be rewriting the init_permission() function a bit)

3. (Going Further) We currently only add permissions to the table if they do not exist but we do not delete permissions/ clean up (e.g. if we disabled Notebooks module we do not remove MANAGE_NOTEBOOK permission)

• Currently, this does not pose as an issue for data.all as the permissions table is infrequently written to and we have other backend and frontend conditional logic to prevent users interacting with module-specific code if module is in a disabled
• Just calling out here to see if others can think of any need we may want to implement some clean up logic to permissions table
  • Others things to consider is the foreign key relationship between permission table and other permission_policy tables and groups --> meaning it is not as simple as a delete operation on the permission table to clean up and may have larger implications

[dlpzx]

I think this is solved by #1145 right @petrkalos?

[petrkalos]

Indeed, that should be solved by #1145, I am closing the issue, please reopen if it's not solved.