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

Fix flaky OIDC test #79

Merged
merged 4 commits into from
Nov 11, 2024
Merged

Conversation

GeorgeJahad
Copy link
Collaborator

@GeorgeJahad GeorgeJahad commented Nov 8, 2024

The following test was failing about 2% of the test runs.

OidcTest#test_aliases_contain_initial_users_email [test/lib/clients/oidc_test.rb:23]:
Expected nil to be truthy.

Can't have two entities with same alias/auth-path

Multiple entities can't have the same entity alias. If an alias gets assigned to an entity, and then to a second entity, it gets removed from the first. (Which is why alias's should be unique across a single auth method.)

That was the problem with this test failure; there were multiple entities with the same entity alias, ("[email protected]").

The error would occur when the alias got taken away by the second entity in the middle of the test of the first.

The fix was simply to change the "initial_user_email" config parameter to "[email protected]"

In addition, to debug the problem, I had to decode the jwt_authorized token. I decided to leave it decoded, as a class variable in test_helper.rb, and recompute it each start up. (It seems clearer that way.)

Fixed incompletely specified aliases

In debugging this problem, I also noticed a different, related problem. I was only using the alias name to specify the alias, but multiple aliases can have the same name if they have different auth paths.

I have fixed this so that alias access now requires both the name and auth-path

Ticket

PR fixes this ticket: #73

@@ -13,11 +13,16 @@ class TestCase

# Helper methods
def jwt_authorized
"eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiJqb2huLmRvZUBleGFtcGxlLmNvbSIsIm5hbWUiOiJKb2huIERvZSIsImlhdCI6MTUxNjIzOTAyMiwiZ3JvdXBzIjpbImdyb3VwMSIsImdyb3VwMiJdLCJhdWQiOiJhc3RyYWwifQ.tfRLXmE_eq-piP88_clwPWrYfMAQbCJAeZQI6OFxZSI"
@@authorized_token ||= JWT.encode(@@authorized_data, Config[:jwt_signing_key])
Copy link
Collaborator

Choose a reason for hiding this comment

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

nice improvement 👍

@GeorgeJahad GeorgeJahad merged commit 184ab37 into G-Research:main Nov 11, 2024
2 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.

2 participants