Skip to content

Provide login endpoint for the REST API with JWT authentication method#14219

Closed
ephraimbuddy wants to merge 26 commits into
apache:masterfrom
astronomer:add-login-endpoint-and-jwtauth
Closed

Provide login endpoint for the REST API with JWT authentication method#14219
ephraimbuddy wants to merge 26 commits into
apache:masterfrom
astronomer:add-login-endpoint-and-jwtauth

Conversation

@ephraimbuddy

@ephraimbuddy ephraimbuddy commented Feb 13, 2021

Copy link
Copy Markdown
Contributor

Currently, the REST API uses a separate authentication method to the webserver authentication. The current authentication method through the auth_backend won't allow us to build a modern UI based on the REST API. This is because it's too flexible. Users can customise it and if we use it for a new UI, it posses the problem that misconfiguration can make the new UI to fail.

This PR seeks to add /login endpoint with jwt authentication backend. This will help us to build a new UI using the REST API.
The JWT authentication works alongside other auth_backends.

The idea is to create a separate authentication for accessing the REST API without failure. When the /login endpoint is requested with email and password. If the email and password are correct, a jwt token is generated for the request and sent back together with the user data. This token can then be used in subsequent requests to other endpoints and are verified/authenticated.

The token is signed with an expiration time which I took from the session_lifetime_minutes configuration.

User data to be returned after a successful login is also being worked on in this PR.

As I want to return the full user data, I'm also working on the permission and role data

Note: auth_backend still works as this new auth backend is just an addition and strictly for v1 stable API


^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code change, Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in UPDATING.md.

@boring-cyborg boring-cyborg Bot added the area:API Airflow's REST/HTTP API label Feb 13, 2021
@github-actions

Copy link
Copy Markdown
Contributor

The Workflow run is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Backport packages$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.

@github-actions

Copy link
Copy Markdown
Contributor

The Workflow run is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Backport packages$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.

Comment thread airflow/api_connexion/endpoints/user_endpoint.py Outdated
@mik-laj mik-laj self-requested a review February 13, 2021 10:01
@mik-laj

mik-laj commented Feb 13, 2021

Copy link
Copy Markdown
Member

Can you add a little more docs?

@ephraimbuddy

Copy link
Copy Markdown
Contributor Author

Can you add a little more docs?

Hi @mik-laj , I have provided more info. Let me know what you think. Thanks

@github-actions

Copy link
Copy Markdown
Contributor

The Workflow run is cancelling this PR. Building images for the PR has failed. Follow the the workflow link to check the reason.

@github-actions

Copy link
Copy Markdown
Contributor

The Workflow run is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Backport packages$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.

@ashb ashb left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to think more about how this plays with the configurable/pluggable auth backends.

As it's written now it only works for U:P which is far from ideal

Comment thread airflow/api_connexion/endpoints/user_endpoint.py Outdated
Comment thread airflow/api_connexion/openapi/v1.yaml Outdated
Comment thread airflow/api_connexion/openapi/v1.yaml Outdated
Comment thread airflow/api_connexion/openapi/v1.yaml Outdated
Comment thread airflow/api_connexion/openapi/v1.yaml Outdated
Comment thread airflow/api_connexion/openapi/v1.yaml Outdated
Comment thread airflow/api_connexion/schemas/view_menu_schema.py Outdated
Comment thread airflow/api_connexion/security.py Outdated
Comment thread tests/api_connexion/test_webserver_auth.py Outdated
@github-actions

Copy link
Copy Markdown
Contributor

The Workflow run is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Backport packages$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.

@github-actions

Copy link
Copy Markdown
Contributor

The Workflow run is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Backport packages$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.

@ephraimbuddy ephraimbuddy force-pushed the add-login-endpoint-and-jwtauth branch from e1815ca to 12cee4a Compare February 16, 2021 01:40
@github-actions

Copy link
Copy Markdown
Contributor

The Workflow run is cancelling this PR. Building images for the PR has failed. Follow the the workflow link to check the reason.

@github-actions

Copy link
Copy Markdown
Contributor

The Workflow run is cancelling this PR. Building images for the PR has failed. Follow the the workflow link to check the reason.

