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.

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

Enable DAB service redirect auth #2252

Merged
merged 2 commits into from
Sep 17, 2024
Merged

Conversation

AlanCoding
Copy link
Member

@AlanCoding AlanCoding commented Sep 4, 2024

What is this PR doing:

Internal reference AAP-30011

These will be used later, but we believe it is formulated in a way that won't interfere with other deployments.

The social auth class is described in:

https://github.com/ansible/django-ansible-base/blob/devel/docs/apps/service_backed_sso.md

No-Issue

Reviewers must know:

Specifically, the reverse-sync setting change is made okay by this change in DAB

ansible/django-ansible-base@2af5f34#diff-8623b469b6e5d0b1173a741c5702169d5a06e643a8d416c860127373d662217d

@github-actions github-actions bot added backport-4.2 This PR should be backported to stable-4.2 (1.2) backport-4.4 This PR should be backported to stable-4.4 (2.1) backport-4.5 This PR should be backported to stable-4.5 (2.2) backport-4.6 This PR should be backported to stable-4.6 (2.3) backport-4.7 This PR should be backported to stable-4.7 (2.4) backport-4.8 This PR should be backported to stable-4.8 (2.4) backport-4.9 This PR should be backported to stable-4.9 (2.4) labels Sep 4, 2024
@AlanCoding
Copy link
Member Author

DAB is on a devel pin:

galaxy_ng/setup.py

Lines 113 to 117 in 08dc2d6

django_ansible_base_branch = os.getenv('DJANGO_ANSIBLE_BASE_BRANCH', 'devel')
django_ansible_base_dependency = (
'django-ansible-base[jwt_consumer] @ '
f'git+https://github.com/ansible/django-ansible-base@{django_ansible_base_branch}'
)

@AlanCoding
Copy link
Member Author

the dab_jwt tests failing look like:


2024-09-04T18:02:44.8187024Z galaxy_ng/tests/integration/package/test_package_install.py::test_package_install[env_vars0] �[31mFAILED�[0m�[31m [ 99%]�[0m
2024-09-04T18:03:07.1038587Z galaxy_ng/tests/integration/package/test_package_install.py::test_package_install[env_vars1] �[31mFAILED�[0m�[31m [100%]�[0m

that might maybe be okay??

@AlanCoding AlanCoding marked this pull request as ready for review September 4, 2024 18:04
@h-kataria
Copy link

@elyezer please review

@AlanCoding
Copy link
Member Author

In ldap check, looks like 1 failure


2024-09-04T18:06:59.5697209Z galaxy_ng/tests/integration/api/test_ui_paths.py::test_api_ui_v1_execution_environments_registries �[31mFAILED�[0m�[31m [ 55%]�[0m
2024-09-04T18:16:20.6497686Z     def wait_for_task_ui_client(gc, task):
2024-09-04T18:16:20.6498175Z         counter = 0
2024-09-04T18:16:20.6498872Z         state = None
2024-09-04T18:16:20.6499675Z         task_id = task["task"].split("v3/tasks/")[1][:-1]
2024-09-04T18:16:20.6500419Z         while state in [None, 'waiting', 'running']:
2024-09-04T18:16:20.6500972Z             counter += 1
2024-09-04T18:16:20.6501375Z             if counter >= 60:
2024-09-04T18:16:20.6501957Z >               raise Exception('Task is taking too long')
2024-09-04T18:16:20.6502754Z �[1m�[31mE               Exception: Task is taking too long�[0m
2024-09-04T18:16:20.6503225Z 

@jctanner jctanner removed backport-4.2 This PR should be backported to stable-4.2 (1.2) backport-4.4 This PR should be backported to stable-4.4 (2.1) backport-4.5 This PR should be backported to stable-4.5 (2.2) backport-4.6 This PR should be backported to stable-4.6 (2.3) backport-4.7 This PR should be backported to stable-4.7 (2.4) backport-4.8 This PR should be backported to stable-4.8 (2.4) backport-4.9 This PR should be backported to stable-4.9 (2.4) labels Sep 5, 2024
No-Issue

Special var to satify linter with long name

Remove the reverse sync setting temporarily
@AlanCoding AlanCoding changed the title Apply settings from recent DAB features Enable DAB service redirect auth Sep 9, 2024
Copy link
Member

@elyezer elyezer left a comment

Choose a reason for hiding this comment

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

Looks good assuming that this will only take effect if the resource server is defined and enabled.

@rochacbruno
Copy link
Member

Following @elyezer comment,

Looks good assuming that this will only take effect if the resource server is defined and enabled.

I agree, the current code is adding for any install using keycloak or social_auth (the latest affects community galaxy)

I think it is better to change it to be conditional on RESOURCE_SERVER

@AlanCoding
Copy link
Member Author

I think it is better to change it to be conditional on RESOURCE_SERVER

I agree, yes, this is the structure we want for the overall topic of settings enablement of these. But I believe we are already doing this on the level of logic within the method here:

https://github.com/ansible/django-ansible-base/blob/f9b284a74ea42d4e2b2833ebcf45a6aa431f9f68/ansible_base/resource_registry/utils/service_backed_sso_pipeline.py#L9

    if (not getattr(settings, 'ENABLE_SERVICE_BACKED_SSO', False)) or (not resource_server_defined()):
        return None

The method resource_server_defined is intended to capture any nuance of the RESOURCE_SERVER setting.

You could also exclude the entry based (or not) in the settings definition logic (this code), but that doesn't seem necessary to me since this is also specific to certain scenarios.

@jctanner jctanner merged commit 22864c6 into ansible:master Sep 17, 2024
18 of 22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants