Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Revoking audit_log permission from all users except admin #37501

Merged
merged 6 commits into from
Feb 19, 2024

Conversation

amoghrajesh
Copy link
Contributor

@amoghrajesh amoghrajesh commented Feb 17, 2024

The viewer role and any other users apart from admin don't need to have audit_log permissions. Revoking it.

Alternatively, I moved the revoked permissions to ADMIN_PERMISSIONS.


^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
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.

@amoghrajesh
Copy link
Contributor Author

The UI elements dont render for audit_logs:
image

Forbidden /api/v1/eventLogs:
image

If I try to access audit logs from the DAG run details:
https://github.com/apache/airflow/assets/35884252/96f9fdf9-d203-4482-8635-0bc467fa46d3

@amoghrajesh amoghrajesh changed the title Revoking audit_log permission from viewer role Revoking audit_log permission from all users except admin Feb 17, 2024
Copy link
Member

@hussein-awala hussein-awala left a comment

Choose a reason for hiding this comment

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

I agree with you that only admin (and custom roles) should have these permissions, but I don't know if we can consider this a bug fix, because otherwise it will be considered a breaking change. (not a big problem anymore after creating fab provider)

docs/apache-airflow/security/security_model.rst Outdated Show resolved Hide resolved
docs/apache-airflow/security/security_model.rst Outdated Show resolved Hide resolved
Copy link
Member

@potiuk potiuk left a comment

Choose a reason for hiding this comment

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

LGMT. Few nits that should be resolved.

@potiuk
Copy link
Member

potiuk commented Feb 18, 2024

I agree with you that only admin (and custom roles) should have these permissions, but I don't know if we can consider this a bug fix, because otherwise it will be considered a breaking change. (not a big problem anymore after creating fab provider)

Yes. Bug-fix. It should not be there in the first place IMHO.

@potiuk
Copy link
Member

potiuk commented Feb 18, 2024

But also we should add a .significant entry in newsfragments about it @amoghrajesh

@eladkal
Copy link
Contributor

eladkal commented Feb 18, 2024

But also we should add a .significant entry in newsfragments about it @amoghrajesh

yeah it's a bit tricky?
The news fragment should be in Airflow 2.9.0 but the actual code change is in the fab provider.

@eladkal eladkal added this to the Airflow 2.9.0 milestone Feb 18, 2024
@potiuk
Copy link
Member

potiuk commented Feb 18, 2024

Not tricky. We agreed with @vincbeck that when we cherry-pick FAB things for 2.8.0, we will cherry-pick them to core - moving the changes back where they were in before 2.9, so once we cherry-pick this one with a newsfragment, it will nicely go into 2.8.2 (or 2.8.3 whatever it will be released in)

@potiuk potiuk modified the milestones: Airflow 2.9.0, Airflow 2.8.2 Feb 18, 2024
@eladkal
Copy link
Contributor

eladkal commented Feb 18, 2024

Not tricky. We agreed with @vincbeck that when we cherry-pick FAB things for 2.8.0, we will cherry-pick them to core - moving the changes back where they were in before 2.9, so once we cherry-pick this one with a newsfragment, it will nicely go into 2.8.2 (or 2.8.3 whatever it will be released in)

Not sure that I get that?
The fab provider will be functional only for Airflow 2.9.0 does it not?
so only when user is upgrading to Airflow 2.9.0 the user will notice the change thus adding it to newsfragmant of 2.8.2 is not helpful.

@potiuk
Copy link
Member

potiuk commented Feb 18, 2024

No. This issue can be cherry-picked to 2.8.2 to the place where Fab was previously. We've already done that

@potiuk
Copy link
Member

potiuk commented Feb 18, 2024

When we separated FAB provider right after we created 2.8.0 - this was what we agreed to with @vincbeck - that security related and small fixeds will be cherry picked to 2.8.* airflow core

