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

[Tests] Add mock-aai to the docker compose setup #1353

Merged
merged 3 commits into from
Feb 12, 2025
Merged

Conversation

jbygdell
Copy link
Collaborator

@jbygdell jbygdell commented Feb 4, 2025

Related issue(s) and PR(s)

Description

This PR adds the ls-aai-mock instance from the GDI starter-kit repo so we will be able to test auth fully.

How to test

make build-all
make sda-s3-up

Use a browser and go to http://localhost:8801/
It should be possible to complete the entire OIDC authentication circle.

The full test should work as normal.

make integrationtest-sda-s3-run

@jbygdell jbygdell marked this pull request as draft February 4, 2025 14:33
@jbygdell jbygdell force-pushed the feature/k3d-ingress branch from 262f5ee to 6aac193 Compare February 5, 2025 07:46
@jbygdell jbygdell force-pushed the tests/mock-aai branch 2 times, most recently from 437b6bf to be523b5 Compare February 5, 2025 13:03
@jbygdell jbygdell requested a review from a team February 5, 2025 14:10
@jbygdell jbygdell force-pushed the feature/k3d-ingress branch from 6aac193 to 81229e8 Compare February 5, 2025 14:10
@jbygdell jbygdell changed the base branch from feature/k3d-ingress to main February 6, 2025 06:32
@jbygdell jbygdell self-assigned this Feb 6, 2025
@jbygdell jbygdell marked this pull request as ready for review February 6, 2025 06:34
@jbygdell jbygdell changed the title Tests/mock aai [Tests] Add mock-aai to the docker compose setup Feb 6, 2025
@jbygdell jbygdell requested a review from a team February 7, 2025 08:39
Copy link
Contributor

@pahatz pahatz left a comment

Choose a reason for hiding this comment

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

It seems to work great and to have required a lot of tedious work.
I am missing a lot of information on choices here. I see a fixed image used from gitlab but I can't really understand the reasoning of the choices, whether we can depend on that to be updated, where is the repo etc.
It would be nice to have at least some context and links in the description.

@jbygdell
Copy link
Collaborator Author

It seems to work great and to have required a lot of tedious work. I am missing a lot of information on choices here. I see a fixed image used from gitlab but I can't really understand the reasoning of the choices, whether we can depend on that to be updated, where is the repo etc. It would be nice to have at least some context and links in the description.

Basically it is an implementation of the GDI starterkit repo in this compose setup. So in essence there is nothing that should be modified since it is here only so we can test the auth-OIDC circle.

@jbygdell jbygdell requested a review from a team February 10, 2025 13:44
pahatz
pahatz previously approved these changes Feb 10, 2025
@jbygdell jbygdell requested a review from nanjiangshu February 11, 2025 09:19
@jbygdell jbygdell enabled auto-merge February 11, 2025 09:19
@jbygdell jbygdell requested a review from a team February 11, 2025 09:20
Copy link
Contributor

@MalinAhlberg MalinAhlberg left a comment

Choose a reason for hiding this comment

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

Nice! Works well when testing to log in, and seems to reduce a lot of tedious work in the future 👍 . I'm a bit unsure about the tests being removed, otherwise I think it looks very good!

Copy link
Contributor

@nanjiangshu nanjiangshu left a comment

Choose a reason for hiding this comment

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

Works well! Great job!

Copy link
Contributor

@aaperis aaperis left a comment

Choose a reason for hiding this comment

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

Looks nice overall, just a couple of comments to discuss :-).

@jbygdell jbygdell requested a review from aaperis February 11, 2025 15:24
Copy link
Contributor

@nanjiangshu nanjiangshu left a comment

Choose a reason for hiding this comment

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

When I tried to run the integration test for sync, I found that the service oidc is failed with the following error

oidc  | create RSA key
oidc  | Traceback (most recent call last):
oidc  |   File "/oidc.py", line 313, in <module>
oidc  |     DATA = _generate_token()
oidc  |            ^^^^^^^^^^^^^^^^^
oidc  |   File "/oidc.py", line 57, in _generate_token
oidc  |     "kid": ec_key1.thumbprint()
oidc  |            ^^^^^^^^^^^^^^^^^^^^
oidc  |   File "/usr/local/lib/python3.11/site-packages/joserfc/rfc7517/models.py", line 163, in thumbprint
oidc  |     return thumbprint(self.dict_value, fields, self.thumbprint_digest_method)
oidc  |                       ^^^^^^^^^^^^^^^
oidc  |   File "/usr/local/lib/python3.11/site-packages/joserfc/rfc7517/models.py", line 142, in dict_value
oidc  |     data = self.binding.convert_raw_key_to_dict(self.raw_value, self.is_private)
oidc  |            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
oidc  |   File "/usr/local/lib/python3.11/site-packages/joserfc/rfc7517/pem.py", line 102, in convert_raw_key_to_dict
oidc  |     value = cls.export_public_key(raw_key)
oidc  |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
oidc  |   File "/usr/local/lib/python3.11/site-packages/joserfc/rfc7518/ec_key.py", line 80, in export_public_key
oidc  |     numbers = key.public_numbers()
oidc  |               ^^^^^^^^^^^^^^^^^^
oidc  | AttributeError: 'cryptography.hazmat.bindings._rust.openssl.rsa.RSA' object has no attribute 'public_numbers'

I guess this is related to the change of the algorithm from EC to RSA.
Since there is a filtering in the workflow of sda-sync, the integration test for sync has not been triggered by the Github Action.

@jbygdell
Copy link
Collaborator Author

I guess this is related to the change of the algorithm from EC to RSA. Since there is a filtering in the workflow of sda-sync, the integration test for sync has not been triggered by the Github Action.

No it has not. And sync is about to be completely rewritten so I deem this a non issue for now.

@jbygdell jbygdell requested a review from nanjiangshu February 12, 2025 06:08
Copy link
Contributor

@nanjiangshu nanjiangshu 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!

Copy link
Contributor

@aaperis aaperis left a comment

Choose a reason for hiding this comment

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

Nice

@jbygdell jbygdell added this pull request to the merge queue Feb 12, 2025
Merged via the queue into main with commit 3c3a855 Feb 12, 2025
8 checks passed
@jbygdell jbygdell deleted the tests/mock-aai branch February 12, 2025 13:58
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