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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

handle custom errors thrown by authorization checker #233

Merged
merged 1 commit into from
Aug 6, 2017

Conversation

twittwer
Copy link
Contributor

@twittwer twittwer commented Aug 1, 2017

Really great module 馃憤

One requirement in our project is to differentiate Unauthorized and Forbidden code during authorization process.
That's why I added an catch to the authorizationChecker, to be able to throw HttpErrors the same as in controllers.
I think that could be helpful for others too.

createExpressServer({
    authorizationChecker: async (action: Action, roles: string[]) => {

        const token = action.request.headers["authorization"];
        if (!token)
            throw new UnauthorizedError();

        const user = await getEntityManager().findOneByToken(User, token);
        if (!user)
            throw new UnauthorizedError();

        if (!roles.length)
            return true;
        if (roles.find(role => user.roles.indexOf(role) !== -1))
            return true;

        throw new ForbiddenError();
    }
});

Thanks

@festen
Copy link

festen commented Aug 5, 2017

I have the same requirement (but in Koa) as well, I imagine it is more or less the same logic/different file?. It now only throws code 500 errors. Excited about this feature 馃憤

EDIT: My bad, the 500 part is by our own general error catcher. Still would be nice to be able to differentiate between 401 and 403.

@MichalLytek
Copy link
Contributor

MichalLytek commented Aug 6, 2017

I don't know why build test failed on the newest node.js engine:

1) action parameters @Session(param) should throw required error when param is empty asserting port 3001:
     TypeError: Cannot read property 'statusCode' of undefined

But it's not related to this feature so I will do the honors and merge this PR with 1 new line 馃槈

@twittwer Thanks for your contribution, really usefull feature! It's a shame that this line wasn't there earlier 馃槅

@MichalLytek MichalLytek merged commit 27055d0 into typestack:master Aug 6, 2017
@NoNameProvided
Copy link
Member

NoNameProvided commented Aug 7, 2017

At first glance without digging, it seems this issue exists in the koa part as well. Why did we merge this without fixing there as well? We should aim consistency between the two driver.

The related part is here: KoaDriver.ts#L106

@NoNameProvided
Copy link
Member

If no one fixes this, I will do it in the evening. Anyone should feel free to create a patch for this.

@MichalLytek
Copy link
Contributor

MichalLytek commented Aug 7, 2017

@NoNameProvided You're right! 馃槥 I've spend whole day digging through issues and PR and I haven't noticed that. It would be great if you would create a patch for koa and even create a test case for this feature 馃槈

BTW I see more and more repeated code in drivers, I think it's time to make a big refactor to keep DRY.

@NoNameProvided
Copy link
Member

NoNameProvided commented Aug 7, 2017

BTW I see more and more repeated code in drivers, I think it's time to make a big refactor to keep DRY.

I agree on that.

It would be great if you would create a patch for koa

I will do it.

@twittwer
Copy link
Contributor Author

@NoNameProvided Sorry, for not answering - Just missed the ongoing conversation 馃檲

@NoNameProvided NoNameProvided mentioned this pull request Aug 12, 2017
@NoNameProvided
Copy link
Member

Patch created in #247.

@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 12, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants