Make the role assigned to anonymous users customizable #14042
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jhtimmins
changed the title
|
Looks perfect, I did some similar tests here and this should work |
ashb
reviewed
Feb 3, 2021
tests/www/test_security.py
Outdated
| user = mock.MagicMock() | ||
| user.is_anonymous = True | ||
| self.app.config['AUTH_ROLE_PUBLIC'] = 'Public' | ||
| assert self.app.appbuilder.sm.get_user_roles(user) == [self.app.appbuilder.sm.find_role("Public")] |
There was a problem hiding this comment.
Suggested change
| assert self.app.appbuilder.sm.get_user_roles(user) == [self.app.appbuilder.sm.find_role("Public")] | |
| assert self.app.appbuilder.sm.get_user_roles(user) == [self.app.appbuilder.sm.get_public_role()] |
ashb
approved these changes
Feb 3, 2021
github-actions
bot
added
the
okay to merge
|
The PR is likely OK to be merged with just subset of tests for default Python and Database versions without running the full matrix of tests, because it does not modify the core of Airflow. If the committers decide that the full tests matrix is needed, they will add the label 'full tests needed'. Then you should rebase to the latest master or amend the last commit of the PR, and push it with --force-with-lease. |
jhtimmins
force-pushed
the
test-allow-non-public-anon-user
branch
from
236ea97
to
80112c3
Compare
kaxil
approved these changes
Feb 4, 2021
kaxil
pushed a commit
that referenced
this pull request
Feb 4, 2021
Fixes the issue wherein regardless of what role anonymous users are assigned (via the `AUTH_ROLE_PUBLIC` env var), they can't see any DAGs. Current behavior causes: Anonymous users are handled as a special case by Airflow's DAG-related security methods (`.has_access()` and `.get_accessible_dags()`). Rather than checking the `AUTH_ROLE_PUBLIC` value to check for role permissions, the methods reject access to view or edit any DAGs. Changes in this PR: Rather than hardcoding permission rules inside the security methods, this change checks the `AUTH_ROLE_PUBLIC` value and gives anonymous users all permissions linked to the designated role. **This places security in the hands of the Airflow users. If the value is set to `Admin`, anonymous users will have full admin functionality.** This also changes how the `Public` role is created. Currently, the `Public` role is created automatically by Flask App Builder. This PR explicitly declares `Public` as a default role with no permissions in `security.py`. This change makes it easier to test. closes: #13340 (cherry picked from commit 78aa921)
kaxil
pushed a commit
that referenced
this pull request
Feb 4, 2021
Fixes the issue wherein regardless of what role anonymous users are assigned (via the `AUTH_ROLE_PUBLIC` env var), they can't see any DAGs. Current behavior causes: Anonymous users are handled as a special case by Airflow's DAG-related security methods (`.has_access()` and `.get_accessible_dags()`). Rather than checking the `AUTH_ROLE_PUBLIC` value to check for role permissions, the methods reject access to view or edit any DAGs. Changes in this PR: Rather than hardcoding permission rules inside the security methods, this change checks the `AUTH_ROLE_PUBLIC` value and gives anonymous users all permissions linked to the designated role. **This places security in the hands of the Airflow users. If the value is set to `Admin`, anonymous users will have full admin functionality.** This also changes how the `Public` role is created. Currently, the `Public` role is created automatically by Flask App Builder. This PR explicitly declares `Public` as a default role with no permissions in `security.py`. This change makes it easier to test. closes: #13340 (cherry picked from commit 78aa921)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes the issue wherein regardless of what role anonymous users are assigned (via the
AUTH_ROLE_PUBLICenv var), they can't see any DAGs.Current behavior causes:
Anonymous users are handled as a special case by Airflow's DAG-related security methods (
.has_access()and.get_accessible_dags()). Rather than checking theAUTH_ROLE_PUBLICvalue to check for role permissions, the methods reject access to view or edit any DAGs.Changes in this PR:
Rather than hardcoding permission rules inside the security methods, this change checks the
AUTH_ROLE_PUBLICvalue and gives anonymous users all permissions linked to the designated role.This places security in the hands of the Airflow users. If the value is set to
Admin, anonymous users will have full admin functionality.This also changes how the
Publicrole is created. Currently, thePublicrole is created automatically by Flask App Builder. This PR explicitly declaresPublicas a default role with no permissions insecurity.py. This change makes it easier to test.closes: #13340