nestjs / nest

A progressive Node.js framework for building efficient, scalable, and enterprise-grade server-side applications with TypeScript/JavaScript 🚀
https://nestjs.com
MIT License
67.44k stars 7.6k forks source link

PassportStrategy validate method should support Rxjs Observerable as return type #4938

Closed hantsy closed 4 years ago

hantsy commented 4 years ago

Feature Request

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

Add Obseverable as an option of the return type.

Describe the solution you'd like

I would like to use Rxjs API as possible in my application because I have some experience of Angular, but unfortunately, in the NestJS application, it mixes Promise and Observable.

I try to use Observable as validating return type, it does not work.

@Injectable()
export class LocalStrategy extends PassportStrategy(Strategy) {
  constructor(private authService: AuthService) {
    super({
      usernameField: 'username',
      passwordField: 'password',
    });
  }

 validate(username: string, password: string): Observable<any> {
   return this.authService
     .validateUser(username, password)
     .pipe(throwIfEmpty(() => new UnauthorizedException()));
 }
}

Teachability, Documentation, Adoption, Migration Strategy

Currently, it only supports Promise, and any simple type, but does not support Rxjs Observable.

  async validate(username: string, password: string): Promise<any> {
    const user = await this.authService
      .validateUser(username, password)
      .toPromise();

    if (!user) {
      throw new UnauthorizedException();
    }

    return user;
  }

What is the motivation / use case for changing the behavior?

Support Rxjs Observable

jmcdo29 commented 4 years ago

It absolutely does support promises. You can see an example here (local) and here (google)

hantsy commented 4 years ago

@jmcdo29 I will verify it one more time, I have changed my codes to Promise to make it work now. Thanks.

hantsy commented 4 years ago

Maybe the pipe is not correct.

.pipe(throwIfEmpty(() => new UnauthorizedException()));

I am not sure if the user not found what Mongoose returns. All mongoose API are using Promise.

jmcdo29 commented 4 years ago

Can't say with the rest of your code, as it isn't visible, but it is possible. For further support, please use our Discord. You're also welcome to look through the rest of the project above to see if it can help you find your problem

hantsy commented 4 years ago

OK, Thanks.

Jintus commented 2 years ago

I do not understand why this Issue as been closed.

The suggested code from @hantsy works well if we add firstValueFrom operator to transform Observable into Promise but if we use the code as it, the request attribute user will be populate with the observable, so the controller will be triggered.

For this controller :

@UseGuards(LocalAuthenticationGuard)
@Post('/auth/login')
login(@Request() req: { user: AuthenticatedUser }) {
  console.log('Controller', req.user);
  return req.user;
}

With bad credentials :

async validate(email: string, password: string): Promise<AuthenticatedUser | undefined> {
  return firstValueFrom(this.authenticationService.validateUser(email, password).pipe(
    throwIfEmpty(() => new UnauthorizedException())
  ));
}

// Will not trigger Controller console log
async validate(email: string, password: string): Observable<AuthenticatedUser | undefined> {
  return this.authenticationService.validateUser(email, password).pipe(
    throwIfEmpty(() => new UnauthorizedException())
  );
}

// Will trigger Controller console log, with req.user to be the entire Observable
kamilmysliwiec commented 2 years ago

Please, use our Discord channel (support) for such questions. We are using GitHub to track bugs, feature requests, and potential improvements.