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/03/12 00:07:30 UTC
[GitHub] [airflow] ephraimbuddy opened a new pull request #14735: Add readonly REST API endpoints for users
ephraimbuddy opened a new pull request #14735:
URL: https://github.com/apache/airflow/pull/14735
This PR adds readonly user endpoints.
Depends on #14664
---
**^ 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] github-actions[bot] commented on pull request #14735: Add readonly REST API endpoints for users
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #14735:
URL: https://github.com/apache/airflow/pull/14735#issuecomment-801221195
The PR is likely OK to be merged with just subset of tests for default Python and Database versions without running the full matrix of tests, because it does not modify the core of Airflow. If the committers decide that the full tests matrix is needed, they will add the label 'full tests needed'. Then you should rebase to the latest master 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] kaxil commented on a change in pull request #14735: Add readonly REST API endpoints for users
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #14735:
URL: https://github.com/apache/airflow/pull/14735#discussion_r595907749
##########
File path: tests/api_connexion/endpoints/test_user_endpoint.py
##########
@@ -0,0 +1,225 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+import pytest
+from flask_appbuilder.security.sqla.models import User
+from parameterized import parameterized
+
+from airflow.api_connexion.exceptions import EXCEPTIONS_LINK_MAP
+from airflow.security import permissions
+from airflow.utils import timezone
+from tests.test_utils.api_connexion_utils import assert_401, create_user, delete_user
+from tests.test_utils.config import conf_vars
+
+DEFAULT_TIME = "2020-06-11T18:00:00+00:00"
+
+
+@pytest.fixture(scope="module")
+def configured_app(minimal_app_for_api):
+ app = minimal_app_for_api
+ create_user(
+ app, # type: ignore
+ username="test",
+ role_name="Test",
+ permissions=[
+ (permissions.ACTION_CAN_LIST, permissions.RESOURCE_USER_DB_MODELVIEW),
+ (permissions.ACTION_CAN_SHOW, permissions.RESOURCE_USER_DB_MODELVIEW),
Review comment:
Just checking -- Based on our previous discussion will this be changed to `ACTION_CAN_READ` in the next PR?
##########
File path: tests/api_connexion/endpoints/test_user_endpoint.py
##########
@@ -0,0 +1,225 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+import pytest
+from flask_appbuilder.security.sqla.models import User
+from parameterized import parameterized
+
+from airflow.api_connexion.exceptions import EXCEPTIONS_LINK_MAP
+from airflow.security import permissions
+from airflow.utils import timezone
+from tests.test_utils.api_connexion_utils import assert_401, create_user, delete_user
+from tests.test_utils.config import conf_vars
+
+DEFAULT_TIME = "2020-06-11T18:00:00+00:00"
+
+
+@pytest.fixture(scope="module")
+def configured_app(minimal_app_for_api):
+ app = minimal_app_for_api
+ create_user(
+ app, # type: ignore
+ username="test",
+ role_name="Test",
+ permissions=[
+ (permissions.ACTION_CAN_LIST, permissions.RESOURCE_USER_DB_MODELVIEW),
+ (permissions.ACTION_CAN_SHOW, permissions.RESOURCE_USER_DB_MODELVIEW),
+ ],
+ )
+ create_user(app, username="test_no_permissions", role_name="TestNoPermissions") # type: ignore
+
+ yield app
+
+ delete_user(app, username="test") # type: ignore
+ delete_user(app, username="test_no_permissions") # type: ignore
+
+
+class TestUserEndpoint:
+ @pytest.fixture(autouse=True)
+ def setup_attrs(self, configured_app) -> None:
+ self.app = configured_app
+ self.client = self.app.test_client() # type:ignore
+ self.session = self.app.appbuilder.get_session
+
+ def teardown_method(self) -> None:
+ # Delete users that have our custom default time
+ users = self.session.query(User).filter(User.changed_on == timezone.parse(DEFAULT_TIME)).all()
+ for user in users:
+ self.session.delete(user)
+ self.session.commit()
+
+ def _create_users(self, count, roles=None):
+ # create users with defined created_on and changed_on date
+ # for easy testing
+ if roles is None:
+ roles = []
+ return [
+ User(
+ first_name=f'test{i}',
+ last_name=f'test{i}',
+ username=f'TEST_USER{i}',
+ email=f'mytest@test{i}.org',
+ roles=roles or [],
+ created_on=timezone.parse(DEFAULT_TIME),
+ changed_on=timezone.parse(DEFAULT_TIME),
+ )
+ for i in range(1, count + 1)
+ ]
+
+
+class TestGetUser(TestUserEndpoint):
+ def test_should_respond_200(self):
+ users = self._create_users(1)
+ self.session.add_all(users)
+ self.session.commit()
+ response = self.client.get("/api/v1/users/TEST_USER1", environ_overrides={'REMOTE_USER': "test"})
+ assert response.status_code == 200
+ assert response.json == {
+ 'active': None,
+ 'changed_on': DEFAULT_TIME,
+ 'created_on': DEFAULT_TIME,
+ 'email': 'mytest@test1.org',
+ 'fail_login_count': None,
+ 'first_name': 'test1',
+ 'last_login': None,
+ 'last_name': 'test1',
+ 'login_count': None,
+ 'roles': [],
+ 'user_id': users[0].id,
+ 'username': 'TEST_USER1',
+ }
+
+ def test_should_respond_404(self):
+ response = self.client.get("/api/v1/users/invalid-user", environ_overrides={'REMOTE_USER': "test"})
+ assert response.status_code == 404
+ assert {
+ 'detail': "The User with username `invalid-user` was not found",
+ 'status': 404,
+ 'title': 'User not found',
+ 'type': EXCEPTIONS_LINK_MAP[404],
+ } == response.json
+
+ def test_should_raises_401_unauthenticated(self):
+ response = self.client.get("/api/v1/users/TEST_USER1")
+ assert_401(response)
+
+ def test_should_raise_403_forbidden(self):
+ response = self.client.get(
+ "/api/v1/users/TEST_USER1", environ_overrides={'REMOTE_USER': "test_no_permissions"}
+ )
+ assert response.status_code == 403
+
+
+class TestGetUsers(TestUserEndpoint):
+ def test_should_response_200(self):
+ response = self.client.get("/api/v1/users", environ_overrides={'REMOTE_USER': "test"})
+ assert response.status_code == 200
+ assert response.json["total_entries"] == 2
+ usernames = [user["username"] for user in response.json["users"] if user]
+ assert usernames == ['test', 'test_no_permissions']
+
+ def test_should_raises_401_unauthenticated(self):
+ response = self.client.get("/api/v1/users")
+ assert_401(response)
+
+ def test_should_raise_403_forbidden(self):
+ response = self.client.get("/api/v1/users", environ_overrides={'REMOTE_USER': "test_no_permissions"})
+ assert response.status_code == 403
+
+
+class TestGetUsersPagination(TestUserEndpoint):
+ @parameterized.expand(
+ [
+ ("/api/v1/users?limit=1", ['test']),
+ ("/api/v1/users?limit=2", ['test', "test_no_permissions"]),
Review comment:
Just for consistency
```suggestion
("/api/v1/users?limit=1", ["test"]),
("/api/v1/users?limit=2", ["test", "test_no_permissions"]),
```
##########
File path: airflow/api_connexion/endpoints/user_endpoint.py
##########
@@ -0,0 +1,53 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+from flask import current_app
+from flask_appbuilder.security.sqla.models import User
+from sqlalchemy import func
+
+from airflow.api_connexion import security
+from airflow.api_connexion.exceptions import NotFound
+from airflow.api_connexion.parameters import check_limit, format_parameters
+from airflow.api_connexion.schemas.user_schema import (
+ UserCollection,
+ user_collection_item_schema,
+ user_collection_schema,
+)
+from airflow.security import permissions
+
+
+@security.requires_access([(permissions.ACTION_CAN_SHOW, permissions.RESOURCE_USER_DB_MODELVIEW)])
+def get_user(username):
+ """Get a user"""
+ ab_security_manager = current_app.appbuilder.sm
+ user = ab_security_manager.find_user(username=username)
+ if not user:
+ raise NotFound(title="User not found", detail=f"The User with username `{username}` was not found")
+ return user_collection_item_schema.dump(user)
+
+
+@security.requires_access([(permissions.ACTION_CAN_LIST, permissions.RESOURCE_USER_DB_MODELVIEW)])
+@format_parameters({'limit': check_limit})
+def get_users(limit=None, offset=None):
+ """Get users"""
+ appbuilder = current_app.appbuilder
+ session = appbuilder.get_session
+ total_entries = session.query(func.count(User.id)).scalar()
+ query = session.query(User)
+
+ users = query.order_by(User.id).offset(offset).limit(limit).all()
Review comment:
Since `query` is not used anywhere else
```suggestion
users = session.query(User).order_by(User.id).offset(offset).limit(limit).all()
```
##########
File path: tests/api_connexion/endpoints/test_user_endpoint.py
##########
@@ -0,0 +1,225 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+import pytest
+from flask_appbuilder.security.sqla.models import User
+from parameterized import parameterized
+
+from airflow.api_connexion.exceptions import EXCEPTIONS_LINK_MAP
+from airflow.security import permissions
+from airflow.utils import timezone
+from tests.test_utils.api_connexion_utils import assert_401, create_user, delete_user
+from tests.test_utils.config import conf_vars
+
+DEFAULT_TIME = "2020-06-11T18:00:00+00:00"
+
+
+@pytest.fixture(scope="module")
+def configured_app(minimal_app_for_api):
+ app = minimal_app_for_api
+ create_user(
+ app, # type: ignore
+ username="test",
+ role_name="Test",
+ permissions=[
+ (permissions.ACTION_CAN_LIST, permissions.RESOURCE_USER_DB_MODELVIEW),
+ (permissions.ACTION_CAN_SHOW, permissions.RESOURCE_USER_DB_MODELVIEW),
+ ],
+ )
+ create_user(app, username="test_no_permissions", role_name="TestNoPermissions") # type: ignore
+
+ yield app
+
+ delete_user(app, username="test") # type: ignore
+ delete_user(app, username="test_no_permissions") # type: ignore
+
+
+class TestUserEndpoint:
+ @pytest.fixture(autouse=True)
+ def setup_attrs(self, configured_app) -> None:
+ self.app = configured_app
+ self.client = self.app.test_client() # type:ignore
+ self.session = self.app.appbuilder.get_session
+
+ def teardown_method(self) -> None:
+ # Delete users that have our custom default time
+ users = self.session.query(User).filter(User.changed_on == timezone.parse(DEFAULT_TIME)).all()
+ for user in users:
+ self.session.delete(user)
+ self.session.commit()
+
+ def _create_users(self, count, roles=None):
+ # create users with defined created_on and changed_on date
+ # for easy testing
+ if roles is None:
+ roles = []
+ return [
+ User(
+ first_name=f'test{i}',
+ last_name=f'test{i}',
+ username=f'TEST_USER{i}',
+ email=f'mytest@test{i}.org',
+ roles=roles or [],
+ created_on=timezone.parse(DEFAULT_TIME),
+ changed_on=timezone.parse(DEFAULT_TIME),
+ )
+ for i in range(1, count + 1)
+ ]
+
+
+class TestGetUser(TestUserEndpoint):
+ def test_should_respond_200(self):
+ users = self._create_users(1)
+ self.session.add_all(users)
+ self.session.commit()
+ response = self.client.get("/api/v1/users/TEST_USER1", environ_overrides={'REMOTE_USER': "test"})
+ assert response.status_code == 200
+ assert response.json == {
+ 'active': None,
+ 'changed_on': DEFAULT_TIME,
+ 'created_on': DEFAULT_TIME,
+ 'email': 'mytest@test1.org',
+ 'fail_login_count': None,
Review comment:
Why is this `None`, should it be 0 instead?
Same for `login_count`
----------------------------------------------------------------
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 #14735: Add readonly REST API endpoints for users
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on a change in pull request #14735:
URL: https://github.com/apache/airflow/pull/14735#discussion_r596026874
##########
File path: tests/api_connexion/endpoints/test_user_endpoint.py
##########
@@ -0,0 +1,225 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+import pytest
+from flask_appbuilder.security.sqla.models import User
+from parameterized import parameterized
+
+from airflow.api_connexion.exceptions import EXCEPTIONS_LINK_MAP
+from airflow.security import permissions
+from airflow.utils import timezone
+from tests.test_utils.api_connexion_utils import assert_401, create_user, delete_user
+from tests.test_utils.config import conf_vars
+
+DEFAULT_TIME = "2020-06-11T18:00:00+00:00"
+
+
+@pytest.fixture(scope="module")
+def configured_app(minimal_app_for_api):
+ app = minimal_app_for_api
+ create_user(
+ app, # type: ignore
+ username="test",
+ role_name="Test",
+ permissions=[
+ (permissions.ACTION_CAN_LIST, permissions.RESOURCE_USER_DB_MODELVIEW),
+ (permissions.ACTION_CAN_SHOW, permissions.RESOURCE_USER_DB_MODELVIEW),
+ ],
+ )
+ create_user(app, username="test_no_permissions", role_name="TestNoPermissions") # type: ignore
+
+ yield app
+
+ delete_user(app, username="test") # type: ignore
+ delete_user(app, username="test_no_permissions") # type: ignore
+
+
+class TestUserEndpoint:
+ @pytest.fixture(autouse=True)
+ def setup_attrs(self, configured_app) -> None:
+ self.app = configured_app
+ self.client = self.app.test_client() # type:ignore
+ self.session = self.app.appbuilder.get_session
+
+ def teardown_method(self) -> None:
+ # Delete users that have our custom default time
+ users = self.session.query(User).filter(User.changed_on == timezone.parse(DEFAULT_TIME)).all()
+ for user in users:
+ self.session.delete(user)
+ self.session.commit()
+
+ def _create_users(self, count, roles=None):
+ # create users with defined created_on and changed_on date
+ # for easy testing
+ if roles is None:
+ roles = []
+ return [
+ User(
+ first_name=f'test{i}',
+ last_name=f'test{i}',
+ username=f'TEST_USER{i}',
+ email=f'mytest@test{i}.org',
+ roles=roles or [],
+ created_on=timezone.parse(DEFAULT_TIME),
+ changed_on=timezone.parse(DEFAULT_TIME),
+ )
+ for i in range(1, count + 1)
+ ]
+
+
+class TestGetUser(TestUserEndpoint):
+ def test_should_respond_200(self):
+ users = self._create_users(1)
+ self.session.add_all(users)
+ self.session.commit()
+ response = self.client.get("/api/v1/users/TEST_USER1", environ_overrides={'REMOTE_USER': "test"})
+ assert response.status_code == 200
+ assert response.json == {
+ 'active': None,
+ 'changed_on': DEFAULT_TIME,
+ 'created_on': DEFAULT_TIME,
+ 'email': 'mytest@test1.org',
+ 'fail_login_count': None,
Review comment:
This is because of the DB model, it doesn't specify a default value. https://github.com/dpgaspar/Flask-AppBuilder/blob/302e4abe32d9d2403f2ce52e4abb31beb59bb252/flask_appbuilder/security/sqla/models.py#L104-L105
So, count comes out as None. I think that, they are updated the first time user signs in
----------------------------------------------------------------
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 #14735: Add readonly REST API endpoints for users
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on a change in pull request #14735:
URL: https://github.com/apache/airflow/pull/14735#discussion_r595979749
##########
File path: tests/api_connexion/endpoints/test_user_endpoint.py
##########
@@ -0,0 +1,225 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+import pytest
+from flask_appbuilder.security.sqla.models import User
+from parameterized import parameterized
+
+from airflow.api_connexion.exceptions import EXCEPTIONS_LINK_MAP
+from airflow.security import permissions
+from airflow.utils import timezone
+from tests.test_utils.api_connexion_utils import assert_401, create_user, delete_user
+from tests.test_utils.config import conf_vars
+
+DEFAULT_TIME = "2020-06-11T18:00:00+00:00"
+
+
+@pytest.fixture(scope="module")
+def configured_app(minimal_app_for_api):
+ app = minimal_app_for_api
+ create_user(
+ app, # type: ignore
+ username="test",
+ role_name="Test",
+ permissions=[
+ (permissions.ACTION_CAN_LIST, permissions.RESOURCE_USER_DB_MODELVIEW),
+ (permissions.ACTION_CAN_SHOW, permissions.RESOURCE_USER_DB_MODELVIEW),
Review comment:
Yes. I left it for James to address in his PR since it might require migration
----------------------------------------------------------------
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 #14735: Add readonly REST API endpoints for users
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #14735:
URL: https://github.com/apache/airflow/pull/14735#issuecomment-797723824
[The Workflow run](https://github.com/apache/airflow/actions/runs/647245933) is cancelling this PR. Building images for the PR has failed. Follow the workflow link to check the reason.
----------------------------------------------------------------
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 #14735: Add readonly REST API endpoints for users
Posted by GitBox <gi...@apache.org>.
ephraimbuddy merged pull request #14735:
URL: https://github.com/apache/airflow/pull/14735
--
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