search

FusionAuth / fusionauth-client-builder

The FusionAuth client library builder
https://fusionauth.io/
Apache License 2.0
6 stars 24 forks source link

Fix missing endpoints from OpenAPI spec #68

Closed vcampitelli closed 1 year ago

vcampitelli commented 1 year ago

We are missing some endpoints that don't have parameters in the query string because we only call build_path(uri, json, paths, true, options) inside if json["params"].

Here are the missing endpoints:

DELETE /api/reactor
GET /api/connector
GET /api/consent
GET /api/entity/type
GET /api/form
GET /api/form/field
GET /api/group
GET /api/integration
GET /api/key
GET /api/messenger
GET /api/reactor
GET /api/reactor/metrics
GET /api/report/totals
GET /api/system-configuration
GET /api/system/reindex
GET /api/system/version
GET /api/tenant
GET /api/tenant/password-validation-rules
GET /api/theme
GET /.well-known/jwks.json
GET /.well-known/openid-configuration
PUT /api/entity/search
PUT /api/reactor
PUT /api/user/search
mooreds commented 1 year ago

@vcampitelli this fails to create a valid java client. If you build the openapi.yaml file, and then run 

npx @openapitools/openapi-generator-cli generate -i openapi.yaml  -g java -o java

You get this error message:

Exception in thread "main" org.openapitools.codegen.SpecValidationException: There were issues with the specification. The option can be disabled via validateSpec (Maven/Gradle) or --skip-validate-spec (CLI).
 | Error count: 1, Warning count: 110
Errors: 
    -attribute paths.'/api/identity-provider'(get).operationId is repeated
Warnings: 
    -attribute paths.'/api/identity-provider'(get).operationId is repeated

    at org.openapitools.codegen.config.CodegenConfigurator.toContext(CodegenConfigurator.java:620)
    at org.openapitools.codegen.config.CodegenConfigurator.toClientOptInput(CodegenConfigurator.java:647)
    at org.openapitools.codegen.cmd.Generate.execute(Generate.java:479)
    at org.openapitools.codegen.cmd.OpenApiGeneratorCommand.run(OpenApiGeneratorCommand.java:32)
    at org.openapitools.codegen.OpenAPIGenerator.main(OpenAPIGenerator.java:66)

Doesn't happen with the current openapi.yaml generated off of the script on master. Please fix that.

mooreds commented 1 year ago

@vcampitelli any feedback or qs here?

vcampitelli commented 1 year ago

@mooreds I've pushed a check for this specific endpoint and I can now build the Java client using the openapi-generator-cli command you sent.

With this PR, I've seen some differences to our current spec. It seems that we haven't generated it in a while. In order to make it easier to compare them both, I had to rearrange some endpoints/methods/parameters, as a lot of them were out of order for some reason (e.g. the descriptions of merged requests were different).

So, openapi-1.46-formatted.yaml.txt and openapi-pr-formatted.yaml.txt are "rearranged" versions of our current spec and the one this PR built.

mooreds commented 1 year ago

@vcampitelli

vcampitelli commented 1 year ago

I'm a bit worried about "JSONWebKey" which converted a "y" to a 1 and an "n" to a 0. That seems like some kind of translation error.

I think there's something wrong with the script I created to rearrange parameters. But the replacement happened to the current 1.46 spec "rearranged" file, so please ignore it. In openapi-pr-formatted.yaml.txt, which would be the "rearranged" version of the spec generated by this PR, parameters "y" and "n" are there. The only change there is that they are actually enclosed by double quotes ("y" and "n"), but I didn't change anything in the ruby script regarding this. Actually, when generating a spec with the master branch, they also have double quotes, which confirms my idea that we haven't generated a new spec in a while (please see comment in the end of this reply).

Note: just to make sure, here is the actual unformatted openapi.yaml file generated by this PR.

Looks like there is at least one missing operation: retrieveEmailTemplate is present in the 1.46 one, but not in the PR one. This pulls all the email templates, and doesn't require an id. That seems problematic.

The script transformed it into a retrieveEmail operation. We would have to do something similar to what is being done here. I think we should improve this if to handle more scenarios.

It seems that the bug that this PR fixes is actually masquerading this scenario. If you check here in the current spec, the description says "Retrieves the email template for the given Id" but there are any Ids in the URI or in the parameters, which would actually be covered by retrieveEmailTemplateWithId in here.

src/main/api/generateTwoFactorSecret.json src/main/api/generateTwoFactorSecretUsingJWT.json collide and have different authorization values (one allows a JWT)

It seems another case of this bug actually masquerading this scenario. If you check here in the current spec, we only have one request (there is no OR in the description and only one operation Id) and no mention of the Authorization header. So, it is not merging similar requests and it will maintain the generateTwoFactorSecretUsingJWTWithId operation Id from that second JSON file only. On the other hand, this PR fixed that bug and now the script sees two requests, merging them in a single one called retrieveTwoFactor (which would be the actual behavior of returning "retrieve" for GET endpoints and them using the 2nd part of the URI). This is one of the scenarios that I mentioned in the item above where we should improve the operation Id generation for merged requests.

To summarize all, it actually seems that we haven't generated a new spec in a while and things changed since then. I've generated the spec from the current master branch (see openapi-master.yaml.txt) and I see some differences there. Maybe we should first update it to make the comparison more simple?

