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

Documentation fixes for Authorization of web endpoints guide #37562

Merged
merged 1 commit into from
Dec 6, 2023
Merged

Documentation fixes for Authorization of web endpoints guide #37562

merged 1 commit into from
Dec 6, 2023

Conversation

michalvavrik
Copy link
Member

closes: #37540

@quarkus-bot quarkus-bot bot added area/docstyle issues related for manual docstyle review area/documentation labels Dec 6, 2023
@michalvavrik
Copy link
Member Author

@jedla97 I'll ask Sergey to review little later, but could you have a look, since you raised these points, please?

@github-actions GitHub Actions
Copy link

github-actions bot commented Dec 6, 2023 •
edited
Loading

🙈 The PR is closed and the preview is expired.

jedla97
jedla97 reviewed Dec 6, 2023
View reviewed changes
Copy link
Contributor

@jedla97 jedla97 left a comment

Choose a reason for hiding this comment

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

Feels good to me. just one comment. It's not forced from my side. We can leave it as it is.

docs/src/main/asciidoc/security-authorize-web-endpoints-reference.adoc Outdated
<1> Grants the permission `media-library`, which permits `read`, `write`, and `list` actions.
Because `MediaLibrary` is the `TvLibrary` class parent, a user with the `admin` role is also permitted to modify `TvLibrary`.

TIP: The `/library/*` path can be tested from a Keycloak provider DEV UI page, because user `alice` created automatically
by the xref:security-openid-connect-dev-services.adoc[Dev Services for Keycloak] has an `admin` role.
Copy link
Contributor

Choose a reason for hiding this comment

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

As the paths are using PUT method shouldn't be better link to section with testing with Swagger UI or GraphQL UI?
It will be looking like this xref:security-openid-connect-dev-services.adoc#test-with-swagger-graphql

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, that's definitely true, but not every application has Swagger UI, while every application with the OIDC extension will have Keycloak provider that links to Swagger UI. I also think that security-openid-connect-dev-services contains sections for other testing options (depending if it is web app or service app), so I'd keep it.

@michalvavrik
Copy link
Member Author

Thanks @jedla97 , the PR might look very different after Sergey review, but I appreciate your feedback and checks.

@jedla97
Copy link
Contributor

jedla97 commented Dec 6, 2023

@michalvavrik Looking at it once again can you add in LibraryService import of LibraryPermission.Library it should be import org.acme.library.LibraryPermission.Library;
My bad that I miss it.

@michalvavrik
Copy link
Member Author

Looking at it once again can you add in LibraryService import of LibraryPermission.Library it should be import org.acme.library.LibraryPermission.Library;
My bad that I miss it.

I don't think it should be necessary as I intentionally put them into same package. Can you re-check, please? Maybe I'm missing something :-/

@michalvavrik michalvavrik force-pushed the feature/docs-fix-perm-findings branch from 46a277c to d4f52b8 Compare December 6, 2023 12:54
@sberyozkin
Copy link
Member

Thanks @michalvavrik for taking care of it

@michalvavrik michalvavrik force-pushed the feature/docs-fix-perm-findings branch from d4f52b8 to a2b0e78 Compare December 6, 2023 12:58
@michalvavrik michalvavrik force-pushed the feature/docs-fix-perm-findings branch from a2b0e78 to 11d2e78 Compare December 6, 2023 13:11
@michalvavrik
Copy link
Member Author

@michalvavrik Looking at it once again can you add in LibraryService import of LibraryPermission.Library it should be import org.acme.library.LibraryPermission.Library;
My bad that I miss it.

you are right, it would need to be LibraryPermission.Library, which looks too verbous, I've added import org.acme.library.LibraryPermission.Library; to both svc and resource.

@sberyozkin sberyozkin self-requested a review December 6, 2023 13:32
@sberyozkin
Copy link
Member

Thanks @michalvavrik @jedla97

@sberyozkin sberyozkin merged commit 09e5f4c into quarkusio:main Dec 6, 2023
5 checks passed
@quarkus-bot quarkus-bot bot added this to the 3.7 - main milestone Dec 6, 2023
@michalvavrik michalvavrik deleted the feature/docs-fix-perm-findings branch December 6, 2023 16:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jedla97 jedla97 jedla97 left review comments

@sberyozkin sberyozkin sberyozkin approved these changes

Assignees
No one assigned
Labels
area/docstyle issues related for manual docstyle review area/documentation kind/bugfix
Projects
Quarkus Documentation
Status: Done
Milestone
3.7.0.CR1
Development

Successfully merging this pull request may close these issues.

Docs: security-authorize-web-endpoints-reference Guide
3 participants
@michalvavrik @jedla97 @sberyozkin