ParsedRequest filter changes from 4.3.x introduce breaking change

[iamjoeker]

I updated to 4.4.1 in early January and notice that request filter handling changed. In some controllers, it is necessary to add additional filters before handing the request off to the service for database operations (e.g. to add authorization criteria to the database query). Below is an example of how I implemented this prior to 4.3.x.

@Override()
async getMany(@ParsedRequest() req: CrudRequest, @User() user: User) {
  // Simulating user only having access to remoteId == 10

  req.parsed.filter.push({
    field: 'remoteId',
    operator: '$eq',
    value: user.remoteId,
  });

  return await this.base.getManyBase(req);
}

After upgrading to 4.4.1, I notice that these filter conditions are not incorporated when generating the database query. I tracked the change down to this commit 702603917e5e3968ae306881ff099a4907eca57a.

I have created a demonstration repository demonstrating the issue: https://github.com/iamjoeker/crud_filter_change_poc

[michaelyali]

Have you tried to use @CrudAuth decorator?

[iamjoeker]

Let me start by saying how much I love this library and its goals and appreciate the efforts to bring it to life.

My apologies as I realize now that I probably didn't do a good job of communicating my intent when I opened the issue.

While I concur that @CrudAuth is the preferred way to handle this starting with version 4.3 (as shared in the crud-4.4.1-working branch of my poc). My concern is that this is a BC break that happened in a minor release (which is not expected per semver). I was lucky to have discovered the issue before it resulted in significant data disclosure. I raise the issue in the hope that the BC break can be corrected and reported for others who may have been potentially impacted.

If it would be welcomed, I'm willing to submit a PR. Based on my research, fixing the BC break would require a new 4.3.x and 4.4.x release (and possibly a 5.x release with the BC break included).

[michaelyali]

thanks, @iamjoeker any PR and help are much appreciated

[idhard]

same here , after update filters stop workings, is there a way to use @CrudAuth decorator by action/route instead of applying it to the whole controller ?

[iamjoeker]

@idhard Currently the CrudAuth decorator can only be applied to classes. I too think it would be nice if it could also be applied to the action.

@zMotivat0r Thanks! I will get a PR submitted over the weekend.

[Diluka]

I wander know if this can use @CrudAuth ?

  async search(user: User, req: CrudRequest) {
    const isMgr = user.hasRole(Role.BA_MGR);

    req.parsed.search = {
      $and: [req.parsed.search, { isEnabled: true }],
    };

    const builder: SelectQueryBuilder<Activity> = await this.createBuilder(req.parsed, req.options);

    if (isMgr) {
      builder.andWhere(`${this.alias}.creatorId = :userId`, { userId: user.id });
    } else {
      builder.innerJoin(`${this.alias}.involvedBAs`, 'BA', 'BA.id = :userId', { userId: user.id });
    }

    return this.doGetMany(builder, req.parsed, req.options);
  }

[tevenFr]

@Diluka i got the same problem when retrieving different data based on role, but i've tried to override the Crud request directly and it's not working.

const newReq: CrudRequest = {
      ...req,
      options: {
        ...req.options,
        query: {
          ...req.options.query,
          filter: () => ({}),
        },
      },
    };

return this.base.getManyBase(newReq);

and this working

    if (!user) {
      req.options.query.filter = {
        isActive: {
          $ne: false,
        },
      };
    }

    return this.base.getManyBase(req);

[Diluka]

@tevenFr maybe try this

    req.parsed.search = {
      $and: [req.parsed.search, { isEnabled: true }],
    };

you should never modify req.options by request because of #368

but there has a bug #432 ┓( ´∀` )┏ (temporary fix patch is working)

[tevenFr]

@Diluka thanks !

I have finished to do different controllers per user role