fix(repository): remove hidden properties from entities

[raymondfeng]

See #1914

Checklist

• npm test passes on your machine
• New tests added or existing tests modified to cover all changes
• Code conforms with the style guide
• API Documentation in code was updated
• Documentation in /docs/site was updated
• Affected artifact templates in packages/cli were updated
• Affected example projects in examples/* were updated

[hacksparrow]

LGTM, AppVeyor is failing though.

[raymondfeng]

@hacksparrow That was due to a timeout.

[bajtos]

Besides the comments above:

Please add few more tests to show and verify:

1. How to work with hidden properties from TS/JS code (e.g. create a custom repository method called login that's accessing hidden password hash).
2. How are hidden properties converted into JSON response bodies and how are they treated by query filters.

[bajtos]

I think this is wrong behavior. In LB4, the Repository API is the only way how to access and modify model data. How are we going to modify secret properties after your change?

IMO, the current implementation is correct, secret properties must stay available to JS/TS code.

What we may need to change is the way how CRUD Controller methods and/or REST layer converts the data returned by remote methods into JSON payload send to the clients.

[raymondfeng]

The hidden/protected(secret) properties are really interpreted as writeonly during serialization in LoopBack 3. They can be written but not read/search. Please note that a secret property can still be modified. For example:

repo.updateById(note.id, {secret: 'new-secret'});

But I agree with you that unconditionally removing them from CRUD is not ideal as we won't be able to support the use cases such as username/password verification.

I propose the following:

1. Improve juggler so that options to control how to honor secret properties can be set at method level too in addition to datasource/model scope.

2. Update CRUD controller code template/base class to hide secret properties for REST requests.

3. For juggler.next, we need to reevaluate such property modifiers.

[bajtos]

Let's stop growing the size of Note model please. Soon it will be way too large to reasonably work with.

Create a new model for the new hiddenProperties tests. This new model should be small and contain only properties needed to verify how hidden properties are treated.

[bajtos]

• Improve juggler so that options to control how to honor secret properties can be set at method level too in addition to datasource/model scope.

• Update CRUD controller code template/base class to hide secret properties for REST requests.

+1

Should we close this pull request then?

---

Based on the recent discussions we had around hidden properties (a black-list based approach), I think we also need to look into ways how to allow developers to use white-list based approach too:

(1) Controller methods returning data should specify a white-list of properties to include in the result. I think this is already available via filter.fields option, but the usage can be cumbersome.

class MyController {
  // ...
  @get('/profiles')
  async findProfiles(
    @param.query.object('filter', getFilterSchemaFor(Todo)) filter?: Filter,
  ): Promise<Profile[]> {
    const WHITELIST = ['id', 'email', 'name', 'created_at'];
    if (!filter.fieds) {
      filter.fields = WHITELIST;
    } else {
      filter.fields = filter.fields.filter(f => WHITELIST.includes(f));
    }
    return await this.repo.find(filter);
  }
}

(2) Controller methods modifying data should specify a white-list of properties that can be modified by the operation. This white-list should be reflected in the OpenAPI spec too (see #1179). A mock-up to illustrate what I mean:

class MyController {
  // ...
 @post('/profiles')
  async createProfile(@requestBody(/* need to configure whitelist */) todo: Todo) {
    return await this.todoRepo.create(todo, {
       // id not allowed
       inputPropertyWhitelist: ['name', 'email'],
       // the response includes auto-created fields, but no secrets
       outputPropertyWhitelist: ['id', 'name', 'email', 'created_at'],
    });
  }
}

[bajtos]

@raymondfeng what's the status of this pull request? Should we close it as abandoned?

[raymondfeng]

Abandon the PR per review discussions.