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

Edits to security authentication mechanisms doc #34070

Merged

Conversation

michelle-purcell
Copy link
Contributor

@michelle-purcell michelle-purcell commented Jun 15, 2023

To enhance and apply style as per the Quarkus docs contributor style guide, mainly:

  • One sentence per line.
  • Converting <<>> to xrefs: to ensure they work downstream (asciidoctorj & other PV2 transform dependencies)
  • Fix table to work downstream (asciidoctorj & other PV2 transform dependencies)
  • Consistent capitalization in references to 'Bearer authentication' and the more generic bearer token term.

@sberyozkin - et al security folks - Do we need to add/update any of this content for the impending 3.2 release? You can add to this PR or tag me in your PRs and I'll help with any editing work that's needed. Thanks 😄

@michelle-purcell michelle-purcell added the triage/qe? Issue could use a quality focused review to further harden it label Jun 15, 2023
@quarkus-bot
Copy link

quarkus-bot bot commented Jun 15, 2023

/cc @mjurc (qe), @rsvoboda (qe)

@sberyozkin
Copy link
Member

Thanks @michelle-purcell I'll have a look tomorrow

@github-actions
Copy link

github-actions bot commented Jun 15, 2023

🙈 The PR is closed and the preview is expired.

@sberyozkin
Copy link
Member

Michelle, @michelle-purcell, very nice updates, thanks, I suggested only a few minor changes

@michelle-purcell
Copy link
Contributor Author

@sberyozkin - Thanks for your review and comments. I have fixed all of them 👍

@sberyozkin
Copy link
Member

Thanks @michelle-purcell, LGTM, one question I'd like to ask though first, do we really want to optimize from Bearer token authentication to Bearer authentication given that if we do it then I think we'll need to remove -token- from the corresponding file names and do more redirects to the website. I don't mind as long as users will find the new files without experiencing 404, but, Bearer token authentication is not that bad, tiny bit redundant but Bearer token is a well known concept.

What do you think ? I'm fine to go with Bearer authentication if you prefer, but I thought I'd double check

@michelle-purcell
Copy link
Contributor Author

@sberyozkin - Apologies if I'm stomping on the same ground again...

After researching other "official" sources, the official scheme name seems to be "Bearer authentication", however, there are lots of docs out there that refer to the same as "Bearer Token authentication" or "Bearer token authentication".

I suggest we use whatever the confirmed official term is for the "Bearer" authentication standard and be consistent throughout the docs, hence, I opted for:
Bearer authentication when referring to the name of this authentication mechanism or scheme.
bearer token when talking generically about bearer-type tokens.
A "Bearer token" is also the correct form too, when it makes sense to compare specific token types or make a statement about a token that is only specific to Bearer.
WDYT?

Regarding the filenames and IDs with ...bearer-token... in them, because token is a good keyword here, I think renaming the URL and filename is not necessary. Keeping token as a keyword in the filename and ID would be helpful for SEO.

I will do another pass at the other content to use the consistent terms above or whatever we agree on.

@sberyozkin
Copy link
Member

sberyozkin commented Jun 16, 2023

Hi @michelle-purcell

Thanks.

As far as the bearer token vs Bearer token is concerned, when we say something like Multiple tenants that can support bearer token or authorization code flow mechanisms - then the lower case does not quite work for me here, it is like we are talking about some individual tokens or authorization codes (it is not what you modified - I just see it in the text).

But, something like: quarkus-oidc requires an OpenID Connect provider such as Keycloak, which can verify the bearer tokens is fine - this is your PR's line - here we are talking about individual tokens.

I'd not be worried about some general guidelines in this context, or how someone else refers to them somewhere else, Bearer token, Authorization code flow are well known concepts.

I propose to use this guideline: if we talk about the individual cases of the token verification, we should use bearer token, but a general concept is Bearer token.

As far as the file names are concerned: if we think token is a good keyword, why move from Bearer token authentication to Bearer authentication in the actual text ? The former is certainly correct and more descriptive ?

Thanks

@michelle-purcell michelle-purcell force-pushed the SECURITY-DOC-AUTH-MECHANISMS branch from 2c57c89 to f3cf6c1 Compare June 19, 2023 09:12
@michelle-purcell
Copy link
Contributor Author

michelle-purcell commented Jun 19, 2023

@sberyozkin - Thanks for more context and clarification around this. We are close 🙏 I think...

So if we follow best practice to consistently use the capitalization and terminology of the 'thing' (proper noun) as defined by its creator/author/inventor, then in this case, then we should use Bearer authentication mechanism or Bearer authentication scheme to name the mechanism/scheme consistently.

Today we use a mix and match of both in Quarkus docs so readers could get confused that we are talking about 2 different things. Plus they could get translated differently and they diverge into 2 different things.

We need to decide and agree on which one to use.
If you think that referring to the Bearer authentication mechanism as 'Bearer token authentication mechanism' will resonate more with Quarkus developers/users and is a common reference to it throughout the industry then let's use that, but call it out in the most prominent topic on the subject. (I notice Red Hat and other popular security content sometimes use this too but the other way is just as popular).

I think the main point is that we are consistent throughout the docs.

I updated this topic as per above. If you agree, I'll fix other docs that contain 'Bearer token' content in later PRs too.

As for the filename bit...
'Bearer token' is perfectly correct when Bearer is used as an adjective. It's used a lot in the docs and therefore makes a good keyword for the filename. A filename plays a part also in SEO, so it's more than about matching a title, which is Quarkus contributing practice. But if we agree on above then the filename comment is resolved.

WDYT? Thanks so much for persevering with this one!

@sberyozkin
Copy link
Member

Hi Michelle @michelle-purcell,

Looks like we are in the agreement, Bearer token authentication, while a tiny bit more verbose, does not change the meaning of the shorter Bearer authentication but makes it a bit more explicit about the underlying credential type involved, and like you mentioned, having token included in the file name (but also across the text) should only improve the searchability.

FYI, Bearer authentication scheme (specifically, scheme) would usually be a technical reference to the content of the Authoriization header, the first word in Authorization header value identifies a scheme (Basic, Bearer, etc)...

Bearer token authentication looks like a good compromise to me, thanks

Thanks so much for persevering with this one!

You are too kind :-), it should be thanks to you for the unlimited patience when dealing with my review comments :-)

@sberyozkin sberyozkin self-requested a review June 19, 2023 13:26
@sberyozkin sberyozkin merged commit b0c63a3 into quarkusio:main Jun 19, 2023
@quarkus-bot quarkus-bot bot added this to the 3.2 - main milestone Jun 19, 2023
@michelle-purcell
Copy link
Contributor Author

Thanks @sberyozkin 👍

@rsvoboda
Copy link
Member

Hi @michelle-purcell. I reviewed the PR and have no objections to the changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/documentation triage/qe? Issue could use a quality focused review to further harden it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants