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.

Sign up for GitHub

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

Jump to bottom

check whether AUTH_ROLE_PUBLIC is set in check_authentication #38924

Merged
merged 3 commits into from
Apr 14, 2024
Merged

check whether AUTH_ROLE_PUBLIC is set in check_authentication #38924

merged 3 commits into from
Apr 14, 2024

Conversation

Lee-W
Copy link
Member

@Lee-W Lee-W commented Apr 11, 2024 •
edited by jedcunningham
Loading

Closes: #38900

Why

Currently, some of the web UI uses API endpoint to grep data. e.g., the following ones

This is because check_authentication blocks users without authentication. But when AUTH_ROLE_PUBLIC is set, it's possible that the public user have permission to do certain operation (including viewing those pages).

What

This PR check whether AUTH_ROLE_PUBLIC is set. If so, it assumes it as a valid user. If the AUTH_ROLE_PUBLIC role does not have proper permission, the authorization step will still blocks the request.

@Lee-W Lee-W changed the title fix(fab): return current_user when auth is not set in auth_current_user return current_user when auth is not set in auth_current_user Apr 11, 2024
@Lee-W Lee-W marked this pull request as draft April 11, 2024 11:27
@Lee-W Lee-W changed the title return current_user when auth is not set in auth_current_user check whether AUTH_ROLE_PUBLIC is set in check_authentication Apr 11, 2024
@Lee-W Lee-W marked this pull request as ready for review April 11, 2024 11:40
@Lee-W Lee-W requested a review from jhtimmins as a code owner April 11, 2024 11:40
@vincbeck
Copy link
Contributor

Please update the tests but otherwise, LGTM

@Lee-W
Copy link
Member Author

Lee-W commented Apr 11, 2024

Please update the tests but otherwise, LGTM

Sure. I updated the conftest.py to ensure AUTH_ROLE_PUBLIC is None

@vincbeck
Copy link
Contributor

Please update the tests but otherwise, LGTM

Sure. I updated the conftest.py to ensure AUTH_ROLE_PUBLIC is None

Could you add a test to cover this new branch?

@Lee-W
Copy link
Member Author

Lee-W commented Apr 11, 2024

Please update the tests but otherwise, LGTM

Sure. I updated the conftest.py to ensure AUTH_ROLE_PUBLIC is None

Could you add a test to cover this new branch?

Yes, already working on it locally. I will add one more test case for this on each of the cases that failed previously: https://github.com/apache/airflow/actions/runs/8646082917/job/23704766901 and will wrap it up tomorrow. Let me change it to draft to avoid accidental merging

@Lee-W Lee-W marked this pull request as draft April 11, 2024 13:47
@Lee-W Lee-W force-pushed the fix-AUTH_ROLE_PUBLIC branch 4 times, most recently from 166ba40 to 636354c Compare April 12, 2024 09:46
@Lee-W Lee-W marked this pull request as ready for review April 12, 2024 09:50
@Lee-W
Copy link
Member Author

Lee-W commented Apr 12, 2024 •
edited
Loading

Added an AUTH_ROLE_PUBLIC test case for the following test cases

  • tests/api_connexion/endpoints/test_config_endpoint.py
    • TestGetConfig
    • TestGetValue
  • tests/api_connexion/endpoints/test_connection_endpoint.py
    • TestDeleteConnection
    • TestGetConnection
    • TestGetConnections
    • TestPatchConnection
    • TestPostConnection
    • TestConnection
  • tests/api_connexion/endpoints/test_dag_endpoint.py
    • TestGetDag
    • TestGetDagDetails
    • TestGetDags
    • TestPatchDag
    • TestPatchDags
    • TestDeleteDagEndpoint
  • tests/api_connexion/endpoints/test_dag_run_endpoint.py
    • TestDeleteDagRun
    • TestGetDagRun
    • TestGetDagRuns
    • TestGetDagRunBatch
    • TestPostDagRun
    • TestPatchDagRunState
    • TestClearDagRun
    • TestGetDagRunDatasetTriggerEvents
    • TestSetDagRunNote
  • tests/api_connexion/endpoints/test_dag_source_endpoint.py
    • TestGetSource
  • tests/api_connexion/endpoints/test_dag_warning_endpoint.py
    • TestGetDagWarningEndpoint
  • tests/api_connexion/endpoints/test_dataset_endpoint.py
    • TestGetDatasetEndpoint
    • TestGetDatasets
    • TestGetDatasetEvents
    • TestPostDatasetEvents
    • TestGetDagDatasetQueuedEvent
    • TestDeleteDagDatasetQueuedEvent
    • TestGetDagDatasetQueuedEvents
    • TestDeleteDagDatasetQueuedEvents
    • TestGetDatasetQueuedEvents
    • TestDeleteDatasetQueuedEvents
  • tests/api_connexion/endpoints/test_event_log_endpoint.py
    • TestGetEventLog
    • TestGetEventLogs

