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

How to set role to use rest api? #43

Closed
zerox12311 opened this issue Mar 22, 2019 · 11 comments
Closed

How to set role to use rest api? #43

zerox12311 opened this issue Mar 22, 2019 · 11 comments

Comments

@zerox12311
Copy link

This is really cool library.
I have some question.
I already have some role pipe to check user can access some api.
How to do this on this library??
My think is override method and set pipe to do this?
please give me some idea.
thanks!!

@lapwat
Copy link

lapwat commented Mar 22, 2019

Yes it could work. Override the method and call the base method so the behavior does not change. You can now add your own decorators.

  @UseGuards(...)
  @UseInterceptors(...)
  @Override()
  getMany(
    @ParsedQuery() query: RestfulParamsDto,
    @ParsedOptions() options: CrudOptions,
  ) {
    return this.base.getManyBase(query, options);
  }

Another option is to add your decorator at the class level (if all endpoints have the same permissions).

@michaelyali
Copy link
Member

Hey. Thanks for your kind words. First, I would suggest not to use Pipes for role validation, but to use Guards instead, because guards are meant for such situations. Also, Guard can be attached to the whole Controller and you won't need to override your methods. Please take a look here in README - I've added some decorators and explained how and where it's better to use them. Please, also keep in mind that I released v3.0.0 a few minutes ago. Cheers!

@Diluka
Copy link
Contributor

Diluka commented Mar 25, 2019

@zMotivat0r Maybe we can add an option to put custom decorators on CRUD methods.

@michaelyali
Copy link
Member

@Diluka I thought about it. But we still have to take in mind this proposal. And that's why I don't want to add a lot of things that might be changed in the future because of this new feature coming in Nest. What do you think?

@Diluka
Copy link
Contributor

Diluka commented Mar 25, 2019

We already has target and name.

getManyBase(target: object, name: string, dto: any, crudOptions: CrudOptions) {
const prototype = (target as any).prototype;
prototype[name] = function getManyBase(

And this can apply custom decorators.

const p = Object.getOwnPropertyDescriptor(prototype, name);
Reflect.decorate(customDecorators, target, name, p);

We can add a feature to do the trick or refactor code to open a port let someone override by themselves.

But currently, the base methods are too heavy to be override.

As you say, this feature may be removed because the new feature in Nest.

And about the CrudOptions, maybe we shall put it in one place the controller's metadata.

export function RestfulParamsInterceptorFactory(crudOptions: CrudOptions): Function {

Interceptors can get controller type by calling ExecutionContext#getClass() method.

@michaelyali
Copy link
Member

@Diluka how much time will it take for you to create a PR for this?

@Diluka
Copy link
Contributor

Diluka commented Mar 25, 2019

@zMotivat0r To implement this feature is quick. If you think it's OK, I'll create PR when I have some time.
But I suggest you can refactor CrudOptions in one place. Now it's merged and passed everywhere.

@michaelyali
Copy link
Member

@Diluka yeah, I'll refactor it. I'll store it in class metadata.

@michaelyali
Copy link
Member

@Diluka is there a chance that you are using Discord? https://discord.gg/T4TN67

@Diluka
Copy link
Contributor

Diluka commented Mar 26, 2019

@zMotivat0r No, I am not using Discord.

Although there is nothing wrong with reading emails and documents. But the difficulty of instant messaging is still a bit too big. (Google Translate ┑( ̄Д  ̄)┍)

@markpenaranda
Copy link

I made a guard for the standard restful endpoints

https://gist.github.com/markpenaranda/c43f0c48b9a54a62f5cc2e631d6cd173

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

No branches or pull requests

5 participants