nest-lab icon indicating copy to clipboard operation
nest-lab copied to clipboard

feat: add option to OrGuard to throw the last or custom error

Open p-dim-popov opened this issue 1 year ago • 3 comments

p-dim-popov avatar May 29 '24 14:05 p-dim-popov

🦋 Changeset detected

Latest commit: 458f98fb1999ad4fad892eb99d8b98480cee3c8b

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@nest-lab/or-guard Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

changeset-bot[bot] avatar May 29 '24 14:05 changeset-bot[bot]

What's the use case for throwing the last encountered error? The first is for a fast fail. What about any errors between the first and last, should they be merged and thrown for better reporting?

jmcdo29 avatar May 29 '24 15:05 jmcdo29

I have this case

@UseGuards(OrGuard([GuardForUser, GuardForApiKey]))

Both guards when used separately throw 401, but if they fail with OrGuard the result is 403. The thing is that I still need them to throw 401 even when used together. Merging errors is a bit gray area as in theory everything can be thrown and it's hard to say how merging will be done (messages, stack traces, type of the new error, etc...). The simplest solution for me is to throw whatever fails last but it can be better to also expose a way to provide either a custom error or errors narrowing function

@UseGuards(OrGuard([GuardForUser, GuardForApiKey], { throwError: new UnauthorizedException('No authorization provided') }))

Let me propose this override as well https://github.com/jmcdo29/nest-lab/pull/44/commits/02b7d08670083de889def2f703ad1e23d3b4e510

p-dim-popov avatar May 29 '24 16:05 p-dim-popov

I'm interested in this feature for the same reason (being able to return 401 status code when multiple auth strategies fail rather than 403).

@jmcdo29 Any chance this could be merged? 🙏

ls-mmacdonald avatar Dec 10 '24 18:12 ls-mmacdonald

Thanks for the ping on this @ls-mmacdonald. This does look like something that's well thought out and could be easily supported. I'll try to make a changeset for this here soon so that it can be merged and published, unless @p-dim-popov wants to get that done, but as it's been a bit I wouldn't expect any quick response.

jmcdo29 avatar Dec 10 '24 18:12 jmcdo29

Tests pass locally. Merging to main

jmcdo29 avatar Dec 10 '24 18:12 jmcdo29