From a94a65a037634e1556859a4f8cdceacfd37be821 Mon Sep 17 00:00:00 2001 From: VineetBala-AOT <90332175+VineetBala-AOT@users.noreply.github.com> Date: Tue, 14 May 2024 15:22:20 -0700 Subject: [PATCH] [To Main] Add metadata management role (#2506) * Adding metadata management role * fixing unit test for sidenav * Removing metadata management role from engagement metadata --- CHANGELOG.MD | 5 ++ .../1407e0ad88f6_add_manage_metadata_role.py | 30 ++++++++ .../met_api/resources/engagement_metadata.py | 70 +++++++++++++------ .../src/met_api/resources/metadata_taxon.py | 14 ++-- met-api/src/met_api/utils/roles.py | 1 + .../unit/api/test_engagement_metadata.py | 31 +++++++- .../layout/SideNav/SideNavElements.tsx | 4 +- met-web/src/routes/AuthenticatedRoutes.tsx | 4 +- met-web/src/services/userService/constants.ts | 1 + .../tests/unit/components/sidenav.test.tsx | 1 + 10 files changed, 129 insertions(+), 32 deletions(-) create mode 100644 met-api/migrations/versions/1407e0ad88f6_add_manage_metadata_role.py diff --git a/CHANGELOG.MD b/CHANGELOG.MD index eabba8543..ad04b08e9 100644 --- a/CHANGELOG.MD +++ b/CHANGELOG.MD @@ -4,6 +4,11 @@ - Fixed the date formatting issue and using the end_date in the email preview - Updated unit tests +- **Feature** Create role for metadata management [🎟️ DESENG-603](https://apps.itsm.gov.bc.ca/jira/browse/DESENG-603) +- Implemented a new role named "manage_metadata" within the Admin group to restrci access for metadata management +- Updated the frontend to restrict access to the "metadata management" link in the menu for users without the newly added role +- Backend changes to incorporate the new role for access control purposes, ensuring only authorized users can perform metadata management actions + ## May 10, 2024 - **Bugfix** Language picker should only appear on engagements with more than one language [🎟️ DESENG-575](https://apps.itsm.gov.bc.ca/jira/browse/DESENG-575) diff --git a/met-api/migrations/versions/1407e0ad88f6_add_manage_metadata_role.py b/met-api/migrations/versions/1407e0ad88f6_add_manage_metadata_role.py new file mode 100644 index 000000000..357c2aa67 --- /dev/null +++ b/met-api/migrations/versions/1407e0ad88f6_add_manage_metadata_role.py @@ -0,0 +1,30 @@ +"""add_manage_metadata_role + +Revision ID: 1407e0ad88f6 +Revises: 614b5376f19c +Create Date: 2024-05-09 20:58:49.586171 + +""" +from alembic import op +import sqlalchemy as sa + + +# revision identifiers, used by Alembic. +revision = '1407e0ad88f6' +down_revision = '614b5376f19c' +branch_labels = None +depends_on = None + + +def upgrade(): + # ### commands auto generated by Alembic - please adjust! ### + op.execute("INSERT INTO user_role (created_date, updated_date, id, name, description) VALUES ('{0}', '{0}', 38, 'manage_metadata', 'Role to access metadata management')".format(sa.func.now())) + op.execute("INSERT INTO group_role_mapping (created_date, updated_date, id, role_id, group_id) VALUES ('{0}', '{0}', 60, 38, 1)".format(sa.func.now())) + # ### end Alembic commands ### + + +def downgrade(): + # ### commands auto generated by Alembic - please adjust! ### + op.execute("DELETE FROM group_role_mapping WHERE id = 60") + op.execute("DELETE FROM user_role WHERE id = 38") + # ### end Alembic commands ### diff --git a/met-api/src/met_api/resources/engagement_metadata.py b/met-api/src/met_api/resources/engagement_metadata.py index 044b67788..4747d8028 100644 --- a/met-api/src/met_api/resources/engagement_metadata.py +++ b/met-api/src/met_api/resources/engagement_metadata.py @@ -24,16 +24,14 @@ from flask_restx import Namespace, Resource, fields from marshmallow import ValidationError from met_api.auth import auth_methods +from met_api.auth import jwt as _jwt +from met_api.constants.membership_type import MembershipType from met_api.services import authorization from met_api.services.engagement_service import EngagementService from met_api.services.engagement_metadata_service import EngagementMetadataService from met_api.utils.roles import Role -from met_api.utils.tenant_validator import require_role from met_api.utils.util import allowedorigins, cors_preflight -EDIT_ENGAGEMENT_ROLES = [Role.EDIT_ENGAGEMENT.value] -VIEW_ENGAGEMENT_ROLES = [ - Role.VIEW_ENGAGEMENT.value, Role.EDIT_ENGAGEMENT.value] API = Namespace('engagement_metadata', path='/engagements//metadata', @@ -75,9 +73,16 @@ class EngagementMetadata(Resource): @cross_origin(origins=allowedorigins()) @API.doc(security='apikey') @API.marshal_list_with(metadata_return_model) - @require_role(VIEW_ENGAGEMENT_ROLES) + @_jwt.requires_auth def get(engagement_id): """Fetch engagement metadata entries by engagement id.""" + authorization.check_auth( + one_of_roles=( + MembershipType.TEAM_MEMBER.name, + Role.VIEW_ENGAGEMENT.value + ), + engagement_id=engagement_id + ) return metadata_service.get_by_engagement(engagement_id) @staticmethod @@ -86,11 +91,16 @@ def get(engagement_id): @API.expect(metadata_create_model) # type: ignore @API.marshal_with(metadata_return_model, code=HTTPStatus.CREATED) - @require_role(EDIT_ENGAGEMENT_ROLES) + @_jwt.requires_auth def post(engagement_id: int): """Create a new metadata entry for an engagement.""" - authorization.check_auth(one_of_roles=EDIT_ENGAGEMENT_ROLES, - engagement_id=engagement_id) + authorization.check_auth( + one_of_roles=( + MembershipType.TEAM_MEMBER.name, + Role.EDIT_ENGAGEMENT.value + ), + engagement_id=engagement_id + ) request_json = request.get_json(force=True) try: engagement_metadata = metadata_service.create( @@ -107,11 +117,16 @@ def post(engagement_id: int): @API.doc(security='apikey') @API.expect(metadata_bulk_update_model, validate=True) @API.marshal_list_with(metadata_return_model) - @require_role(EDIT_ENGAGEMENT_ROLES) + @_jwt.requires_auth def patch(engagement_id): """Update the values of existing metadata entries for an engagement.""" - authorization.check_auth(one_of_roles=EDIT_ENGAGEMENT_ROLES, - engagement_id=engagement_id) + authorization.check_auth( + one_of_roles=( + MembershipType.TEAM_MEMBER.name, + Role.EDIT_ENGAGEMENT.value + ), + engagement_id=engagement_id + ) data = request.get_json(force=True) taxon_id = data['taxon_id'] updated_values = data['values'] @@ -131,11 +146,16 @@ class EngagementMetadataById(Resource): @staticmethod @cross_origin(origins=allowedorigins()) - @require_role(VIEW_ENGAGEMENT_ROLES) + @_jwt.requires_auth def get(engagement_id, metadata_id): """Fetch an engagement metadata entry by id.""" - authorization.check_auth(one_of_roles=VIEW_ENGAGEMENT_ROLES, - engagement_id=engagement_id) + authorization.check_auth( + one_of_roles=( + MembershipType.TEAM_MEMBER.name, + Role.VIEW_ENGAGEMENT.value + ), + engagement_id=engagement_id + ) try: metadata = metadata_service.get(metadata_id) if str(metadata['engagement_id']) != str(engagement_id): @@ -150,12 +170,17 @@ def get(engagement_id, metadata_id): @staticmethod @cross_origin(origins=allowedorigins()) - @require_role(EDIT_ENGAGEMENT_ROLES) @API.expect(metadata_update_model) + @_jwt.requires_auth def patch(engagement_id, metadata_id): """Update the values of an existing metadata entry for an engagement.""" - authorization.check_auth(one_of_roles=EDIT_ENGAGEMENT_ROLES, - engagement_id=engagement_id) + authorization.check_auth( + one_of_roles=( + MembershipType.TEAM_MEMBER.name, + Role.EDIT_ENGAGEMENT.value + ), + engagement_id=engagement_id + ) request_json = request.get_json(force=True) try: value = request_json.get('value') @@ -175,12 +200,17 @@ def patch(engagement_id, metadata_id): @staticmethod @cross_origin(origins=allowedorigins()) - @require_role(EDIT_ENGAGEMENT_ROLES) + @_jwt.requires_auth def delete(engagement_id, metadata_id): """Delete an existing metadata entry for an engagement.""" try: - authorization.check_auth(one_of_roles=EDIT_ENGAGEMENT_ROLES, - engagement_id=engagement_id) + authorization.check_auth( + one_of_roles=( + MembershipType.TEAM_MEMBER.name, + Role.EDIT_ENGAGEMENT.value + ), + engagement_id=engagement_id + ) metadata = metadata_service.get(metadata_id) if str(metadata['engagement_id']) != str(engagement_id): raise ValidationError('Metadata does not belong to this ' diff --git a/met-api/src/met_api/resources/metadata_taxon.py b/met-api/src/met_api/resources/metadata_taxon.py index 0a2565ed3..05d168b3c 100644 --- a/met-api/src/met_api/resources/metadata_taxon.py +++ b/met-api/src/met_api/resources/metadata_taxon.py @@ -33,8 +33,7 @@ from met_api.utils.util import allowedorigins, cors_preflight -VIEW_TAXA_ROLES = [Role.VIEW_ENGAGEMENT.value, Role.CREATE_ENGAGEMENT.value] -MODIFY_TAXA_ROLES = [Role.EDIT_ENGAGEMENT.value] +MANAGE_METADATA_ROLE = [Role.MANAGE_METADATA.value] TAXON_NOT_FOUND_MSG = 'Metadata taxon was not found' API = Namespace('metadata_taxa', description='Endpoints for managing the taxa ' @@ -117,7 +116,6 @@ class MetadataTaxa(Resource): @cross_origin(origins=allowedorigins()) @API.marshal_list_with(taxon_return_model) @ensure_tenant_access() - @require_role(VIEW_TAXA_ROLES) def get(tenant: Tenant): """Fetch a list of metadata taxa for the current tenant.""" tenant_taxa = taxon_service.get_by_tenant(tenant.id) @@ -129,7 +127,7 @@ def get(tenant: Tenant): # type: ignore @API.marshal_with(taxon_return_model, code=HTTPStatus.CREATED) @ensure_tenant_access() - @require_role(MODIFY_TAXA_ROLES) + @require_role(MANAGE_METADATA_ROLE) def post(tenant: Tenant): """Create a new metadata taxon for a tenant and return it.""" request_json = request.get_json(force=True) @@ -146,7 +144,7 @@ def post(tenant: Tenant): @API.expect(taxon_ids_model) @API.marshal_list_with(taxon_return_model) @ensure_tenant_access() - @require_role(MODIFY_TAXA_ROLES) + @require_role(MANAGE_METADATA_ROLE) def patch(tenant: Tenant): """Reorder the tenant's metadata taxa.""" request_json = request.get_json(force=True) @@ -173,7 +171,7 @@ class MetadataTaxon(Resource): @staticmethod @cross_origin(origins=allowedorigins()) @ensure_tenant_access() - @require_role(VIEW_TAXA_ROLES) + @require_role(MANAGE_METADATA_ROLE) @API.marshal_with(taxon_return_model) def get(tenant: Tenant, taxon_id: int): """Fetch a single metadata taxon matching the provided id.""" @@ -187,7 +185,7 @@ def get(tenant: Tenant, taxon_id: int): @API.expect(taxon_modify_model) @API.marshal_with(taxon_return_model) @ensure_tenant_access() - @require_role(MODIFY_TAXA_ROLES) + @require_role(MANAGE_METADATA_ROLE) def patch(tenant: Tenant, taxon_id: int): """Update a metadata taxon.""" metadata_taxon = taxon_service.get_by_id(taxon_id) @@ -199,7 +197,7 @@ def patch(tenant: Tenant, taxon_id: int): @staticmethod @cross_origin(origins=allowedorigins()) @ensure_tenant_access() - @require_role(MODIFY_TAXA_ROLES) + @require_role(MANAGE_METADATA_ROLE) @API.doc(responses={**responses, HTTPStatus.NO_CONTENT.value: 'Taxon deleted'}) def delete(tenant: Tenant, taxon_id: int): """Delete a metadata taxon.""" diff --git a/met-api/src/met_api/utils/roles.py b/met-api/src/met_api/utils/roles.py index 83ed7f67b..40166e070 100644 --- a/met-api/src/met_api/utils/roles.py +++ b/met-api/src/met_api/utils/roles.py @@ -61,3 +61,4 @@ class Role(Enum): EXPORT_INTERNAL_COMMENT_SHEET = 'export_internal_comment_sheet' EXPORT_PROPONENT_COMMENT_SHEET = 'export_proponent_comment_sheet' SUPER_ADMIN = 'super_admin' + MANAGE_METADATA = 'manage_metadata' diff --git a/met-api/tests/unit/api/test_engagement_metadata.py b/met-api/tests/unit/api/test_engagement_metadata.py index ff4fa6744..353b0e390 100644 --- a/met-api/tests/unit/api/test_engagement_metadata.py +++ b/met-api/tests/unit/api/test_engagement_metadata.py @@ -109,7 +109,7 @@ def test_add_engagement_metadata_invalid_user(client, jwt, session): headers=headers, data=json.dumps(metadata_info), content_type=ContentType.JSON.value) - assert response.status_code == HTTPStatus.UNAUTHORIZED + assert response.status_code == HTTPStatus.FORBIDDEN def test_update_engagement_metadata(client, jwt, session): @@ -131,6 +131,13 @@ def test_update_engagement_metadata(client, jwt, session): assert response.json.get('engagement_id') == engagement.id assert response.json.get('value') == 'new value' + # Test if Metadata does not belong to the engagement + engagement_id = fake.pyint() + response = client.get(f'/api/engagements/{engagement_id}/metadata/{metadata.id}', + headers=headers, + content_type=ContentType.JSON.value) + assert response.status_code == HTTPStatus.BAD_REQUEST + def test_bulk_update_engagement_metadata(client, jwt, session): """Test that metadata values can be updated in bulk.""" @@ -182,3 +189,25 @@ def test_delete_engagement_metadata(client, jwt, session): content_type=ContentType.JSON.value) assert response.status_code == HTTPStatus.NO_CONTENT, (f'Wrong response code; ' f'HTTP {response.status_code} -> {response.text}') + + +def test_get_engagement_metadata_by_id(client, jwt, session): + """Test that metadata can be fetched by id.""" + taxon, engagement, _, headers = factory_metadata_requirements(jwt) + metadata = factory_engagement_metadata_model({ + 'engagement_id': engagement.id, + 'taxon_id': taxon.id, + 'value': fake.sentence(), + }) + response = client.get(f'/api/engagements/{engagement.id}/metadata/{metadata.id}', + headers=headers, + content_type=ContentType.JSON.value) + assert response.status_code == HTTPStatus.OK, (f'Wrong response code; ' + f'HTTP {response.status_code} -> {response.text}') + + # Test if Metadata does not belong to the engagement + engagement_id = fake.pyint() + response = client.get(f'/api/engagements/{engagement_id}/metadata/{metadata.id}', + headers=headers, + content_type=ContentType.JSON.value) + assert response.status_code == HTTPStatus.BAD_REQUEST diff --git a/met-web/src/components/layout/SideNav/SideNavElements.tsx b/met-web/src/components/layout/SideNav/SideNavElements.tsx index 8fc0ba3f3..373da2164 100644 --- a/met-web/src/components/layout/SideNav/SideNavElements.tsx +++ b/met-web/src/components/layout/SideNav/SideNavElements.tsx @@ -28,8 +28,8 @@ export const Routes: Route[] = [ name: 'Metadata Management', path: '/metadatamanagement', base: '/metadatamanagement', - authenticated: false, - allowedRoles: [], + authenticated: true, + allowedRoles: [USER_ROLES.MANAGE_METADATA], }, { name: 'User Management', diff --git a/met-web/src/routes/AuthenticatedRoutes.tsx b/met-web/src/routes/AuthenticatedRoutes.tsx index 480459d64..0028fb8a0 100644 --- a/met-web/src/routes/AuthenticatedRoutes.tsx +++ b/met-web/src/routes/AuthenticatedRoutes.tsx @@ -61,7 +61,9 @@ const AuthenticatedRoutes = () => { } /> } /> } /> - } /> + }> + } /> + }> } /> diff --git a/met-web/src/services/userService/constants.ts b/met-web/src/services/userService/constants.ts index c0be8711f..283d95527 100644 --- a/met-web/src/services/userService/constants.ts +++ b/met-web/src/services/userService/constants.ts @@ -38,6 +38,7 @@ export const USER_ROLES = { EXPORT_ALL_TO_CSV: 'export_all_to_csv', EXPORT_INTERNAL_COMMENT_SHEET: 'export_internal_comment_sheet', EXPORT_PROPONENT_COMMENT_SHEET: 'export_proponent_comment_sheet', + MANAGE_METADATA: 'manage_metadata', }; export type UserStatusName = 'ACTIVE' | 'INACTIVE'; diff --git a/met-web/tests/unit/components/sidenav.test.tsx b/met-web/tests/unit/components/sidenav.test.tsx index f75602630..698ad6179 100644 --- a/met-web/tests/unit/components/sidenav.test.tsx +++ b/met-web/tests/unit/components/sidenav.test.tsx @@ -26,6 +26,7 @@ jest.mock('react-redux', () => ({ USER_ROLES.VIEW_USERS, USER_ROLES.VIEW_FEEDBACKS, USER_ROLES.SUPER_ADMIN, + USER_ROLES.MANAGE_METADATA, ]; }), }));