Skip to content

Conversation

@norm
Copy link
Contributor

@norm norm commented Feb 9, 2022

As part of AIP-42, the auth_backend setting is expanded to auth_backends, and on an API request each is tried one after the other until one succeeds. A new auth backend of session is added that will validate against the signed-in user in the case where requests are made via JavaScript from the UI.

@boring-cyborg boring-cyborg bot added area:API Airflow's REST/HTTP API area:dev-tools area:helm-chart Airflow Helm Chart area:providers area:UI Related to UI/UX. For Frontend Developers. area:webserver Webserver related Issues kind:documentation provider:google Google (including GCP) related issues labels Feb 9, 2022
@norm norm marked this pull request as draft February 9, 2022 17:11
UPDATING.md Outdated

### `auth_backends` replaces `auth_backend` configuration setting

Previously, only one backend was used to authorize use of the experimental REST API.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Previously, only one backend was used to authorize use of the experimental REST API.
Previously, only one backend was used to authorize use of the REST API.

As (confusingly) the auth_backend is used by both new and old APIs.

Hmmm I wonder if this would un-intentionally make the old API available again? The Default auth backend of deny_all effectively made the old API not usable I think)

auth_backends = api.load_auth()
session = None
session_factory = getattr(auth_backend, 'create_client_session', None)
session_factory = getattr(auth_backends, 'create_client_session', None)
Copy link
Member

Choose a reason for hiding this comment

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

This would need to loop over the list and check for these attributes in some form or other.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed, I pushed the branch after a search/replace but not necessarily in a shippable state. :)

I've fixed this somewhat in 3f3126f, and whilst it now satisfies the tests it is not doing the right thing. There's an assumption of one auth in the returned api_client which means more work than merely making the tests pass by returning the first backend.

@norm norm force-pushed the rest-api-backends branch from d97e888 to ead413a Compare February 15, 2022 15:00
@norm norm changed the title WIP backend -> backends Implement multiple API auth backends Feb 15, 2022
@norm norm marked this pull request as ready for review February 15, 2022 15:25
@norm
Copy link
Contributor Author

norm commented Feb 15, 2022

In the card description @uranusjr wrote: "The backends are queried one by one, and the first valid identity returned by anyone is used (and 403 if none of the backends recognise the request)." but in the existing tests there was a difference between deny_all->403 and auth_failed->401, so I have retained that.

@ashb ashb merged commit 8d980cb into apache:main Feb 15, 2022
@ashb ashb deleted the rest-api-backends branch February 15, 2022 22:00
ephraimbuddy added a commit to astronomer/ap-airflow that referenced this pull request Feb 17, 2022
Following the change in Airflow apache/airflow#21472,
we update this as well
kaxil pushed a commit to astronomer/ap-airflow that referenced this pull request Feb 17, 2022
Following the change in Airflow apache/airflow#21472,
we update this as well
@jedcunningham jedcunningham added the type:improvement Changelog: Improvements label Feb 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:API Airflow's REST/HTTP API area:dev-tools area:helm-chart Airflow Helm Chart area:providers area:UI Related to UI/UX. For Frontend Developers. area:webserver Webserver related Issues kind:documentation provider:google Google (including GCP) related issues type:improvement Changelog: Improvements

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants