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

feat: authentication should be reflected into children controller #2525

Open
EinfachHans opened this issue Nov 12, 2023 · 12 comments
Open

feat: authentication should be reflected into children controller #2525

EinfachHans opened this issue Nov 12, 2023 · 12 comments

Comments

@EinfachHans
Copy link
Contributor

Is your feature request related to a problem? Please describe.

When i declare a controller like

@Controller({
  path: '/path',
  children: [ChildrenController]
})
@CustomAuth()
export class ParentController {

i would expect the Authentication to be reflected to the children controller as well 🤔

Describe the solution you'd like

No response

Describe alternatives you've considered

No response

Additional context

No response

Acceptance criteria

No response

@Romakita
Copy link
Collaborator

Hello @EinfachHans

I already answered this question. I’m not favorable of this feature. It’s preferable to define over each method (or controller) the authentication strategy. because you don’t want auth a specific sub endpoint it won’t be possible to do that. Adding also the complexity to apply correctly the middleware for each controler and children controler, and this feature become a nightmare for me compared to just adding the decorator at the right place for dev.

See you

@EinfachHans
Copy link
Contributor Author

Alright, didn't found an request for that 🤔

because you don’t want auth a specific sub endpoint it won’t be possible to do that

That's right, but imo defining the Auth to the parent controller would be wrong anyways then

This feature become a nightmare for me compared to just adding the decorator at the right place for dev

Okay, i understand this, maybe just let this feature request open and label it with "help wanted" if someone wants to tackle this in the future?

In general i would say it would be the expected behaviour

@Romakita
Copy link
Collaborator

Maybe the problem is just related to the useBefore behavior on class. I have to re read the code because isn’t maybe clear for me ^^. I’ll keep this issue open, because your not the firt dev that ask me for that!

@Romakita
Copy link
Collaborator

Ok after investigation I found the reason why it's not possible.

The problem is here:
https://github.com/tsedio/tsed/blob/production/packages/platform/platform-middlewares/src/decorators/useAuth.ts#L49

Actually to apply auth middleware on inherited method, I have to use decorateMethodsOf utils. This function create a proxy method with the appropriate metadata instead of adding it over base class, for example:

class CrudController {
   @Get('/:id')
    get() {}

    @Get('/')
    getList() {}
   // etc...
}

@Controller("/users")
@UseAuth(MyAuthMiddleware)
class UsersController extends CrudController {
     @Get('/:id')
     getRoles() {}
}

@Controller("/public-data")
class PublicDataController extends CrudController {
     @Get('/:id')
     getOwners()
}

In this example, we want to apply AuthMiddleware only on UsersController. Without decorateMethodsOf, typescript add metadata on all methods including over CrudController methods. By side effect, the inherited PublicDataController methods (get/getList) has also impacted by the AuthMiddleware. (see #1535).
decorateMethodsOf solve this problem by monkey patching the child class (it add get and getList method).

The solution could be to use the Use decorator on class instead UseBefore. BUT isn't possible because for some usecase, we need to override the Auth options on some method using the @AuthOptions decorator.

@Controller("/users")
@UseAuth(MyAuthMiddleware, {scope: 'read write'})
class UsersController extends CrudController {
     @Get('/:id')
     @AuthOptions({scope: 'admin'})
     getRoles() {}

     @Get('/:id/other')
     other() {}
}

In this case:

  • proxy method get has read write scopes
  • proxy method getList has read write scopes
  • method getRoles has admin scope
  • method other has read write scopes

Now you have the overview of the actual implementation and you must see why the subject is complex ^^

I think the problem is currently with the decorator implementation. he has too much responsibility. we would just have to store the information from the auth middleware and apply it correctly when constructing the routes.

Here is the schema of the middleware call sequence:

call-sequence

See you ;)

@Romakita
Copy link
Collaborator

Note: UseAuth is a kind of UseBeforeEach

@EinfachHans
Copy link
Contributor Author

Good morning @Romakita ,

thanks for the explanation. In my case i don't extend controller classes, i use the children option of the @Controller decorator.

I understand that extending the controller brings problems with it, in your example above the CrudController methods should be protected within the UsersController, but not in the PublicDataController - i see the problem with this.

But do we have the same problem with the children option? 🤔

@Romakita
Copy link
Collaborator

Romakita commented Nov 14, 2023

But do we have the same problem with the children option? 🤔

Yes the same mechanism should be propagated to children because it will be one of the first things asked by the devs (class inheritance support).

@Romakita
Copy link
Collaborator

Romakita commented Jun 1, 2024

Here is an example of how to forward a decorator to each nested controller:
https://github.com/Romakita/tsed-example-params-in.git

https://github.com/Romakita/tsed-example-params-in/blob/main/src/middlewares/ContextIdMiddleware.ts#L23

Isn't recursive but it could be possible to implement that by consuming the metadata.

@Romakita
Copy link
Collaborator

Romakita commented Jun 1, 2024

@Romakita
Copy link
Collaborator

Romakita commented Jun 1, 2024

@EinfachHans it should solve your issue :)

@EinfachHans
Copy link
Contributor Author

@Romakita can't open the links, is the repo private maybe?

@Romakita
Copy link
Collaborator

Romakita commented Jun 1, 2024

@EinfachHans Fixed ^^

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

No branches or pull requests

2 participants