Docs - Add missing steps to Basic authentication how-to #37121
Conversation
quarkus-bot
bot
added
area/docstyle
|
@sberyozkin, @rsvoboda - Please review and approve these doc updates for both the |
michelle-purcell
force-pushed
the
doc-qe-fix-security-ba
branch
from
6e76eab to
082ab4c
Compare
docs/src/main/asciidoc/security-basic-authentication-howto.adoc
Outdated
Show resolved
Hide resolved
| + | ||
| [source,properties] | ||
| ---- | ||
| security.users.embedded.enabled=true |
There was a problem hiding this comment.
Sorry Michelle but I'm not sure we need to highlight this property at all, I don't recall ever using it for tests
There was a problem hiding this comment.
It is enabled by default I guess, so may be below it works but if if it is enabled by default then it is 1 extra property to configure
There was a problem hiding this comment.
Sorry, I use it here https://github.com/quarkusio/quarkus/blob/main/integration-tests/csrf-reactive/src/main/resources/application.properties#L6 :-) but I'd not highlight in isolation, below it is fine
There was a problem hiding this comment.
Sorry for the noise, if you'd like to highlight then it is fine, but IMHO it it should all be again done below, otherwise it reads as if it can be used outside of the testing, which it technically can be but we don't recommend it
There was a problem hiding this comment.
The default is false - see https://quarkus.io/guides/all-config#quarkus-elytron-security-properties-file_quarkus.security.users.embedded.enabled
I think it's fine to have the property just in the example bellow. IMHO no need to highlight this property in this separate part
There was a problem hiding this comment.
+1 to @rsvoboda's suggestion.
There was a problem hiding this comment.
@sberyozkin , @rsvoboda , @jsmrcka
Thank you for your input and reviews on this bit too.
I think we have to keep in mind that the person reading this introductory-level how-to topic is likely to be new to Quarkus and is therefore likely 'playing' in a test environment. So while we don't recommend it for production, +1 to keeping it in and being consistent elsewhere.
I made some adjustments to the procedure to highlight that some configuration steps are purely for testing purposes. How does this look now?
There was a problem hiding this comment.
@rsvoboda & @jsmrcka - I also propose that we diverge in the product version a little and either:
- Add a support note to the prereq section to highlight the extensions that we don't support
- Remove the extension altogether from the list in the product version
( @tqvarnst @inoxx03 & @[email protected] FYI)
There was a problem hiding this comment.
prereq section should be sufficient, otherwise we would need to drop all 3 mentioned extensions :)
docs/src/main/asciidoc/security-basic-authentication-howto.adoc
Outdated
Show resolved
Hide resolved
| + | ||
| [source,properties] | ||
| ---- | ||
| security.users.embedded.enabled=true |
There was a problem hiding this comment.
The default is false - see https://quarkus.io/guides/all-config#quarkus-elytron-security-properties-file_quarkus.security.users.embedded.enabled
I think it's fine to have the property just in the example bellow. IMHO no need to highlight this property in this separate part
michelle-purcell
force-pushed
the
doc-qe-fix-security-ba
branch
from
082ab4c to
f83639d
Compare
michelle-purcell
force-pushed
the
doc-qe-fix-security-ba
branch
from
f83639d to
b3f9673
Compare
|
Thanks Michelle, I was a bit tired yesterday so forgot I was using that property everywhere :-) |
quarkus-bot
bot
added
area/dependencies
michelle-purcell
force-pushed
the
doc-qe-fix-security-ba
branch
from
b3f9673 to
898d136
Compare
|
@rsvoboda I believe your change request can be dropped now ? Please merge once you are happy with the updates |
Thanks for the updates, approved |
| * You have installed at least one extension that provides an `IdentityProvider` based on username and password. | ||
| For example: | ||
|
|
||
| ** xref:security-basic-authentication-tutorialsecurity-jpa.adoc[Quarkus Security Jakarta Persistence extensions (`security-jpa` or `security-jpa-reactive`)] |
There was a problem hiding this comment.
This link is probably broken. It should be IMO xref:security-jpa.adoc
There was a problem hiding this comment.
The link is not broken. Question is, if the author wants to have that rendered Name for the xref link.
There was a problem hiding this comment.
@sberyozkin , @jsmrcka , @rsvoboda , @MichalMaler - Thanks all for reviewing 💚
I fixed the broken link! I couldn't decide whether the tutorial or concept for JPA worked best so I accidently combined both. Apologies for the churn. It is now fixed. Thanks!
|
|
michelle-purcell
force-pushed
the
doc-qe-fix-security-ba
branch
from
898d136 to
bc6a3ce
Compare
|
🎊 PR Preview 64d9892 has been successfully built and deployed to https://quarkus-pr-main-37121-preview.surge.sh/version/main/guides/ |
|
Just an little nitpick for the |
|
@michelle-purcell , Michelle, something did not work with the rebase , 5 commits are shown |
michelle-purcell
force-pushed
the
doc-qe-fix-security-ba
branch
2 times, most recently
from
ed15fb7 to
12ae479
Compare
SME and QE fixes fix bullet fix bad link Fix xref
michelle-purcell
force-pushed
the
doc-qe-fix-security-ba
branch
from
12ae479 to
e60c3d5
Compare
|
@sberyozkin - Thanks so much for flagging. Sorry b2b in mtgs... My git world is all caught up now (I hope!). Think we are ready to merge?! |
|
Thanks @rsvoboda 👍 |
The QE review of 3.2.8 product docs highlighted some missing content that would be helpful for folks unfamiliar with enabling Basic authentication in Quarkus.
This PR:
(quarkus.security.users.embedded.enabled=true), which is needed to enable the security via the embedded realm.👀 @jsmrcka & @sberyozkin (REF QUARKUS-3651)