-
Notifications
You must be signed in to change notification settings - Fork 19
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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 ### |
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 | ||
def get(engagement_id): | ||
"""Fetch engagement metadata entries by engagement id.""" | ||
authorization.check_auth( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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): | ||
|
@@ -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 ' | ||
|
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): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
There was a problem hiding this comment.
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?