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.

Sign up for GitHub

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

Jump to bottom

Review & edit Keycloak external authentication user stories #3079

Merged
merged 3 commits into from
Aug 5, 2024
Merged

Review & edit Keycloak external authentication user stories #3079

merged 3 commits into from
Aug 5, 2024

Conversation

asteflova
Copy link
Contributor

@asteflova asteflova commented Jun 18, 2024 •
edited

A follow-up on #2938

The goal is to cover the Keycloak external authentication user stories from beginning to end and make sure they are easy to read and follow. The changes revolve mostly around adding introductions to assemblies and modules, de-duplication, streamlining the web UI procedures, adding a few links to Keycloak docs, and general editing.

To explain the de-duplication effort in more detail:

  • I'm introducing a single configuration assembly for Wildfly-based Keycloak and a single one for Quarkus-based Keycloak. Before, the use cases for basic configuration, TOTP, and PIV cards use cases were each covered in a separate assembly. With this PR, I'm combining them because I believe the assemblies covering all three configurations are still sufficiently easy to follow, even though users will need to make some decisions with regards to which procedures apply to their situation ("If you want users to log in by using TOTP...").

  • Some modules are now reused between the Wildfly-based and Quarkus-based assemblies. They were almost identical.

  • I am okay with my commits getting squashed when you merge this PR.

  • I am familiar with the contributing guidelines.

Please cherry-pick my commits into:

  • Foreman 3.11/Katello 4.13
  • Foreman 3.10/Katello 4.12
  • Foreman 3.9/Katello 4.11 (Satellite 6.15; orcharhino 6.8)
  • Foreman 3.8/Katello 4.10
  • Foreman 3.7/Katello 4.9 (Satellite 6.14)
  • Foreman 3.6/Katello 4.8
  • Foreman 3.5/Katello 4.7 (Satellite 6.13; orcharhino 6.6/6.7)
  • We do not accept PRs for Foreman older than 3.5.

@github-actions GitHub Actions
Copy link

github-actions bot commented Jun 18, 2024 •
edited

The PR preview for 4fa4292 is available at theforeman-foreman-documentation-preview-pr-3079.surge.sh

The following output files are affected by this PR:

show diff

show diff as HTML

@asteflova
Copy link
Contributor Author

This is still just a draft. As a next step, I plan to test the procedures with a Keycloak engineer. Then I'll ask for docs peer review.

@lumarel Just letting you know that I'm proposing these changes to follow up on your #2926

@asteflova asteflova force-pushed the ext_auth_keycloak branch 4 times, most recently from d6b0cea to ed15c12 Compare July 18, 2024 07:59
@asteflova asteflova marked this pull request as ready for review July 18, 2024 08:33
@asteflova
Copy link
Contributor Author

I've updated the PR description and it's ready for review now.

FYI @maximiliankolb I'm also considering labeling the Wildfly-based assembly as deprecated. And I'm not yet sure if I'll be able to include the Quarkus-based one in our d/s. Does that somehow affect orcharhino?

@maximiliankolb
Copy link
Contributor

FYI @maximiliankolb I'm also considering labeling the Wildfly-based assembly as deprecated. And I'm not yet sure if I'll be able to include the Quarkus-based one in our d/s. Does that somehow affect orcharhino?

I have no idea but will ask around. I will get back to you probably in the beginning of next week.

Lennonka
Lennonka reviewed Jul 19, 2024
View reviewed changes
Copy link
Contributor

@Lennonka Lennonka left a comment

Choose a reason for hiding this comment

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

This really isn't my area, but I have a couple of observations.

guides/common/modules/con_configuring-keycloak-quarkus-authentication-for-project.adoc Show resolved Hide resolved
guides/common/modules/con_configuring-keycloak-wildfly-authentication-for-project.adoc Show resolved Hide resolved
guides/common/modules/proc_configuring-keycloak-settings-for-authentication-with-cac-cards.adoc Outdated Show resolved Hide resolved
...modules/proc_logging-in-to-project-configured-with-keycloak-as-an-authentication-source.adoc Show resolved Hide resolved
...modules/proc_logging-in-to-project-configured-with-keycloak-as-an-authentication-source.adoc Outdated Show resolved Hide resolved
@maximiliankolb
Copy link
Contributor

