AIP-84 | Add Auth for Dags

[jason810496]

related: #42360

---

^ 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.

[jason810496]

Hi @pierrejeambrun,

Here is a draft for the first entity that introduces authentication and permissions. I want to ensure consistency across other entities and would appreciate your advice on the following changes:

• tests/api_fastapi/conftest.py

  • Added a bearer token with an admin role to the original test_client fixture.
  • Renamed the original test_client to unauthenticated_test_client (since adding the requires_access_* dependencies to routers means we should now respect the authorization header).

• tests/api_fastapi/core_api/routes/public/test_dags.py

  • Is adding _should_response_401 test cases for each router sufficient?
  • Or should we cover more scenarios, such as Vertical Privilege Escalation?

Looking forward to your thoughts!
cc @rawwar

[pierrejeambrun]

Great start 🎉

Is adding _should_response_401 test cases for each router sufficient?

I think so yes.

[jason810496]

After discussing with @rawwar offline, we determined that the previous test failure with 403 status code was caused by JWT and test cases using time_machine.

The test cases with time_machine are decorated with @time_machine.travel(timezone.utcnow(), tick=False). However, when the method is executed, the test_client fixture is recreated, causing the token to be perceived as being from the future.

After further discussion, by setting iat and nbf to a much earlier time (when Airflow was created) and exp to 24 hours in the future resolves the issue.

• iat: Issued at
• nbf: Not before
• exp: Expiration time

cc @pierrejeambrun

[jason810496]

The rest of CI failures are caused by some side effect of auth_manage with FastAPI app, but I still can't figure out what is the root cause:

e.g.

• tests/api_fastapi/core_api/routes/public/test_assets.py::TestPostAssetMaterialize
• tests/api_fastapi/core_api/routes/public/test_task_instances.py::TestGetTaskInstance
• tests/api_fastapi/core_api/routes/public/test_task_instances.py::TestGetTaskInstanceTry
• tests/api_fastapi/core_api/routes/public/test_task_instances.py::TestGetTaskInstanceTries

If I remove the conf_vars, those test case works well, I have also tried with replace conf_vars with try, finally to setup, teardown [core/auth_manager] config without with block and it doesn't help.

@pytest.fixture
def test_client():
    with conf_vars(
        {
            (
                "core",
                "auth_manager",
            ): "airflow.auth.managers.simple.simple_auth_manager.SimpleAuthManager",
        }
    ):
        yield TestClient(create_app())

Only session.commit explicitly can resolve this case ( bring out by @rawwar in #47136 )
cc @pierrejeambrun

[pierrejeambrun]

Looks like this is the first one that we need to merge to unblock other permissions PR.

Do you need help fixing the CI ?

edit: Let me take a look.

[jason810496]

Looks like this is the first one that we need to merge to unblock other permissions PR.

Indeed, this is the first PR for permissions.

Do you need help fixing the CI ?

Yes, I need more thought for the CI errors, thanks!

As I mentioned in #47062 (comment) explicitly commit the session can resolve, but I think that is kind of workaround way to solve it. I haven't come out other idea to solving it.

[pierrejeambrun]

I took a look at the failing TestPostAssetMaterialize for test_should_respond_200.

Indeed we are missing a commit there in the create_dags fixture.dag_maker will just not commit the change I believe if the session is provided from the caller.

I don't get why it was working before, maybe changing the client fixture to a context manager updated the pytest fixture resolution order, in such a way that we were lucky before, a fixture with a commit appeared later than the create_dags one and was hidding the issue ? I don't know.

Basically it's the same for configure_git_connection_for_dag_bundle fixture. Theoritically nobody is committing the new connection. If there are no other fixtures called or init code, we will end up without the connection in the db. (and locking error on sqlite).

Also test_should_respond_200_with_versions (reason for another failed test). In the loop make_dag_with_multiple_versions the last iteration will be missing the commit, dag.sync_to_db() is acting like it, you can just move it to the end of the loop I think to achieve that.

I guess other cases are similar to that.

Basically fixture creating db objects should commit. (or not commit if the helper code they call do the commit for them)

[pierrejeambrun]

I found this piece of code in the dag_maker:

            if AIRFLOW_V_3_0_PLUS:
                from airflow.models.dagbundle import DagBundleModel

                if (
                    self.session.query(DagBundleModel).filter(DagBundleModel.name == self.bundle_name).count()
                    == 0
                ):
                    self.session.add(DagBundleModel(name=self.bundle_name))
                    self.session.commit()

            return self

[pierrejeambrun]

We'll merge #47136 first, you can rebase then it will fix your CI issues.

[jason810496]

maybe changing the client fixture to a context manager updated the pytest fixture resolution order, in such a way that we were lucky before, a fixture with a commit appeared later than the create_dags one and was hidding the issue ?

I think that is the only way to illustrate this wired error, thanks for the explanation !

Basically fixture creating db objects should commit. (or not commit if the helper code they call do the commit for them)

Thanks to point out, so that we should commit explicitly when using session fixture.

Maybe, the test I wrote with before utilize provide_session decorator, it will commit at the end of context, so I won't add commit explicitly in test.

[pierrejeambrun]

Nice, this needs a rebase I think.

[jason810496]

Nice, this needs a rebase I think.

Just rebased, thanks!

There's still an open discussion regarding SimpleAuthManager that needs confirmation: #47062 (comment).
Mentioning it again here to ensure it’s not overlooked.

[pierrejeambrun]

Needs rebase + conflict soliving.

Nice, just a few suggestions

[pierrejeambrun]

LGTM just a few nits (non blocking)

[jason810496]

LGTM just a few nits (non blocking)

Thanks @pierrejeambrun , just resolved and rebased.