diff --git a/CHANGELOG.md b/CHANGELOG.md index a21793b07c..39b1b4a78b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -22,6 +22,7 @@ and this project adheres to - πŸ›(makefile) Windows compatibility fix for Docker volume mounting #1264 - πŸ›(minio) fix user permission error with Minio and Windows #1264 +- πŸ›link role update #1287 ## [3.5.0] - 2025-07-31 diff --git a/src/backend/core/api/serializers.py b/src/backend/core/api/serializers.py index 83afc260d9..d998526e3c 100644 --- a/src/backend/core/api/serializers.py +++ b/src/backend/core/api/serializers.py @@ -486,6 +486,10 @@ class LinkDocumentSerializer(serializers.ModelSerializer): We expose it separately from document in order to simplify and secure access control. """ + link_reach = serializers.ChoiceField( + choices=models.LinkReachChoices.choices, required=True + ) + class Meta: model = models.Document fields = [ @@ -493,6 +497,66 @@ class Meta: "link_reach", ] + def validate(self, attrs): + """Validate that link_role and link_reach are compatible using get_select_options.""" + link_reach = attrs.get("link_reach") + link_role = attrs.get("link_role") + + if not link_reach: + raise serializers.ValidationError( + {"link_reach": _("This field is required.")} + ) + + # Get available options based on ancestors' link definition + available_options = models.LinkReachChoices.get_select_options( + **self.instance.ancestors_link_definition + ) + + # Validate link_reach is allowed + if link_reach not in available_options: + msg = _( + "Link reach '%(link_reach)s' is not allowed based on parent document configuration." + ) + raise serializers.ValidationError( + {"link_reach": msg % {"link_reach": link_reach}} + ) + + # Validate link_role is compatible with link_reach + allowed_roles = available_options[link_reach] + + if link_reach == models.LinkReachChoices.RESTRICTED: + # For restricted reach, link_role is ignored but + # we shouldn't allow meaningful roles to be set + if link_role is not None and link_role in [ + models.LinkRoleChoices.READER, + models.LinkRoleChoices.EDITOR, + ]: + msg = _( + "Cannot set link_role when link_reach is 'restricted'. " + "Link role must be null for restricted reach." + ) + raise serializers.ValidationError({"link_role": msg}) + # For non-restricted reach, validate link_role is in allowed roles + elif link_role is not None and link_role not in allowed_roles: + msg = _( + "Link role '%(link_role)s' is not allowed for link reach '%(link_reach)s'. " + "Allowed roles: %(allowed_roles)s" + ) + raise serializers.ValidationError( + { + "link_role": msg + % { + "link_role": link_role, + "link_reach": link_reach, + "allowed_roles": ", ".join(allowed_roles) + if allowed_roles + else "none", + } + } + ) + + return attrs + class DocumentDuplicationSerializer(serializers.Serializer): """ diff --git a/src/backend/core/tests/documents/test_api_documents_link_configuration.py b/src/backend/core/tests/documents/test_api_documents_link_configuration.py index 7683880579..2c271ca1e7 100644 --- a/src/backend/core/tests/documents/test_api_documents_link_configuration.py +++ b/src/backend/core/tests/documents/test_api_documents_link_configuration.py @@ -133,7 +133,10 @@ def test_api_documents_link_configuration_update_authenticated_related_success( client = APIClient() client.force_login(user) - document = factories.DocumentFactory() + document = factories.DocumentFactory( + link_reach=models.LinkReachChoices.AUTHENTICATED, + link_role=models.LinkRoleChoices.READER, + ) if via == USER: factories.UserDocumentAccessFactory(document=document, user=user, role=role) elif via == TEAM: @@ -143,7 +146,10 @@ def test_api_documents_link_configuration_update_authenticated_related_success( ) new_document_values = serializers.LinkDocumentSerializer( - instance=factories.DocumentFactory() + instance=factories.DocumentFactory( + link_reach=models.LinkReachChoices.PUBLIC, + link_role=models.LinkRoleChoices.EDITOR, + ) ).data with mock_reset_connections(document.id): @@ -158,3 +164,240 @@ def test_api_documents_link_configuration_update_authenticated_related_success( document_values = serializers.LinkDocumentSerializer(instance=document).data for key, value in document_values.items(): assert value == new_document_values[key] + + +def test_api_documents_link_configuration_update_role_restricted_forbidden(): + """ + Test that trying to set link_role on a document with restricted link_reach + returns a validation error. + """ + user = factories.UserFactory() + client = APIClient() + client.force_login(user) + + document = factories.DocumentFactory( + link_reach=models.LinkReachChoices.RESTRICTED, + link_role=models.LinkRoleChoices.READER, + ) + + factories.UserDocumentAccessFactory( + document=document, user=user, role=models.RoleChoices.OWNER + ) + + # Try to set a meaningful role on a restricted document + new_data = { + "link_reach": models.LinkReachChoices.RESTRICTED, + "link_role": models.LinkRoleChoices.EDITOR, + } + + response = client.put( + f"/api/v1.0/documents/{document.id!s}/link-configuration/", + new_data, + format="json", + ) + + assert response.status_code == 400 + assert "link_role" in response.json() + assert ( + "Cannot set link_role when link_reach is 'restricted'" + in response.json()["link_role"][0] + ) + + +def test_api_documents_link_configuration_update_link_reach_required(): + """ + Test that link_reach is required when updating link configuration. + """ + user = factories.UserFactory() + client = APIClient() + client.force_login(user) + + document = factories.DocumentFactory( + link_reach=models.LinkReachChoices.PUBLIC, + link_role=models.LinkRoleChoices.READER, + ) + + factories.UserDocumentAccessFactory( + document=document, user=user, role=models.RoleChoices.OWNER + ) + + # Try to update without providing link_reach + new_data = {"link_role": models.LinkRoleChoices.EDITOR} + + response = client.put( + f"/api/v1.0/documents/{document.id!s}/link-configuration/", + new_data, + format="json", + ) + + assert response.status_code == 400 + assert "link_reach" in response.json() + assert "This field is required" in response.json()["link_reach"][0] + + +def test_api_documents_link_configuration_update_restricted_without_role_success( + mock_reset_connections, # pylint: disable=redefined-outer-name +): + """ + Test that setting link_reach to restricted without specifying link_role succeeds. + """ + user = factories.UserFactory() + client = APIClient() + client.force_login(user) + + document = factories.DocumentFactory( + link_reach=models.LinkReachChoices.PUBLIC, + link_role=models.LinkRoleChoices.READER, + ) + + factories.UserDocumentAccessFactory( + document=document, user=user, role=models.RoleChoices.OWNER + ) + + # Only specify link_reach, not link_role + new_data = { + "link_reach": models.LinkReachChoices.RESTRICTED, + } + + with mock_reset_connections(document.id): + response = client.put( + f"/api/v1.0/documents/{document.id!s}/link-configuration/", + new_data, + format="json", + ) + + assert response.status_code == 200 + document.refresh_from_db() + assert document.link_reach == models.LinkReachChoices.RESTRICTED + + +@pytest.mark.parametrize( + "reach", [models.LinkReachChoices.PUBLIC, models.LinkReachChoices.AUTHENTICATED] +) +@pytest.mark.parametrize("role", models.LinkRoleChoices.values) +def test_api_documents_link_configuration_update_non_restricted_with_valid_role_success( + reach, + role, + mock_reset_connections, # pylint: disable=redefined-outer-name +): + """ + Test that setting non-restricted link_reach with valid link_role succeeds. + """ + user = factories.UserFactory() + client = APIClient() + client.force_login(user) + + document = factories.DocumentFactory( + link_reach=models.LinkReachChoices.RESTRICTED, + link_role=models.LinkRoleChoices.READER, + ) + + factories.UserDocumentAccessFactory( + document=document, user=user, role=models.RoleChoices.OWNER + ) + + new_data = { + "link_reach": reach, + "link_role": role, + } + + with mock_reset_connections(document.id): + response = client.put( + f"/api/v1.0/documents/{document.id!s}/link-configuration/", + new_data, + format="json", + ) + + assert response.status_code == 200 + document.refresh_from_db() + assert document.link_reach == reach + assert document.link_role == role + + +def test_api_documents_link_configuration_update_with_ancestor_constraints(): + """ + Test that link configuration respects ancestor constraints using get_select_options. + This test may need adjustment based on the actual get_select_options implementation. + """ + user = factories.UserFactory() + client = APIClient() + client.force_login(user) + + parent_document = factories.DocumentFactory( + link_reach=models.LinkReachChoices.PUBLIC, + link_role=models.LinkRoleChoices.READER, + ) + + child_document = factories.DocumentFactory( + parent=parent_document, + link_reach=models.LinkReachChoices.PUBLIC, + link_role=models.LinkRoleChoices.READER, + ) + + factories.UserDocumentAccessFactory( + document=child_document, user=user, role=models.RoleChoices.OWNER + ) + + # Try to set child to PUBLIC when parent is RESTRICTED + new_data = { + "link_reach": models.LinkReachChoices.RESTRICTED, + "link_role": models.LinkRoleChoices.READER, + } + + response = client.put( + f"/api/v1.0/documents/{child_document.id!s}/link-configuration/", + new_data, + format="json", + ) + + assert response.status_code == 400 + assert "link_reach" in response.json() + assert ( + "Link reach 'restricted' is not allowed based on parent" + in response.json()["link_reach"][0] + ) + + +def test_api_documents_link_configuration_update_invalid_role_for_reach_validation(): + """ + Test the specific validation logic that checks if link_role is allowed for link_reach. + This tests the code section that validates allowed_roles from get_select_options. + """ + user = factories.UserFactory() + client = APIClient() + client.force_login(user) + + parent_document = factories.DocumentFactory( + link_reach=models.LinkReachChoices.AUTHENTICATED, + link_role=models.LinkRoleChoices.EDITOR, + ) + + child_document = factories.DocumentFactory( + parent=parent_document, + link_reach=models.LinkReachChoices.RESTRICTED, + link_role=models.LinkRoleChoices.READER, + ) + + factories.UserDocumentAccessFactory( + document=child_document, user=user, role=models.RoleChoices.OWNER + ) + + new_data = { + "link_reach": models.LinkReachChoices.AUTHENTICATED, + "link_role": models.LinkRoleChoices.READER, # This should be rejected + } + + response = client.put( + f"/api/v1.0/documents/{child_document.id!s}/link-configuration/", + new_data, + format="json", + ) + + assert response.status_code == 400 + assert "link_role" in response.json() + error_message = response.json()["link_role"][0] + assert ( + "Link role 'reader' is not allowed for link reach 'authenticated'" + in error_message + ) + assert "Allowed roles: editor" in error_message diff --git a/src/frontend/apps/e2e/__tests__/app-impress/doc-inherited-share.spec.ts b/src/frontend/apps/e2e/__tests__/app-impress/doc-inherited-share.spec.ts index 87ee6220e5..2b724ee183 100644 --- a/src/frontend/apps/e2e/__tests__/app-impress/doc-inherited-share.spec.ts +++ b/src/frontend/apps/e2e/__tests__/app-impress/doc-inherited-share.spec.ts @@ -52,6 +52,12 @@ test.describe('Inherited share accesses', () => { await expect(docVisibilityCard.getByText('Connected')).toBeVisible(); await expect(docVisibilityCard.getByText('Reading')).toBeVisible(); + await docVisibilityCard.getByText('Reading').click(); + await page.getByRole('menuitem', { name: 'Editing' }).click(); + + await expect(docVisibilityCard.getByText('Reading')).toBeHidden(); + await expect(docVisibilityCard.getByText('Editing')).toBeVisible(); + // Verify inherited link await docVisibilityCard.getByText('Connected').click(); await expect( @@ -61,17 +67,13 @@ test.describe('Inherited share accesses', () => { // Update child link await page.getByRole('menuitem', { name: 'Public' }).click(); - await docVisibilityCard.getByText('Reading').click(); - await page.getByRole('menuitem', { name: 'Editing' }).click(); - await expect(docVisibilityCard.getByText('Connected')).toBeHidden(); - await expect(docVisibilityCard.getByText('Reading')).toBeHidden(); await expect( docVisibilityCard.getByText('Public', { exact: true, }), ).toBeVisible(); - await expect(docVisibilityCard.getByText('Editing')).toBeVisible(); + await expect( docVisibilityCard.getByText( 'The link sharing rules differ from the parent document', diff --git a/src/frontend/apps/impress/src/features/docs/doc-management/api/useUpdateDocLink.tsx b/src/frontend/apps/impress/src/features/docs/doc-management/api/useUpdateDocLink.tsx index 1926d311ef..bc6df9f404 100644 --- a/src/frontend/apps/impress/src/features/docs/doc-management/api/useUpdateDocLink.tsx +++ b/src/frontend/apps/impress/src/features/docs/doc-management/api/useUpdateDocLink.tsx @@ -6,8 +6,8 @@ import { APIError, errorCauses, fetchAPI } from '@/api'; import { Doc, KEY_DOC } from '@/docs/doc-management'; import { useBroadcastStore } from '@/stores'; -export type UpdateDocLinkParams = Pick & - Partial>; +export type UpdateDocLinkParams = Pick & + Partial>; export const updateDocLink = async ({ id, diff --git a/src/frontend/apps/impress/src/features/docs/doc-share/components/DocVisibility.tsx b/src/frontend/apps/impress/src/features/docs/doc-share/components/DocVisibility.tsx index 36e97c2e61..ae00b17a72 100644 --- a/src/frontend/apps/impress/src/features/docs/doc-share/components/DocVisibility.tsx +++ b/src/frontend/apps/impress/src/features/docs/doc-share/components/DocVisibility.tsx @@ -17,6 +17,7 @@ import { LinkReach, LinkRole, getDocLinkReach, + getDocLinkRole, useDocUtils, useUpdateDocLink, } from '@/docs/doc-management'; @@ -36,7 +37,7 @@ export const DocVisibility = ({ doc }: DocVisibilityProps) => { const { spacingsTokens, colorsTokens } = useCunninghamTheme(); const canManage = doc.abilities.accesses_manage; const docLinkReach = getDocLinkReach(doc); - const docLinkRole = doc.computed_link_role ?? LinkRole.READER; + const docLinkRole = getDocLinkRole(doc); const { isDesynchronized } = useDocUtils(doc); const { linkModeTranslations, linkReachChoices, linkReachTranslations } = useTranslatedShareSettings(); @@ -85,7 +86,12 @@ export const DocVisibility = ({ doc }: DocVisibilityProps) => { const isDisabled = !options.includes(key); return { label: linkModeTranslations[key], - callback: () => updateDocLink({ id: doc.id, link_role: key }), + callback: () => + updateDocLink({ + id: doc.id, + link_role: key, + link_reach: docLinkReach, + }), isSelected: docLinkRole === key, disabled: isDisabled, };