-
-
Notifications
You must be signed in to change notification settings - Fork 141
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
base: master
Are you sure you want to change the base?
Njb/nest module #840
Conversation
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); |
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does this test pass tho? Does accounts set
There was a problem hiding this comment.
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.
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
nice PR, I'm waiting for merge! |
@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 |
You mean your app would or this PR does? Feel free to review and add comments |
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? |
Nest 7 is slightly buggy atm
Best,
Eliezer
…On Wed, Mar 18 2020 at 9:43 AM, Lee Siong Chan < ***@***.*** > wrote:
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?
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub (
#840 (comment) ) ,
or unsubscribe (
https://github.com/notifications/unsubscribe-auth/AAXSQX4LQXCENF5VQTUIR4DRIB3TZANCNFSM4JCD7GXQ
).
|
have not saw bugs yet, can you point please |
They’ve fixed a few since my post. For example not being able to use args in field resolvers
Best,
Eliezer
…On Thu, Mar 19 2020 at 7:34 AM, Trej Gun < ***@***.*** > wrote:
have not saw bugs yet, can you point please
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub (
#840 (comment) ) ,
or unsubscribe (
https://github.com/notifications/unsubscribe-auth/AAXSQX4SDYDSUGMPLB5GXEDRIGVFBANCNFSM4JCD7GXQ
).
|
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 |
Anyone got NestJS to work with AccountsJS? |
@agustif have you tried this package with next 6.x? |
Nope, I havent tried yet. 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... |
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 |
Worth skipping if it's a simple project and you already have accounts working
Best,
Eliezer
…On Fri, May 01, 2020 at 9:57 PM, Agusti Fernandez < ***@***.*** > wrote:
>
>
> @ agustif ( https://github.com/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
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub (
#840 (comment) ) ,
or unsubscribe (
https://github.com/notifications/unsubscribe-auth/AAXSQXYSPAUSCMPLIQPRA6TRPMLQVANCNFSM4JCD7GXQ
).
|
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? |
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)
}
} |
3d3efb6
to
20969e7
Compare
bdf442b
to
3e57ec8
Compare
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.