Implement login and logout in AWS auth manager by vincbeck · Pull Request #35488 · apache/airflow

vincbeck · 2023-11-06T20:49:32Z

This PR introduces the AWS auth manager. The target is to create an auth manager using AWS services: AWS Identity center and Amazon Verified Permissions. This PR handles the login and logout mechanism. Other features such as authorization will come in a separate PR. The auth manager wont be usable as such because of missing feature but since it is not documented anywhere, it should not be used by users.

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

vincbeck · 2023-11-06T20:50:18Z

For now, returns whether the user is logged-in. The actual authorization will be implemented in a separate PR. This is true for all the authorization APIs

…rV2`

o-nikolas

Builds are still going, but other than that, the changes look good to me

ferruzzi · 2023-11-20T22:46:45Z

+
+ return redirect(saml_auth.logout())
+
+ @csrf.exempt


Nitpick, maybe this is obvious to someone with a better background in auth backends, but I have no clue what CSRF is without looking it up, or why this method should be exempt. A comment might be helpful here.

Just added a very simple comment. I only know I need to disable CSRF otherwise Identity center redirection wont work :)

ferruzzi

Left a couple nitpicks and a clarification question, but nothing blocking. LGTM (pending the CI going green.. tests are currently in progress)

utkarsharma2 · 2023-11-22T13:19:37Z

+ if not user:
+ self.log.error("Calling 'get_user_name()' but the user is not signed in.")
+ raise AirflowException("The user must be signed in.")
+ return user.get_name()


@vincbeck This looks like a breaking change since get_name() method was just defined in the BaseUser class. WDYT?

No because the auth manager interface is not public interface YET. It will become in the near future but work in auth manager is still in progress. It is not documented anywhere how to build an auth manager or even how to use it so I assume nobody uses it

vincbeck · 2023-11-06T20:49:32Z

This PR introduces the AWS auth manager. The target is to create an auth manager using AWS services: AWS Identity center and Amazon Verified Permissions. This PR handles the login and logout mechanism. Other features such as authorization will come in a separate PR. The auth manager wont be usable as such because of missing feature but since it is not documented anywhere, it should not be used by users.

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

vincbeck · 2023-11-06T20:50:18Z

For now, returns whether the user is logged-in. The actual authorization will be implemented in a separate PR. This is true for all the authorization APIs

…rV2`

o-nikolas

Builds are still going, but other than that, the changes look good to me

ferruzzi · 2023-11-20T22:46:45Z

+
+ return redirect(saml_auth.logout())
+
+ @csrf.exempt


Nitpick, maybe this is obvious to someone with a better background in auth backends, but I have no clue what CSRF is without looking it up, or why this method should be exempt. A comment might be helpful here.

Just added a very simple comment. I only know I need to disable CSRF otherwise Identity center redirection wont work :)

ferruzzi

Left a couple nitpicks and a clarification question, but nothing blocking. LGTM (pending the CI going green.. tests are currently in progress)

utkarsharma2 · 2023-11-22T13:19:37Z

+ if not user:
+ self.log.error("Calling 'get_user_name()' but the user is not signed in.")
+ raise AirflowException("The user must be signed in.")
+ return user.get_name()


@vincbeck This looks like a breaking change since get_name() method was just defined in the BaseUser class. WDYT?

No because the auth manager interface is not public interface YET. It will become in the near future but work in auth manager is still in progress. It is not documented anywhere how to build an auth manager or even how to use it so I assume nobody uses it

vincbeck · 2023-11-06T20:49:32Z

This PR introduces the AWS auth manager. The target is to create an auth manager using AWS services: AWS Identity center and Amazon Verified Permissions. This PR handles the login and logout mechanism. Other features such as authorization will come in a separate PR. The auth manager wont be usable as such because of missing feature but since it is not documented anywhere, it should not be used by users.

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

vincbeck · 2023-11-06T20:50:18Z

For now, returns whether the user is logged-in. The actual authorization will be implemented in a separate PR. This is true for all the authorization APIs

