-
Notifications
You must be signed in to change notification settings - Fork 264
add swagger openIdConnect #2330
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
base: master
Are you sure you want to change the base?
add swagger openIdConnect #2330
Conversation
thjaeckle
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @hu-ahmed
As far as I see, this added OIDC integration will only work if there is an URL serving the environment data.
The Ditto UI however does not rely on such an endpoint (it is an optional thing to configure) - it falls back to local browser storage to load either a default or manually added/edited environments from this local storage.
I think it makes sense to "re-use" the approach of this environment-URL - in order to effectively be able to just have one endpoint for the Ditto UI and the Swagger UI.
The mechanism however must also work without having such an endpoint.
Could you have another look in that direction, please?
| if (!envs || typeof envs !== "object") { | ||
| return null; | ||
| } | ||
| const name = envName || Object.keys(envs)[0]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hu-ahmed the problem with this approach is:
If our environments contain multiple entries, here a random one gets picked.
I tried locally with the default Ditto environments (or which were still in my local storage) and it chose an environment which does not even have an oidc provider configured.
I think we should support that case if no envName is provided explicitly.
But then already in this method choose a oidc supporting provider. If we have multiple, we can choose the first one I would say. 👍
documentation/src/main/resources/openapi/sources/api-2-index.yml
Outdated
Show resolved
Hide resolved
| Google: | ||
| $ref: "./security/google.yml" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove
| await SwaggerOidc.initSwaggerUiWithOidc({ | ||
| specUrl: openApiPath, | ||
| openApiFileName: "ditto-api-2.yml", | ||
| oauth2RedirectPath: "/oauth2-redirect.html", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hu-ahmed are you sure that this works? I would assume this has to be /apidoc/oauth2-redirect.html?
I tried the flow and it tried to redirect me to http://localhost:8080/oauth2-redirect.html
| urls: [{url: "./openapi/ditto-api-2.yml", name:"Ditto API v2"}], | ||
| primaryName: "Ditto API v2", | ||
| openApiFileName: "ditto-api-2.yml", | ||
| oauth2RedirectPath: "/oauth2-redirect.html", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here - before the redirect uri was: oauth2RedirectUrl: window.location.origin + "./oauth2-redirect.html" - I don't think an absolute /oauth2-redirect.html works
| SwaggerUIBundle.plugins.DownloadUrl | ||
| ], | ||
| layout: "StandaloneLayout", | ||
| oauth2RedirectUrl: global.location.origin + (opts.oauth2RedirectPath || "/oauth2-redirect.html"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hu-ahmed ah, I understand .. you wanted to add the /apidoc path here ..
This seems however not to work.
For me this resolved to: http://localhost:8080 - the apidocs locally served at url http://localhost:8080/apidoc/
f110517 to
6c307e7
Compare
a9d79d5 to
7e7c9af
Compare
No description provided.