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
Changes from 1 commit
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
Next Next commit
Adding metadata management role
VineetBala-AOT committed May 13, 2024
commit 984c4805dee00319d1656daff4400d7af384c470
5 changes: 5 additions & 0 deletions CHANGELOG.MD
Original file line number Diff line number Diff line change
@@ -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)
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
@@ -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',
@@ -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.MANAGE_METADATA.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.MANAGE_METADATA.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.MANAGE_METADATA.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
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.MANAGE_METADATA.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.MANAGE_METADATA.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.MANAGE_METADATA.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 '
15 changes: 7 additions & 8 deletions met-api/src/met_api/resources/metadata_taxon.py
Original file line number Diff line number Diff line change
@@ -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,7 @@ class MetadataTaxa(Resource):
@cross_origin(origins=allowedorigins())
@API.marshal_list_with(taxon_return_model)
@ensure_tenant_access()
@require_role(VIEW_TAXA_ROLES)
@require_role(MANAGE_METADATA_ROLE)
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 +128,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 +145,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 +172,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 +186,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 +198,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."""
1 change: 1 addition & 0 deletions met-api/src/met_api/utils/roles.py
Original file line number Diff line number Diff line change
@@ -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
@@ -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):
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
@@ -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',
4 changes: 3 additions & 1 deletion met-web/src/routes/AuthenticatedRoutes.tsx
Original file line number Diff line number Diff line change
@@ -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>
1 change: 1 addition & 0 deletions met-web/src/services/userService/constants.ts
Original file line number Diff line number Diff line change
@@ -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';

Unchanged files with check annotations Beta

import { Grid } from '@mui/material';
export default class EngagementBannerWC extends HTMLElement {
root: any;

Check warning on line 14 in met-web/src/web-components/components/engagement-banner-wc.tsx

GitHub Actions / linting (16.x)

Unexpected any. Specify a different type
observer: MutationObserver;
shadowContainer: any;

Check warning on line 16 in met-web/src/web-components/components/engagement-banner-wc.tsx

GitHub Actions / linting (16.x)

Unexpected any. Specify a different type
constructor(componentToMount: React.ComponentType) {
super();
this.observer = new MutationObserver(() => this.update());
});
const shadowTheme = createWcTheme(shadowRootElement);
this.root = ReactDOM.createRoot(shadowRootElement);
const props: any = {

Check warning on line 46 in met-web/src/web-components/components/engagement-banner-wc.tsx

GitHub Actions / linting (16.x)

Unexpected any. Specify a different type
...this.getProps(this.attributes),
...this.getEvents(),
};
this.unmount();
this.mount();
}
getProps(attributes: any) {

Check warning on line 87 in met-web/src/web-components/components/engagement-banner-wc.tsx

GitHub Actions / linting (16.x)

Unexpected any. Specify a different type
return [...attributes]
.filter((attr) => attr.name !== 'style')
.map((attr) => this.convert(attr.name, attr.value))
.reduce(
(events, ev) => ({
...events,
[ev.name]: (args: any) => this.dispatchEvent(new CustomEvent(ev.name, { ...args })),

Check warning on line 99 in met-web/src/web-components/components/engagement-banner-wc.tsx

GitHub Actions / linting (16.x)

Unexpected any. Specify a different type
}),
{},
);
}
convert(attrName: any, attrValue: any) {

Check warning on line 104 in met-web/src/web-components/components/engagement-banner-wc.tsx

GitHub Actions / linting (16.x)

Unexpected any. Specify a different type

Check warning on line 104 in met-web/src/web-components/components/engagement-banner-wc.tsx

GitHub Actions / linting (16.x)

Unexpected any. Specify a different type
let value = attrValue;
if (attrValue === 'true' || attrValue === 'false') value = attrValue === 'true';
else if (!isNaN(attrValue) && attrValue !== '') value = +attrValue;
class WCBaseELement extends HTMLElement {
ComponentToMount: React.ComponentType;
root: any;

Check warning on line 12 in met-web/src/web-components/components/wcBase.tsx

GitHub Actions / linting (16.x)

Unexpected any. Specify a different type
observer: MutationObserver;
shadowContainer: any;

Check warning on line 14 in met-web/src/web-components/components/wcBase.tsx

GitHub Actions / linting (16.x)

Unexpected any. Specify a different type
constructor(componentToMount: React.ComponentType) {
super();
this.ComponentToMount = componentToMount;
this.unmount();
this.mount();
}
getProps(attributes: any) {

Check warning on line 74 in met-web/src/web-components/components/wcBase.tsx

GitHub Actions / linting (16.x)

Unexpected any. Specify a different type
return [...attributes]
.filter((attr) => attr.name !== 'style')
.map((attr) => this.convert(attr.name, attr.value))