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

Review & edit IPA external authentication user story #3015

Merged
merged 17 commits into from
Aug 5, 2024

Conversation

asteflova
Copy link
Contributor

@asteflova asteflova commented May 13, 2024

A follow-up on #2938

The goal is to cover the IPA user story from beginning to end and make sure it's easy to read and follow.

UPDATE: To summarize the changes:

  • The assembly introduction now lists the available login methods

  • Where possible, the docs link to RHEL Identity Management docs for FreeIPA procedures. (FreeIPA does not have a documentation set that would mirror the IdM content; but we could search for corresponding FreeIPA resources upstream, like blog posts and developer docs. But probably not in this PR.)

  • The old FreeIPA procedures were re-implemented as examples of the simplest(ish) way to achieve the goal (see the Example boxes).

  • The assembly now also includes login procedures because they are useful for verification.

  • A lot of editing, shortening, streamlining, and fine-tuning.

  • 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)
  • Foreman 3.4/Katello 4.6 (EL8 only)
  • Foreman 3.3/Katello 4.5 on EL7 & EL8 (Satellite 6.12 on EL8 only; orcharhino 6.4/6.5 on EL8 only)
  • We do not accept PRs for Foreman older than 3.3.

@asteflova
Copy link
Contributor Author

@domiborges will do a peer review to provide the FreeIPA/Identity Management's docs team perspective.

@asteflova asteflova force-pushed the ext_auth_ipa_story branch 2 times, most recently from a25c267 to d89fb74 Compare July 22, 2024 12:16
@asteflova asteflova marked this pull request as ready for review July 26, 2024 19:53
@asteflova
Copy link
Contributor Author

Thanks a lot @domiborges for your review!

I updated the PR description with a summary of the changes.

@asteflova
Copy link
Contributor Author

@domiborges We talked about the possibility to add links to FreeIPA docs upstream to make sure that even builds that don't include links to RH downstream docs point to some useful resources. I'm afraid I found only one such resource: see Additional resources in 5.2.3. Configuring host-based access control for FreeIPA users logging in to orcharhino (this is the html build for orcharhino for which all links to RH docs are excluded).

The problem is that the FreeIPA upstream documentation is no longer maintained and upstream users are being redirected to RH docs. So I think that finding good upstream-only resources will be tricky. Domi, let me know if you can think of any other useful links to add; otherwise, I'll leave things as they are now, which means:

  • The Satellite d/s build includes RH d/s links as usual
  • The orcharhino d/s build doesn't include any RH d/s links and one FreeIPA-only u/s link
  • Upstream builds include both RH d/s links and the FreeIPA-only u/s link

Copy link
Contributor

@maximiliankolb maximiliankolb left a comment

Choose a reason for hiding this comment

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

many minor suggestions; some questions. I only had a quick look at the diff; I am no expert with FreeIPA/IDM.

asteflova and others added 2 commits July 30, 2024 10:11
Co-authored-by: Maximilian Kolb <mail@maximilian-kolb.de>
Copy link
Contributor Author

@asteflova asteflova left a comment

Choose a reason for hiding this comment

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

Thanks for the review @maximiliankolb

You noticed quite a few blank lines that should be removed and inconsistencies between headings/IDs/file names. If you don't mind pointing out every single one like this, great! :) But if doing it doesn't exactly spark joy for you, you can also just let me know that I should check the whole PR myself to fix them.

@asteflova
Copy link
Contributor Author

many minor suggestions; some questions. I only had a quick look at the diff; I am no expert with FreeIPA/IDM.

Myself and @domiborges have covered the FreeIPA/IdM part. What is needed now is a style check to make sure we can merge it. I'm updating the labels to clarify that.

@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 Not yet reviewed labels Jul 30, 2024
@asteflova asteflova added style review done No issues from docs style/grammar perspective and removed Needs style review Requires a review from docs style/grammar perspective labels Aug 5, 2024
@asteflova
Copy link
Contributor Author

Based on a conversation with @maximiliankolb and on having resolved all open threads, I'm going to go ahead and merge.

@asteflova asteflova merged commit e65c2fb into theforeman:master Aug 5, 2024
9 checks passed
@asteflova asteflova deleted the ext_auth_ipa_story branch August 5, 2024 06:51
asteflova added a commit that referenced this pull request Aug 5, 2024
* Redefine FreeIPA attributes for RH d/s

* Review and edit the FreeIPA external authentication story

* Review and clarify configuring Hammer for FreeIPA

Based on https://github.com/theforeman/hammer-cli-foreman/blob/master/doc/configuration.md

* Drop warning about restart after satellite-maintain

---------

Co-authored-by: Maximilian Kolb <mail@maximilian-kolb.de>
(cherry picked from commit e65c2fb)
@asteflova
Copy link
Contributor Author

Merged to "master" and cherry-picked:

5a2f7e8..a4b868f 3.11 -> 3.11

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
style review done No issues from docs style/grammar perspective tech review done No issues from the technical perspective
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants