middleware icon indicating copy to clipboard operation
middleware copied to clipboard

Proposal: Implementation of Casbin Middleware

Open sugar-cat7 opened this issue 1 year ago • 4 comments

Casbin is a library that simplifies authorization control.(ACL, RBAC, etc...) While major frameworks like Express and Nest have user-defined middleware available, Hono did not have such an implementation. Therefore, I have created one for Hono.

https://casbin.org/docs/middlewares/

sugar-cat7 avatar Jul 31 '24 16:07 sugar-cat7

🦋 Changeset detected

Latest commit: e05e1bb605761a74e7befe515569e4116bd90de9

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

This PR includes changesets to release 1 package
Name Type
@hono/casbin Major

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 Jul 31 '24 16:07 changeset-bot[bot]

Hi @sugar-cat7 !

Sorry for the delayed response. I didin't know the Casbin, but looks good.

I have questions, though I may not understand your intention perfectly. Does the defaultCheckPermission check only the user name of the Basic Authentication header? Does this mean it does not authorize a user with a password from the header?

I wonder if we can use some logic for Basic Auth in hono core. Or is defaultCheckPermission just the default, so we don't have to be so picky?

yusukebe avatar Aug 07 '24 10:08 yusukebe

@yusukebe Hello! Thank you for checking this!

First of all, as a premise: Casbin is more of a framework for authorization rather than authentication. Essentially, it controls "who" can do "what" on "which" resources. Therefore, it is meant to be used in conjunction with authentication (e.g., something like Basic Auth). (To give a more concrete example, when using Bearer Auth, you can control whether to allow or deny a user's request by comparing a custom claim, such as "role," contained in the JWT after authentication, with a predefined policy.)

Based on the above:

Does the defaultCheckPermission check only the user name of the Basic Authentication header?

Since this is a framework for authorization, it primarily looks at information about users and their permissions. Regarding defaultCheckPermission, it checks the username and the predefined policy to determine if a user can access a particular endpoint. (By the way, the reason the default is implemented to look at the Basic Authentication header is to align with the middleware of other frameworks.)

Does this mean it does not authorize a user with a password from the header?

No. Authentication is assumed to be implemented separately, and defaultCheckPermission looks at whether access should be allowed after the user has been authenticated.

I wonder if we can use some logic for Basic Auth in hono core. Or is defaultCheckPermission just the default, so we don't have to be so picky?

It seems that the logic in the following link could be used here:

https://github.com/honojs/hono/blob/8ba02273e829318d7f8797267f52229e531b8bd5/src/middleware/basic-auth/index.ts#L16-L33

However, it would be difficult to use the middleware as-is, so it would require re-implementing the same function (called auth) that retrieves the user. Currently, exporting the private function implemented on the original Hono side seems a bit awkward for users, right? (It’s not middleware in the first place...)

sugar-cat7 avatar Aug 10 '24 10:08 sugar-cat7

Hi @sugar-cat7 Thank you for the response!

I understood the purpose of Casbin well thanks to your explanation.

Therefore, it is meant to be used in conjunction with authentication (e.g., something like Basic Auth). (To give a more concrete example, when using Bearer Auth, you can control whether to allow or deny a user's request by comparing a custom claim, such as "role," contained in the JWT after authentication, with a predefined policy.)

I think this is ideal and wonderful if we could use this middleware with the current hono's Basic Auth Middleware, Bearer Auth Middleware, or others. Can we do that? If so, I want to see the code base example (it seems you have to change the current code).

Currently, exporting the private function implemented on the original Hono side seems a bit awkward for users, right? (It’s not middleware in the first place...)

Exactly. We don't welcome exporting the private funcitons.

yusukebe avatar Aug 11 '24 02:08 yusukebe

@yusukebe Sorry for the delay.

I’ve made some modifications to the codebase.

The changes are as follows:

  • I separated the main casbin middleware and the authorizer we provide into different directories (src/helper).
  • I explicitly passed the authorizer to the casbin middleware.
  • I renamed defaultPermissionCheck to basicAuthorizer.
  • The basicAuthorizer now uses the same logic as the original Hono implementation. (This is something I’d like to discuss: I’m thinking it might be better to move the auth method to utils. It seems that in other middleware, non-middleware processes are called from utils.)
  • I also defined jwtAuthorizer in the helper.
  • Updated the Readme.

I think this is ideal and wonderful if we could use this middleware with the current hono's Basic Auth Middleware, Bearer Auth Middleware, or others. Can we do that? If so, I want to see the code base example (it seems you have to change the current code).

I've included the specific usage for combining them in the README! Additionally, you might find it easier to understand how to use them together by looking at the test suites for BasicAuthorizer with hono/basic-auth and JWTAuthorizer with hono/jwt.

sugar-cat7 avatar Aug 26 '24 10:08 sugar-cat7

Hi @sugar-cat7

Thank you for your explanation!

Creating basicAuthorizer and jwtAuthorizer is good!

The basicAuthorizer now uses the same logic as the original Hono implementation. (This is something I’d like to discuss: I’m thinking it might be better to move the auth method to utils. It seems that in other middleware, non-middleware processes are called from utils.)

As you said, it's better to create auth function in utils. I'm not 100% sure, but I also faced the same issue when I wanted to implement Basic Auth.

The type of auth will be good with the following:

type Auth = (request: Request) => { username: string; password: string }

I can do it later, but can you create a PR to refactor like that on honojs/hono?

yusukebe avatar Sep 01 '24 05:09 yusukebe

@yusukebe

I can do it later, but can you create a PR to refactor like that on honojs/hono?

I have created a PR to move the functions to the utils directory! https://github.com/honojs/hono/pull/3359

sugar-cat7 avatar Sep 02 '24 08:09 sugar-cat7

@yusukebe I have incorporated the content from hono v4.5.11 and removed any duplicate sections.

sugar-cat7 avatar Sep 03 '24 09:09 sugar-cat7

@sugar-cat7

Thanks! I've added some comments.

yusukebe avatar Sep 03 '24 10:09 yusukebe

Sorry, I accidentally closed it.

yusukebe avatar Sep 03 '24 10:09 yusukebe

@yusukebe Thank you for the review! I've made the corrections!

sugar-cat7 avatar Sep 08 '24 15:09 sugar-cat7

@sugar-cat7

Thank! Let's go!

yusukebe avatar Sep 09 '24 12:09 yusukebe

@yusukebe I'm sorry, it seems like the method I used to resolve the yarn.lock conflict wasn't good, and it's causing the CI to fail. Should I revert it?

sugar-cat7 avatar Sep 09 '24 13:09 sugar-cat7

@sugar-cat7 No worry. I'm fixing it.

yusukebe avatar Sep 09 '24 13:09 yusukebe

Fixed!

yusukebe avatar Sep 09 '24 13:09 yusukebe

@sugar-cat7 I created https://github.com/casbin/casbin-website-v2/pull/264 for Hono users 😉

exoego avatar Sep 09 '24 14:09 exoego