@ephraimbuddy ephraimbuddy marked this pull request as ready for review February 17, 2021 06:52
@ephraimbuddy ephraimbuddy requested a review from kaxil as a code owner February 17, 2021 06:52
@ephraimbuddy ephraimbuddy force-pushed the add-login-endpoint-and-jwtauth branch from a680ffc to 64e02d9 Compare February 18, 2021 14:16
@github-actions

Copy link
Copy Markdown
Contributor

The Workflow run is cancelling this PR. Building images for the PR has failed. Follow the the workflow link to check the reason.

@ephraimbuddy ephraimbuddy force-pushed the add-login-endpoint-and-jwtauth branch from 64e02d9 to 5b9bb91 Compare February 18, 2021 15:23
@github-actions

Copy link
Copy Markdown
Contributor

The Workflow run is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Backport packages$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.

@ephraimbuddy ephraimbuddy force-pushed the add-login-endpoint-and-jwtauth branch 2 times, most recently from 7d7f585 to c1a69c6 Compare February 18, 2021 20:48
@github-actions

Copy link
Copy Markdown
Contributor

The Workflow run is cancelling this PR. Building images for the PR has failed. Follow the the workflow link to check the reason.

@ephraimbuddy

Copy link
Copy Markdown
Contributor Author

Currently, I believe this can handle any type of authentication. The /login endpoint just looks for current user in security manager, and if there is, it generates a token.

If the endpoint is called with basic authentication header, it verifies and logs the user in and then generates the auth token. If not called with any authentication header, it checks for current_user which might have logged in through Oauth and then generate a token. If no user is logged in, it raises 404.

I have a unittest where I used remote user and it works fine.

Any thoughts on this implementation?

@ashb

ashb commented Feb 19, 2021

Copy link
Copy Markdown
Member

Taking a look

Comment thread airflow/api_connexion/openapi/v1.yaml Outdated
@ephraimbuddy ephraimbuddy force-pushed the add-login-endpoint-and-jwtauth branch 2 times, most recently from 56c5929 to 41ce948 Compare February 25, 2021 21:10
@ephraimbuddy ephraimbuddy force-pushed the add-login-endpoint-and-jwtauth branch from 8e32352 to b3dc520 Compare March 22, 2021 08:40
@ephraimbuddy ephraimbuddy added the AIP-38 Modern Web Application label Mar 22, 2021
default: "airflow.api.auth.backend.deny_all"
- name: jwt_access_token_expires
description: How long an access token should live before it expires(in minutes).
version_added: 2.0.2

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2.1 for new features.

@ashb ashb left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The general approach of the login/logout endpoints are fine, and the way of handling JWT is okay, but the auth_current_user approach needs re-thinking.

We have essentially built a parallel auth backend system here, but one that is not pluggable (can only disable).

Instead we should use the existing auth_backend mechanism, and accept that if the user has set the mechanism to DenyAll then they won't be able to log in via the new UI.

In future we will also then extend the API to add an endpoint for the UI to introspect the server about the auth mechanisms (i.e. "Should I show a username-password form? Or should I show some 'Log In with X' buttons?)

description: Logout user by unsetting cookies
x-openapi-router-controller: airflow.api_connexion.endpoints.auth_endpoint
operationId: logout
tags: [WebserverAuth]

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
tags: [WebserverAuth]
tags: [Authentication]

Verify user and set jwt token in cookies
x-openapi-router-controller: airflow.api_connexion.endpoints.auth_endpoint
operationId: login
tags: [WebserverAuth]

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
tags: [WebserverAuth]
tags: [Authentication]

log = logging.getLogger(__name__)


def auth_current_user():

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't strictly only tied to webserver use of the API, so lets have this function live in security.py

raise Unauthenticated(headers=response.headers)
if response.status_code == 200:
return
if auth_current_user():

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By having this here, it means that any/all requests would accept and process Authorization headers, which is probably not what we want?

@ephraimbuddy

Copy link
Copy Markdown
Contributor Author

closing this for #15042

@ephraimbuddy ephraimbuddy deleted the add-login-endpoint-and-jwtauth branch March 26, 2021 21:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AIP-38 Modern Web Application area:API Airflow's REST/HTTP API

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants