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.

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

feat: enable swagger-ui token #267

Closed
wants to merge 4 commits into from

Conversation

@jannyHou
Copy link
Contributor

jannyHou commented Sep 9, 2019

The spike PR for strongloop/loopback-next#2027
Details are explained in the readme file.

@jannyHou jannyHou force-pushed the spike/swagger-ui-token branch from 6fe8163 to 9c4a77c Sep 9, 2019
@dhmlau dhmlau referenced this pull request Sep 9, 2019
3 of 3 tasks complete
@hacksparrow

This comment has been minimized.

Copy link
Member

hacksparrow commented Sep 10, 2019

With the limitations and the next plans explained in the docs, this is looking good. Excellent job @jannyHou 👍

Copy link
Member

bajtos left a comment

Great work, @jannyHou. I especially like the screenshot you are adding to README 👍

I'd like to discuss the implementation details though, please see my second comment below.

README.md Outdated
`swagger-ui` module is built with authorization component, which will show up by
adding the security schema and operation security spec in the OpenAPI spec.

You can check the swagger

This comment has been minimized.

Copy link
@bajtos

bajtos Sep 10, 2019

Member
Suggested change
You can check the swagger
You can check the OpenAPI
packages/shopping/src/index.ts Outdated Show resolved Hide resolved
Signed-off-by: jannyHou <juehou@ca.ibm.com>
@jannyHou jannyHou force-pushed the spike/swagger-ui-token branch from e106cbe to ee75b64 Sep 12, 2019
...SECURITY_SCHEMA_SPEC,
},
security: SECURITY_SPEC,
}),

This comment has been minimized.

Copy link
@bajtos

bajtos Sep 13, 2019

Member

I believe it should be fine to call this.api() just with Security parts, because the code gathering spec from controllers & routes will merge that new spec with the object supplied to this.api().

Have you tried to call this.api() from the constructor?

export class ShoppingApplication extends BootMixin(
  ServiceMixin(RepositoryMixin(RestApplication)),
) {
  constructor(options?: ApplicationConfig) {
    super(options);

    this.api({
      components: {
          ...SECURITY_SCHEMA_SPEC,
      },
      security: SECURITY_SPEC,
  });

  // etc.
}

In my opinion, the security spec does not depend on the app being started, therefore it should be available even before app.start() is called. Thoughts?

This comment has been minimized.

Copy link
@bajtos

bajtos Sep 13, 2019

Member

Should we perhaps add a story to improve app.api() to intelligently merge the new spec with any spec provided by previous calls of app.api()?

This comment has been minimized.

Copy link
@jannyHou

jannyHou Sep 13, 2019

Author Contributor

@bajtos Now app.api() takes in an entire spec in type OpenAPISpec, not Partial<OpenAPISpec>, that's why it's not possible to merge a partial spec into the generated server's spec.

Do we want to change its behaviour to take in a partial OpenAPI spec? If so I can create a story for it :), see comment strongloop/loopback-next#2027 (comment)

This comment has been minimized.

Copy link
@bajtos

bajtos Sep 16, 2019

Member

Well, what are the required fields? Isn't it just a paths object that can be empty?

this.api({
  components: {
    ...SECURITY_SCHEMA_SPEC,
  },
  security: SECURITY_SPEC,
  paths: {
    // will be filled by LoopBack from controller metadata
  }
});

This comment has been minimized.

Copy link
@jannyHou

jannyHou Sep 16, 2019

Author Contributor

@bajtos Not really, the problem is the openapi and info fields, they are required and especially the info object, you cannot provide an empty value like '' or {}.

See my last commit, to call this.api(), I have to provide valid value for openapi and info, but the rest server doesn't overwrite them later.

The good spec filled by rest server:
Screen Shot 2019-09-16 at 11 04 53 AM

The one generated by calling this.api() with "empty" openapi and info fields:

Screen Shot 2019-09-16 at 11 08 58 AM

We can improve this by defining and checking the empty value of openapi and info, like

