Skip to content

Conversation

@vandonr-amz
Copy link
Contributor

@vandonr-amz vandonr-amz commented Aug 23, 2023

Moved the admin user creation to the security manager so that

  • there is no dependency on FAB user management there
  • other auth providers can eventually provide their own default admin user.

@vandonr-amz vandonr-amz marked this pull request as ready for review September 12, 2023 16:00
@uranusjr
Copy link
Member

Why does this logic live in the override instead of the base class? The logic seems entirely generic to me.

@vincbeck
Copy link
Contributor

Why does this logic live in the override instead of the base class? The logic seems entirely generic to me.

Yeah I can see both opinions on whether we want to have this logic regardless of the auth manager used. But I think you are right and we might want to keep it.

However, doing so means exposing find_user, add_user and find_role APIs in auth manager.

@vandonr-amz
Copy link
Contributor Author

yeah my understanding was that there was a pretty decent chance that alternative auth managers would not offer user and role management through airflow, but use their own UI/API instead.
In that context, it might be weird to ask them to expose add_user for instance. Especially if it's only for this very specific use case that will probably never happen in a corporate setting (airflow standalone is made mostly for experimenting).

@potiuk
Copy link
Member

potiuk commented Sep 14, 2023

Yes. Creating and managing users is IMHO completely outside of AuthManager. I think we should simply make assumption that "standalone" option always uses FABAuthManager, hard-code it and use it directly. It makes very little sense to have "generic" Auth Manager for "standalone" airflow.

Standalone means much more than "one command" - it also means "everything is self-contained" including authenticaiton/authorisation. "Standalone" almost by definition means "I do not need anything external to run".

@vandonr-amz
Copy link
Contributor Author

I think we should simply make assumption that "standalone" option always uses FABAuthManager

The current change here at least lets other auth managers work with standalone if they want to. Standalone still comes with a bit of flexibility, it uses sqlite by default for example, but can be configured to rely on a "real" DB.
Do you think this is fine ?

The alternative, if we really want standalone to only work with FAB, is to move the airflow standalone command to FAB CLI (like we did for the users and roles CLI commands). But do we really want to introduce this restriction when it's easy to offer more flexibility ?

@vincbeck
Copy link
Contributor

vincbeck commented Sep 25, 2023

@uranusjr does it make more sense now?

@uranusjr
Copy link
Member

I don’t have strong feelings on this so since multiple people say this is fine I’ll flow with this.

@vincbeck vincbeck merged commit d6db11e into apache:main Sep 26, 2023
@vincbeck vincbeck deleted the vandonr/fab2 branch September 26, 2023 13:18
@ephraimbuddy ephraimbuddy added the changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) label Oct 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AIP-56 Extensible user management area:CLI area:providers area:webserver Webserver related Issues changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) provider:celery

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants