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

Include missing RBAC roles #9133

Merged
merged 1 commit into from
Jun 15, 2020
Merged

Include missing RBAC roles #9133

merged 1 commit into from
Jun 15, 2020

Conversation

thesuperzapper
Copy link
Contributor

@thesuperzapper thesuperzapper commented Jun 4, 2020

This is a simple fix which includes the missing RBAC roles.

  • can_failed RBAC permission for the User role
    • this omission was clearly a mistake as the set_failed RBAC permission is already present in the User role
  • can_last_dagruns RBAC permission for the Viewer role

Make sure to mark the boxes below before creating PR: [x]

  • Description above provides context of the change
  • Unit tests coverage for changes (not needed for documentation changes)
  • Target Github ISSUE in description if exists
  • Commits follow "How to write a good git commit message"
  • Relevant documentation is updated including usage instructions.
  • I will engage committers as explained in Contribution Workflow Example.

@thesuperzapper
Copy link
Contributor Author

@potiuk can you review this?

@thesuperzapper thesuperzapper changed the title Include 'can_failed' in User RBAC Include missing RBAC roles Jun 4, 2020
@ashb
Copy link
Member

ashb commented Jun 4, 2020

@thesuperzapper Tests need updating.

@@ -1983,7 +1983,7 @@ def test_failed_success(self):
past="false",
)
resp = self.client.post('failed', data=form)
self.check_content_in_response('Redirecting', resp, 302)
self.check_content_in_response('Redirecting', resp)
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't seem like the right fix. If it's saying "redirecting" but not issuing a 302 something else needs changing here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ashb That test has been broken since it was created (mostly because can_failure was missing).

You're probably right, about 'Redirecting' what content does that POST return?

What text should we check for, other tests in that file check for example_bash_operator in the response, should this do the same?

@thesuperzapper
Copy link
Contributor Author

@ashb, I have made the change requested, is this PR now acceptable?

@ashb ashb merged commit c44f1b1 into apache:master Jun 15, 2020
@ashb ashb added this to the Airflow 1.10.11 milestone Jun 15, 2020
kaxil pushed a commit to kaxil/airflow that referenced this pull request Jun 27, 2020
cfei18 pushed a commit to cfei18/incubator-airflow that referenced this pull request Mar 5, 2021
@thesuperzapper thesuperzapper deleted the user_rbac branch March 20, 2024 20:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:webserver Webserver related Issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants