-
Notifications
You must be signed in to change notification settings - Fork 16.3k
[api] Add logic redacted sensitive fields via the Public API and UI #59873
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
Conversation
|
That's far too much. There are just a few API calls that return potentially sensitive data: config, variables, connections, related export (should be removed)/import (should mention that export can only be done via local CLI) features in UI. Only those are affected - there is absolutely no need to redact all possible Pydantic models. Just checking that APIs and UI displaying it are handling masked data well and that that proper |
jason810496
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's far too much. There are just a few API calls that return potentially sensitive data: config, variables, connections, related export (should be removed)/import (should mention that export can only be done via local CLI) features in UI. Only those are affected - there is absolutely no need to redact all possible Pydantic models. Just checking that APIs and UI displaying it are handling masked data well and that that proper
Yes, maybe searching 'connection', 'config' and 'variable' across the api_fastapi directory would be enough. Thanks!
|
@potiuk, @jason810496, I tested connection, config, and variable, so I think I only need to modify logic of API for: |
84af972 to
41368cd
Compare
jason810496
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your update!
airflow-core/tests/unit/api_fastapi/core_api/routes/ui/test_connections.py
Show resolved
Hide resolved
airflow-core/src/airflow/api_fastapi/core_api/datamodels/config.py
Outdated
Show resolved
Hide resolved
41368cd to
009d932
Compare
73e5b19 to
6d4aa1d
Compare
|
“I see there is already a PR in progress. |
|
@abhijeets25012-tech, I think that it is enough for this task. |
jason810496
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that it is enough for this task.
Yes, only the final nit then the PR is good to go.
airflow-core/tests/unit/api_fastapi/core_api/routes/ui/test_connections.py
Show resolved
Hide resolved
|
@jason810496, I had two tests for this case:
-> So I think that it is correct to pass. |
jason810496
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
input: {"extra_fields": "test-extra_fields"} -> expected: {"extra_fields": "test-extra_fields"} (normal data)
I see, thanks for the clarification.
|
Hi @jason810496, @potiuk, this pr (59880) handles not exposing sensitive fields of config via API. So, it is redundant to redact at |
1738881 to
40accff
Compare
jason810496
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
handles not exposing sensitive fields of config via API. So, it is redundant to redact at ConfigOption model of api. I can remove redact in airflow-core/src/airflow/api_fastapi/core_api/datamodels/config.py. It is the cause of the failed tests
Thanks for sharing the context. If this is the case, then I think the PR is good to go. Thanks!
airflow-core/tests/unit/api_fastapi/core_api/routes/ui/test_connections.py
Outdated
Show resolved
Hide resolved
ace135b to
fea3b36
Compare
|
I rebased the PR there was an airflowctl failure, let's see if it was intermittent |
…pache#59873) * Marked sensitive value in config API * Ignore tuple type * Marked sensitive value in Connection UI API * Fix mypy * Fix ruff check * fix redact_value of ConfigOption * Fix mypy * Add a test with dict value inclue sensitive and normal fields * Fix logic after pr 59880 * Remove print in test_connections.py --------- Co-authored-by: nhuan.bc <[email protected]> Co-authored-by: Jason(Zhe-You) Liu <[email protected]>
…pache#59873) * Marked sensitive value in config API * Ignore tuple type * Marked sensitive value in Connection UI API * Fix mypy * Fix ruff check * fix redact_value of ConfigOption * Fix mypy * Add a test with dict value inclue sensitive and normal fields * Fix logic after pr 59880 * Remove print in test_connections.py --------- Co-authored-by: nhuan.bc <[email protected]> Co-authored-by: Jason(Zhe-You) Liu <[email protected]>


closes: 59842
What:
How:
^ 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.rstor{issue_number}.significant.rst, in airflow-core/newsfragments.