Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[To Main] Add metadata management role #2506

Merged
merged 3 commits into from
May 14, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions CHANGELOG.MD
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
Original file line number Diff line number Diff line change
@@ -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 ###
70 changes: 50 additions & 20 deletions met-api/src/met_api/resources/engagement_metadata.py
Original file line number Diff line number Diff line change
Expand Up @@ -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/<engagement_id>/metadata',
Expand Down Expand Up @@ -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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will public users still be able to get engagement metadata?

def get(engagement_id):
"""Fetch engagement metadata entries by engagement id."""
authorization.check_auth(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Levels of abstraction are tricky here, but the ticket still calls for engagement metadata to allow CRUD operations for users who can edit engagements.

I believe it's the metadata_taxon file that will restrict its operations to the MANAGE_METADATA role.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, changed it back to use edit engagement. The only change here being allowing team members assigned to the engagement to edit as well.

one_of_roles=(
MembershipType.TEAM_MEMBER.name,
Role.VIEW_ENGAGEMENT.value
),
engagement_id=engagement_id
)
return metadata_service.get_by_engagement(engagement_id)

@staticmethod
Expand All @@ -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(
Expand All @@ -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']
Expand All @@ -131,11 +146,16 @@ class EngagementMetadataById(Resource):

@staticmethod
@cross_origin(origins=allowedorigins())
@require_role(VIEW_ENGAGEMENT_ROLES)
@_jwt.requires_auth
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we sure about requiring authorization for this route? What if we're pulling engagement metadata on the frontend for public users?

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):
Expand All @@ -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')
Expand All @@ -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 '
Expand Down
14 changes: 6 additions & 8 deletions met-api/src/met_api/resources/metadata_taxon.py
Original file line number Diff line number Diff line change
Expand Up @@ -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 '
Expand Down Expand Up @@ -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)
Expand All @@ -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)
Expand All @@ -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)
Expand All @@ -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."""
Expand All @@ -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)
Expand All @@ -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."""
Expand Down
1 change: 1 addition & 0 deletions met-api/src/met_api/utils/roles.py
Original file line number Diff line number Diff line change
Expand Up @@ -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'
31 changes: 30 additions & 1 deletion met-api/tests/unit/api/test_engagement_metadata.py
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Expand All @@ -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."""
Expand Down Expand Up @@ -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):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have a way to test requests based on roles? It would be nice to have some tests directly related to the work for this ticket. Although I appreciate that you added a couple more general metadata tests

"""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
4 changes: 2 additions & 2 deletions met-web/src/components/layout/SideNav/SideNavElements.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand Down
4 changes: 3 additions & 1 deletion met-web/src/routes/AuthenticatedRoutes.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,9 @@ const AuthenticatedRoutes = () => {
<Route path="/:slug/dashboard/:dashboardType" element={<PublicDashboard />} />
<Route path="/engagements/:engagementId/dashboard/:dashboardType" element={<PublicDashboard />} />
<Route path="/:slug/comments/:dashboardType" element={<EngagementComments />} />
<Route path="/metadatamanagement" element={<MetadataManagement />} />
<Route element={<AuthGate allowedRoles={[USER_ROLES.MANAGE_METADATA]} />}>
<Route path="/metadatamanagement" element={<MetadataManagement />} />
</Route>
<Route element={<AuthGate allowedRoles={[USER_ROLES.SUPER_ADMIN]} />}>
<Route path="/tenantadmin" element={<TenantManagement />} />
</Route>
Expand Down
1 change: 1 addition & 0 deletions met-web/src/services/userService/constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down
1 change: 1 addition & 0 deletions met-web/tests/unit/components/sidenav.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ jest.mock('react-redux', () => ({
USER_ROLES.VIEW_USERS,
USER_ROLES.VIEW_FEEDBACKS,
USER_ROLES.SUPER_ADMIN,
USER_ROLES.MANAGE_METADATA,
];
}),
}));
Expand Down
Loading