dataall icon indicating copy to clipboard operation
dataall copied to clipboard

In Admin settings Teams Permission Dialog Opens Blank

Open sandeephs1 opened this issue 1 year ago • 6 comments

In Admin settings page -> Teams -> Click "Permissions" => Blank Page Opens.

It should have opened, Tenant Permissions list, where "Manage ----" set of permissions for team can be set.

When we checked the API response, Noticed a "null," attribute (Refer below snippet: Between Manage Datasets and Worksheets)

Appreciate your suggestion to Resolve this issue

{
    "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"
            },
            {
                "name": "MANAGE_GLOSSARIES",
                "description": "Manage glossaries",
                "__typename": "Permission"
            },
            {
                "name": "MANAGE_DATASETS",
                "description": "Manage datasets",
                "__typename": "Permission"
            },
            null,
            {
                "name": "MANAGE_WORKSHEETS",
                "description": "Manage worksheets",
                "__typename": "Permission"
            }
        ]
    }
}

sandeephs1 avatar Feb 14 '24 10:02 sandeephs1

Hi @sandeephs1, thanks for opening an issue. I have tried to reproduce your issue but I do not get the null. From what I see, you do not have all modules active, so maybe the issue is there. Is there any module that should be there and is not?

The code that lists the permissions is in dataall/core/permissions/db/tenant_policy_repositories.py My guess is that the functionfind_permission_by_name is returning null in one case and it is appending it.

    @staticmethod
    def list_tenant_permissions(session, username, groups):
        if not TenantPolicy.is_tenant_admin(groups):
            raise exceptions.UnauthorizedOperation(
                action='LIST_TENANT_TEAM_PERMISSIONS',
                message=f'User: {username} is not allowed to manage tenant permissions',
            )
        group_invitation_permissions = []
        for p in permissions.TENANT_ALL:
            group_invitation_permissions.append(
                Permission.find_permission_by_name(
                    session=session,
                    permission_name=p,
                    permission_type=PermissionType.TENANT.name,
                )
            )
        return group_invitation_permissions

If we take a look at the find_permission_by_name function we see that it queries the Permission table. The next step would be to compare the permission that is giving the null (from a module that is enabled?) and then try to find it in the RDS table.

We will debug again tomorrow, if you are still blocked we can schedule a meeting

dlpzx avatar Feb 20 '24 20:02 dlpzx

Thanks @dlpzx yes "MANAGE_DASHBOARDS" was missing, which was enabled recently. Updated type in "permission" table. it resolved the issue

sandeephs1 avatar Feb 23 '24 10:02 sandeephs1

I think this issue will be solved by #1145 @petrkalos can you confirm?

dlpzx avatar Apr 09 '24 13:04 dlpzx

@dlpzx I just checked and indeed the issue will be fixed by #1145. To test it I first reproduced the issue (I removed the rows manually and got the blank screen) and then re-executed the lambda and it started working again.

Nevertheless, I wonder if the logic in list_tenant_permissions is correct. Why do we iterate over TENANT_ALL instead of querying what already exists in the database, i.e use find_permission_by_type(permission_type=PermissionType.TENANT.name)?

petrkalos avatar Apr 10 '24 11:04 petrkalos

Good point @petrkalos, I think that it checks it this way because TENANT_ALL contains all ACTIVE modules tenant permissions and then it goes and finds the permission in RDS. If we just filter from RDS we would get all permissions in RDS independently from whether the module is active or not. We could do it the other way around, list the RDS permissions and check which ones are in TENANT_ALL, but I prefer the error to show that there are missing permissions in RDS

    @staticmethod
    def list_tenant_permissions():
        context = get_context()
        username = context.username
        groups = context.groups

        TenantPolicyValidationService.validate_admin_access(username, groups, 'LIST_TENANT_TEAM_PERMISSIONS')

        group_invitation_permissions = []
        with context.db_engine.scoped_session() as session:
            for p in TENANT_ALL:
                group_invitation_permissions.append(
                    PermissionRepository.find_permission_by_name(
                        session=session,
                        permission_name=p,
                        permission_type=PermissionType.TENANT.name,
                    )
                )
            return group_invitation_permissions

dlpzx avatar Apr 10 '24 12:04 dlpzx

It doesn't feel like this is the correct place to check if the app permissions are in sync with the DB and vice versa. I think we must trust that the DB is the ultimate source of truth.

petrkalos avatar Apr 10 '24 12:04 petrkalos

Then we need to delete the permissions for the disabled modules whenever a module is disabled, which is a bit scary. What do you think?

dlpzx avatar Apr 11 '24 09:04 dlpzx

Indeed, that's complicating matters more. I guess so far when we disable a module it doesn't mean that we are also wiping the data that it generated when it was active, right?

petrkalos avatar Apr 11 '24 12:04 petrkalos

Exactly, that is the current behavior. Although cleaning up the RDS database looks good in theory I am afraid that in real life it could happen that someone disables a module by mistake and that could be a bit catastrophical - many ghost resources created by data.all and not visible in data.all.

dlpzx avatar Apr 11 '24 13:04 dlpzx

I'll close this item as complete, if in the future we want to revisit the way we initialize permissions it can be reopened

dlpzx avatar Apr 11 '24 14:04 dlpzx