// in rest server
if (spec.openapi == '') {// generate openapi field }
if (spec.info.title == '' && spec.info.version == '') {// generate info field}

Thought? If this is the right direction for improvement I can create an issue (or just submit a PR)

This comment has been minimized.

Copy link
@bajtos

bajtos Sep 16, 2019

Member

I see, thank you for explaining the details!

I think that ideally, I'd like:

  • app.api() require fields like info to force the user to provide these required fields. The current implementation is good for that 👍
  • Have a way for extensions to contribute additional OpenAPI metadata. This could be a new method in Application or RestServer, but we can also use binding-based approach where the app collects OpenAPI metadata from bindings with a given tag (or other metadata-based specification). I think this is out of scope of your current work (both on the spike and on enabling auth in swagger-ui), so let's create a spike issue to research different options.

As for this spike PR, I think the current proposal (as shown in the committed code) is good enough and no further changes are necessary 👍

/cc @raymondfeng

This comment has been minimized.

Copy link
@raymondfeng

raymondfeng Sep 16, 2019

Member

We're already exploring the idea to create an extension point to enhance the generated OpenAPI spec so that the application or other extensions can add code to transform the spec.

@@ -83,6 +95,7 @@ export class UserController {
}

@get('/users/{userId}', {
security: SECURITY_SPEC_OPERATION,

This comment has been minimized.

Copy link
@bajtos

bajtos Sep 13, 2019

Member

IIUC, this is configuring basicAuth schema. Is that intentional? I thought that we use bearerAuth schema everywhere. Or is this just for the spike?

This comment has been minimized.

Copy link
@jannyHou

jannyHou Sep 13, 2019

Author Contributor

@bajtos Oh this is just a demo, for spike only, I just want to verify that a operation level security policy(which is different than the global bearerAuth) works.

I thought that we use bearerAuth schema everywhere

Exactly 💯

Copy link

agnes512 left a comment

I don't know much about the implement part. But the README writeup is pretty straightforward~! Just a few comments.

README.md Outdated
![set-token](/imgs/set-token.png)

Paste the token you just copied in the field, then click "Authorize". The token
will be be hidden:

This comment has been minimized.

Copy link
@agnes512

agnes512 Sep 13, 2019

-> will be hidden

README.md Outdated
}
```
The "Authorize" dialog will show up your new entry as:

This comment has been minimized.

Copy link
@agnes512

agnes512 Sep 13, 2019

your new entry -> the new added entry better?

Signed-off-by: jannyHou <juehou@ca.ibm.com>
@jannyHou jannyHou force-pushed the spike/swagger-ui-token branch from ee75b64 to 6283c6b Sep 13, 2019
@bajtos
bajtos approved these changes Sep 16, 2019
Copy link
Member

bajtos left a comment

LGTM with a small comment to consider.

@@ -47,6 +48,13 @@ export class ShoppingApplication extends BootMixin(
) {
constructor(options?: ApplicationConfig) {
super(options);
this.api({
openapi: '3.0.0',
info: {title: '', version: ''},

This comment has been minimized.

Copy link
@bajtos

bajtos Sep 16, 2019

Member

Can you fill title and version with something more meaningful please? Personally, I'd use metadata from package.json.

import * as package from '../package.json';
// ...

info: {title: package.name, version: package.version}

This comment has been minimized.

Copy link
@jannyHou

jannyHou Sep 16, 2019

Author Contributor

updated 👌

@raymondfeng

This comment has been minimized.

Copy link
Member

raymondfeng commented Sep 16, 2019

@jannyHou Please fix the code-lint issue.

Signed-off-by: jannyHou <juehou@ca.ibm.com>
@jannyHou jannyHou force-pushed the spike/swagger-ui-token branch from 434a7d2 to 9e0bd5f Sep 16, 2019
@jannyHou

This comment has been minimized.

Copy link
Contributor Author

jannyHou commented Sep 16, 2019

Follow up stories see strongloop/loopback-next#2027 (comment)

@jannyHou jannyHou referenced this pull request Oct 1, 2019
0 of 3 tasks complete
this.api({
openapi: '3.0.0',
info: {title: pkg.name, version: pkg.version},
paths: {},

This comment has been minimized.

Copy link
@jannyHou

jannyHou Oct 8, 2019

Author Contributor

servers: [{url: '/'}]

This comment has been minimized.

Copy link
@jannyHou
@jannyHou

This comment has been minimized.

Copy link
Contributor Author

jannyHou commented Oct 22, 2019

Closed as the tutorial is added :)

@jannyHou jannyHou closed this Oct 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.