…rV2`

o-nikolas

Builds are still going, but other than that, the changes look good to me

ferruzzi · 2023-11-20T22:46:45Z

+
+ return redirect(saml_auth.logout())
+
+ @csrf.exempt


Nitpick, maybe this is obvious to someone with a better background in auth backends, but I have no clue what CSRF is without looking it up, or why this method should be exempt. A comment might be helpful here.

Just added a very simple comment. I only know I need to disable CSRF otherwise Identity center redirection wont work :)

ferruzzi

Left a couple nitpicks and a clarification question, but nothing blocking. LGTM (pending the CI going green.. tests are currently in progress)

utkarsharma2 · 2023-11-22T13:19:37Z

+ if not user:
+ self.log.error("Calling 'get_user_name()' but the user is not signed in.")
+ raise AirflowException("The user must be signed in.")
+ return user.get_name()


@vincbeck This looks like a breaking change since get_name() method was just defined in the BaseUser class. WDYT?

No because the auth manager interface is not public interface YET. It will become in the near future but work in auth manager is still in progress. It is not documented anywhere how to build an auth manager or even how to use it so I assume nobody uses it

vincbeck · 2023-11-06T20:49:32Z

This PR introduces the AWS auth manager. The target is to create an auth manager using AWS services: AWS Identity center and Amazon Verified Permissions. This PR handles the login and logout mechanism. Other features such as authorization will come in a separate PR. The auth manager wont be usable as such because of missing feature but since it is not documented anywhere, it should not be used by users.

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

vincbeck · 2023-11-06T20:50:18Z

For now, returns whether the user is logged-in. The actual authorization will be implemented in a separate PR. This is true for all the authorization APIs

…rV2`

o-nikolas

Builds are still going, but other than that, the changes look good to me

ferruzzi · 2023-11-20T22:46:45Z

+
+ return redirect(saml_auth.logout())
+
+ @csrf.exempt


Nitpick, maybe this is obvious to someone with a better background in auth backends, but I have no clue what CSRF is without looking it up, or why this method should be exempt. A comment might be helpful here.

Just added a very simple comment. I only know I need to disable CSRF otherwise Identity center redirection wont work :)

ferruzzi

Left a couple nitpicks and a clarification question, but nothing blocking. LGTM (pending the CI going green.. tests are currently in progress)

utkarsharma2 · 2023-11-22T13:19:37Z

+ if not user:
+ self.log.error("Calling 'get_user_name()' but the user is not signed in.")
+ raise AirflowException("The user must be signed in.")
+ return user.get_name()


@vincbeck This looks like a breaking change since get_name() method was just defined in the BaseUser class. WDYT?

No because the auth manager interface is not public interface YET. It will become in the near future but work in auth manager is still in progress. It is not documented anywhere how to build an auth manager or even how to use it so I assume nobody uses it

vincbeck · 2023-11-06T20:49:32Z

This PR introduces the AWS auth manager. The target is to create an auth manager using AWS services: AWS Identity center and Amazon Verified Permissions. This PR handles the login and logout mechanism. Other features such as authorization will come in a separate PR. The auth manager wont be usable as such because of missing feature but since it is not documented anywhere, it should not be used by users.

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

vincbeck · 2023-11-06T20:50:18Z

For now, returns whether the user is logged-in. The actual authorization will be implemented in a separate PR. This is true for all the authorization APIs

…rV2`

o-nikolas

Builds are still going, but other than that, the changes look good to me

ferruzzi · 2023-11-20T22:46:45Z

+
+ return redirect(saml_auth.logout())
+
+ @csrf.exempt


Nitpick, maybe this is obvious to someone with a better background in auth backends, but I have no clue what CSRF is without looking it up, or why this method should be exempt. A comment might be helpful here.

Just added a very simple comment. I only know I need to disable CSRF otherwise Identity center redirection wont work :)

ferruzzi

Left a couple nitpicks and a clarification question, but nothing blocking. LGTM (pending the CI going green.. tests are currently in progress)

utkarsharma2 · 2023-11-22T13:19:37Z

+ if not user:
+ self.log.error("Calling 'get_user_name()' but the user is not signed in.")
+ raise AirflowException("The user must be signed in.")
+ return user.get_name()


@vincbeck This looks like a breaking change since get_name() method was just defined in the BaseUser class. WDYT?

No because the auth manager interface is not public interface YET. It will become in the near future but work in auth manager is still in progress. It is not documented anywhere how to build an auth manager or even how to use it so I assume nobody uses it

vincbeck · 2023-11-06T20:49:32Z

This PR introduces the AWS auth manager. The target is to create an auth manager using AWS services: AWS Identity center and Amazon Verified Permissions. This PR handles the login and logout mechanism. Other features such as authorization will come in a separate PR. The auth manager wont be usable as such because of missing feature but since it is not documented anywhere, it should not be used by users.

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

vincbeck · 2023-11-06T20:50:18Z

For now, returns whether the user is logged-in. The actual authorization will be implemented in a separate PR. This is true for all the authorization APIs

…rV2`