FYI @maximiliankolb I'm also considering labeling the Wildfly-based assembly as deprecated.

@asteflova This is OK for orcharhino.

lumarel
lumarel suggested changes Jul 24, 2024
View reviewed changes
Copy link
Contributor

@lumarel lumarel left a comment

Choose a reason for hiding this comment

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

Sorry for not making it earlier 🙂
I gave a bit of thought to a few parts, if they are not really what was intended with these changes, feel free to reject them.

guides/common/modules/proc_configuring-group-mapping-for-keycloak-authentication.adoc Show resolved Hide resolved
guides/common/modules/proc_configuring-group-mapping-for-keycloak-authentication.adoc Outdated Show resolved Hide resolved
...mon/modules/proc_configuring-project-settings-for-keycloak-authentication-using-the-cli.adoc Outdated Show resolved Hide resolved
guides/common/modules/proc_configuring-project-with-keycloak-for-totp-authentication.adoc Outdated Show resolved Hide resolved
guides/common/modules/con_configuring-keycloak-quarkus-authentication-for-project.adoc Show resolved Hide resolved
guides/common/modules/con_configuring-keycloak-wildfly-authentication-for-project.adoc Outdated Show resolved Hide resolved
.../modules/proc_configuring-project-settings-for-keycloak-authentication-using-the-web-ui.adoc Outdated Show resolved Hide resolved
@pr-processor pr-processor bot added Waiting on contributor Requires an action from the author and removed Not yet reviewed labels Jul 24, 2024
@pr-processor pr-processor bot added Needs re-review and removed Waiting on contributor Requires an action from the author labels Jul 30, 2024
@asteflova asteflova force-pushed the ext_auth_keycloak branch 6 times, most recently from 38b4167 to f153bfa Compare August 1, 2024 06:00
@asteflova asteflova added tech review done No issues from the technical perspective Needs style review Requires a review from docs style/grammar perspective and removed Needs re-review labels Aug 1, 2024
@asteflova
Copy link
Contributor Author

QE confirms that the Red Hat build of Keycloak (Quarkus-based Keycloak) workflow steps do result in a working setup. (Thanks @lhellebr!) As a result, I'm setting the labels to show that tech review has been done.

lumarel
lumarel approved these changes Aug 1, 2024
View reviewed changes
Copy link
Contributor

@lumarel lumarel left a comment

Choose a reason for hiding this comment

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

Max talk in the Community Demo today, made me realize that I set a blocker, I'm sorry for that, was not my intention 🙂

asteflova and others added 3 commits August 5, 2024 17:19
@asteflova
Edit Keycloak ext auth procedures
f00f48f 
* Implement content reuse for easier maintainability
* Review the docs for clarity and readability
* Group login procedures into a single assemblye
  assembly
* Test the Quarkus-based procedures
* List login methods available with Keycloak
* Review attributes related to Keycloak
@asteflova @lumarel
Group mapping not configured with keycloak results in Default role pe…
e47a105 
…rmissions

Co-authored-by: Lukas Magauer <42647570+lumarel@users.noreply.github.com>
@asteflova
Auth with both webUI and CLI doesn't work with quarkus-keycloak
4fa4292
@asteflova
Copy link
Contributor Author

Rebased to resolve a conflict. We got a working setup following the Quarkus-based docs, which covers tech review. This whole PR is basically me peer reviewing #2938. All threads have been resolved. Based on a conversation with @maximiliankolb, I'm going to go ahead and merge.

@asteflova asteflova merged commit c3399b7 into theforeman:master Aug 5, 2024
9 checks passed
@asteflova
Copy link
Contributor Author

Merged to "master" and cherry-picked:

e1bbc8e..d611b6c 3.11 -> 3.11

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

@ekohl ekohl ekohl left review comments

@lhellebr lhellebr lhellebr left review comments

@maximiliankolb maximiliankolb maximiliankolb left review comments

@Lennonka Lennonka Lennonka left review comments

@lumarel lumarel lumarel approved these changes

Assignees
No one assigned
Labels
Needs style review Requires a review from docs style/grammar perspective tech review done No issues from the technical perspective
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@asteflova @maximiliankolb @ekohl @lhellebr @Lennonka @lumarel