8000 13203 Adds documentation for Account API by Hronom · Pull Request #25974 · keycloak/keycloak · GitHub
[go: up one dir, main page]
Skip to content

13203 Adds documentation for Account API - #25974

Closed
Hronom wants to merge 3 commits into
keycloak:mainfrom
Hronom:account-openapi-addoption
Closed

13203 Adds documentation for Account API#25974
Hronom wants to merge 3 commits into
keycloak:mainfrom
Hronom:account-openapi-addoption

Conversation

@Hronom
@Hronom Hronom commented Jan 8, 2024
Copy link
Copy Markdown

Extend OpenAPI documentation with Account API.

This should help with this #13203 but there some nuances.

How-to check

Generate schema:

mvn clean install -DskipTests=true smallrye-open-api:generate-schema -P jboss-release -pl services

Check generated file in:

${project.build.directory}/apidocs-rest/swagger/apidocs/openapi.yaml

Nuances

There issue related to how API organized.
Specifically there sub resources here:

    @Path("/")
    public Object getAccountService() {

and here:

    @Path("{version : v\\d[0-9a-zA-Z_\\-]*}")
    @Produces(MediaType.APPLICATION_JSON)
    public Object getVersionedAccountRestService(final @PathParam("version") String version) {

During generation of OpenAPI plugin smallrye-open-api-maven-plugin not able to to get a clue about what Sub Resource will be returned by was methods.

So in order to provide at least something instead of nothing(for Account API) - there was added groups of fake methods in AccountLoader.

    /**
     * This needed to provide examples for Account API.
     * Here we return a specific sub resource, so OpenAPI generator able to scan it.
     */
    @Path("/account-api-cors-example")
    public CorsPreflightService getAccountApiCorsExample() {
        throw new ForbiddenException();
    }

    /**
     * This needed to provide examples for Account API.
     * Here we return a specific sub resource, so OpenAPI generator able to scan it.
     */
    @Path("/account-api-example")
    public AccountRestService getAccountApiExample() {
        throw new ForbiddenException();
    }

    /**
     * This needed to provide examples for Account API.
     * Here we return a specific sub resource, so OpenAPI generator able to scan it.
     */
    @Path("{version : v\\d[0-9a-zA-Z_\\-]*}/account-api-cors-example")
    @Produces(MediaType.APPLICATION_JSON)
    public CorsPreflightService getVersionedAccountApiCorsExample(final @PathParam("version") String version) {
        throw new ForbiddenException();
    }

    /**
     * This needed to provide examples for Account API.
     * Here we return a specific sub resource, so OpenAPI generator able to scan it.
     */
    @Path("{version : v\\d[0-9a-zA-Z_\\-]*}/account-api-example")
    @Produces(MediaType.APPLICATION_JSON)
    public AccountRestService getVersionedAccountApiExample(final @PathParam("version") String version) {
        throw new ForbiddenException();
    }

This allow smallrye-open-api-maven-plugin to generate OpenAPI spec. However it generates them under *-example paths.

To make it actual we need to do simple replace of /account-api-cors-example and /account-api-example to empty string. This helps developers to easily adopt generated OpenAPI for other services that need to use Account API.

@Hronom
Hronom requested a review from a team as a code owner January 8, 2024 02:00
@Hronom
Hronom commented Jan 8, 2024
Copy link
Copy Markdown
Author

cc @ahus1

I'm totally ok if maintainers edit this PR.

Merging this PR helps a lot to others who need to work with Account API e.g. create custom Account Console

@Hronom Hronom mentioned this pull request Jan 28, 2024
Closed
@ahus1 ahus1 self-assigned this Feb 7, 2024
thomasdarimont
thomasdarimont suggested changes Feb 7, 2024
View reviewed changes
@thomasdarimont thomasdarimont left a comment
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for your PR! Changes look good to me, just a few nitpicks :)

Comment thread services/src/main/java/org/keycloak/services/resources/account/AccountLoader.java Resolved
Comment thread services/src/main/java/org/keycloak/services/resources/account/AccountRestService.java Resolved
Comment thread services/src/main/java/org/keycloak/services/resources/account/AccountRestService.java Resolved
Comment thread services/src/main/java/org/keycloak/services/resources/account/AccountRestService.java Resolved
Comment thread services/src/main/java/org/keycloak/services/resources/account/AccountRestService.java Resolved
stianst
stianst requested changes Feb 8, 2024
View reviewed changes
@stianst stianst left a comment
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This would be great to get in, but it needs a bit of work.

We should not have a single docs for both account and admin I'd say, but rather have two separate docs.

The annotations here also need some more review and tuning.

For example this is not the right URL (it should be /realms/{realm}/account)
image

Another example is CORS preflight stuff shouldn't be listed I'd say:
image

If you're still wanting to work on this, I'd be happy to review it in more depth and can also work on the website side of things if you want.

@stianst stianst self-assigned this Feb 8, 2024
@Hronom
Hronom commented Mar 3, 2024
Copy link
Copy Markdown
Author

@thomasdarimont I'm ok if maintainers will do all changes that they think we need to do.
I wanna make sure that this PR will be not forgotten.

@stianst I wrote reasons in description about why it's done like this. I consider this ok solutions, since right now keycloak don't have any documentation for /accounts API's and it requested by many people. So instead of nothing I select something.

Maintainers, please adjust whatever you think to need to adjust. Let's not put this changes in forgotten mode.
I'm providing my verbal OK to do any changes in this PR, or feel free to create new one.

@Hronom
Hronom requested review from stianst and thomasdarimont March 3, 2024 17:05
@Hronom
13203 Adds documentation for Account API
e2237f1 
Signed-off-by: Yevhen Tienkaiev <hronom@gmail.com>
@Hronom
Hronom force-pushed the account-openapi-addoption branch from 47fc76c to e2237f1 Compare March 3, 2024 17:12
@stianst
stianst commented Mar 14, 2024
Copy link
Copy Markdown
Contributor

@Hronom I'm afraid at this point we don't have capacity to work on this PR, and as I mentioned it really should be a separate set of docs, as the use-case is completely different, it's a completely different API, and the admin API is already way to big and confusing on its own not to intermix it with account API. It should not be particularly hard to adjust your PR to do that, and as I mentioned before if you did that I'd be happy to help with doing the updates needed to get it publishes.

@ahus1 ahus1 removed their assignment Mar 14, 2024
@Hronom
Hronom commented Mar 16, 2024
Copy link
Copy Markdown
Author

@stianst you probably really out of context and didn't read description of the PR.
The way how Keycloack organize API there no possibility to make paths:

/accounts

And I would say this is the reasons why Accounts API still don't have documentation, because whenever maintainers of Keycloack tried to do it the hit same wall as I and drop work on it.

There was done CRITICAL architecture mistake in the beginning when Accounts API was created.
Probably persons who created that was not care too much about it.
Unfortunately the documentation libraries can't support generic API's.

@stianst stianst closed this Mar 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@thomasdarimont thomasdarimont Awaiting requested review from thomasdarimont
@stianst stianst Awaiting requested review from stianst

Assignees

@stianst stianst

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@Hronom @stianst @thomasdarimont @ahus1
0