o-nikolas

Builds are still going, but other than that, the changes look good to me

ferruzzi · 2023-11-20T22:46:45Z

+
+ return redirect(saml_auth.logout())
+
+ @csrf.exempt


Nitpick, maybe this is obvious to someone with a better background in auth backends, but I have no clue what CSRF is without looking it up, or why this method should be exempt. A comment might be helpful here.

Just added a very simple comment. I only know I need to disable CSRF otherwise Identity center redirection wont work :)

ferruzzi

Left a couple nitpicks and a clarification question, but nothing blocking. LGTM (pending the CI going green.. tests are currently in progress)

utkarsharma2 · 2023-11-22T13:19:37Z

+ if not user:
+ self.log.error("Calling 'get_user_name()' but the user is not signed in.")
+ raise AirflowException("The user must be signed in.")
+ return user.get_name()


@vincbeck This looks like a breaking change since get_name() method was just defined in the BaseUser class. WDYT?

No because the auth manager interface is not public interface YET. It will become in the near future but work in auth manager is still in progress. It is not documented anywhere how to build an auth manager or even how to use it so I assume nobody uses it

vincbeck · 2023-11-06T20:49:32Z

This PR introduces the AWS auth manager. The target is to create an auth manager using AWS services: AWS Identity center and Amazon Verified Permissions. This PR handles the login and logout mechanism. Other features such as authorization will come in a separate PR. The auth manager wont be usable as such because of missing feature but since it is not documented anywhere, it should not be used by users.

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

vincbeck · 2023-11-06T20:50:18Z

For now, returns whether the user is logged-in. The actual authorization will be implemented in a separate PR. This is true for all the authorization APIs

…rV2`

o-nikolas

Builds are still going, but other than that, the changes look good to me

ferruzzi · 2023-11-20T22:46:45Z

+
+ return redirect(saml_auth.logout())
+
+ @csrf.exempt


Nitpick, maybe this is obvious to someone with a better background in auth backends, but I have no clue what CSRF is without looking it up, or why this method should be exempt. A comment might be helpful here.

Just added a very simple comment. I only know I need to disable CSRF otherwise Identity center redirection wont work :)

ferruzzi

Left a couple nitpicks and a clarification question, but nothing blocking. LGTM (pending the CI going green.. tests are currently in progress)

utkarsharma2 · 2023-11-22T13:19:37Z

+ if not user:
+ self.log.error("Calling 'get_user_name()' but the user is not signed in.")
+ raise AirflowException("The user must be signed in.")
+ return user.get_name()


@vincbeck This looks like a breaking change since get_name() method was just defined in the BaseUser class. WDYT?

No because the auth manager interface is not public interface YET. It will become in the near future but work in auth manager is still in progress. It is not documented anywhere how to build an auth manager or even how to use it so I assume nobody uses it

vincbeck · 2023-11-06T20:49:32Z

This PR introduces the AWS auth manager. The target is to create an auth manager using AWS services: AWS Identity center and Amazon Verified Permissions. This PR handles the login and logout mechanism. Other features such as authorization will come in a separate PR. The auth manager wont be usable as such because of missing feature but since it is not documented anywhere, it should not be used by users.

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

vincbeck · 2023-11-06T20:50:18Z

For now, returns whether the user is logged-in. The actual authorization will be implemented in a separate PR. This is true for all the authorization APIs

…rV2`

o-nikolas

Builds are still going, but other than that, the changes look good to me

ferruzzi · 2023-11-20T22:46:45Z

+
+ return redirect(saml_auth.logout())
+
+ @csrf.exempt


Nitpick, maybe this is obvious to someone with a better background in auth backends, but I have no clue what CSRF is without looking it up, or why this method should be exempt. A comment might be helpful here.

Just added a very simple comment. I only know I need to disable CSRF otherwise Identity center redirection wont work :)

ferruzzi

Left a couple nitpicks and a clarification question, but nothing blocking. LGTM (pending the CI going green.. tests are currently in progress)

utkarsharma2 · 2023-11-22T13:19:37Z

+ if not user:
+ self.log.error("Calling 'get_user_name()' but the user is not signed in.")
+ raise AirflowException("The user must be signed in.")
+ return user.get_name()


@vincbeck This looks like a breaking change since get_name() method was just defined in the BaseUser class. WDYT?

