Disable default allowing the testing of connections in UI, API and CLI #32052

pankajkoti · 2023-06-21T14:39:19Z

With this PR, the test connection functionality will be disabled
by default across Airflow UI, API and CLI. The availability of the
functionality can be controlled by the test_connection flag in
the core section of the Airflow configuration (airflow.cfg).
It can also be controlled by the environment variable
AIRFLOW__CORE__TEST_CONNECTION.

The following values are accepted for this config param:

Disabled: Disables the test connection functionality and
disables(greys out) the Test Connection button in the UI.
This is also the default value set in the Airflow configuration.
Enabled: Enables the test connection functionality and
activates the Test Connection button in the UI.
Hidden: Disables the test connection functionality and
hides the Test Connection button in UI.

^ 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.rst or {issue_number}.significant.rst, in newsfragments.

o-nikolas

Nice!

airflow/config_templates/config.yml

airflow/www/static/js/connection_form.js

jedcunningham

We also need to disable endpoints that serve this functionality! It can't be a UI only thing.

airflow/www/static/js/connection_form.js

tests/www/views/test_views.py

airflow/config_templates/config.yml

potiuk

Few small nits, but LGTM

potiuk · 2023-06-27T05:47:09Z

We also need a significant newsfragment for that one @pankajkoti

pankajkoti · 2023-06-27T08:45:17Z

We also need a significant newsfragment for that one @pankajkoti

Thank you. Naive to add this one. Added one by following the guideline here which says it can be like a git commit message for significant types and also taking reference of this PR

pankajkoti · 2023-06-27T09:50:50Z

The tests are failing because of an unrelated error

=========================== short test summary info ============================
FAILED tests/providers/amazon/aws/triggers/test_athena.py::TestAthenaTrigger::test_run_success - TypeError: async_wait() got an unexpected keyword argument 'max_attempts'
=========== 1 failed, 1890 passed, 19 warnings in 898.71s (0:14:58) ============

Looks like the PR which added this change is reverted now. I will rebase now.

Users can enable test connection functionaility in UI with caution by setting the `enable_test_connection` key to `True` in the `[webserver]` section of airflow.cfg or by setting the environment variable `AIRFLOW__WEBSERVER__ENABLE_TEST_CONNECTION` to `True`.

potiuk · 2023-06-27T10:59:05Z

One more small comment. I've been thiknig about it.. Currently we have this but only in comments.

Before setting this to Enabled, make sure that you review the users who are able to add/edit
        connections and ensure they are trusted. Connection testing can be done maliciously leading to
        undesired and insecure outcomes.

Should we make also add some more information (link to the right part of the security model documentation once we merge #32098 should be enough ) - both in the newsfragment (effectively release notes) and in the place in documentation where we explain test connection, to explain why it is - potentially - dangerous to enable test connection?

I think we should be very explicit about it if we want to make our model influence decisions of our users.

pankajkoti · 2023-06-27T11:07:52Z

yes makes sense. Which part of the PR would we like to link it to? Some section in this file docs/apache-airflow/security/index.rst ?

Will wait for the PR to be merged and then link it here.
I am also thinking to reword Deployment admin -> Deployment Manager as was suggested earlier and in the security model. Will do that to when I link the section in my next commit.

potiuk · 2023-06-27T17:10:48Z

yes makes sense. Which part of the PR would we like to link it to? Some section in this file docs/apache-airflow/security/index.rst ?

Likely. docs/apache-airflow/security/index.rst#capabilities_of_authenticated_ui_users if I got it right :)

potiuk · 2023-06-27T17:11:56Z

Or maybe better will be to add reference and link to it. I might add it now.

…y-on-ui

Following up PR apache#32052 the test connection is disabled in UI, API and CLI. The API and CLI strictly check for the config value to be set as `Enabled` for the functionality to be enabled, whereas the UI just checks that is it not set to `Disabled`. As a result setting values to the config param other than `Disabled`, enables the button in the UI. Even though the button gets enabled, the API forbids it as there is a strict check in the API that the value is set to `Enabled` and only then allows, however, it makes sense to also strictly check in the UI that value is set to `Enabled`.

Following up PR #32052 the test connection is disabled in UI, API and CLI. The API and CLI strictly check for the config value to be set as `Enabled` for the functionality to be enabled, whereas the UI just checks that is it not set to `Disabled`. As a result setting values to the config param other than `Disabled`, enables the button in the UI. Even though the button gets enabled, the API forbids it as there is a strict check in the API that the value is set to `Enabled` and only then allows, however, it makes sense to also strictly check in the UI that value is set to `Enabled`.

Following up PR #32052 the test connection is disabled in UI, API and CLI. The API and CLI strictly check for the config value to be set as `Enabled` for the functionality to be enabled, whereas the UI just checks that is it not set to `Disabled`. As a result setting values to the config param other than `Disabled`, enables the button in the UI. Even though the button gets enabled, the API forbids it as there is a strict check in the API that the value is set to `Enabled` and only then allows, however, it makes sense to also strictly check in the UI that value is set to `Enabled`. (cherry picked from commit 50765eb)

…3342) Following up PR apache#32052 the test connection is disabled in UI, API and CLI. The API and CLI strictly check for the config value to be set as `Enabled` for the functionality to be enabled, whereas the UI just checks that is it not set to `Disabled`. As a result setting values to the config param other than `Disabled`, enables the button in the UI. Even though the button gets enabled, the API forbids it as there is a strict check in the API that the value is set to `Enabled` and only then allows, however, it makes sense to also strictly check in the UI that value is set to `Enabled`.

