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

Make ecoinvent SSO client id configurable #37

Merged
merged 1 commit into from
Nov 25, 2024
Merged

Make ecoinvent SSO client id configurable #37

merged 1 commit into from
Nov 25, 2024

Conversation

jsvgoncalves
Copy link
Member

#34 (comment)

@jsvgoncalves jsvgoncalves marked this pull request as ready for review October 28, 2024 18:20
@jsvgoncalves
Copy link
Member Author

@asergios My guess is the client id isn't configured on keycloak yet?

@asergios
Copy link

@asergios My guess is the client id isn't configured on keycloak yet?

Just took care of it now, give it a try!

@jsvgoncalves
Copy link
Member Author

@asergios there seems to still be something wrong in the reply back from apollo

@asergios
Copy link

@jsvgoncalves Sorry for the late reply. offline_access was indeed missing for the new client. I tested locally and seems to be fine now. Can you re-run the tests please?

@jsvgoncalves
Copy link
Member Author

Sorry for the late reply. offline_access was indeed missing for the new client. I tested locally and seems to be fine now. Can you re-run the tests please?

Cool, tests are passing on CI too. https://github.com/brightway-lca/ecoinvent_interface/actions/runs/12008197826

* As requested on #34, this sends a different client id to keycloak

#34 (comment)

Signed-off-by: João Gonçalves <[email protected]>
@jsvgoncalves jsvgoncalves requested review from cmutel and tngTUDOR and removed request for cmutel November 25, 2024 10:44
@asergios
Copy link

We won't disable Direct Grant from apollo-ui for now to allow users of ei-interface to migrate to a more recent version where brightway-ei is used instead.

What do you think is a reasonable time for that? 3 months?

@cmutel cmutel merged commit c7d6d22 into main Nov 25, 2024
@cmutel
Copy link
Member

cmutel commented Nov 25, 2024

What do you think is a reasonable time for that? 3 months?

Sure, that seems reasonable. Thanks a lot @asergios for your help, very much appreciated, and a good sign of cooperation across institutions!

@tngTUDOR
Copy link
Contributor

This makes ecoinvent_interface provide a different client id to the target API, not a fix for #34

@tngTUDOR
Copy link
Contributor

tngTUDOR commented Nov 25, 2024

@jsvgoncalves did we miss some spots?

"ecoinvent-api-client-library": "ecoinvent_interface",

"ecoinvent-api-client-library": "ecoinvent_interface",

"ecoinvent-api-client-library": "ecoinvent_interface",

Or there is no difference or trouble when using a client-id and a different api-library.

@jsvgoncalves jsvgoncalves deleted the ecoquery-client branch November 25, 2024 17:39
@jsvgoncalves
Copy link
Member Author

@jsvgoncalves did we miss some spots?

"ecoinvent-api-client-library": "ecoinvent_interface",

"ecoinvent-api-client-library": "ecoinvent_interface",

"ecoinvent-api-client-library": "ecoinvent_interface",

Or there is no difference or trouble when using a client-id and a different api-library.

Correct, the header that keycloak needs is the client_id. The ecoinvent-api-client-library was just a temporary nicety for ecoinvent folks to be able to tell the requests coming from this library apart, i.e. it's non-functional.

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.

4 participants