No because the auth manager interface is not public interface YET. It will become in the near future but work in auth manager is still in progress. It is not documented anywhere how to build an auth manager or even how to use it so I assume nobody uses it

vincbeck requested review from ashb, bbovenzi, eladkal, o-nikolas, pierrejeambrun, potiuk and ryanahamilton as code owners November 6, 2023 20:49

boring-cyborgBot added area:dev-tools area:production-image Production image improvements and fixes area:providers area:webserver Webserver related Issues kind:documentation provider:amazon AWS/Amazon - related issues labels Nov 6, 2023

vincbeck commented Nov 6, 2023
View reviewed changes

vincbeck added the AIP-56 Extensible user management label Nov 6, 2023

vincbeck changed the title ~~Implement log in and log out in AWS auth manager~~Implement login and logout in AWS auth managerNov 6, 2023

vincbeck force-pushed the vincbeck/aws_auth_manager branch 2 times, most recently from 74e587b to 17d1ceaCompare November 7, 2023 16:59

Implement AWS auth manager
fab6e97

vincbeck force-pushed the vincbeck/aws_auth_manager branch from 17d1cea to fab6e97Compare November 7, 2023 18:18

vincbeck added 7 commits November 7, 2023 15:35

Mark test as db test
2f638af

Merge branch 'main' into vincbeck/aws_auth_manager
1e572ee

Add tests
240401f

Merge branch 'main' into vincbeck/aws_auth_manager
7b7431d

Merge branch 'main' into vincbeck/aws_auth_manager
b0cce0f

Pass only appbuilder to auth manager
107eda2

Make AwsSecurityManagerOverride inherit from `AirflowSecurityManage…
8f5aaf1
…rV2`

vincbeck force-pushed the vincbeck/aws_auth_manager branch from dd1caa9 to 8f5aaf1Compare November 10, 2023 21:11

vincbeck added 2 commits November 14, 2023 10:30

Fix tests
cc69de5

Fix tests
601504d

vincbeck added 2 commits November 17, 2023 12:43

Rename use_experimental config to enable
e553a6b

Specify python3-saml as additional-extra
b8d8340

o-nikolas approved these changes Nov 17, 2023
View reviewed changes

vincbeck added 8 commits November 17, 2023 16:32

Add message when python3-saml is not installed
19a6a48

Merge branch 'main' into vincbeck/aws_auth_manager
83b92d8

Add logout_callback endpoint
6a21cc7

Move import
b3eb860

Skip test if module is not installed
28b053e

Remove abstractmethod from get_user_name
190b9be

Fix test
476221d

Define the extra at the Airflow level
dd16f5a

ferruzzi reviewed Nov 20, 2023
View reviewed changes

Comment threadairflow/providers/amazon/provider.yaml

ferruzzi reviewed Nov 20, 2023
View reviewed changes

Comment threadairflow/www/security_manager.py Outdated

vincbeck added 2 commits November 20, 2023 17:19

Fix static checks
105b8c3

Fix tests
3285b36

ferruzzi reviewed Nov 20, 2023
View reviewed changes

ferruzzi approved these changes Nov 20, 2023
View reviewed changes

vincbeck added 2 commits November 21, 2023 11:15

Add "saml" word to spellcheck dictionary
8d1d365

nit
b11fbe9

vincbeck merged commit 379b7c0 into apache:mainNov 21, 2023

vincbeck deleted the vincbeck/aws_auth_manager branch November 21, 2023 17:35

utkarsharma2 reviewed Nov 22, 2023
View reviewed changes

ephraimbuddy added the changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) label Nov 23, 2023

ephraimbuddy pushed a commit that referenced this pull request Nov 23, 2023
Implement login and logout in AWS auth manager (#35488)
e2e8966

eladkal mentioned this pull request Nov 24, 2023
Status of testing Providers that were prepared on November 24, 2023 #35845
Closed
69 tasks

ephraimbuddy pushed a commit that referenced this pull request Nov 26, 2023
Implement login and logout in AWS auth manager (#35488)
3e1d7bc

VladaZakharova mentioned this pull request Dec 6, 2023
Port to Connexion 3 (WIP) RobbeSneyders/airflow#1
Draft

potiuk mentioned this pull request May 9, 2024
apache-airflow-providers-amazon added xmlsec as new dependency and pinned to a version that doesn't have wheels for new python versions #39437
Closed
2 tasks

jedcunningham mentioned this pull request May 9, 2024
Fix xmlsec version pin #39528
Closed