Following up PR apache/airflow#32052 the test connection is disabled in UI, API and CLI. The API and CLI strictly check for the config value to be set as `Enabled` for the functionality to be enabled, whereas the UI just checks that is it not set to `Disabled`. As a result setting values to the config param other than `Disabled`, enables the button in the UI. Even though the button gets enabled, the API forbids it as there is a strict check in the API that the value is set to `Enabled` and only then allows, however, it makes sense to also strictly check in the UI that value is set to `Enabled`. (cherry picked from commit 50765eb0883652c16b40d69d8a1ac78096646610) GitOrigin-RevId: eda4bc4b87deb6095cb009825ace9bf87eead369

boring-cyborg bot added area:UI Related to UI/UX. For Frontend Developers. area:webserver Webserver related Issues labels Jun 21, 2023

pankajkoti requested review from pierrejeambrun and potiuk June 21, 2023 14:39

pankajkoti force-pushed the disable-default-test-connection-functionality-on-ui branch 3 times, most recently from 2b38466 to 1a5678a Compare June 22, 2023 18:22

o-nikolas reviewed Jun 22, 2023

View reviewed changes

airflow/config_templates/config.yml Outdated Show resolved Hide resolved

airflow/config_templates/config.yml Outdated Show resolved Hide resolved

airflow/www/static/js/connection_form.js Outdated Show resolved Hide resolved

jedcunningham reviewed Jun 22, 2023

View reviewed changes

airflow/www/static/js/connection_form.js Outdated Show resolved Hide resolved

pankajkoti requested a review from bbovenzi June 23, 2023 09:04

pankajkoti force-pushed the disable-default-test-connection-functionality-on-ui branch from f1dfe9f to 036dd59 Compare June 24, 2023 13:44

pankajkoti commented Jun 24, 2023

View reviewed changes

tests/www/views/test_views.py Outdated Show resolved Hide resolved

pankajkoti force-pushed the disable-default-test-connection-functionality-on-ui branch from 2af54f5 to 9ba87b8 Compare June 24, 2023 13:52

pankajkoti requested review from o-nikolas and jedcunningham June 24, 2023 14:09

potiuk reviewed Jun 24, 2023

View reviewed changes

airflow/config_templates/config.yml Outdated Show resolved Hide resolved

potiuk reviewed Jun 24, 2023

View reviewed changes

airflow/config_templates/config.yml Outdated Show resolved Hide resolved

potiuk reviewed Jun 24, 2023

View reviewed changes

pankajkoti marked this pull request as ready for review June 24, 2023 17:25

pankajkoti requested review from ryanahamilton, ashb and ephraimbuddy as code owners June 24, 2023 17:25

