diff --git a/api/CHANGELOG.md b/api/CHANGELOG.md index b2084c1969..1aaa3a65ab 100644 --- a/api/CHANGELOG.md +++ b/api/CHANGELOG.md @@ -46,6 +46,7 @@ All notable changes to the **Prowler API** are documented in this file. ### 🔄 Changed - Allowlisted idempotent background tasks are no longer lost when a worker is stopped or crashes mid-task; tasks with external side effects are marked terminal instead of blindly re-running [(#11416)](https://github.com/prowler-cloud/prowler/pull/11416) +- SAML logins no longer wipe a user's roles when the IdP does not send the `userType` attribute; existing roles are kept, and when `userType` names a role that does not exist it is now created with read-only access (visibility over all providers, no management permissions) instead of no permissions at all [(#11520)](https://github.com/prowler-cloud/prowler/pull/11520) ### 🐞 Fixed diff --git a/api/src/backend/api/tests/test_views.py b/api/src/backend/api/tests/test_views.py index 410bd23e19..67fb6c0663 100644 --- a/api/src/backend/api/tests/test_views.py +++ b/api/src/backend/api/tests/test_views.py @@ -12840,7 +12840,7 @@ class TestTenantFinishACSView: "firstName": ["John"], "lastName": ["Doe"], "organization": ["testing_company"], - "userType": ["no_permissions"], + "userType": ["platform_team"], }, ) @@ -12895,12 +12895,21 @@ class TestTenantFinishACSView: assert user.name == "John Doe" assert user.company_name == "testing_company" - role = Role.objects.using(MainRouter.admin_db).get(name="no_permissions") + role = Role.objects.using(MainRouter.admin_db).get( + name="platform_team", tenant=tenants_fixture[0] + ) assert role.tenant == tenants_fixture[0] + assert not role.manage_users + assert not role.manage_account + assert not role.manage_billing + assert not role.manage_providers + assert not role.manage_integrations + assert not role.manage_scans + assert role.unlimited_visibility assert ( UserRoleRelationship.objects.using(MainRouter.admin_db) - .filter(user=user, tenant_id=tenants_fixture[0].id) + .filter(user=user, role=role, tenant_id=tenants_fixture[0].id) .exists() ) @@ -12953,7 +12962,7 @@ class TestTenantFinishACSView: assert response.status_code == 302 assert "sso_saml_failed=true" in response.url - def test_dispatch_skips_role_mapping_when_single_manage_account_user( + def test_dispatch_keeps_existing_roles_when_usertype_missing( self, create_test_user, tenants_fixture, @@ -12962,7 +12971,7 @@ class TestTenantFinishACSView: settings, monkeypatch, ): - """Test that role mapping is skipped when tenant has only one user with MANAGE_ACCOUNT role""" + """Test that roles are left untouched when the IdP does not send userType""" monkeypatch.setenv("SAML_SSO_CALLBACK_URL", "http://localhost/sso-complete") user = create_test_user tenant = tenants_fixture[0] @@ -12971,6 +12980,7 @@ class TestTenantFinishACSView: UserRoleRelationship.objects.using(MainRouter.admin_db).create( user=user, role=admin_role, tenant_id=tenant.id ) + roles_before = Role.objects.using(MainRouter.admin_db).count() social_account = SocialAccount( user=user, @@ -12979,7 +12989,6 @@ class TestTenantFinishACSView: "firstName": ["John"], "lastName": ["Doe"], "organization": ["testing_company"], - "userType": ["no_permissions"], # This should be ignored }, ) @@ -13017,24 +13026,162 @@ class TestTenantFinishACSView: assert response.status_code == 302 - # Verify the admin role is still assigned (not changed to no_permissions) + # Verify the existing role assignment was not modified assert ( UserRoleRelationship.objects.using(MainRouter.admin_db) .filter(user=user, role=admin_role, tenant_id=tenant.id) .exists() ) - # Verify no_permissions role was NOT created in the database - assert ( - not Role.objects.using(MainRouter.admin_db) - .filter(name="no_permissions", tenant=tenant) + # Verify no new role was created + assert Role.objects.using(MainRouter.admin_db).count() == roles_before + + def test_dispatch_assigns_no_role_to_new_user_when_usertype_missing( + self, + create_test_user, + tenants_fixture, + saml_setup, + settings, + monkeypatch, + ): + """Test that a user without roles gets none assigned when userType is missing""" + monkeypatch.setenv("SAML_SSO_CALLBACK_URL", "http://localhost/sso-complete") + user = create_test_user + tenant = tenants_fixture[0] + roles_before = Role.objects.using(MainRouter.admin_db).count() + + social_account = SocialAccount( + user=user, + provider="saml", + extra_data={ + "firstName": ["John"], + "lastName": ["Doe"], + "organization": ["testing_company"], + }, + ) + + request = RequestFactory().get( + reverse("saml_finish_acs", kwargs={"organization_slug": "testtenant"}) + ) + request.user = user + request.session = {} + + with ( + patch( + "allauth.socialaccount.providers.saml.views.get_app_or_404" + ) as mock_get_app_or_404, + patch( + "allauth.socialaccount.models.SocialApp.objects.get" + ) as mock_socialapp_get, + patch( + "allauth.socialaccount.models.SocialAccount.objects.get" + ) as mock_sa_get, + patch("api.models.SAMLDomainIndex.objects.get") as mock_saml_domain_get, + patch("api.models.SAMLConfiguration.objects.get") as mock_saml_config_get, + patch("api.models.User.objects.get") as mock_user_get, + ): + mock_get_app_or_404.return_value = MagicMock( + provider="saml", client_id="testtenant", name="Test App", settings={} + ) + mock_sa_get.return_value = social_account + mock_socialapp_get.return_value = MagicMock(provider_id="saml") + mock_saml_domain_get.return_value = SimpleNamespace(tenant_id=tenant.id) + mock_saml_config_get.return_value = MagicMock() + mock_user_get.return_value = user + + view = TenantFinishACSView.as_view() + response = view(request, organization_slug="testtenant") + + assert response.status_code == 302 + + # Verify no role was created or assigned + assert Role.objects.using(MainRouter.admin_db).count() == roles_before + assert not ( + UserRoleRelationship.objects.using(MainRouter.admin_db) + .filter(user=user, tenant_id=tenant.id) .exists() ) - # Verify no_permissions role was NOT assigned to the user - assert not ( + # Membership is still created so the user belongs to the tenant + assert ( + Membership.objects.using(MainRouter.admin_db) + .filter(user=user, tenant=tenant) + .exists() + ) + + def test_dispatch_skips_role_mapping_when_last_manage_account_user_maps_to_new_role( + self, + create_test_user, + tenants_fixture, + admin_role_fixture, + saml_setup, + settings, + monkeypatch, + ): + """Test that a new read-only role is neither created nor assigned if it would remove the last MANAGE_ACCOUNT user""" + monkeypatch.setenv("SAML_SSO_CALLBACK_URL", "http://localhost/sso-complete") + user = create_test_user + tenant = tenants_fixture[0] + + admin_role = admin_role_fixture + UserRoleRelationship.objects.using(MainRouter.admin_db).create( + user=user, role=admin_role, tenant_id=tenant.id + ) + + social_account = SocialAccount( + user=user, + provider="saml", + extra_data={ + "firstName": ["John"], + "lastName": ["Doe"], + "organization": ["testing_company"], + "userType": ["brand_new_role"], + }, + ) + + request = RequestFactory().get( + reverse("saml_finish_acs", kwargs={"organization_slug": "testtenant"}) + ) + request.user = user + request.session = {} + + with ( + patch( + "allauth.socialaccount.providers.saml.views.get_app_or_404" + ) as mock_get_app_or_404, + patch( + "allauth.socialaccount.models.SocialApp.objects.get" + ) as mock_socialapp_get, + patch( + "allauth.socialaccount.models.SocialAccount.objects.get" + ) as mock_sa_get, + patch("api.models.SAMLDomainIndex.objects.get") as mock_saml_domain_get, + patch("api.models.SAMLConfiguration.objects.get") as mock_saml_config_get, + patch("api.models.User.objects.get") as mock_user_get, + ): + mock_get_app_or_404.return_value = MagicMock( + provider="saml", client_id="testtenant", name="Test App", settings={} + ) + mock_sa_get.return_value = social_account + mock_socialapp_get.return_value = MagicMock(provider_id="saml") + mock_saml_domain_get.return_value = SimpleNamespace(tenant_id=tenant.id) + mock_saml_config_get.return_value = MagicMock() + mock_user_get.return_value = user + + view = TenantFinishACSView.as_view() + response = view(request, organization_slug="testtenant") + + assert response.status_code == 302 + + # The admin role is still assigned and the new role was not created + assert ( UserRoleRelationship.objects.using(MainRouter.admin_db) - .filter(user=user, role__name="no_permissions", tenant_id=tenant.id) + .filter(user=user, role=admin_role, tenant_id=tenant.id) + .exists() + ) + assert ( + not Role.objects.using(MainRouter.admin_db) + .filter(name="brand_new_role", tenant=tenant) .exists() ) diff --git a/api/src/backend/api/v1/views.py b/api/src/backend/api/v1/views.py index 0942adc8e1..7bcf9a9e69 100644 --- a/api/src/backend/api/v1/views.py +++ b/api/src/backend/api/v1/views.py @@ -801,60 +801,70 @@ class TenantFinishACSView(FinishACSView): .tenant ) + # Only remap roles when the IdP provides a userType attribute. + # Without it, the user's current roles are left untouched. role_name = ( - extra.get("userType", ["no_permissions"])[0].strip() - if extra.get("userType") - else "no_permissions" + extra.get("userType", [""])[0].strip() if extra.get("userType") else "" ) - role = ( - Role.objects.using(MainRouter.admin_db) - .filter(name=role_name, tenant=tenant) - .first() - ) - - # Only skip mapping if it would remove the last MANAGE_ACCOUNT user - remaining_manage_account_users = ( - UserRoleRelationship.objects.using(MainRouter.admin_db) - .filter(role__manage_account=True, tenant_id=tenant.id) - .exclude(user_id=user_id) - .values("user") - .distinct() - .count() - ) - user_has_manage_account = ( - UserRoleRelationship.objects.using(MainRouter.admin_db) - .filter(role__manage_account=True, tenant_id=tenant.id, user_id=user_id) - .exists() - ) - role_manage_account = role.manage_account if role else False - would_remove_last_manage_account = ( - user_has_manage_account - and remaining_manage_account_users == 0 - and not role_manage_account - ) - - if not would_remove_last_manage_account: - if role is None: - role = Role.objects.using(MainRouter.admin_db).create( - name=role_name, - tenant=tenant, - manage_users=False, - manage_account=False, - manage_billing=False, - manage_providers=False, - manage_integrations=False, - manage_scans=False, - unlimited_visibility=False, + if role_name: + with transaction.atomic(using=MainRouter.admin_db): + role = ( + Role.objects.using(MainRouter.admin_db) + .filter(name=role_name, tenant=tenant) + .first() ) - UserRoleRelationship.objects.using(MainRouter.admin_db).filter( - user=user, - tenant_id=tenant.id, - ).delete() - UserRoleRelationship.objects.using(MainRouter.admin_db).create( - user=user, - role=role, - tenant_id=tenant.id, - ) + + # Only skip mapping if it would remove the last MANAGE_ACCOUNT user + remaining_manage_account_users = ( + UserRoleRelationship.objects.using(MainRouter.admin_db) + .filter(role__manage_account=True, tenant_id=tenant.id) + .exclude(user_id=user_id) + .values("user") + .distinct() + .count() + ) + user_has_manage_account = ( + UserRoleRelationship.objects.using(MainRouter.admin_db) + .filter( + role__manage_account=True, + tenant_id=tenant.id, + user_id=user_id, + ) + .exists() + ) + role_manage_account = role.manage_account if role else False + would_remove_last_manage_account = ( + user_has_manage_account + and remaining_manage_account_users == 0 + and not role_manage_account + ) + + if not would_remove_last_manage_account: + if role is None: + # Roles auto-created from userType get read-only access: + # visibility over all providers, no management permissions + role, _ = Role.objects.using(MainRouter.admin_db).get_or_create( + name=role_name, + tenant=tenant, + defaults={ + "manage_users": False, + "manage_account": False, + "manage_billing": False, + "manage_providers": False, + "manage_integrations": False, + "manage_scans": False, + "unlimited_visibility": True, + }, + ) + UserRoleRelationship.objects.using(MainRouter.admin_db).filter( + user=user, + tenant_id=tenant.id, + ).delete() + UserRoleRelationship.objects.using(MainRouter.admin_db).create( + user=user, + role=role, + tenant_id=tenant.id, + ) membership, _ = Membership.objects.using(MainRouter.admin_db).get_or_create( user=user, tenant=tenant, diff --git a/docs/user-guide/tutorials/prowler-app-sso-google-workspace.mdx b/docs/user-guide/tutorials/prowler-app-sso-google-workspace.mdx index 2a349c0204..2f10d443ae 100644 --- a/docs/user-guide/tutorials/prowler-app-sso-google-workspace.mdx +++ b/docs/user-guide/tutorials/prowler-app-sso-google-workspace.mdx @@ -108,10 +108,10 @@ Prowler App updates user attributes each time a user logs in. Any changes made i The `userType` attribute controls which Prowler role is assigned to the user: - If `userType` matches an existing Prowler role name, the user receives that role automatically. -- If `userType` does not match any existing role, Prowler App creates a new role with that name **without permissions**. -- If `userType` is not set, the user receives the `no_permissions` role. +- If `userType` does not match any existing role, Prowler App creates a new role with that name **with read-only access** (visibility over all providers, no management permissions). A Prowler administrator can adjust its permissions afterward through the [RBAC Management](/user-guide/tutorials/prowler-app-rbac) tab. +- If `userType` is not set, the user's existing roles are left unchanged. -In all cases where the resulting role has no permissions, a Prowler administrator must configure the appropriate permissions through the [RBAC Management](/user-guide/tutorials/prowler-app-rbac) tab. The `userType` value is **case-sensitive** - for example, `Backend` and `backend` are treated as different roles. +The `userType` value is **case-sensitive** - for example, `Backend` and `backend` are treated as different roles. @@ -223,9 +223,9 @@ To test the `userType` → role mapping, set the **Department** attribute in the After a successful SSO login, the user profile in Prowler App reflects the attributes sent by Google Workspace: - **Name**: Populated from the `firstName` and `lastName` attributes. -- **Role**: Created automatically from the `userType` attribute (e.g., `Backend`). If the role did not exist previously, it is created with no permissions by default. -- **Permissions**: In the screenshot below, the user has no permissions because the `Backend` role did not exist prior to login and was created automatically without any permissions. To resolve this, a Prowler administrator can either: - - Assign the appropriate permissions to the new role via the [RBAC Management](/user-guide/tutorials/prowler-app-rbac) tab. +- **Role**: Created automatically from the `userType` attribute (e.g., `Backend`). If the role did not exist previously, it is created with read-only access by default. +- **Permissions**: If the assigned permissions need to be adjusted, a Prowler administrator can either: + - Edit the permissions of the new role via the [RBAC Management](/user-guide/tutorials/prowler-app-rbac) tab. - Set the `userType` attribute in the IdP to match an existing Prowler role that already has the desired permissions. The updated role is applied on the next SAML login. For more details on role assignment behavior and attribute mapping, refer to the [SAML SSO Configuration](/user-guide/tutorials/prowler-app-sso#configure-attribute-mapping-in-the-idp) page. diff --git a/docs/user-guide/tutorials/prowler-app-sso.mdx b/docs/user-guide/tutorials/prowler-app-sso.mdx index d4d2c18d11..1e61ec1060 100644 --- a/docs/user-guide/tutorials/prowler-app-sso.mdx +++ b/docs/user-guide/tutorials/prowler-app-sso.mdx @@ -87,7 +87,7 @@ Choose a Method: |----------------|---------------------------------------------------------------------------------------------------------|----------| | `firstName` | The user's first name. | Yes | | `lastName` | The user's last name. | Yes | - | `userType` | Determines which Prowler role the user receives (e.g., `admin`, `auditor`). If a role with that name already exists, the user receives it automatically; if it does not exist, Prowler App creates a new role with that name without permissions. If `userType` is not defined, the user is assigned the `no_permissions` role. Role permissions can be edited in the [RBAC Management tab](/user-guide/tutorials/prowler-app-rbac). | No | + | `userType` | Determines which Prowler role the user receives (e.g., `admin`, `auditor`). If a role with that name already exists, the user receives it automatically; if it does not exist, Prowler App creates a new role with that name with read-only access (visibility over all providers, no management permissions). If `userType` is not defined, the user's existing roles are left unchanged. Role permissions can be edited in the [RBAC Management tab](/user-guide/tutorials/prowler-app-rbac). | No | | `organization` | The user's company name. | No | @@ -140,7 +140,7 @@ Choose a Method: ![Okta User Profile — First Name and Last Name](/images/prowler-app/saml/okta-user-profile-name.png) * **Organization** (`organization`): Maps to the company name displayed in Prowler App. This attribute is optional. - * **User type** (`userType`): Determines the Prowler role assigned to the user. This attribute is **case-sensitive** and must match the exact name of an existing role in Prowler App. + * **User type** (`userType`): Determines the Prowler role assigned to the user. This attribute is **case-sensitive**: if it matches the exact name of an existing role in Prowler App the user receives that role; if no role with that name exists, a new one is created with read-only access. ![Okta User Profile — User Type and Organization](/images/prowler-app/saml/okta-user-profile-attributes.png) @@ -152,14 +152,10 @@ Choose a Method: The `userType` attribute controls which Prowler role is assigned to the user: * If a role with the specified name already exists in Prowler App, the user automatically receives that role. - * If the role does not exist, Prowler App creates a new role with that exact name but without any permissions, preventing the user from performing any actions. - * If `userType` is not defined in the user's Okta profile, the user is assigned the `no_permissions` role. + * If the role does not exist, Prowler App creates a new role with that exact name with read-only access: the user can see all providers and their findings but cannot manage anything. A Prowler administrator (a user whose role includes the "Manage Account" permission) can adjust its permissions afterward through the [RBAC Management tab](/user-guide/tutorials/prowler-app-rbac). + * If `userType` is not defined in the user's Okta profile, the user's existing roles in Prowler App are left unchanged. - In all cases where the resulting role has no permissions, a Prowler administrator (a user whose role includes the "Manage Account" permission) must configure the appropriate permissions through the [RBAC Management tab](/user-guide/tutorials/prowler-app-rbac). - - This behavior is intentional: by defaulting to no permissions, Prowler App ensures that a misconfiguration in Okta cannot inadvertently grant elevated access. - - **Example:** To assign the `IT` role to a user, set the `userType` value to `IT` in Okta. If a role named `IT` already exists in Prowler App, the user receives it automatically upon login. If it does not exist, Prowler App creates a new role called `IT` without permissions, and a Prowler administrator must configure the desired permissions for it. + **Example:** To assign the `IT` role to a user, set the `userType` value to `IT` in Okta. If a role named `IT` already exists in Prowler App, the user receives it automatically upon login. If it does not exist, Prowler App creates a new role called `IT` with read-only access, and a Prowler administrator can adjust its permissions as needed.