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 docs fix wrong link and add small enhancement #37748

Merged
merged 1 commit into from
Dec 14, 2023

Conversation

jedla97
Copy link
Contributor

@jedla97 jedla97 commented Dec 14, 2023

Fix wrong link to Quarkus as the links return 404.

Fix wrong imports.

Add notes where to find the quarkus-realm.json and keycloak-keystore.jks with links to it.

Copy link

github-actions bot commented Dec 14, 2023

🙈 The PR is closed and the preview is expired.

@@ -199,6 +199,8 @@ NOTE: Adding a `%prod.` profile prefix to `quarkus.oidc.auth-server-url` ensures

NOTE: By default, applications using the `quarkus-oidc` extension are marked as a `service` type application (see `quarkus.oidc.application-type`). This extension also supports only `web-app` type applications but only if the access token returned as part of the authorization code grant response is marked as a source of roles: `quarkus.oidc.roles.source=accesstoken` (`web-app` type applications check ID token roles by default).

NOTE: The configurated `quarkus-realm.json` file for this example can be found in https://github.com/quarkusio/quarkus-quickstarts/blob/main/security-keycloak-authorization-quickstart/config/quarkus-realm.json[quarkus-quickstarts/security-keycloak-authorization-quickstart/config]
Copy link
Member

@sberyozkin sberyozkin Dec 14, 2023

Choose a reason for hiding this comment

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

Suggested change
NOTE: The configurated `quarkus-realm.json` file for this example can be found in https://github.com/quarkusio/quarkus-quickstarts/blob/main/security-keycloak-authorization-quickstart/config/quarkus-realm.json[quarkus-quickstarts/security-keycloak-authorization-quickstart/config]

I'm not sure adding this note does not introduce duplication as just a few lines below there is in instruction to import it with link to it

Import the realm configuration file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I'll drop this note

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Dropped

@@ -210,7 +212,7 @@ To start a Keycloak Server you can use Docker and just run the following command
docker run --name keycloak -e KEYCLOAK_ADMIN=admin -e KEYCLOAK_ADMIN_PASSWORD=admin -p 8543:8443 -v "$(pwd)"/config/keycloak-keystore.jks:/etc/keycloak-keystore.jks quay.io/keycloak/keycloak:{keycloak.version} start --hostname-strict=false --https-key-store-file=/etc/keycloak-keystore.jks
----

where `keycloak.version` should be set to `17.0.0` or higher.
where `keycloak.version` should be set to `23.0.0` or higher and the `keycloak-keystore.jks` can be found in https://github.com/quarkusio/quarkus-quickstarts/blob/main/security-keycloak-authorization-quickstart/config/keycloak-keystore.jks[quarkus-quickstarts/security-keycloak-authorization-quickstart/config]
Copy link
Member

@sberyozkin sberyozkin Dec 14, 2023

Choose a reason for hiding this comment

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

Suggested change
where `keycloak.version` should be set to `23.0.0` or higher and the `keycloak-keystore.jks` can be found in https://github.com/quarkusio/quarkus-quickstarts/blob/main/security-keycloak-authorization-quickstart/config/keycloak-keystore.jks[quarkus-quickstarts/security-keycloak-authorization-quickstart/config]
where `keycloak.version` should be set to `23.0.0` or higher.

Users are not expected to use it somehow, the command above picks it up with "$(pwd)"/config/keycloak-keystore.jks, so it is not clear what adding this information helps with, alongside a more important KC version update info

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My point in here is that if user not using quickstart but go through this by him self. He can easily miss this. I can drop it if you stand on this.

Copy link
Member

Choose a reason for hiding this comment

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

@jedla97 Sure, makes sense to keep it, thanks

@sberyozkin
Copy link
Member

sberyozkin commented Dec 14, 2023

@jedla97 Hi, thanks for your help here, I have only proposed to drop two notes regarding the location of resources.

@jedla97 jedla97 force-pushed the keycloak-doc-small-fixes branch from 6dc006f to e4c6ecb Compare December 14, 2023 19:26
@jedla97 jedla97 requested a review from sberyozkin December 14, 2023 19:27
@sberyozkin sberyozkin merged commit e1c4c96 into quarkusio:main Dec 14, 2023
5 checks passed
@quarkus-bot quarkus-bot bot added this to the 3.7 - main milestone Dec 14, 2023
@@ -210,7 +210,7 @@ To start a Keycloak Server you can use Docker and just run the following command
docker run --name keycloak -e KEYCLOAK_ADMIN=admin -e KEYCLOAK_ADMIN_PASSWORD=admin -p 8543:8443 -v "$(pwd)"/config/keycloak-keystore.jks:/etc/keycloak-keystore.jks quay.io/keycloak/keycloak:{keycloak.version} start --hostname-strict=false --https-key-store-file=/etc/keycloak-keystore.jks
----

where `keycloak.version` should be set to `17.0.0` or higher.
where `keycloak.version` should be set to `23.0.0` or higher and the `keycloak-keystore.jks` can be found in https://github.com/quarkusio/quarkus-quickstarts/blob/main/security-keycloak-authorization-quickstart/config/keycloak-keystore.jks[quarkus-quickstarts/security-keycloak-authorization-quickstart/config]
Copy link
Member

Choose a reason for hiding this comment

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

Please don't use direct links, use variables instead: link:{quickstarts-blob-url}/security-keycloak-authorization-quickstart/config/keycloak-keystore.jks[text]

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

Successfully merging this pull request may close these issues.

3 participants