@Lee-W Lee-W marked this pull request as draft April 12, 2024 09:59
vincbeck
vincbeck approved these changes Apr 12, 2024
View reviewed changes
Copy link
Contributor

@vincbeck vincbeck left a comment

Choose a reason for hiding this comment

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

LGTM

@vincbeck
Copy link
Contributor

Feel free to put it in review mode

@Lee-W
Copy link
Member Author

Lee-W commented Apr 12, 2024 •
edited
Loading

Feel free to put it in review mode

I just found out one missing case related to problem-json. Will wrap it up and make it in review mode today. Thanks for your prompt review! After a second thought, I think I misunderstood 🤔 Let me put it in review mode!

@Lee-W Lee-W marked this pull request as ready for review April 12, 2024 15:31
@eladkal eladkal added this to the Airflow 2.9.1 milestone Apr 13, 2024
@eladkal eladkal added the type:bug-fix Changelog: Bug Fixes label Apr 13, 2024
@Taragolis Taragolis merged commit 7b60825 into apache:main Apr 14, 2024
39 checks passed
potiuk added a commit to potiuk/airflow that referenced this pull request Apr 14, 2024
@potiuk
Revert "check whether AUTH_ROLE_PUBLIC is set in check_authentication (…
918ffc4 
…apache#38924)"

This reverts commit 7b60825.
@potiuk potiuk mentioned this pull request Apr 14, 2024
Merged
potiuk added a commit that referenced this pull request Apr 14, 2024
@potiuk
Revert "check whether AUTH_ROLE_PUBLIC is set in check_authentication (…
7fc2169 
…#38924)" (#39009)

This reverts commit 7b60825.
potiuk added a commit to potiuk/airflow that referenced this pull request Apr 14, 2024
@potiuk
Trigger FAB provider tests on API change
9a3e702 
Follow up after apache#38924 which was not triggered when API changed
@potiuk potiuk mentioned this pull request Apr 14, 2024
Merged
potiuk added a commit that referenced this pull request Apr 14, 2024
@potiuk
Trigger FAB provider tests on API change (#39010)
ac1f744 
Follow up after #38924 which was not triggered when API changed
@Lee-W Lee-W mentioned this pull request Apr 15, 2024
Merged
Lee-W added a commit to astronomer/airflow that referenced this pull request Apr 15, 2024
@Lee-W
Reapply "check whether AUTH_ROLE_PUBLIC is set in check_authentication (
40b082e 
apache#38924)" (apache#39009)

This reverts commit 7fc2169.
@jedcunningham jedcunningham removed this from the Airflow 2.9.1 milestone Apr 26, 2024
@jedcunningham jedcunningham added changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) and removed type:bug-fix Changelog: Bug Fixes labels Apr 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vincbeck vincbeck vincbeck approved these changes

@Taragolis Taragolis Taragolis approved these changes

@jhtimmins jhtimmins Awaiting requested review from jhtimmins

Assignees
No one assigned
Labels
changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

401 UNAUTHORIZED when using AUTH_ROLE_PUBLIC = "Admin"
5 participants
@Lee-W @vincbeck @Taragolis @eladkal @jedcunningham