Skip to content

Conversation

@jedcunningham
Copy link
Member

@jedcunningham jedcunningham commented May 13, 2025

This is just config for the UI, not the whole Airflow instances config, which is what the config permission for users controls. If the user doesn't have the config permission, without this change, the user cannot use the UI at all.

So, instead we just check that the user is authenticated at all.

You can reproduce the issue by using this auth manager:

class NoConfigSimpleAuthManager(SimpleAuthManager):
    def is_authorized_configuration(
        self,
        *,
        method: ResourceMethod,
        user: SimpleAuthManagerUser,
        details: ConfigurationDetails | None = None,
    ) -> bool:
        return False

This is just config for the UI, not the whole Airflow instances config,
which is what the config permission for users controls. If the user
doesn't have the config permission, without this change, the user cannot
use the UI at all.

So, instead we just check that the user is authenticated at all.
@jedcunningham jedcunningham force-pushed the fix_ui_without_config_read branch from c56f7d6 to ff5d0ee Compare May 13, 2025 20:48
@jedcunningham jedcunningham requested a review from vincbeck May 13, 2025 20:48
Copy link
Member

@jason810496 jason810496 left a comment

Choose a reason for hiding this comment

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

Middleware is applying to all endpoints, right? In this case, we may want to limit if the endpoint is /config.

IMHO, the current requires_authenticated authorization dependency is still more appropriate than using middleware. Middleware is better suited when all routes require the same condition, but in our case, we only want to limit access to the /config endpoint.

Alternatively, using _: GetUserDep directly in the route might be simpler.
I just checked that all UI endpoints use requires_access_<entity> dependencies, except for get_auth_menus, which uses user: GetUserDep.

Copy link
Member

@pierrejeambrun pierrejeambrun left a comment

Choose a reason for hiding this comment

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

Looks good to me.

With the current usage I think the dependency is more convenient than a whole Middleware just for 1 route.

Beside the small nits mentioned above. (GetUserDep)

Co-authored-by: LIU ZHE YOU <68415893+jason810496@users.noreply.github.com>
@pierrejeambrun pierrejeambrun added this to the Airflow 3.0.2 milestone May 14, 2025
@pierrejeambrun pierrejeambrun added the backport-to-v3-1-test Mark PR with this label to backport to v3-1-test branch label May 14, 2025
@jedcunningham
Copy link
Member Author

Merging, failure is unrelated and already fixed.

@jedcunningham jedcunningham merged commit 0dad2bb into apache:main May 14, 2025
94 of 95 checks passed
@jedcunningham jedcunningham deleted the fix_ui_without_config_read branch May 14, 2025 16:14
github-actions bot pushed a commit that referenced this pull request May 14, 2025
This is just config for the UI, not the whole Airflow instances config,
which is what the config permission for users controls. If the user
doesn't have the config permission, without this change, the user cannot
use the UI at all.

So, instead we just check that the user is authenticated at all.
(cherry picked from commit 0dad2bb)

Co-authored-by: Jed Cunningham <66968678+jedcunningham@users.noreply.github.com>
Co-authored-by: LIU ZHE YOU <68415893+jason810496@users.noreply.github.com>
@github-actions
Copy link

Backport successfully created: v3-0-test

Status Branch Result
v3-0-test PR Link

github-actions bot pushed a commit to aws-mwaa/upstream-to-airflow that referenced this pull request May 14, 2025
)

This is just config for the UI, not the whole Airflow instances config,
which is what the config permission for users controls. If the user
doesn't have the config permission, without this change, the user cannot
use the UI at all.

So, instead we just check that the user is authenticated at all.
(cherry picked from commit 0dad2bb)

Co-authored-by: Jed Cunningham <66968678+jedcunningham@users.noreply.github.com>
Co-authored-by: LIU ZHE YOU <68415893+jason810496@users.noreply.github.com>
pierrejeambrun pushed a commit that referenced this pull request May 15, 2025
…50619)

This is just config for the UI, not the whole Airflow instances config,
which is what the config permission for users controls. If the user
doesn't have the config permission, without this change, the user cannot
use the UI at all.

So, instead we just check that the user is authenticated at all.
(cherry picked from commit 0dad2bb)

Co-authored-by: Jed Cunningham <66968678+jedcunningham@users.noreply.github.com>
Co-authored-by: LIU ZHE YOU <68415893+jason810496@users.noreply.github.com>
kaxil pushed a commit that referenced this pull request Jun 3, 2025
…50619)

This is just config for the UI, not the whole Airflow instances config,
which is what the config permission for users controls. If the user
doesn't have the config permission, without this change, the user cannot
use the UI at all.

So, instead we just check that the user is authenticated at all.
(cherry picked from commit 0dad2bb)

Co-authored-by: Jed Cunningham <66968678+jedcunningham@users.noreply.github.com>
Co-authored-by: LIU ZHE YOU <68415893+jason810496@users.noreply.github.com>
sanederchik pushed a commit to sanederchik/airflow that referenced this pull request Jun 7, 2025
This is just config for the UI, not the whole Airflow instances config,
which is what the config permission for users controls. If the user
doesn't have the config permission, without this change, the user cannot
use the UI at all.

So, instead we just check that the user is authenticated at all.

Co-authored-by: LIU ZHE YOU <68415893+jason810496@users.noreply.github.com>
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 backport-to-v3-1-test Mark PR with this label to backport to v3-1-test branch

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants