You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2021/04/04 15:29:24 UTC
[GitHub] [airflow] ephraimbuddy opened a new pull request #14840: Add CUD REST API endpoints for Roles
ephraimbuddy opened a new pull request #14840:
URL: https://github.com/apache/airflow/pull/14840
Closes: #14830
Add create, update and delete endpoints for Roles
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
In case of fundamental code change, Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvements+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
In case of backwards incompatible changes please leave a note in [UPDATING.md](https://github.com/apache/airflow/blob/master/UPDATING.md).
--
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:
users@infra.apache.org
[GitHub] [airflow] ephraimbuddy closed pull request #14840: Add CUD REST API endpoints for Roles
Posted by GitBox <gi...@apache.org>.
ephraimbuddy closed pull request #14840:
URL: https://github.com/apache/airflow/pull/14840
--
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:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #14840: Add CUD REST API endpoints for Roles
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #14840:
URL: https://github.com/apache/airflow/pull/14840#discussion_r598713335
##########
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))
+
+
+@security.requires_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:
Ah obviously failed in my search -- I thought I checked there first. Obviously not.
--
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:
users@infra.apache.org
[GitHub] [airflow] ephraimbuddy merged pull request #14840: Add CUD REST API endpoints for Roles
Posted by GitBox <gi...@apache.org>.
ephraimbuddy merged pull request #14840:
URL: https://github.com/apache/airflow/pull/14840
--
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:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #14840: Add CUD REST API endpoints for Roles
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #14840:
URL: https://github.com/apache/airflow/pull/14840#issuecomment-803968508
[The Workflow run](https://github.com/apache/airflow/actions/runs/675765799) is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.
--
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:
users@infra.apache.org
[GitHub] [airflow] ephraimbuddy commented on a change in pull request #14840: Add CUD REST API endpoints for Roles
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on a change in pull request #14840:
URL: https://github.com/apache/airflow/pull/14840#discussion_r598788711
##########
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))
+
+
+@security.requires_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
+
+
+@security.requires_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)
+
+
+@security.requires_access([(permissions.ACTION_CAN_ADD, permissions.RESOURCE_ROLE_MODEL_VIEW)])
Review comment:
Exactly, I have returned it back. I realized Admin doesn't have can_create on RoleModelView. @jhtimmins PR on using resources will address this
--
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:
users@infra.apache.org
[GitHub] [airflow] ephraimbuddy commented on a change in pull request #14840: Add CUD REST API endpoints for Roles
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on a change in pull request #14840:
URL: https://github.com/apache/airflow/pull/14840#discussion_r598584571
##########
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))
+
+
+@security.requires_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:
Yes, it was added in airflow, see https://github.com/apache/airflow/blob/fa92657490bc188d272fb4d8f3b09933815bcea1/airflow/www/security.py#L194-L207
--
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:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #14840: Add CUD REST API endpoints for Roles
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #14840:
URL: https://github.com/apache/airflow/pull/14840#issuecomment-811493957
The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest master at your convenience, or amend the last commit of the PR, and push it with --force-with-lease.
--
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:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #14840: Add CUD REST API endpoints for Roles
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #14840:
URL: https://github.com/apache/airflow/pull/14840#discussion_r598714354
##########
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))
+
+
+@security.requires_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
+
+
+@security.requires_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)
+
+
+@security.requires_access([(permissions.ACTION_CAN_ADD, permissions.RESOURCE_ROLE_MODEL_VIEW)])
Review comment:
Since this is a built-in FAB view, it will likely use CAN_ADD, so we'll need to customize that view (and create a migration) to avoid having two permissions for the same resource)
--
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:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #14840: Add CUD REST API endpoints for Roles
Posted by GitBox <gi...@apache.org>.
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))
+
+
+@security.requires_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))
+
+
+@security.requires_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
+
+
+@security.requires_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)
+
+
+@security.requires_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:
users@infra.apache.org