pankajkoti force-pushed the disable-default-test-connection-functionality-on-ui branch from 51652f9 to dc44dfa Compare June 24, 2023 18:10

pankajkoti changed the title ~~Disable allowing by default testing of connnections in UI~~ Disable allowing by default the testing of connections in UI, API and CLI Jun 24, 2023

pankajkoti changed the title ~~Disable allowing by default the testing of connections in UI, API and CLI~~ Disable default allowing the testing of connections in UI, API and CLI Jun 24, 2023

pankajkoti requested a review from potiuk June 24, 2023 18:18

potiuk approved these changes Jun 24, 2023

View reviewed changes

o-nikolas approved these changes Jun 26, 2023

View reviewed changes

pankajkoti force-pushed the disable-default-test-connection-functionality-on-ui branch from 35d4232 to 4a52e3e Compare June 27, 2023 08:46

pankajkoti added 7 commits June 27, 2023 15:20

Fix test_views.py test

40b42e8

Address review feedback

b59d3a6

Update tests/www/views/test_views.py

d994203

Add tests, fix CI failures for tests and docs

a7b2b47

Address @potiuk's comments

d6d14bc

Add significant type newsfragment for the PR

934c1e4

pankajkoti force-pushed the disable-default-test-connection-functionality-on-ui branch from 4a52e3e to 934c1e4 Compare June 27, 2023 09:51

pankajkoti added this to the Airflow 2.7.0 milestone Jun 30, 2023

pankajkoti added 2 commits July 4, 2023 17:09

Merge branch 'main' into disable-default-test-connection-functionalit…

b22fa6b

…y-on-ui

Add link to capabilities of users in documentation

3ba18af

potiuk approved these changes Jul 4, 2023

View reviewed changes

Merge branch 'main' into disable-default-test-connection-functionalit…

c62ed8a

…y-on-ui

potiuk merged commit e4c3ecf into apache:main Jul 4, 2023
42 checks passed

ephraimbuddy added the type:improvement Changelog: Improvements label Jul 6, 2023

pankajkoti mentioned this pull request Aug 2, 2023

Revert "fix: save codemirror content to extra textarea (#32893)" #33032

Merged

pankajkoti mentioned this pull request Aug 12, 2023

Enable test connection in UI only if explicitly set to Enabled #33342

Merged

vatsrahul1001 mentioned this pull request Oct 13, 2023

Update documentation to enable test connection #34905

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Disable default allowing the testing of connections in UI, API and CLI #32052

Disable default allowing the testing of connections in UI, API and CLI #32052

pankajkoti commented Jun 21, 2023 •

edited

Loading

o-nikolas left a comment

jedcunningham left a comment

potiuk left a comment

potiuk commented Jun 27, 2023

pankajkoti commented Jun 27, 2023 •

edited

Loading

pankajkoti commented Jun 27, 2023

potiuk commented Jun 27, 2023 •

edited

Loading

pankajkoti commented Jun 27, 2023 •

edited

Loading

potiuk commented Jun 27, 2023

potiuk commented Jun 27, 2023

Disable default allowing the testing of connections in UI, API and CLI #32052

Disable default allowing the testing of connections in UI, API and CLI #32052

Conversation

pankajkoti commented Jun 21, 2023 • edited Loading

o-nikolas left a comment

Choose a reason for hiding this comment

jedcunningham left a comment

Choose a reason for hiding this comment

potiuk left a comment

Choose a reason for hiding this comment

potiuk commented Jun 27, 2023

pankajkoti commented Jun 27, 2023 • edited Loading

pankajkoti commented Jun 27, 2023

potiuk commented Jun 27, 2023 • edited Loading

pankajkoti commented Jun 27, 2023 • edited Loading

potiuk commented Jun 27, 2023

potiuk commented Jun 27, 2023

pankajkoti commented Jun 21, 2023 •

edited

Loading

pankajkoti commented Jun 27, 2023 •

edited

Loading

potiuk commented Jun 27, 2023 •

edited

Loading

pankajkoti commented Jun 27, 2023 •

edited

Loading