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

use new version of keycloak server with unit tests #74

Merged
merged 7 commits into from
Apr 16, 2024
Merged

Conversation

vbrik
Copy link
Contributor

@vbrik vbrik commented Apr 10, 2024

No description provided.

tests/test_api_users.py Outdated Show resolved Hide resolved
Comment on lines -205 to -207
'fo=o', # invalid char
'fo o', # space
'f\'oo', # quote
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These usernames can no longer be created in newer keycloak versions, which breaks test_user_put_invalid()

Copy link
Contributor

Choose a reason for hiding this comment

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

Does it break on client = await rest(username)? I guess then it's useful to split this into two lists, the second of which goes to test_invalid_usernames() and test_username_invalid().

@vbrik vbrik requested a review from dsschult April 10, 2024 20:33
@vbrik
Copy link
Contributor Author

vbrik commented Apr 10, 2024

@dsschult Could you take a look? Especially the changes in user_mgmt/handler.py. I am not sure if disappearance of 'groups' is significant.

Copy link
Contributor

@dsschult dsschult left a comment

Choose a reason for hiding this comment

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

Generally looks good.

tests/test_api_users.py Outdated Show resolved Hide resolved
Comment on lines -205 to -207
'fo=o', # invalid char
'fo o', # space
'f\'oo', # quote
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it break on client = await rest(username)? I guess then it's useful to split this into two lists, the second of which goes to test_invalid_usernames() and test_username_invalid().

@dsschult
Copy link
Contributor

@dsschult Could you take a look? Especially the changes in user_mgmt/handler.py. I am not sure if disappearance of 'groups' is significant.

So, the groups field in the token is supposed to always be there, and comes from the Keycloak client setup done in https://github.com/WIPACrepo/keycloak-rest-services/blob/master/krs/bootstrap.py#L350

I'm not sure what that means if that's not working. I'd probably need to see more debugging.

@vbrik
Copy link
Contributor Author

vbrik commented Apr 12, 2024

@dsschult Could you take a look? Especially the changes in user_mgmt/handler.py. I am not sure if disappearance of 'groups' is significant.

So, the groups field in the token is supposed to always be there, and comes from the Keycloak client setup done in https://github.com/WIPACrepo/keycloak-rest-services/blob/master/krs/bootstrap.py#L350

I'm not sure what that means if that's not working. I'd probably need to see more debugging.

Looks like this is by design: keycloak/keycloak#22340

@dsschult
Copy link
Contributor

@dsschult Could you take a look? Especially the changes in user_mgmt/handler.py. I am not sure if disappearance of 'groups' is significant.

So, the groups field in the token is supposed to always be there, and comes from the Keycloak client setup done in https://github.com/WIPACrepo/keycloak-rest-services/blob/master/krs/bootstrap.py#L350
I'm not sure what that means if that's not working. I'd probably need to see more debugging.

Looks like this is by design: keycloak/keycloak#22340

Ah, makes sense. In prod that's basically not a problem, but the .get() syntax is correct then.

@vbrik vbrik merged commit dd7860d into main Apr 16, 2024
12 checks passed
@vbrik vbrik deleted the keycloak_upgrade branch April 16, 2024 14:50
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