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

Njb/nest module #840

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Conversation

NickBolles
Copy link
Contributor

@NickBolles NickBolles commented Oct 18, 2019

Looking for testers for a nestjs module. Please let me know if there's anything We can do better, especially pay attention to creating a new package in the monorepo and the package metadata. I wasn't 100% sure I have it all accurate.

const gqlParams: GQLParam = [ctx.getRoot(), ctx.getArgs(), gqlCtx, ctx.getInfo()];
// use the authenticated function from accounts-js. All that's really needed is context
await authenticated(() => null)(...gqlParams);
return this.runValidators(gqlCtx.user, context, gqlParams);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it gqlCtx.req.user?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yea, I think you're right

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, no what I have is correct. This is where Accounts.js builds the context, it sets user, as well as some more properties on the context.

https://github.com/accounts-js/accounts/blob/master/packages/graphql-api/src/utils/context-builder.ts#L37-L43

Interestingly, it doesn't set the request on the context, but it looks like it requires it to be set on the context by something else earlier on

packages/nestjs/README.md Show resolved Hide resolved
@ducan-ne
Copy link

ducan-ne commented Dec 4, 2019

nice PR, I'm waiting for merge!

@NickBolles
Copy link
Contributor Author

@Quaj Have you tested it out? I was using it my app but I haven;t been working on it lately.

@ducan-ne
Copy link

ducan-ne commented Dec 8, 2019

@Quaj Have you tested it out? I was using it my app but I haven;t been working on it lately.

not yet, but i thinks need some refactor

@NickBolles
Copy link
Contributor Author

not yet, but i thinks need some refactor

You mean your app would or this PR does? Feel free to review and add comments

@NickBolles
Copy link
Contributor Author

Still looking for review on this @Quaj and @TrejGun would you be interested in actually trying this out? You should be able to do an NPM install from my github branch, otherwise pull it, run yarn link and then do a yarn link @accounts/nestjs in your app folder

@leesiongchan
Copy link

I am interested in checking this out, but do you think if you can bump to the latest nest v7? And do you think it is possible to include Federation support?

@elie222
Copy link
Contributor

elie222 commented Mar 18, 2020 via email

@TrejGun
Copy link

TrejGun commented Mar 19, 2020

have not saw bugs yet, can you point please

@elie222
Copy link
Contributor

elie222 commented Mar 19, 2020 via email

@NickBolles
Copy link
Contributor Author

NickBolles commented Mar 20, 2020

@leesiongchan

I am interested in checking this out, but do you think if you can bump to the latest nest v7? And do you think it is possible to include Federation support?

Is there some incompatiblity? Maybe we can loosen up the peer dependency range for @nestjs/core. I don't remember any part of this module relying on nest directly. Give it a shot. Download the module and run E2E tests with nest 7. The E2E tests are very robust.

@leesiongchan
Copy link

@leesiongchan

I am interested in checking this out, but do you think if you can bump to the latest nest v7? And do you think it is possible to include Federation support?

Is there some incompatiblity? Maybe we can loosen up the peer dependency range for @nestjs/core. I don't remember any part of this module relying on nest directly. Give it a shot. Download the module and run E2E tests with nest 7. The E2E tests are very robust.

Yes you have to rename all type-graphql to @nestjs/graphql, see https://docs.nestjs.com/migration-guide#graphql

@agustif
Copy link
Contributor

agustif commented Apr 27, 2020

Anyone got NestJS to work with AccountsJS?

@NickBolles
Copy link
Contributor Author

@agustif have you tried this package with next 6.x?

@agustif
Copy link
Contributor

agustif commented Apr 29, 2020

@agustif have you tried this package with next 6.x?

Nope, I havent tried yet.
But I like the idea of both using AccountsJS and NestJS for my project's backend.

I also plan to not use angular but React/NextJS on the frontend, but I guess that shouldn't affect muchh the other parts of the stack since all communication happens between grahpql/apollo/express etc...

@agustif
Copy link
Contributor

agustif commented May 1, 2020

@agustif have you tried this package with next 6.x?

Does it work only with v6? Any plans for v7 support? I might skip nestJS for now as I will have a pretty simple backend on top of AccountsJS User System

@elie222
Copy link
Contributor

elie222 commented May 2, 2020 via email

@NickBolles
Copy link
Contributor Author

I can update it to v7, I just updated my app and it was literally a find and replace. Then update context guards to be more robust.

@elie222 how are you doing auth guards and such with nestjs?

@elie222
Copy link
Contributor

elie222 commented May 7, 2020

Auth guards (I'm using Auth0 in examples below). Not using AccountsJS atm:

  @Query(() => Number)
  @UseGuards(GqlAuthGuard, RolesGuard)
  @Roles('admin')
  async studentsCount() {
    return this.studentService.count()
  }
@Injectable()
export class GqlAuthGuard extends AuthGuard('jwt') {
  getRequest(context: ExecutionContext) {
    const ctx = GqlExecutionContext.create(context)
    return ctx.getContext().req
  }
}
@Injectable()
export class RolesGuard implements CanActivate {
  constructor(
    private readonly reflector: Reflector,
    private readonly configService: ConfigService
  ) {}

  canActivate(context: ExecutionContext): boolean | Promise<boolean> | Observable<boolean> {
    const routeRoles = this.reflector.get<string[]>('roles', context.getHandler())

    if (!routeRoles) return true

    const ctx = GqlExecutionContext.create(context)
    const user = ctx.getContext().req.user
    const userRoles = user[this.configService.get('AUTH0_ROLES_FIELD')] || []

    return hasRole(routeRoles, userRoles)
  }
}

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

Successfully merging this pull request may close these issues.

6 participants