ashb commented on a change in pull request #14840:
URL: https://github.com/apache/airflow/pull/14840#discussion_r598568593
##########
File path: airflow/api_connexion/endpoints/role_and_permission_endpoint.py
##########
@@ -64,3 +66,70 @@ def get_permissions(limit=None, offset=None):
query = session.query(Permission)
actions = query.offset(offset).limit(limit).all()
return action_collection_schema.dump(ActionCollection(actions=actions,
total_entries=total_entries))
+
+
[email protected]_access([(permissions.ACTION_CAN_DELETE,
permissions.RESOURCE_ROLE_MODEL_VIEW)])
+def delete_role(role_name):
+ """Delete a role"""
+ ab_security_manager = current_app.appbuilder.sm
+ role = ab_security_manager.find_role(name=role_name)
+ if not role:
+ raise NotFound(title="Role not found", detail=f"The Role with name
`{role_name}` was not found")
+ ab_security_manager.delete_role(role_name=role_name)
Review comment:
I don't see a `delete_role` method in
https://github.com/dpgaspar/Flask-AppBuilder/blob/302e4abe32d9d2403f2ce52e4abb31beb59bb252/flask_appbuilder/security/manager.py#L1775-L1791
So this needs to be `find_role()` and then `session.delete(role)` I think.
##########
File path: airflow/security/permissions.py
##########
@@ -50,6 +50,7 @@
RESOURCE_PERMISSION_MODEL_VIEW = "PermissionModelView"
# Action Constants
+ACTION_CAN_ADD = "can_add"
Review comment:
```suggestion
```
##########
File path: tests/api_connexion/endpoints/test_role_and_permission_endpoint.py
##########
@@ -146,3 +168,224 @@ def test_should_raise_403_forbidden(self):
"/api/v1/permissions", environ_overrides={'REMOTE_USER':
"test_no_permissions"}
)
assert response.status_code == 403
+
+
+class TestPostRole(TestRoleEndpoint):
+ def test_post_should_respond_200(self):
+ payload = {
+ 'name': 'Test2',
+ 'actions': [{'resource': {'name': 'Connections'}, 'action':
{'name': 'can_create'}}],
+ }
+ response = self.client.post("/api/v1/roles", json=payload,
environ_overrides={'REMOTE_USER': "test"})
+ assert response.status_code == 200
+ role = self.app.appbuilder.sm.find_role('Test2')
+ assert role is not None
+
+ def test_post_should_respond_400_for_invalid_payload(self):
+ payload = {
+ 'actions': [{'resource': {'name': 'Connections'}, 'action':
{'name': 'can_create'}}],
+ }
+ response = self.client.post("/api/v1/roles", json=payload,
environ_overrides={'REMOTE_USER': "test"})
+ assert response.status_code == 400
+ assert response.json == {
+ 'detail': "{'name': ['Missing data for required field.']}",
+ 'status': 400,
+ 'title': 'Bad Request',
+ 'type': EXCEPTIONS_LINK_MAP[400],
+ }
+
+ def test_post_should_respond_409_already_exist(self):
+ payload = {
+ 'name': 'Test',
+ 'actions': [{'resource': {'name': 'Connections'}, 'action':
{'name': 'can_create'}}],
+ }
+ response = self.client.post("/api/v1/roles", json=payload,
environ_overrides={'REMOTE_USER': "test"})
+ assert response.status_code == 409
+ assert response.json == {
+ 'detail': "Role with name `Test` already exist. Please update with
patch endpoint",
+ 'status': 409,
+ 'title': 'Conflict',
+ 'type': EXCEPTIONS_LINK_MAP[409],
+ }
+
+ def test_should_raises_401_unauthenticated(self):
+ response = self.client.post(
+ "/api/v1/roles",
+ json={
+ 'name': 'Test2',
+ 'actions': [{'resource': {'name': 'Connections'}, 'action':
{'name': 'can_create'}}],
+ },
+ )
+
+ assert_401(response)
+
+ def test_should_raise_403_forbidden(self):
+ response = self.client.post(
+ "/api/v1/roles",
+ json={
+ "name": "mytest2",
+ "actions": [{"resource": {"name": "Connections"}, "action":
{"name": "can_create"}}],
+ },
+ environ_overrides={'REMOTE_USER': "test_no_permissions"},
+ )
+ assert response.status_code == 403
+
+
+class TestDeleteRole(TestRoleEndpoint):
+ def test_delete_should_respond_204(self, session):
+ role = create_role(self.app, "mytestrole")
+ response = self.client.delete(f"/api/v1/roles/{role.name}",
environ_overrides={'REMOTE_USER': "test"})
+ assert response.status_code == 204
+ role_obj = session.query(Role).filter(Role.name == role.name).all()
+ assert len(role_obj) == 0
+
+ def test_delete_should_respond_404(self):
+ response = self.client.delete(
+ "/api/v1/roles/invalidrolename", environ_overrides={'REMOTE_USER':
"test"}
+ )
+ assert response.status_code == 404
+ assert response.json == {
+ 'detail': "The Role with name `invalidrolename` was not found",
+ 'status': 404,
+ 'title': 'Role not found',
+ 'type': EXCEPTIONS_LINK_MAP[404],
+ }
+
+ def test_should_raises_401_unauthenticated(self):
+ response = self.client.delete("/api/v1/roles/test")
+
+ assert_401(response)
+
+ def test_should_raise_403_forbidden(self):
+ response = self.client.delete(
+ "/api/v1/roles/test", environ_overrides={'REMOTE_USER':
"test_no_permissions"}
+ )
+ assert response.status_code == 403
+
+
+class TestPatchRole(TestRoleEndpoint):
+ @parameterized.expand(
+ [
+ ({"name": "mytest"}, "mytest", []),
+ (
+ {
+ "name": "mytest2",
+ "actions": [{"resource": {"name": "Connections"},
"action": {"name": "can_create"}}],
+ },
+ "mytest2",
+ [{"resource": {"name": "Connections"}, "action": {"name":
"can_create"}}],
+ ),
+ ]
+ )
+ def test_patch_should_respond_200(self, payload, expected_name,
expected_actions):
+ role = create_role(self.app, 'mytestrole')
+ response = self.client.patch(
+ f"/api/v1/roles/{role.name}", json=payload,
environ_overrides={'REMOTE_USER': "test"}
+ )
+ assert response.status_code == 200
+ assert response.json['name'] == expected_name
+ assert response.json["actions"] == expected_actions
+
+ @parameterized.expand(
+ [
+ (
+ "?update_mask=name",
+ {
+ "name": "mytest2",
+ "actions": [{"resource": {"name": "Connections"},
"action": {"name": "can_create"}}],
+ },
+ "mytest2",
+ [],
+ ),
+ (
+ "?update_mask=name, actions", # both name and actions in
update mask
+ {
+ "name": "mytest2",
+ "actions": [{"resource": {"name": "Connections"},
"action": {"name": "can_create"}}],
+ },
+ "mytest2",
+ [{"resource": {"name": "Connections"}, "action": {"name":
"can_create"}}],
+ ),
+ ]
+ )
+ def test_patch_should_respond_200_with_update_mask(
+ self, update_mask, payload, expected_name, expected_actions
+ ):
+ role = create_role(self.app, "mytestrole")
+ assert role.permissions == []
+ response = self.client.patch(
+ f"/api/v1/roles/{role.name}{update_mask}",
+ json=payload,
+ environ_overrides={'REMOTE_USER': "test"},
+ )
+ assert response.status_code == 200
+ assert response.json['name'] == expected_name
+ assert response.json['actions'] == expected_actions
+
+ def test_patch_should_respond_400_for_invalid_fields_in_update_mask(self):
+ role = create_role(self.app, "mytestrole")
+ payload = {"name": "testme"}
+ response = self.client.patch(
+ f"/api/v1/roles/{role.name}?update_mask=invalid_name",
+ json=payload,
+ environ_overrides={'REMOTE_USER': "test"},
+ )
+ assert response.status_code == 400
+ assert response.json['detail'] == "'invalid_name' in update_mask is
unknown"
+
+ @parameterized.expand(
+ [
+ (
+ {
+ "name": "testme",
+ "permissions": [ # Using permissions instead of actions
should raise
+ {"resource": {"name": "Connections"}, "action":
{"name": "can_create"}}
+ ],
+ },
+ "{'permissions': ['Unknown field.']}",
+ ),
+ (
+ {
+ "name": "testme",
+ "actions": [
+ {
+ "view_menu": {"name": "Connections"}, # Using
view_menu instead of resource
+ "action": {"name": "can_create"},
+ }
+ ],
+ },
+ "{'actions': {0: {'view_menu': ['Unknown field.']}}}",
+ ),
+ ]
+ )
Review comment:
We should also test what happens if the "structure" is valid, but a
resource or action are invalid (i.e. if you have ` {"resource": {"name":
"FooBars"}`)
##########
File path: airflow/api_connexion/endpoints/role_and_permission_endpoint.py
##########
@@ -64,3 +66,70 @@ def get_permissions(limit=None, offset=None):
query = session.query(Permission)
actions = query.offset(offset).limit(limit).all()
return action_collection_schema.dump(ActionCollection(actions=actions,
total_entries=total_entries))
+
+
[email protected]_access([(permissions.ACTION_CAN_DELETE,
permissions.RESOURCE_ROLE_MODEL_VIEW)])
+def delete_role(role_name):
+ """Delete a role"""
+ ab_security_manager = current_app.appbuilder.sm
+ role = ab_security_manager.find_role(name=role_name)
+ if not role:
+ raise NotFound(title="Role not found", detail=f"The Role with name
`{role_name}` was not found")
+ ab_security_manager.delete_role(role_name=role_name)
+ return NoContent, 204
+
+
[email protected]_access([(permissions.ACTION_CAN_EDIT,
permissions.RESOURCE_ROLE_MODEL_VIEW)])
+def patch_role(role_name, update_mask=None):
+ """Update a role"""
+ appbuilder = current_app.appbuilder
+ security_manager = appbuilder.sm
+ body = request.json
+ try:
+ data = role_schema.load(body)
+ except ValidationError as err:
+ raise BadRequest(detail=str(err.messages))
+ role = security_manager.find_role(name=role_name)
+ if not role:
+ raise NotFound(title="Role not found", detail=f"Role with name:
`{role_name} was not found")
+ if update_mask:
+ update_mask = [i.strip() for i in update_mask]
+ data_ = {}
+ for field in update_mask:
+ if field in data and not field == "permissions":
+ data_[field] = data[field]
+ elif field == "actions":
+ data_["permissions"] = data['permissions']
+ else:
+ raise BadRequest(detail=f"'{field}' in update_mask is unknown")
+ data = data_
+ perms = data.get("permissions", [])
+ if perms:
+ perms = [
+ (item['permission']['name'], item['view_menu']['name']) for item
in data['permissions'] if item
+ ]
+ security_manager.update_role(pk=role.id, name=data['name'])
+ security_manager.init_role(role_name=data['name'], perms=perms or
role.permissions)
+ return role_schema.dump(role)
+
+
[email protected]_access([(permissions.ACTION_CAN_ADD,
permissions.RESOURCE_ROLE_MODEL_VIEW)])
Review comment:
```suggestion
@security.requires_access([(permissions.ACTION_CAN_CREATE,
permissions.RESOURCE_ROLE_MODEL_VIEW)])
```
And if that is not the permission that is actually used then please ask
@jhtimmins how to change it so it is :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
