feat(rest): add OAS Enhancer service #4554

dougal83 · 2020-02-03T16:05:10Z

Add OpenAPI enhancer service in @loopback/rest

Implements #4365

Checklist

npm test passes on your machine
New tests added or existing tests modified to cover all changes
Code conforms with the style guide
API Documentation in code was updated
Documentation in /docs/site was updated

packages/openapi-v3/src/__tests__/unit/enhancers/fixtures/application.ts

examples/lb3-application/src/__tests__/acceptance/lb3app.acceptance.ts

raymondfeng · 2020-02-03T18:35:27Z

packages/openapi-v3/src/enhancers/extensions/security.spec.extension.ts

 * `spec.component.securitySchemes`
 */
 @bind(asSpecEnhancer)
 export class SecuritySpecEnhancer implements OASEnhancer {
  name = 'security';

  modifySpec(spec: OpenApiSpec): OpenApiSpec {
-    const patchSpec = {components: {securitySchemes: SECURITY_SCHEME_SPEC}};
+    if (spec?.components?.securitySchemes?.jwt) return spec;


I would expect the security spec enhancers to be contributed by authentication strategies instead of being hard-coded in @loopback/rest module. This code was added as part of the test fixture intentionally.

Subject to the story(#4386) it is a default and I've opted to not override an existing item. @jannyHou What to do?

@raymondfeng I think @dougal83 is adding the default authorizor button in explorer.

@raymondfeng I think we agree to not bind a built-in jwt strategy into LB4 app, so the only thing we can provide the the security spec, as what @dougal83 adds here.

How about introduce an option useDefaultSecuritySpec, sets to true by default so that our explorer shows the button. If people provide their own auth strategy & security spec enhancer , they can turn off the option.

I would expect the security spec enhancers to be contributed by authentication strategies instead of being hard-coded in @loopback/rest module.

+1

How about introduce an option useDefaultSecuritySpec, sets to true by default so that our explorer shows the button. If people provide their own auth strategy & security spec enhancer , they can turn off the option.

As I mentioned in my other comment, it's my understanding that the REST component does not provide any authentication mechanism out of the box, there is no "default security". Therefore the option useDefaultSecuritySpec does not make sense to me.

IMO, the security spec should be contributed by @loopback/authentication, because that's the extension adding support for bearer tokens. The enhancer should be clever enough to handle the case when there are multiple authentication extensions and/or security specs involved (e.g. Bearer token, HTTP Basic/Digest auth, etc.) and preserve those additional security specs.

@raymondfeng I think @dougal83 is adding the default authorizor button in explorer.

@jannyHou #4386 Having read the issue/story properly just now... I had made a big mistake on my understanding... I thought that is was generally adding a default to the rest server! 🤦‍♂ To make amends I guess I'll have to look into explorer and making the change as per the story!

jannyHou · 2020-02-07T20:15:32Z

packages/rest/src/rest.server.ts

@@ -717,6 +742,10 @@ export class RestServer extends Context implements Server, HttpServerLike {
      spec = this.updateSpecFromRequest(spec, requestContext);
    }

+    // Apply OAS enhancers to the OpenAPI specification
+    this.OASEnhancer.spec = spec;
+    spec = await this.OASEnhancer.applyEnhancerByName('security');


I would suggest rename it to be lb-default-security

jannyHou

@dougal83 Thank you so much for taking care of the spec enhancer story!

The way how the enhancer service gets added looks reasonable to me. A few minor suggestions.

When created the story, I didn't realize applying the enhancer would turn the getApiSpec into an async function. I tend to keep it sync and apply the default security enhancer somewhere else, like _serveOpenApiSpec. And I would like to hear more opinions from @bajtos @raymondfeng @hacksparrow who's more familiar with the rest module regarding where to apply the default security enhancer 🙇‍♀ .

jannyHou · 2020-02-07T20:17:35Z

packages/openapi-v3/src/enhancers/extensions/security.spec.extension.ts

    type: 'http',
    scheme: 'bearer',
    bearerFormat: 'JWT',
  },
 };

 /**
- * A spec enhancer to add bearer token OpenAPI security entry to
+ * A spec enhancer to add jwt bearer token OpenAPI security entry to
 * `spec.component.securitySchemes`
 */
 @bind(asSpecEnhancer)
 export class SecuritySpecEnhancer implements OASEnhancer {
  name = 'security';


same as https://github.com/strongloop/loopback-next/pull/4554/files#r376586868, I would rename it to lb-default-security

bajtos

Thank you @dougal83 for contributing the pull request.

I feel the proposed implementation is going in a wrong direction at high level, see my comment below.

bajtos · 2020-02-17T14:13:03Z

packages/rest/src/rest.server.ts

@@ -717,6 +742,10 @@ export class RestServer extends Context implements Server, HttpServerLike {
      spec = this.updateSpecFromRequest(spec, requestContext);
    }

+    // Apply OAS enhancers to the OpenAPI specification
+    this.OASEnhancer.spec = spec;
+    spec = await this.OASEnhancer.applyEnhancerByName('security');


I don't think the REST server should be invoking specific enhancers defined by a name, because that makes it impossible for other extensions to contribute new enhancers.

As I understand the proposed design:

The REST API invokes all registered API enhancers via applyAllEnhancers().

Extensions like @loopback/authentication contribute custom API enhancers to provide additional OpenAPI metadata like the authentication scheme.

The REST component (@loopback/rest) should not include any security schemas in the OpenAPI spec out of the box, simply because it does not implement any authentication out of the box.

Totally agree, I was being a slave to example in story #4380 Could you provide direction on following:

Add option to disable all enhancers on rest server? Alternatively option to override by providing array list of enhancers to apply?

Would I follow similar pattern to authentication strategies say spec-enhancers folder alongside or would I seek to embed the enhancers within strategies. If you have the time, perhaps a top level stepwise implementation path would be useful to follow.

Add option to disable all enhancers on rest server? Alternatively option to override by providing array list of enhancers to apply?

I am proposing to leave this part out of the initial implementation and wait until there is user demand for that.

Would I follow similar pattern to authentication strategies say spec-enhancers folder alongside or would I seek to embed the enhancers within strategies. If you have the time, perhaps a top level stepwise implementation path would be useful to follow.

IMO, it's not a concern of this pull request how the spec enhancer classes are registered.

I am expecting the @loopback/authentication extension to contribute a binding with @bind(asSpecEnhancer) decorator. As I understand the current implementation of applyAllEnhancers, it will automatically pick up such class.

See my ModelApiBooter spike in #3617 for an example. I really wish we had better documentation about the ExtensionPoint/Extension pattern :-/

Here the app decides to register a component (it's CrudRestComponent in the spike, it would be @loopback/authentication in your case):

https://github.com/strongloop/loopback-next/blob/d8fe4736fd07fc9da34ed345cbb77bdbd761b752/examples/todo/src/application.ts#L29

Here the component implements an Extension:

https://github.com/strongloop/loopback-next/blob/d8fe4736fd07fc9da34ed345cbb77bdbd761b752/packages/rest-crud/src/crud-rest-builder.plugin.ts#L30-L31

And here is the Extension implementation contributed by the Component to the Application context:

https://github.com/strongloop/loopback-next/blob/d8fe4736fd07fc9da34ed345cbb77bdbd761b752/packages/rest-crud/src/crud-rest.component.ts#L9-L11

dougal83 · 2020-02-17T20:10:17Z

@bajtos I've cut this PR down down to a singular purpose. Please let me know if it is back on track.

jannyHou · 2020-02-24T16:54:59Z

@dougal83 @bajtos got distracted by the auth migration last week, sorry for replying late.

Let me clarify the goal first:

For Add bearer auth scheme as the default security scheme #4386, the ultimate goal is to enable the "authorize" button in the explorer out-of-box in a LB4 app, and the default scheme(bearer auth scheme) should be the spec snippet I post in story description. Do we still agree on it? If yes, we can discuss how to do it.
For Add OpenAPI enhancer service in @loopback/rest #4380, the ultimate goal is to add an OAI enhancer service in the rest server, regardless of what enhancers it picks up by OAIEnhancer.applyEnhancerByName() or OAIEnhancer.applyAllEnhancers(), and where those enhancers are applied(inside getApiSpec or in an app's constructor).

And if we still agree on ^, let's discuss each module's responsibility. I think I somehow misunderstand the expected approaches and my understanding has been "@loopback/rest provides the bearer auth scheme enhancer" and "it should be applied inside the rest server class, like the consolidation enhancer added in #4365" all the time. If this is not the expected behaviour anymore, first sorry for misleading @dougal83 on the implementation... then let's agree on which module provides the enhancer and where to apply it.

I am expecting the @loopback/authentication extension to contribute a binding with @Bind(asSpecEnhancer) decorator. As I understand the current implementation of applyAllEnhancers, it will automatically pick up such class.

@loopback/authentication is good to me. And yes applyAllEnhancers would automatically pick up it. Do we want to call applyAllEnhancers inside the rest server class? inside getApiSpec or _serveOpenApiSpec? Or in the app constructor?

When created the story, I didn't realize applying the enhancer would turn the getApiSpec into an async function. I tend to keep it sync and apply the default security enhancer somewhere else, like _serveOpenApiSpec

If we modify the app template and apply the enhancers in app constructor, then we don't have the concern above. WDYT?

dougal83 · 2020-02-24T17:23:15Z

@jannyHou Thank you for the reply. Don't worry about misleading, it's more likely my misunderstanding. :)

From looking at the stories and attempting PRs, it does make sense to have the rest server apply the extensions by default via applyAllEnhancers but you make a good alternative suggestion. 👍

From comments made I also attempted to extend the AuthStrategies provider to contribute the OASEhancer. Love to hear your thoughts on the idea!

raymondfeng · 2020-02-24T17:29:43Z

@dougal83 The PR looks good to me now. Would you please add a test to verify that an enhancer is invoked as part of getApiSpec() call?

BREAKING CHANGE: Api specifications are now emitted as a Promise instead of a value object. Calls to getApiSpec function must switch from the old style to new style as follows: 1. Old style ```ts function() { // ... const spec = restApp.restServer.getApiSpec(); // ... } ``` 2. New style ```ts async function() { // ... const spec = await restApp.restServer.getApiSpec(); // ... } ```

add openapi spec enhancer to rest server impl. loopbackio#4380 Signed-off-by: Douglas McConnachie <[email protected]>

dougal83 · 2020-02-27T15:35:45Z

@bajtos @raymondfeng Please let me know if further changes are required.

jannyHou

👏 LGTM Good to apply the security spec in a separate PR. And really appreciate your effort spent on the OAS enhancer stories. I am looking at the other PRs.

bajtos

The new version looks great 👏 Thank you @dougal83 for the persistence in pushing this change forward, despite long silences on our side ❤️

raymondfeng · 2020-03-05T16:29:11Z

packages/rest/src/rest.server.ts

@@ -229,6 +238,16 @@ export class RestServer extends Context implements Server, HttpServerLike {
    this.bind(RestBindings.HANDLER).toDynamicValue(() => this.httpHandler);
  }

+  protected _setupOASEnhancerIfNeeded() {
+    if (this._OASEnhancer != null) return;


Nitpick: we could use this._OASEnhancer = this.getSync(OAS_ENHANCER_SERVICE, {optional: true}); to check so that the service can be bound to the server.

dougal83 added OpenAPI community-contribution labels Feb 3, 2020

dougal83 requested review from bajtos and raymondfeng as code owners February 3, 2020 16:05

dougal83 requested a review from jannyHou February 3, 2020 17:00

raymondfeng reviewed Feb 3, 2020

View reviewed changes

packages/openapi-v3/src/__tests__/unit/enhancers/fixtures/application.ts Outdated Show resolved Hide resolved

raymondfeng reviewed Feb 3, 2020

View reviewed changes

examples/lb3-application/src/__tests__/acceptance/lb3app.acceptance.ts Show resolved Hide resolved

raymondfeng reviewed Feb 3, 2020

View reviewed changes

dougal83 force-pushed the add-oas-enhancer-service branch 2 times, most recently from aeb2ea5 to c788a70 Compare February 3, 2020 21:04

jannyHou self-assigned this Feb 7, 2020

jannyHou reviewed Feb 7, 2020

View reviewed changes

dhmlau mentioned this pull request Feb 13, 2020

Add OpenAPI enhancer service in @loopback/rest #4380

Closed

3 tasks

bajtos suggested changes Feb 17, 2020

View reviewed changes

dougal83 force-pushed the add-oas-enhancer-service branch from c788a70 to e633dd9 Compare February 17, 2020 18:04

dougal83 requested review from deepakrkris, emonddr, hacksparrow and nabdelgadir as code owners February 17, 2020 18:04

dougal83 requested a review from bajtos February 17, 2020 20:07

This comment has been minimized.

Sign in to view

dougal83 force-pushed the add-oas-enhancer-service branch from e633dd9 to 2da2b42 Compare February 18, 2020 09:16

dougal83 changed the title ~~Add OpenAPI enhancer service in @loopback/rest~~ feat(rest): add OAS Enhancer service Feb 20, 2020

This was referenced Feb 20, 2020

feat(Authentication) enable authentication strategies to contribute OASEnhancer #4693

Merged

feat(rest): add openapi schema consolidation #4365

Merged

dougal83 requested a review from raymondfeng February 20, 2020 23:19

dougal83 force-pushed the add-oas-enhancer-service branch from 2da2b42 to f572622 Compare February 24, 2020 19:49

dougal83 added 2 commits February 24, 2020 22:03

feat(rest): add openapi enhancer service

7558cee

add openapi spec enhancer to rest server impl. loopbackio#4380 Signed-off-by: Douglas McConnachie <[email protected]>

dougal83 force-pushed the add-oas-enhancer-service branch from f572622 to 7558cee Compare February 24, 2020 22:05

dougal83 mentioned this pull request Feb 25, 2020

Initial openapi schema #3884

Closed

3 tasks

dhmlau added this to the Mar 2020 milestone Mar 3, 2020

jannyHou approved these changes Mar 4, 2020

View reviewed changes

bajtos approved these changes Mar 5, 2020

View reviewed changes

raymondfeng reviewed Mar 5, 2020

View reviewed changes

raymondfeng approved these changes Mar 5, 2020

View reviewed changes

raymondfeng merged commit 62d55eb into loopbackio:master Mar 5, 2020

bajtos added breaking-change feature labels Mar 5, 2020

dougal83 deleted the add-oas-enhancer-service branch March 5, 2020 21:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(rest): add OAS Enhancer service #4554

feat(rest): add OAS Enhancer service #4554

dougal83 commented Feb 3, 2020 •

edited

Loading

raymondfeng Feb 3, 2020

dougal83 Feb 3, 2020

jannyHou Feb 7, 2020

jannyHou Feb 7, 2020

bajtos Feb 17, 2020

dougal83 Feb 20, 2020

jannyHou Feb 7, 2020

jannyHou left a comment

jannyHou Feb 7, 2020

bajtos left a comment

bajtos Feb 17, 2020

dougal83 Feb 17, 2020

bajtos Feb 17, 2020

dougal83 commented Feb 17, 2020

This comment has been minimized.

jannyHou commented Feb 24, 2020

dougal83 commented Feb 24, 2020

raymondfeng commented Feb 24, 2020

dougal83 commented Feb 27, 2020

jannyHou left a comment

bajtos left a comment

raymondfeng Mar 5, 2020

feat(rest): add OAS Enhancer service #4554

feat(rest): add OAS Enhancer service #4554

Conversation

dougal83 commented Feb 3, 2020 • edited Loading

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jannyHou left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bajtos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dougal83 commented Feb 17, 2020

This comment has been minimized.

jannyHou commented Feb 24, 2020

dougal83 commented Feb 24, 2020

raymondfeng commented Feb 24, 2020

dougal83 commented Feb 27, 2020

jannyHou left a comment

Choose a reason for hiding this comment

bajtos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dougal83 commented Feb 3, 2020 •

edited

Loading