amoghrajesh and others added 3 commits February 19, 2024 09:21
Co-authored-by: Jed Cunningham <66968678+jedcunningham@users.noreply.github.com>
Co-authored-by: Jed Cunningham <66968678+jedcunningham@users.noreply.github.com>
@amoghrajesh
Copy link
Contributor Author

Thanks for the reviews! @potiuk / @eladkal pushed a newsfragment

@jedcunningham nits handled

@amoghrajesh
Copy link
Contributor Author

@potiuk do I have to do anything with respect to cherry picking to earlier branches?

@potiuk
Copy link
Member

potiuk commented Feb 19, 2024

@potiuk do I have to do anything with respect to cherry picking to earlier branches?

Thanks for the offer :). We'll handle it. Usually cherry-picking is done by release managers and those who directly help them and are familiar with the process - becuase this is rather delicate process and there are a number of decisions to made on the spot (currently those people are @ephraimbuddy @jedcunningham @eladkal and myself mostly. So doing it just for one PR introduces potential more issues than it solves. But if at some point in time you would like to join the release team and cherry-pick next changes (after seeing how it's done - absolutely :D . Just look what's dicussed in #release-management channel and follow it, and then raise hand before the release. Though - extra care is needed while doing it

@potiuk potiuk merged commit f2ea8a3 into apache:main Feb 19, 2024
58 checks passed
@amoghrajesh
Copy link
Contributor Author

@potiuk do I have to do anything with respect to cherry picking to earlier branches?

Thanks for the offer :). We'll handle it. Usually cherry-picking is done by release managers and those who directly help them and are familiar with the process - becuase this is rather delicate process and there are a number of decisions to made on the spot (currently those people are @ephraimbuddy @jedcunningham @eladkal and myself mostly. So doing it just for one PR introduces potential more issues than it solves. But if at some point in time you would like to join the release team and cherry-pick next changes (after seeing how it's done - absolutely :D . Just look what's dicussed in #release-management channel and follow it, and then raise hand before the release. Though - extra care is needed while doing it

Thank you, i was aware of the people who do it, and slightly about the process too, thanks for clarifying. I was curious if anything special was needed because this is a different kind of fix. Got it cleared now, thanks

potiuk pushed a commit that referenced this pull request Feb 19, 2024
---------

Co-authored-by: Jed Cunningham <66968678+jedcunningham@users.noreply.github.com>
(cherry picked from commit f2ea8a3)
@ephraimbuddy ephraimbuddy added the type:bug-fix Changelog: Bug Fixes label Feb 19, 2024
potiuk added a commit to potiuk/airflow that referenced this pull request Feb 19, 2024
The test for security/permissions had not been run after modifying
default permissions in apache#37501 (to be investigated why). This PR
makes main green again.
potiuk added a commit that referenced this pull request Feb 19, 2024
The test for security/permissions had not been run after modifying
default permissions in #37501 (to be investigated why). This PR
makes main green again.
potiuk added a commit to potiuk/airflow that referenced this pull request Feb 19, 2024
Yet another test was failing after changing audit log permissions
in apache#37501
potiuk added a commit that referenced this pull request Feb 19, 2024
Yet another test was failing after changing audit log permissions
in #37501
potiuk added a commit that referenced this pull request Feb 19, 2024
The test for security/permissions had not been run after modifying
default permissions in #37501 (to be investigated why). This PR
makes main green again.
potiuk added a commit that referenced this pull request Feb 19, 2024
Yet another test was failing after changing audit log permissions
in #37501
ephraimbuddy pushed a commit that referenced this pull request Feb 22, 2024
---------

Co-authored-by: Jed Cunningham <66968678+jedcunningham@users.noreply.github.com>
(cherry picked from commit f2ea8a3)
ephraimbuddy pushed a commit that referenced this pull request Feb 22, 2024
The test for security/permissions had not been run after modifying
default permissions in #37501 (to be investigated why). This PR
makes main green again.
ephraimbuddy pushed a commit that referenced this pull request Feb 22, 2024
Yet another test was failing after changing audit log permissions
in #37501
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants