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

Don't add User role perms to custom roles. #13856

Merged
merged 1 commit into from
Jan 25, 2021

Conversation

jhtimmins
Copy link
Contributor

@jhtimmins jhtimmins commented Jan 22, 2021

Solves the problem of roles getting incorrectly populated with all permissions of the User class. Now they are auto-populated with Website.can_read and nothing else.

Expected Behavior

When a custom role is created, the role should not include any extra permissions beyond what the user added.

Actual Behavior

All permissions for the default User role are copied into each custom rule.

Update Behavior

Rather than adding all permissions from the User role, the only permission added now is Website.can_read. This is required to allow access to the homepage.

closes: #9245
related: #9245

@boring-cyborg boring-cyborg bot added the area:webserver Webserver related Issues label Jan 22, 2021
@kaxil
Copy link
Member

kaxil commented Jan 23, 2021

That was quick :) -- Can you please add PR description describing the change and reference the Github issue it solves?

@jhtimmins
Copy link
Contributor Author

@kaxil My b, I thought I had added it. Updated!

@kaxil
Copy link
Member

kaxil commented Jan 23, 2021

cc @davido912

@kaxil kaxil added this to the Airflow 2.0.1 milestone Jan 23, 2021
@kaxil
Copy link
Member

kaxil commented Jan 23, 2021

Thanks James, will take a deeper look over the weekend or Monday

This might fix #13511 too

@davido912
Copy link
Contributor

I can confirm this does indeed fix the issue where a role would be populated with the USER permissions. I opened another issue that still remains. I've yet to find a fix for this but thanks for looking into this!
#13891

@kaxil @jhtimmins

@kaxil kaxil merged commit 35b5a38 into apache:master Jan 25, 2021
@kaxil kaxil deleted the unwanted-role-permissions branch January 25, 2021 14:01
@kaxil
Copy link
Member

kaxil commented Jan 25, 2021

Thanks @jhtimmins for the PR and @davido912 for testing it :)

@kaxil
Copy link
Member

kaxil commented Jan 25, 2021

I have started seeing this warnings @jhtimmins -- after merging this to Master and running Airflow locally with it:

[2021-01-25 15:01:35,448] {manager.py:549} WARNING - Refused to delete permission view, assoc with role exists Airflow.can_elasticsearch Admin
[2021-01-25 15:01:35,456] {manager.py:549} WARNING - Refused to delete permission view, assoc with role exists Airflow.can_pickle_info Admin
[2021-01-25 15:01:35,462] {manager.py:549} WARNING - Refused to delete permission view, assoc with role exists Airflow.can_elasticsearch Admin
[2021-01-25 15:01:35,469] {manager.py:549} WARNING - Refused to delete permission view, assoc with role exists Airflow.can_pickle_info Admin
[2021-01-25 15:01:35,628] {manager.py:549} WARNING - Refused to delete permission view, assoc with role exists Airflow.can_elasticsearch Admin
[2021-01-25 15:01:35,635] {manager.py:549} WARNING - Refused to delete permission view, assoc with role exists Airflow.can_pickle_info Admin
[2021-01-25 15:01:35,665] {manager.py:549} WARNING - Refused to delete permission view, assoc with role exists Airflow.can_elasticsearch Admin
[2021-01-25 15:01:35,673] {manager.py:549} WARNING - Refused to delete permission view, assoc with role exists Airflow.can_pickle_info Admin
[2021-01-25 15:01:56 +0000] [389] [INFO] Handling signal: winch

kaxil pushed a commit that referenced this pull request Jan 28, 2021
kaxil pushed a commit that referenced this pull request Jan 29, 2021
kaxil pushed a commit that referenced this pull request Jan 29, 2021
kaxil pushed a commit that referenced this pull request Feb 4, 2021
kaxil pushed a commit that referenced this pull request Feb 4, 2021
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.

Role is automatically populated with permissions ?
3 participants