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

Update Keycloak SPA example #45728

Merged
merged 1 commit into from
Jan 23, 2025
Merged

Conversation

jedla97
Copy link
Contributor

@jedla97 jedla97 commented Jan 20, 2025

Fixes #37845

The .success and .error was removed in KC 22 (https://www.keycloak.org/docs/latest/upgrading/#legacy-promise-api-removed-from-keycloak-js-adapter)

With KC 26 the js libs are no longer available on server (https://www.keycloak.org/docs/latest/upgrading/#keycloak-js), Also they now expected the script in modules.

I play with it and can say it can't be backported to 3.15 as it is so I would start with this from 3.19 or possibly backport it to 3.18 as it's contain KC 26.

Note that I not expert in js so there can be more nicer code how to do this

cc @rolfedh as you make some workaround regarding this for RHBQ docs

@quarkus-bot quarkus-bot bot added area/docstyle issues related for manual docstyle review area/documentation labels Jan 20, 2025
Copy link

github-actions bot commented Jan 20, 2025

🙈 The PR is closed and the preview is expired.

This comment has been minimized.

@gsmet
Copy link
Member

gsmet commented Jan 21, 2025

@mabartos could someone from the Keycloak team review this?

Also maybe @phillip-kruger for the JS part.

Copy link
Member

@phillip-kruger phillip-kruger left a comment

Choose a reason for hiding this comment

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

LGTM. You can also use :

<dependency>
	<groupId>org.mvnpm</groupId>
	<artifactId>keycloak-js</artifactId>
	<version>26.1.0</version>
	<scope>runtime</scope>
</dependency>

and then :

import Keycloak from "/_static/keycloak-js/26.1.0/lib/keycloak.js"

@sberyozkin sberyozkin self-requested a review January 21, 2025 13:39
Copy link
Member

@sberyozkin sberyozkin left a comment

Choose a reason for hiding this comment

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

Thanks @jedla97

@sberyozkin
Copy link
Member

@rolfedh Hi Rolfe, are you OK with this PR to me merged ?

@sberyozkin
Copy link
Member

Sorry, @gsmet, you asked about the feedback from the Keycloak team... Let me CC @pedroigor too.

@rolfedh
Copy link
Contributor

rolfedh commented Jan 21, 2025

@rolfedh Hi Rolfe, are you OK with this PR to be merged ?

Yes, I’m fine with this PR being merged. I’ve provided a suggestion and am hoping to see a response or update, but it’s not essential if the author prefers to proceed as is.

@jedla97
Copy link
Contributor Author

jedla97 commented Jan 21, 2025

LGTM. You can also use :

 <groupId>org.mvnpm</groupId>
 <artifactId>keycloak-js</artifactId>
 <version>26.1.0</version>
 <scope>runtime</scope>
</dependency>
and then :
import Keycloak from "/_static/keycloak-js/26.1.0/lib/keycloak.js"

I looked at this and seems like only few version is available https://mvnrepository.com/artifact/org.mvnpm/keycloak-js. So I didn't add any notes about it.

Copy link
Contributor

@rolfedh rolfedh left a comment

Choose a reason for hiding this comment

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

LGTM

This comment has been minimized.

@rolfedh
Copy link
Contributor

rolfedh commented Jan 21, 2025

Hi @jedla97,

I’m not entirely sure how the team usually handles this, but I was wondering if these changes might require a review or updates to the quarkus-quickstarts/security-keycloak-authorization-quickstart. Let me know if I can help with anything!

@sberyozkin
Copy link
Member

sberyozkin commented Jan 21, 2025

Hi @rolfedh, This is only a basic scrip example (as far as I recall, provided by Pedro awhile back) explaining how users can create SPAs which can login users into Keycloak - at this point Quarkus is not even involved, and it is not related to keycloak-authorization.

Hi @jedla97 Did you try this updated SPA fragment ? If you did, given it must've been working for you, IMHO we can just merge, since this snippet is not meant to offer a production-quality SPA script code that users can copy and paste. May be it is worth updating the note to let users know that they should work with the Keycloak documentation to create a fully functioning SPA which integrates with Keycloak

Copy link

quarkus-bot bot commented Jan 21, 2025

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit e5b2cc1.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

You can consult the Develocity build scans.


Flaky tests - Develocity

⚙️ JVM Tests - JDK 17

📦 extensions/infinispan-cache/deployment

io.quarkus.cache.infinispan.InfinispanCacheTest.testGetAsyncWithParallelCalls - History

  • expected: "thread1" but was: "thread2" - org.opentest4j.AssertionFailedError
org.opentest4j.AssertionFailedError: 

expected: "thread1"
 but was: "thread2"
	at io.quarkus.cache.infinispan.InfinispanCacheTest.testGetAsyncWithParallelCalls(InfinispanCacheTest.java:283)
	at java.base/java.lang.reflect.Method.invoke(Method.java:569)
	at io.quarkus.test.QuarkusUnitTest.runExtensionMethod(QuarkusUnitTest.java:513)
	at io.quarkus.test.QuarkusUnitTest.interceptTestMethod(QuarkusUnitTest.java:427)

@jedla97
Copy link
Contributor Author

jedla97 commented Jan 21, 2025

Hi @sberyozkin

Hi @jedla97 Did you try this updated SPA fragment ? If you did, given it must've been working for you, IMHO we can just merge, since this snippet is not meant to offer a production-quality SPA script code that users can copy and paste.

Yes I try it and it worked

May be it is worth updating the note to let users know that they should work with the Keycloak documentation to create a fully functioning SPA which integrates with Keycloak

In note I pointing to Keycloak docs For more information, see the link:https://www.keycloak.org/documentation[Keycloak documentation].

Would you like it more like: The code provides an example of building Quarkus single-page applications integrated with Keycloak. For more details about creating single-page applications integrating Keycloak, refer to the official link:https://www.keycloak.org/documentation[Keycloak documentation].

@sberyozkin
Copy link
Member

Hi @jedla97 Great, I think an update like that would be perfect and then I believe we should merge, if someone from Keycloak team finds some time to suggest further edits then we can apply them easily

@jedla97 jedla97 force-pushed the fix-keycloak-spa branch 2 times, most recently from 847235f to f478c00 Compare January 21, 2025 20:30
@jedla97 jedla97 requested review from sberyozkin and rolfedh January 21, 2025 20:30
@jedla97
Copy link
Contributor Author

jedla97 commented Jan 21, 2025

Changed the text refering to Keycloak docs, so now it should be fine

This comment has been minimized.

@sberyozkin
Copy link
Member

Thanks @jedla97, I'll try ti finalize tomorrow, Guillaume, hope you are ok with us merging it soon

@mabartos
Copy link
Contributor

@vmuzikar Could you please check it, or notify some people as I'm currently on PTO? Thank you very much!

@sberyozkin
Copy link
Member

Thanks @jedla97. IMHO we are ready to merge, this is a basic script fragment, confirmed to be working, and with links to the Keycloak doc page providing all the details, so hope everyone is OK with us merging it shortly, CC @gsmet @mabartos @vmuzikar, and of course we will always welcome further feedback. Cheers.

Copy link

quarkus-bot bot commented Jan 22, 2025

Status for workflow Quarkus Documentation CI

This is the status report for running Quarkus Documentation CI on commit ee47731.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

@sberyozkin
Copy link
Member

Let me merge now

@sberyozkin sberyozkin merged commit 235a788 into quarkusio:main Jan 23, 2025
5 checks passed
@quarkus-bot quarkus-bot bot added this to the 3.19 - main milestone Jan 23, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Docs: security-oidc-bearer-token-authentication guide Keycloak SPA not working
6 participants