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/02/15 13:06:50 UTC
[GitHub] [airflow] ashb commented on a change in pull request #14219: Provide login endpoint for the REST API with JWT authentication method
ashb commented on a change in pull request #14219:
URL: https://github.com/apache/airflow/pull/14219#discussion_r576173777
##########
File path: airflow/api_connexion/endpoints/user_endpoint.py
##########
@@ -0,0 +1,47 @@
+# 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, request
+from flask_appbuilder.const import AUTH_LDAP
+from flask_login import login_user
+from marshmallow import ValidationError
+
+from airflow.api_connexion.exceptions import BadRequest, NotFound
+from airflow.api_connexion.schemas.user_schema import auth_login_shcema, logged_in_user_data_schema
+from airflow.api_connexion.webserver_auth import encode_auth_token
+
+
+def login():
+ """User login"""
+ body = request.json
+ try:
+ data = auth_login_shcema.load(body)
+ except ValidationError as err:
+ raise BadRequest(detail=str(err.messages))
+ username = data["username"]
+ password = data['password']
+ ab_security_manager = current_app.appbuilder.sm
+ user = None
+ if ab_security_manager.auth_type == AUTH_LDAP:
+ user = ab_security_manager.auth_user_ldap(username, password)
+ if user is None:
+ user = ab_security_manager.auth_user_db(username, password)
Review comment:
This should not be handled by us -- there is likely a generic method in the SecurityManager that we can use instead.
(The problem here is that the Auth manager has ~5 different auth backends, we have only supported two of them.)
##########
File path: airflow/api_connexion/openapi/v1.yaml
##########
@@ -1381,11 +1381,198 @@ paths:
schema:
$ref: '#/components/schemas/VersionInfo'
+ /login:
+ post:
+ summary: User login
+ description: |
+ Verify user and return a user object and JWT token as well
+ x-openapi-router-controller: airflow.api_connexion.endpoints.user_endpoint
+ operationId: login
+ tags: [User]
+ requestBody:
+ required: true
+ content:
+ application/json:
+ schema:
+ $ref: '#/components/schemas/Login'
+ responses:
+ '200':
+ description: Success.
+ content:
+ application/json:
+ schema:
+ $ref: '#/components/schemas/UserLogin'
+ '400':
+ $ref: '#/components/responses/BadRequest'
+ '401':
+ $ref: '#/components/responses/Unauthenticated'
components:
# Reusable schemas (data models)
schemas:
# Database entities
+ UserCollectionItem:
+ description: >
+ User collection item
+ type: object
+ properties:
+ id:
+ type: string
+ description: The user id
+ readOnly: true
+ first_name:
+ type: string
+ description: The user firstname
+ last_name:
+ type: string
+ description: The user lastname
+ username:
+ type: string
+ description: The username
+ email:
+ type: string
+ description: The user's email
+ active:
+ type: boolean
+ description: Whether the user is active
+ last_login:
+ type: string
+ format: datetime
+ description: The last user login
+ readOnly: true
+ login_count:
+ type: integer
+ description: The login count
+ readOnly: true
+ failed_login_count:
+ type: integer
+ description: The number of times the login failed
+ readOnly: true
+ roles:
+ type: array
+ description: User roles
+ items:
+ $ref: '#/components/schemas/RoleCollectionItem'
+ readOnly: true
+ nullable: true
+ created_on:
+ type: string
+ format: datetime
+ description: The date user was created
+ readOnly: true
+ changed_on:
+ type: string
+ format: datetime
+ description: The date user was changed
+ readOnly: true
+
+ UserCollection:
+ description: User collection
+ type: object
+ properties:
+ users:
+ type: array
+ items:
+ $ref: '#/components/schemas/UserCollectionItem'
+
+ UserLogin:
+ description: Login item
+ allOf:
+ - $ref: '#/components/schemas/UserCollectionItem'
Review comment:
```suggestion
- $ref: '#/components/schemas/User'
```
##########
File path: airflow/api_connexion/openapi/v1.yaml
##########
@@ -1381,11 +1381,198 @@ paths:
schema:
$ref: '#/components/schemas/VersionInfo'
+ /login:
+ post:
+ summary: User login
+ description: |
+ Verify user and return a user object and JWT token as well
+ x-openapi-router-controller: airflow.api_connexion.endpoints.user_endpoint
+ operationId: login
+ tags: [User]
+ requestBody:
+ required: true
+ content:
+ application/json:
+ schema:
+ $ref: '#/components/schemas/Login'
+ responses:
+ '200':
+ description: Success.
+ content:
+ application/json:
+ schema:
+ $ref: '#/components/schemas/UserLogin'
+ '400':
+ $ref: '#/components/responses/BadRequest'
+ '401':
+ $ref: '#/components/responses/Unauthenticated'
components:
# Reusable schemas (data models)
schemas:
# Database entities
+ UserCollectionItem:
+ description: >
+ User collection item
+ type: object
+ properties:
+ id:
+ type: string
+ description: The user id
+ readOnly: true
+ first_name:
+ type: string
+ description: The user firstname
+ last_name:
+ type: string
+ description: The user lastname
+ username:
+ type: string
+ description: The username
+ email:
+ type: string
+ description: The user's email
+ active:
+ type: boolean
+ description: Whether the user is active
+ last_login:
+ type: string
+ format: datetime
+ description: The last user login
+ readOnly: true
+ login_count:
+ type: integer
+ description: The login count
+ readOnly: true
+ failed_login_count:
+ type: integer
+ description: The number of times the login failed
+ readOnly: true
+ roles:
+ type: array
+ description: User roles
+ items:
+ $ref: '#/components/schemas/RoleCollectionItem'
+ readOnly: true
+ nullable: true
+ created_on:
+ type: string
+ format: datetime
+ description: The date user was created
+ readOnly: true
+ changed_on:
+ type: string
+ format: datetime
+ description: The date user was changed
+ readOnly: true
+
+ UserCollection:
+ description: User collection
+ type: object
+ properties:
+ users:
+ type: array
+ items:
+ $ref: '#/components/schemas/UserCollectionItem'
+
+ UserLogin:
+ description: Login item
+ allOf:
+ - $ref: '#/components/schemas/UserCollectionItem'
+ - type: object
+ properties:
+ token:
+ type: string
+ nullable: false
+ description: JWT token
+
+ RoleCollectionItem:
+ description: Role collection item
+ type: object
+ properties:
+ id:
+ type: string
+ description: The role ID
+ name:
+ type: string
+ description: The name of the role
+ permissions:
+ type: array
+ items:
+ $ref: '#/components/schemas/PermissionView'
+
+ RoleCollection:
+ description: Role Collections
+ type: object
+ properties:
+ roles:
+ type: array
+ items:
+ $ref: '#/components/schemas/RoleCollectionItem'
+
+ PermissionCollectionItem:
+ description: Permission Collection Item
+ type: object
+ properties:
+ id:
+ type: string
+ description: The permission ID
+ name:
+ type: string
+ description: The name of the permission
+ nullable: false
+
+ PermissionCollection:
+ description: Permission Collection
+ type: object
+ properties:
+ permissions:
+ type: array
+ items:
+ $ref: '#/components/schemas/PermissionCollectionItem'
+
+ PermissionView:
+ description: Permission view item
+ type: object
+ properties:
+ id:
+ type: string
+ description: The PermissionView ID
+ permission_id:
+ type: string
+ description: The permission ID
+ permission:
+ type: string
+ description: The name of the permission
+ view_menu_id:
+ type: string
+ description: The view menu id
+ view_menu_name:
+ type: string
+ description: The view menu name
+
+ ViewMenuCollectionItem:
+ description: ViewMenu Collection Item
+ type: object
+ properties:
+ id:
+ type: string
+ description: The ViewMenu ID
+ name:
+ type: string
+ description: The name of the ViewMenu
+ nullable: false
+
+ ViewMenuCollection:
+ description: ViewMenu Collection
+ type: object
+ properties:
+ view_menus:
+ type: array
+ items:
+ $ref: '#/components/schemas/ViewMenuCollectionItem'
Review comment:
@ryanahamilton @jhtimmins Does this unduly tie is un to using FABs permissions model forever?
Do we perhaps want to re-think this and choose what/how we expose permissions without just blindly copying FAB?
##########
File path: airflow/api_connexion/security.py
##########
@@ -14,24 +14,27 @@
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.
-
from functools import wraps
from typing import Callable, Optional, Sequence, Tuple, TypeVar, cast
from flask import Response, current_app
from airflow.api_connexion.exceptions import PermissionDenied, Unauthenticated
+from airflow.api_connexion.webserver_auth import requires_authentication
T = TypeVar("T", bound=Callable) # pylint: disable=invalid-name
def check_authentication() -> None:
"""Checks that the request has valid authorization information."""
response = current_app.api_auth.requires_authentication(Response)()
- if response.status_code != 200:
+ jwt_response = requires_authentication(Response)()
Review comment:
This is not going to be accepted like this, sorry.
It means you _have_ to log in to API before you can use it which represents a massive breaking change in the behaviour to the API from 2.0.0/.1.
##########
File path: airflow/api_connexion/schemas/view_menu_schema.py
##########
@@ -0,0 +1,48 @@
+# 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 typing import List, NamedTuple
+
+from flask_appbuilder.security.sqla.models import ViewMenu
+from marshmallow import Schema, fields
+from marshmallow_sqlalchemy import SQLAlchemySchema, auto_field
+
+
+class ViewMenuCollectionItemSchema(SQLAlchemySchema):
Review comment:
I'm not a huge fan of so many files -- I think all the permissions schemas (View, Role, Permissions, etc) should be in a single `schemas/permissions_schemas` or similar file.
##########
File path: airflow/api_connexion/openapi/v1.yaml
##########
@@ -1381,11 +1381,198 @@ paths:
schema:
$ref: '#/components/schemas/VersionInfo'
+ /login:
+ post:
+ summary: User login
+ description: |
+ Verify user and return a user object and JWT token as well
+ x-openapi-router-controller: airflow.api_connexion.endpoints.user_endpoint
+ operationId: login
+ tags: [User]
+ requestBody:
+ required: true
+ content:
+ application/json:
+ schema:
+ $ref: '#/components/schemas/Login'
Review comment:
This is only ever used here, do we should just "inline" it, rather than making a schema for it.
_If_ we even have parameters for it, rather than just let the auth backend handle it (i.e. the `Authorization` header)
##########
File path: tests/api_connexion/test_webserver_auth.py
##########
@@ -0,0 +1,75 @@
+# 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 unittest
+
+from flask_login import current_user
+
+from airflow.www.app import create_app
+from tests.test_utils.config import conf_vars
+
+
+class TestWebserverAuth(unittest.TestCase):
+ def setUp(self) -> None:
+ with conf_vars(
+ {("webserver", "session_lifetime_minutes"): "1", ("webserver", "secret_key"): "secret"}
+ ):
+ self.app = create_app(testing=True)
+
+ self.appbuilder = self.app.appbuilder # pylint: disable=no-member
+ role_admin = self.appbuilder.sm.find_role("Admin")
+ self.tester = self.appbuilder.sm.find_user(username="test")
+ if not self.tester:
+ self.appbuilder.sm.add_user(
+ username="test",
+ first_name="test",
+ last_name="test",
+ email="test@fab.org",
+ role=role_admin,
+ password="test",
+ )
+
+ def test_successful_login(self):
+ payload = {"username": "test", "password": "test"}
+ with self.app.test_client() as test_client:
+ response = test_client.post("api/v1/login", json=payload)
+ assert isinstance(response.json["token"], str)
+ assert response.json["user"]['email'] == "test@fab.org"
+
+ def test_can_view_other_endpoints(self):
+ payload = {"username": "test", "password": "test"}
+ with self.app.test_client() as test_client:
+ response = test_client.post("api/v1/login", json=payload)
+ token = response.json["token"]
+
+ response2 = test_client.get("/api/v1/pools", headers={"Authorization": "Bearer " + token})
+ assert current_user.email == "test@fab.org"
+
+ assert response2.status_code == 200
+ assert response2.json == {
+ "pools": [
+ {
+ "name": "default_pool",
+ "slots": 128,
+ "occupied_slots": 0,
+ "running_slots": 0,
+ "queued_slots": 0,
+ "open_slots": 128,
+ },
+ ],
+ "total_entries": 1,
+ }
Review comment:
Please add a test that _ensures_ that we never accept a JWT header that uses the `none` algorithm https://auth0.com/blog/critical-vulnerabilities-in-json-web-token-libraries/
##########
File path: airflow/api_connexion/openapi/v1.yaml
##########
@@ -1381,11 +1381,198 @@ paths:
schema:
$ref: '#/components/schemas/VersionInfo'
+ /login:
+ post:
+ summary: User login
+ description: |
+ Verify user and return a user object and JWT token as well
+ x-openapi-router-controller: airflow.api_connexion.endpoints.user_endpoint
+ operationId: login
+ tags: [User]
+ requestBody:
+ required: true
+ content:
+ application/json:
+ schema:
+ $ref: '#/components/schemas/Login'
+ responses:
+ '200':
+ description: Success.
+ content:
+ application/json:
+ schema:
+ $ref: '#/components/schemas/UserLogin'
+ '400':
+ $ref: '#/components/responses/BadRequest'
+ '401':
+ $ref: '#/components/responses/Unauthenticated'
components:
# Reusable schemas (data models)
schemas:
# Database entities
+ UserCollectionItem:
+ description: >
+ User collection item
Review comment:
```suggestion
User:
description: >
A User object
```
##########
File path: airflow/api_connexion/openapi/v1.yaml
##########
@@ -1381,11 +1381,198 @@ paths:
schema:
$ref: '#/components/schemas/VersionInfo'
+ /login:
+ post:
+ summary: User login
+ description: |
+ Verify user and return a user object and JWT token as well
+ x-openapi-router-controller: airflow.api_connexion.endpoints.user_endpoint
+ operationId: login
+ tags: [User]
+ requestBody:
+ required: true
+ content:
+ application/json:
+ schema:
+ $ref: '#/components/schemas/Login'
+ responses:
+ '200':
+ description: Success.
+ content:
+ application/json:
+ schema:
+ $ref: '#/components/schemas/UserLogin'
+ '400':
+ $ref: '#/components/responses/BadRequest'
+ '401':
+ $ref: '#/components/responses/Unauthenticated'
components:
# Reusable schemas (data models)
schemas:
# Database entities
+ UserCollectionItem:
+ description: >
+ User collection item
+ type: object
+ properties:
+ id:
+ type: string
+ description: The user id
+ readOnly: true
+ first_name:
+ type: string
+ description: The user firstname
+ last_name:
+ type: string
+ description: The user lastname
+ username:
+ type: string
+ description: The username
+ email:
+ type: string
+ description: The user's email
+ active:
+ type: boolean
+ description: Whether the user is active
+ last_login:
+ type: string
+ format: datetime
+ description: The last user login
+ readOnly: true
+ login_count:
+ type: integer
+ description: The login count
+ readOnly: true
+ failed_login_count:
+ type: integer
+ description: The number of times the login failed
+ readOnly: true
+ roles:
+ type: array
+ description: User roles
+ items:
+ $ref: '#/components/schemas/RoleCollectionItem'
+ readOnly: true
+ nullable: true
+ created_on:
+ type: string
+ format: datetime
+ description: The date user was created
+ readOnly: true
+ changed_on:
+ type: string
+ format: datetime
+ description: The date user was changed
+ readOnly: true
+
+ UserCollection:
+ description: User collection
+ type: object
+ properties:
+ users:
+ type: array
+ items:
+ $ref: '#/components/schemas/UserCollectionItem'
+
Review comment:
```suggestion
```
Not used in this PR.
----------------------------------------------------------------
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