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

AuthGuard does not work with authentication-only strategies #1505

Open
2 of 4 tasks
juona opened this issue Dec 1, 2023 · 1 comment
Open
2 of 4 tasks

AuthGuard does not work with authentication-only strategies #1505

juona opened this issue Dec 1, 2023 · 1 comment
Labels
bug Something isn't working needs triage

Comments

@juona
Copy link

juona commented Dec 1, 2023

Is there an existing issue for this?

  • I have searched the existing issues

Current behavior

Quote from a comment in the PassportJS source code for the built-in session strategy:

[i]f login session data is not present, the request will be passed to the next middleware, rather than failing authentication - which is the behavior of most other strategies.

Not sure if "most strategies" is an accurate claim, but I will say that two rather popular strategies do not actually work like that. Both passport-jwt and passport-local reject requests if they are unauthenticated. So at the very least there is a lack of consistency in PassportJS strategies.

Here's a simplified excerpt from the built-in session strategy:

SessionStrategy.prototype.authenticate = function(req, options) {
  // Obtain user object from the session somehow...
  var su = {} //...

  if (su) {
    this._deserializeUser(su, req, function(err, user) {
      if (err) { return self.error(err); }

      if (user) {
        // IMPORTANT! Sets the user property on the request
        req.user = user;
      }

      // JWT / Local strategies would both call self.success() at this point, indicating that
      // the request is not only authenticated, but also authorized
      self.pass();
    });
  } else {
    // This strategy does not fail if there is no user in the session (i.e. request is unauthenticated)
    self.pass();
  }
};

Unlike the JWT / local strategies, the session strategy does not invoke self.fail() / self.success() and instead always calls self.pass(), regardless of whether the client is authenticated or not. Its purpose is to:

  1. retrieve the user object from the session;
  2. set the user property on the request object;
  3. proceed to the next middleware with or without the user object.

Now onto NestJS. The AuthGuard is a useful wrapper around PassportJS middleware, but it does not support strategies that only authenticate requests but do not authorize them (such as the session strategy). Here's the relevant excerpt from the canActivate function in the AuthGuard:

const user = await passportFn(
    type || this.options.defaultStrategy,
    options,
    (err, user, info, status) =>
      this.handleRequest(err, user, info, context, status)
);
request[options.property || defaultOptions.property] = user;
return true;

With an authenticate-only strategy, if the request is not authenticated:

  1. passportFn does not fail,
  2. user is undefined,
  3. canActivate returns true, allowing an unauthenticated request to pass through.

This is fine, because it sort of respects the design of the strategy. In that case the authorization check should be delegated to a later stage. Normally, when working with PassportJS, one could use req.isAuthenticated() in the next guard for that very purpose.

However, the problem is that the strategy, and consequently the AuthGuard, behave exactly the same if the client is authenticated:

  1. passportFn does not fail,
  2. user is undefined,
  3. canActivate returns true.

Since user is undefined, it is no longer possible to use req.isAuthentiated() after canActivate finishes. This causes a bit of a headache down the line, because I have to resort to a different system altogether for working with such strategies.

Minimum reproduction code

N/A

Steps to reproduce

No response

Expected behavior

I expect either of the two:

  1. The AuthGuard completely conforms to authenticate-only strategies thus not overwriting req.user with undefined values.
  2. The AuthGuard rejects unauthenticated requests after checking req.isAuthenticated().

Option #1 is probably preferable.

Package version

10.0.2

Passport version

0.7.0

NestJS version

10

Node.js version

16

In which operating systems have you tested?

  • macOS
  • Windows
  • Linux

Other

No response

@juona juona added bug Something isn't working needs triage labels Dec 1, 2023
@boris-w
Copy link

boris-w commented Jan 16, 2024

I have also the same issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working needs triage
Projects
None yet
Development

No branches or pull requests

2 participants