data-dot-all / dataall

A modern data marketplace that makes collaboration among diverse users (like business, analysts and engineers) easier, increasing efficiency and agility in data projects on AWS.
https://data-dot-all.github.io/dataall/
Apache License 2.0
226 stars 82 forks source link

Admin Settings view doesn't load Cognito groups as Tenant Teams until users in those groups log in #825

Open rbernotas opened 10 months ago

rbernotas commented 10 months ago

Describe the bug

Groups/teams don't actually show up in the data.all Admin Settings UI view, until at least one user who is a member of the team logs on to data.all (I assume it's not pulling the cognito groups as a whole for this screen, but rather the teams aren't populated for the admin until the user shows up). So the flow is a bit broken for an Admin that wants to set permissions on Teams, just after they created the groups in Cognito.

Admin creates groups in Cognito -> assigns users to group -> admin can't see respective Team in Admin Settings until a user in the Group signs in again.

This means I can't actually toggle the permissions for these Teams until one of the users signs back in at an indeterminate time. It seems like that Admin Settings view should pull the groups straight from Cognito at the point the Admin asks for them, even if there are no users in some of the Groups and/or the users haven't signed in yet. Should be able to set the permissions on the Teams preemptively, not after the fact. If they're set after the fact, it opens up the opportunity for a new Team to create and manage things we don't want them to (before the admin notices the team members have logged on), because the default permissions assigned to a team are that all of the permissions are toggled on.

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

Admin creates groups in Cognito -> assigns users to group -> admin can't see respective Team in Admin Settings until a user in the Group signs in again.

Expected behavior

I would expect as the Admin, that I should be able to administer the settings for a Team as soon as that group is created in Cognito.

Additionally it is probably worth some discussion if a new Team should have its permissions toggled on by default (Manage teams, manage environments, manage orgs, manage datasets)

Your project

No response

Screenshots

No response

OS

n/a

Python version

n/a

AWS data.all version

2.0

Additional context

No response

dlpzx commented 10 months ago

Hi @rbernotas thanks for opening an issue. Yes, you are right about the behavior:

If I understand it correctly your issue is that there is no way of controlling permissions until a user logs in resulting in over-permissive permissions to new groups. At first sight we got 2 options each focused on one of the points above:

Which one would suit you best?

rbernotas commented 10 months ago

Hi @dlpzx , I think currently it would be nice to have the data.all teams created as soon as they are created in Cognito. Or at least when the admin pulls up the tenant teams settings, if at that point the admin can see all the teams that are in Cognito, and set permissions on them. This at least provides the admin with the ability to set permissions at the same time that they create the Cognito group, even if there are no users in that group yet, and it prevents the overly permissive permissions in the interim. This would be the preferred solution for us.

Regarding granting no permissions by default, if we have the above instead (data.all groups as soon as they are in Cognito), I don't think it's necessary. However, it also doesn't cause a problem for us currently, so if this is additionally done, it's okay. However, in the future, if we are to use purely a third party IdP and a different roles management system than Cognito groups (and one that is perhaps self serve by the users where they can create some new groups/roles themselves), I can see to limit the initial permissions or perhaps be able to programmatically set initial permissions of a group when it is created. For now however I think the current behavior is okay.

dlpzx commented 9 months ago

Thanks for the input @rbernotas we will discuss internally when to address this issue. We can re-use some of the methods that we already have for Cognito, but integrations with other IdPs might require custom code