vincbeck commented on PR #34924: URL: https://github.com/apache/airflow/pull/34924#issuecomment-1769107921
> Obviously. I missed the fact that "session_auth" remained where it was. I see that "session" will remain where it was. Clear. And expectation is that if you implement an AuthManger api_auth should return the session. > But that sparks another question. Maybe (just maybe and maybe that is the next step) we should not be to hasty to move few others to FAB? What if we also keep dany_all and default in "api" rather than move them to FAB ? They do not seem to have any relation to FAB whatsoever and are directly reusable - for example if you would like to have no API, you'd get AuthManager return deny_all in api_auth property? That seems like very easy thing to do (especially if we describe that those are example api_auth methods that can be returned. Then FAB Auth Manager would simply become the first of the Auth Managers that will use them (when configured in the FAB Auth Manager configuration). > And maybe (that's a bit more tricky and maybe it is next step or follow-up) - we also keep kerberos_auth in "reusable" api package, only we parameterise them in the way, that they can be easiy used by Auth Manager - for example get kerberos_auth and expose find_user method. Then anyone implementing their AuthManager could choose to return kerberos_auth (adding just find_user implementation returning the right user ). I think there is much more "airflow/kerberos" code there than "FAB" in the kerberos_auth - it looks like "almost reusable" implementation of kerberos authentication - nicely integrated with how airflow kerberos CLI works with refreshing the kerberos token - the only thing it needs is find_user implementation. That's very good point actually. That makes sense to me. `basic_auth`, `default` and `deny_all` should stay where they are. I'll modify the PR accordingly. Regarding kerberos, I agree with you, it is MOSTLY independent besides the usage of `find_user`. My only concern is, it looks "expensive" to force auth managers to implement `find_user` just for this usage. Some auth managers might not even want to use API, or Kerberos and they'll have to implement `find_user`. Another solution would be to return `NotImplemented` by default and try/catch this exception. If this exception is thrown, then no Kerberos -- 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. To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
