patgarz opened a new pull request, #34051:
URL: https://github.com/apache/airflow/pull/34051
<!--
Licensed to the Apache Software Foundation (ASF) under one
or more contributor license agreements. See the NOTICE file
distributed with this work for additional information
regarding copyright ownership. The ASF licenses this file
to you under the Apache License, Version 2.0 (the
"License"); you may not use this file except in compliance
with the License. You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing,
software distributed under the License is distributed on an
"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
KIND, either express or implied. See the License for the
specific language governing permissions and limitations
under the License.
-->
<!--
Thank you for contributing! Please make sure that your code changes
are covered with tests. And in case of new features or big changes
remember to adjust the documentation.
Feel free to ping committers for the review!
In case of an existing issue, reference it using one of the following:
closes: #ISSUE
related: #ISSUE
How to write a good git commit message:
http://chris.beams.io/posts/git-commit/
-->
CC: @vincbeck as owner of AIP-56
apache/airflow#32217 implemented the base and FAB user management
interfaces. As a part of that PR, `get_user_name` was set to be `FirstName
LastName` for FAB.
This is a poor choice. A username should be both invariant and unique;
FirstName and LastName are self-configurable by all user roles by default
(aside from `Public`) in the Airflow UI, and for users who configure a dirsync
SSO to FAB and do not give permission to Edit My Profile, the user's display
name is commonly self-configurable in those systems as well. Further, any
sufficiently large system will eventually encounter a name clash between two
concurrent users, essentially leaving a username (identifier) clash. As a
result, in today's version of the code:
- In the places where `get_user_name` is implemented (for example the Audit
Log), the Audit Log cannot be used to establish an event change if a user
decides to change their name (which they can do by default without the aid of
an administrator)
- If two users share a First Name and Last Name, the Audit Log likewise
cannot be used to identify the responsible user
- In the worst case scenario, a malicious user could affect the ability for
a security team to identify a chain of events by disguising themselves as a
legitimate user, or by rapidly changing their name in between actions
However, the user's display name is still an important and useful tool to
provide a better UX. As a result, we fix this by performing the following
changes:
1. Add a new abstract method, `auth_manager.get_user_display_name` to
augment the original API as proposed in AIP-56
2. Change `get_user_name` to have fallback values available for backwards
compatibility, but choose values that are sufficiently unique and invariant
(email is not a perfect choice but it is better than display name)
3. Implement `get_user_display_name` as the original `FirstName LastName`
and migrate UI elements to use this new method instead of `get_user_name`.
4. To fix the resultant regression of apache/airflow#30185, a new field
`friendly_owner` is added to the `Log` table which stores the user's Display
Name for easier human-based audits of actions. (NOTE: This may be something to
split out into a separate PR, which can be bundled with a minor version update.
This is included in a separate commit to aid in this.)
This has a few implications:
1. Audit logs now read the user's username (or email, or DB ID) instead of
the user's full name to provide proper accountability. The `owner` field is
also single-valued, which makes for easier queries and cross-analysis with
other log sources.
2. Though I don't see any place immediately this would happen, it's not
unlikely that `get_auth_manager.get_user_name` will make its way into actual
security code eventually. If it does, this type of clash leads into a simple
and very dangerous privilege escalation.
3. User display name is properly segmented out in the base class so that
future developers of their own auth extensions can show the proper UI elements
for a user's display name (instead of showing everything as the user
identifier).
Additional ref:
https://cwiki.apache.org/confluence/display/AIRFLOW/AIP-56+Extensible+user+management
<!-- Please keep an empty line above the dashes. -->
---
**^ Add meaningful description above**
Read the **[Pull Request
Guidelines](https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#pull-request-guidelines)**
for more information.
In case of fundamental code changes, an Airflow Improvement Proposal
([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvement+Proposals))
is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party
License Policy](https://www.apache.org/legal/resolved.html#category-x).
In case of backwards incompatible changes please leave a note in a
newsfragment file, named `{pr_number}.significant.rst` or
`{issue_number}.significant.rst`, in
[newsfragments](https://github.com/apache/airflow/tree/main/newsfragments).
--
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
