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

Keycloak ID key change #854

Closed
nijel opened this issue Nov 9, 2023 · 5 comments · Fixed by #858
Closed

Keycloak ID key change #854

nijel opened this issue Nov 9, 2023 · 5 comments · Fixed by #858
Labels

Comments

@nijel
Copy link
Member

nijel commented Nov 9, 2023

Expected behaviour

Upgrade from 4.4.2 to 4.5.0 changes behavior of Keycloak authentication, see WeblateOrg/docker#2048.

Actual behaviour

#815 by @derlin is probably the root cause here:

  • It removes ID_KEY from the backend, makes it effectively use value from a parent class:

  • While this attribute is still used in the partial pipeline:

    # Normally when resuming a pipeline, request_data will be empty. We
    # only need to check for a uid match if new data was provided (i.e.
    # if current request specifies the ID_KEY).
    if backend.ID_KEY in request_data:
    id_from_partial = partial.kwargs.get("uid")
    id_from_request = request_data.get(backend.ID_KEY)
    if id_from_partial != id_from_request:
    partial_matches_request = False

Any other comments?

Possible solutions:

There is at least one additional backend (Seznam) affected by this.

@RJPercival
Copy link
Contributor

I've created a revert PR (#858) since a fix PR doesn't seem to be forthcoming.

@derlin
Copy link
Contributor

derlin commented Nov 16, 2023

I am sorry for the disagreement, I really wasn't thinking this would be hardcoded in utils. I still believe having a way to configure the ID_KEY for OAuth-based backend is an important feature.

I unfortunately don't have time to provide a fix in the upcoming week, would someone else have some time to investigate?

@RJPercival
Copy link
Contributor

@nijel, what's the path forward here?

@RJPercival
Copy link
Contributor

@nijel, friendly ping - it's been a week. Can we merge the revert PR please so this unblocks releasing a new version?

@nijel
Copy link
Member Author

nijel commented Nov 29, 2023

I've merged that, given we don't have a better solution for now. #862 is the issue to track this feature.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging a pull request may close this issue.

3 participants