Bug 1442252 - Reorg of the Configuring Authentication and User Agent page #4196
Conversation
|
Preview page: |
|
@liggitt The changes are mostly organizational in nature. I moved things around in the Identity Providers section so that each sub-section has the same flow:
Any information apart from how to edit the master config in a given section, I moved after the master config section. I did some editing also; want to make sure I didn't affect any technical meaning. Thank you in advance. |
| When set to the default `*claim*` value, OAuth will fail if the identity is | ||
| mapped to a previously-existing user name. The following table outlines the use | ||
| cases for the available `*mappingMethod*` parameter values: | ||
| <1> When `fasle` true, unauthenticated token requests from web clients (like the web console) are no redirected to a login page backed by this provider.. |
There was a problem hiding this comment.
Not sure what happened. Apologies.
I wanted to highlight this parameter as it is the only identity provider type in the group that takes a false and attempt to explain what false does.
<1> Set to false. Unauthenticated token requests from web clients (like the web console) are not redirected to a login page backed by this provider.
There was a problem hiding this comment.
it can be either true or false... it is immaterial to the mappingMethod example
There was a problem hiding this comment.
the meaning and impact of that field are described in every identity provider section, e.g. https://github.com/openshift/openshift-docs/pull/4196/files/c3f66e2ac1028aba572fb5893a09378e5653e39b#diff-c98c76519811b1acc60caf55898fa747R385
<2> When true, unauthenticated token requests from web clients (like the web console) are redirected to a login page backed by this provider.
| user name. Fails if a user with that user name is already mapped to another | ||
| identity. | ||
| [[mapping-identities-to-users-table]] | ||
| . Set the `login`parameter to `false`. |
There was a problem hiding this comment.
remove this, the login parameter has nothing to do with mappingMethod
There was a problem hiding this comment.
Should this be true?
login: true
There was a problem hiding this comment.
it doesn't matter... everything except mappingMethod could be elided
|
|
||
| [cols="2,8"] | ||
| |=== | ||
| |Parameter | Description |
There was a problem hiding this comment.
I find the table easier to read, personally
There was a problem hiding this comment.
I put the table back under Step 1.
| === Allow All | ||
| Set *AllowAllPasswordIdentityProvider* in the `*identityProviders*` stanza to | ||
| allow any non-empty user name and password to log in. This is the default | ||
| === Configuring the Allow All Identity Provider |
There was a problem hiding this comment.
do we want subpages for the different providers?
There was a problem hiding this comment.
I was thinking the same thing. Some are very long. Maybe Phase II?
|
|
||
| . Generate a search filter by combining the attribute and filter in the | ||
| The following is the {product-title} process. {product-title}: |
There was a problem hiding this comment.
this reads as "The following is the OpenShift Container Platform process. OpenShift Container Platform:" which doesn't make any sense
| validated against a remote URL that is protected by Basic authentication and | ||
| returns JSON. | ||
|
|
||
| A `401` response indicates failed authentication. |
There was a problem hiding this comment.
this got dropped, and is essential information
There was a problem hiding this comment.
ah, it got moved below the example, nevermind
|
|
||
| A `401` response indicates failed authentication. | ||
|
|
||
| A non-`200` status, or the presence of a non-empty "error" key, indicates an |
There was a problem hiding this comment.
this got dropped, and is essential information
There was a problem hiding this comment.
Appears after the example in a section called "Failed Authentication":
Failed Authentication
A 401 response indicates failed authentication.
A non-200 status, or the presence of a non-empty "error" key, indicates an error:
|
|
||
| To redirect unauthenticated requests from clients expecting `WWW-Authenticate` challenges: | ||
| [[redirect-www-challenges]] | ||
| *Configuire for `WWW-Authenticate` Challenges* |
|
|
||
| * `${url}` is replaced with the current URL, escaped to be safe in a query parameter. | ||
| + | ||
| // tag::IDP-urlquerytokens[] |
There was a problem hiding this comment.
This is a tag to create a section of text that I imported later in the topic.
| . Configure which link:http://openid.net/specs/openid-connect-core-1_0.html#StandardClaims[claims] to use: | ||
| + | ||
| ---- | ||
| claims: |
There was a problem hiding this comment.
this snippet is disconnected from the larger example, which makes it difficult to understand (for example, the indent level would be wrong if someone copied this into their config)
There was a problem hiding this comment.
Good point. I wanted to highlight and explain that section better; but, I can see how that could be confusing. Removed the second snippet.
| + | ||
| ---- | ||
| claims: | ||
| id: <4> |
| ---- | ||
| + | ||
| .. At least one claim to use as the user's identity. The standard identity claim is `sub`. |
There was a problem hiding this comment.
sentence fragment, not connected to the line in the example by a footnote reference
| endif::[] | ||
|
|
||
| [[open_id_connect_full]] | ||
| *Configuring Full Master Configuration* |
There was a problem hiding this comment.
this is rendering in a monospace font, which looks really weird to me for a section header
| . Configure which link:http://openid.net/specs/openid-connect-core-1_0.html#StandardClaims[claims] to use: | ||
| + | ||
| ---- | ||
| claims: |
| + | ||
| ---- | ||
| claims: | ||
| id: <4> |
| ---- | ||
| + | ||
| .. At least one claim to use as the user's identity. The standard identity claim is `sub`. |
There was a problem hiding this comment.
and lack of number correlation with example
| When adding or changing identity providers, you can map identities from the new | ||
| provider to existing users by setting the `*mappingMethod*` parameter to | ||
| `*add*`. | ||
|
|
||
| After saving changes to the master configuration file, you must restart the {product-title} services for the changes to take effect: |
There was a problem hiding this comment.
This is true for every item in the master config file. Do we really need to repeat this after every section on every page?
There was a problem hiding this comment.
Good question. I am not a typical user and had trouble with setting an identity because I did not restart, as the step was not in the docs. Is this something our typical users would know to do? I was asked to make that requirement "more explicit" on the page.
I was thinking that the user might jump right to the section he wants, say HTPasswd, and not see the restart requirement if not right there in the procedure.
It does seem redundant, but only when reading through all of the provider types.
There was a problem hiding this comment.
restarting a server after changing config is typical, and I would expect cluster operators to know that. repeating restart instructions after every example that references changing the master config adds noise, imo.
| ---- | ||
| ... | ||
| oauthConfig: | ||
| identityProviders: | ||
| - name: htpasswd_auth | ||
| challenge: true | ||
| login: false | ||
| mappingMethod: "claim" | ||
| login: false <1> |
There was a problem hiding this comment.
remove this footnote number, only the mappingMethod is important in this section
| . Set the `mappingMethod`parameter as needed. | ||
| + | ||
| * `claim`. The default value. Provisions a user with the identity's preferred | ||
| user name. OAuth fails if a user with that user name is already mapped to another |
|
did a quick pass. there were enough typos and formatting issues introduced that it deserves a review from a doc team member as well |
|
@liggitt What if, for the restart, we do something like: See the Allow All section. |
|
@adellape @bmcelvee PTAL. I reorganized the information in this topic to create a better flow, particularly in the Identity Providers section. I gave the sections an "Edit the master config, example config, restart services" flow and moved additional information after the restart, where appropriate. |
|
@mburke5678 I haven't gone through the changes in this PR yet, but I would agree with it probably being time to break up this topic into a subdirectory of multiple topics, kinda like the "Configuring Persistent Storage" topics here: https://docs.openshift.com/container-platform/3.5/install_config/persistent_storage/ |
|
@adellape Can I ask you to take a look at the changes I have made to make sure they look OK by tech writing and RH doc perspective before I ask if @liggitt could look to make sure I didn't make any changes that affect things from a technical perspective? |
mburke5678
force-pushed
the
mburke-BZ-1442252
branch
2 times, most recently
from
0545b2b to
228663b
Compare
_topic_map.yml
Outdated
| Topics: | ||
| - Name: Overview | ||
| File: index | ||
| - Name: Configuring Allow All Authentication |
There was a problem hiding this comment.
I think all of these "Configuring"s could be dropped at the beginning of the various identity provider topics, just to clean up how it looks in the left-nav. But leave "Configuring" in the title of the .adocs themselves.
| :toc-title: | ||
| :prewrap!: | ||
|
|
||
|
|
There was a problem hiding this comment.
For all of these topics that don't have multiple subtopics in them and therefore no need for a TOC, you can add the following here to make the spacing look a little better at the top of the page:
{nbsp} +
Or maybe just go ahead and add the toc::[] anyway as well, in case later some subtopics are added (the TOC won't render until there actually are subtopics):
toc::[]
{nbsp} +
|
|
||
| To configure the Allow All identity provider: | ||
|
|
||
| //tag::configuring_authentication_common_steps1[] |
| :prewrap!: | ||
|
|
||
|
|
||
| Setting the *AllowAllPasswordIdentityProvider* in the `*identityProviders*` stanza allows any non-empty user name and password to log in. This is the default |
There was a problem hiding this comment.
Throughout, any place where we still have backtick+asterisk like this, while you're here might as well simplify them down to just backticks (since we updated our guidelines to no longer do the former). The single asterisks (like *AllowAllPasswordIdentityProvider*) could also just be backticks now too.
| |=== | ||
| //end::configuring_authentication_common_steps2[] | ||
|
|
||
| . Specify the following values to configure the allow all provider: |
| [[advanced-install-configuring-oauth]] | ||
| === Configuring OAuth | ||
|
|
||
| xref:../architecture/infrastructure_components/kubernetes_infrastructure.adoc#master[master] |
There was a problem hiding this comment.
Missing a word at the front here.
mburke5678
force-pushed
the
mburke-BZ-1442252
branch
from
7f9ddd5 to
af81f91
Compare
mburke5678
force-pushed
the
mburke-BZ-1442252
branch
from
e34b5f7 to
be5621b
Compare
mburke5678
changed the title
mburke5678
force-pushed
the
mburke-BZ-1442252
branch
from
d05917d to
ac81831
Compare
|
@liggitt Can you please take another look to make sure I made your changes as expected? |
Unfortunately, I can't track the content changes among the topic split |
mburke5678
changed the title
mburke5678
force-pushed
the
mburke-BZ-1442252
branch
from
c901dda to
0855621
Compare
ncbaratta
added
the
peer-review-done
|
@mburke5678 - not sure what the status of this is. Did you want to close it and create a new PR once 3.7 is done? |
mburke5678
force-pushed
the
mburke-BZ-1442252
branch
from
0855621 to
dccec32
Compare
openshift-ci-robot
added
the
size/XXL
mburke5678
force-pushed
the
mburke-BZ-1442252
branch
from
85c4466 to
e90c408
Compare
openshift-bot
added
the
needs-rebase
|
@mburke5678: PR needs rebase. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
Thanks @mburke5678 👋 |
https://bugzilla.redhat.com/show_bug.cgi?id=1446656