We can also chat tomorrow so I can explain it better if you want me to.

vcampitelli commented 1 year ago

@mooreds here's what we discussed on our huddle earlier: I'm going to investigate why fixing this bug caused some strange operation Ids (e.g. retrieveEmail instead of retrieveEmailTemplate) and check why its description has two others merged in together ("Retrieves all the email templates. OR Retrieves the email template for the given Id. If you don't specify the id, this will return all of the email templates") even though we have a dedicated endpoint for the latter (retrieveEmailTemplateWithId), which is probably the root cause for the first problem.

vcampitelli commented 1 year ago

@mooreds after a lot of back and forth with this, here's another attempt.

I've reverted my initial logic to always add a path and I'm now "deferring" it to be done later by the script, because maybe we'll encounter another endpoint with the same URI but with optional parameters that can be merged.

I've also rewritten the part that generate operation Ids to now consider the 3rd path (e.g. /api/email/template) when it's not an parameter. With this, we're now generating both retrieveEmailTemplate and retrieveEmailTemplateWithId again, for instance.

Here's the generated spec: openapi-pr.yaml.txt. I've used the openapi-generator-cli and it successfully generated a Java client.

PS: I think we should need to fix how the current spec treats generateTwoFactorSecret and generateTwoFactorSecretUsingJWT, as we don't have anything regarding the special JWT in the Authorization header. The builder only cares about disabling security but it doesn't give any hint on how to use the JWT.

mooreds commented 1 year ago

This looks good to me. I like that we give more details and it's great we're picking up the missing GET api calls. I actually prefer the more precise operationIds: retrieveIdentityProviderLink vs retrieveIdentityProvider, deleteJwtRefresh vs deleteJwt

As discussed, please file an issue to address the API endpoints that accept JWTs. we'll need to tackle that later.

mooreds commented 1 year ago

For other reviewers, here's the diff between the openapi.yaml file generated on this branch vs on master:

10856c10856
<       operationId: retrieveIdentityProvider
---
>       operationId: retrieveIdentityProviderLink
11298c11298
<       operationId: deleteJwt
---
>       operationId: deleteJwtRefresh
13315c13315
<       operationId: retrieveReport
---
>       operationId: retrieveReportLogin
14603c14603
<       operationId: retrieveJwt
---
>       operationId: retrieveJwtPublicKey
16842a16843,16952
>   "/.well-known/openid-configuration":
>     get:
>       description: Returns the well known OpenID Configuration JSON document
>       operationId: retrieveOpenIdConfigurationWithId
>       security: []
>       parameters: []
>       responses:
>         '200':
>           description: Success
>           content:
>             application/json:
>               schema:
>                 "$ref": "#/components/schemas/OpenIdConfiguration"
>         default:
>           description: Error
>   "/api/reactor/metrics":
>     get:
>       description: Retrieves the FusionAuth Reactor metrics.
>       operationId: retrieveReactorMetricsWithId
>       parameters: []
>       responses:
>         '200':
>           description: Success
>           content:
>             application/json:
>               schema:
>                 "$ref": "#/components/schemas/ReactorMetricsResponse"
>         default:
>           description: Error
>   "/api/system/version":
>     get:
>       description: Retrieves the FusionAuth version string.
>       operationId: retrieveVersionWithId
>       parameters: []
>       responses:
>         '200':
>           description: Success
>           content:
>             application/json:
>               schema:
>                 "$ref": "#/components/schemas/VersionResponse"
>         default:
>           description: Error
>           content:
>             application/json:
>               schema:
>                 "$ref": "#/components/schemas/Errors"
>   "/.well-known/jwks.json":
>     get:
>       description: Returns public keys used by FusionAuth to cryptographically verify
>         JWTs using the JSON Web Key format.
>       operationId: retrieveJsonWebKeySetWithId
>       security: []
>       parameters: []
>       responses:
>         '200':
>           description: Success
>           content:
>             application/json:
>               schema:
>                 "$ref": "#/components/schemas/JWKSResponse"
>         default:
>           description: Error
>   "/api/report/totals":
>     get:
>       description: Retrieves the totals report. This contains all of the total counts
>         for each application and the global registration count.
>       operationId: retrieveTotalReportWithId
>       parameters: []
>       responses:
>         '200':
>           description: Success
>           content:
>             application/json:
>               schema:
>                 "$ref": "#/components/schemas/TotalsReportResponse"
>         default:
>           description: Error
>   "/api/tenant/password-validation-rules":
>     get:
>       description: Retrieves the password validation rules for a specific tenant.
>         This method requires a tenantId to be provided  through the use of a Tenant
>         scoped API key or an HTTP header X-FusionAuth-TenantId to specify the Tenant
>         Id.  This API does not require an API key.
>       operationId: retrievePasswordValidationRulesWithId
>       security: []
>       parameters: []
>       responses:
>         '200':
>           description: Success
>           content:
>             application/json:
>               schema:
>                 "$ref": "#/components/schemas/PasswordValidationRulesResponse"
>         default:
>           description: Error
>   "/api/key":
>     get:
>       description: Retrieves all the keys.
>       operationId: retrieveKeysWithId
>       parameters: []
>       responses:
>         '200':
>           description: Success
>           content:
>             application/json:
>               schema:
>                 "$ref": "#/components/schemas/KeyResponse"
>         default:
>           description: Error