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

fix: optional auth metadata #2218

Merged
merged 1 commit into from
Jan 9, 2019
Merged

fix: optional auth metadata #2218

merged 1 commit into from
Jan 9, 2019

Conversation

jannyHou
Copy link
Contributor

@jannyHou jannyHou commented Jan 7, 2019

Description

static endpoint like the homepage in the shopping example doesn't have a controller, thus the metadata injected for the auth action should be optional.
Related to loopbackio/loopback4-example-shopping#26

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
  • Affected artifact templates in packages/cli were updated
  • Affected example projects in examples/* were updated

@bajtos
Copy link
Member

bajtos commented Jan 7, 2019

static endpoint like the homepage in the shopping example doesn't have a controller, thus the metadata injected for the auth action should be optional.

How about handler-based routes? There is not controller name & method name for these routes too, but I think there should be a way how to invoke authentication for them.

const spec: OperationObject = { /*...*/ };
function greet(name: string) {
  return `hello ${name}`;
}

app.route('get', '/', spec, greet);

This is just something to consider, no need to fix this as part of this pull request.

The changes looks reasonable to me when I consider them as a quick fix, please add some tests for verification.

Copy link
Member

@bajtos bajtos left a comment

Choose a reason for hiding this comment

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

Please add some tests for verification.

@raymondfeng
Copy link
Contributor

@bajtos The security property of OpenAPI OperationObject can be potentially used to include such metadata. For non-controller routes, we probably should bind the operation spec in request context.

@bajtos
Copy link
Member

bajtos commented Jan 8, 2019

@jannyHou based on the comments above, I think this pull request is going in the right direction. The remaining task is to add tests to verify the changes.

Copy link
Contributor

@b-admike b-admike left a comment

Choose a reason for hiding this comment

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

besides one suggestion/comment, LGTM overall.

@@ -65,6 +65,17 @@ describe('AuthMetadataProvider', () => {
);
expect(authMetadata).to.be.undefined();
});

it('returns undefined when the class or method is missing', async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we separate this test into two? i.e. one for class missing and the other class defined but method missing? or is that not useful?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@b-admike The method lives in the controller, so if the controller class is missing then the method will definitely be missing. Which means the only valid scenario is no controller class + no method.

What we could improve in the next PR is the scenario that Miroslav brings in his comment:

const spec: OperationObject = { /*...*/ };
function greet(name: string) {
  return `hello ${name}`;
}

app.route('get', '/', spec, greet);

And as Raymond said, the auth metadata could be included in the OpenAPI spec.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants