darraghoriordan / eslint-plugin-nestjs-typed

Some eslint rules for working with NestJs projects
http://www.darraghoriordan.com
171 stars 34 forks source link

feat: Add UseGuards detection to controller methods #48

Closed Fishbowler closed 1 year ago

Fishbowler commented 1 year ago

Description

Adds a lint to ensure that any endpoint method has a @UseGuards decorator, either at the method or class level.

Commentary

It's super useful for ensuring correct endpoints have appropriate authentication + authorisation, but it's useless for an open API.

Outstanding work

I've added it to Recommended for testing, but I don't think it really belongs there. Could go into Recommended in an "off" state, allowing people to override it?

It doesn't yet have docs - I'll add those as soon as I know where it's going (i.e. that this will be accepted as a PR in some state)

Fishbowler commented 1 year ago

I've updated the setting in Recommended to be Off, which gets it into the import, but I think makes this safe to include for everyone.

darraghoriordan commented 1 year ago

Hey, thanks for the PR!

I wonder if this scenario is better handled with a global guard instead. You can attach the guard directly to the app and nestjs will ensure all routes are protected

app.useGlobalGuards(new RolesGuard());

(assuming RolesGuard is some custom guard you created)

then for the occasional public route, instead of @Guard you could instead create a @PublicEndpoint() metadata decorator. Then you could update RolesGuard to check for the presence of this metadata and allow access to anyone if some IsPublic metadata is present.

Fishbowler commented 1 year ago

Nest provides for separate Authentication and Authorization, where the latter is implemented in Guards, and can take advantage of other controller/endpoint metadata (like a RolesGuard using a Role annotation, from your example).

We hit a problem where with different authentication mechanisms (e.g. Firebase, Google SSO, internal) conflating the authorization to work across them was messy, could compromise the SRP, and so add risk to the critical nature of the component. Further, splitting guards out to be explicit at a controller or endpoint level ensures that it's a conscious developer choice in our context.

Definitely a rule that wants to be off by default, since it won't apply to all situations

I wonder if there's a way to detect the useGlobalGuards, to make it even less intrusive?

darraghoriordan commented 1 year ago

Nest provides for separate Authentication and Authorization, where the latter is implemented in Guards, and can take advantage of other controller/endpoint metadata (like a RolesGuard using a Role annotation, from your example).

We hit a problem where with different authentication mechanisms (e.g. Firebase, Google SSO, internal) conflating the authorization to work across them was messy, could compromise the SRP, and so add risk to the critical nature of the component. Further, splitting guards out to be explicit at a controller or endpoint level ensures that it's a conscious developer choice in our context.

Definitely a rule that wants to be off by default, since it won't apply to all situations

I wonder if there's a way to detect the useGlobalGuards, to make it even less intrusive?

ah fair enough. sounds like a tricky situation. i'll take a closer look at this over the next couple of days then hopefully and get it merged

darraghoriordan commented 1 year ago

Hey it all lgtm. it's turned off by default anyway so should be fine.

Any chance you can add a section to README.md describing what the rule does with one or two examples?

Fishbowler commented 1 year ago

Done 👍

darraghoriordan commented 1 year ago

:tada: This PR is included in version 3.19.0 :tada:

The release is available on:

Your semantic-release bot :package::rocket: