Comment that getting audit logs is a global admin feature#64627
Comment that getting audit logs is a global admin feature#64627raboof wants to merge 1 commit intoapache:mainfrom
Conversation
There was a problem hiding this comment.
Thank you for opening this PR.
Roles aren't an airflow core concept but relative to the auth manager. "Admin" can mean something for FabAuthManager and not mean anything for another custom auth manager or the SimpleAuthManager. (Even if there is an admin there, conceptually we shouldn't talk about 'admin' in core because roles are defined in providers).
I don't think those comments are relevant because the security model is explicit about this. What confusion are you trying to solve there?
(Just wondering because there are other 'global' type of permissions, for connections, variables, etc.... and we usually do not put such comment there)
|
(AFAICT the CI failures are unrelated so this PR is good to go, LMK if I misinterpreted that) |
|
@raboof Yes, CI failure are unrelated, I will rebase the branch to see if errors sticks.
I am still trying to understand the motivation of this. And as mentioned in my previous comment it needs to be adjusted. (no mention of any role, that does not exist for core) (I wouldn't add an explicit comment there because it's the same for all entities that are not a |
|
Yeah. I think we might want to synchronizes the code and security model docs.
I am planning (And maybe finally will do it tomorrow) to generate complete security model docs synced with our code - including our token auth mechanisms - where we have textual description of the model and code comments that refer to appropriate sections of the model. I think those should be kept up-to date by agent AI doing the heavy lifting and simply reading the code and understanding the intentions provided by the human docs. |
Apologies, I missed your comment somehow, and should've made the motivation clearer. The motivation is that while the security model clearly states this, we're still getting regular invalid security reports about this behavior, so I hoped by cross-referencing the code and the security model this would become clearer. I'll await Jarek's experiment |
|
Yes for this specific task we have #43430. Indeed, AI or AST manipulation, but something that reads the permissions and update the security model accordingly. (table with endpoint -> permission maybe or something) |
There was a problem hiding this comment.
Pull request overview
This PR adds inline clarification in the FastAPI Event Logs (audit logs) public API routes that accessing audit logs is an admin-level capability affecting the whole Airflow installation, with a link to the security model documentation.
Changes:
- Added explanatory comments above the Event Log GET-by-id endpoint about audit log access being an admin feature.
- Added the same explanatory comments above the Event Log list endpoint, referencing the official security model docs.
| # Getting audit logs is an admin feature giving access to the logs for | ||
| # the whole Airflow installation, not on a more granular level. This is documented | ||
| # at https://airflow.apache.org/docs/apache-airflow/stable/security/security_model.html#audit-log-users |
There was a problem hiding this comment.
The same 3-line explanatory comment is duplicated above both endpoints. To avoid having to keep two copies in sync, consider keeping a single module-level comment (e.g., above event_logs_router) or folding this into one endpoint’s docstring/description and removing the duplicate block.
With reference to https://airflow.apache.org/docs/apache-airflow/stable/security/security_model.html#audit-log-users
Was generative AI tooling used to co-author this PR?
{pr_number}.significant.rst, in airflow-core/newsfragments. You can add this file in a follow-up commit after the PR is created so you know the PR number.