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

Add ability to only expose repository methods explicitly declared for exposure [DATAREST-1176] #1510

Closed
spring-projects-issues opened this issue Jan 12, 2018 · 15 comments

Comments

@spring-projects-issues
Copy link

@spring-projects-issues spring-projects-issues commented Jan 12, 2018

Tobias Weiss opened DATAREST-1176 and commented

The currently available detection strategies in SDR only allow to restrict REST repositories on class level. So when a Repository is exported, all of its methods are exported, too. Only by using RestResource(exported = false), you can prevent SDR from exporting a given method.

We identified in our project, that there is a certain security risk in that case. Developers are not always aware of all the methods that are automatically exported via REST by the application. By simply adding new Repositories and just wanting a findAll()-method to be publicly available, even save and delete methods are exported by default. As most applications want to apply security especially on the write methods, an additional "pessimistic" strategy can be useful in Spring. That way you can still profit from all the benefits SDR provides, but you can be sure, that only methods you explicitly added and annotated with @RestResource are exported.

The following example shows how the exporting with the new strategy should work:

@RepositoryRestResource
interface PersonRepository extends Repository<Person, Long> {

  @RestResource
  Iterable<Person> findAll();

  Iterable<Person> findByFirstname(@Param("firstname") String firstname);
}

In that case, only the findAll() method is exported via REST. The findByFirstName and all CRUD methods like save or delete are not exported via REST by default. They have to be added explictily and annotated with @RestResource if they shall be exported via REST


Issue Links:

  • DATAREST-1034 Allow overriding exposure defined at the type level on the method
    ("is duplicated by")
  • DATAREST-1268 Support RepositoryDetectionStrategies.EXPLICIT_METHOD_ANNOTATED

Backported to: 3.0.3 (Kay SR3), 2.6.10 (Ingalls SR10)

1 votes, 5 watchers

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented Jan 12, 2018

Oliver Drotbohm commented

If you want to be so very selective about the exposed functionality, why not write a single controller method that just calls that very method?

I am not sure we should broaden the semantics of RepositoryDetectionStrategy. The name strongly suggests that it's about detecting repositories, not about the exposure of individual methods. What I can imagine though is to add an attribute to @RepositoryRestResource that allows inverting the default for the method exposure detection so that methods would only be considered exported if explicitly annotated

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented Jan 15, 2018

Tobias Weiss commented

We had the same idea of adding an attribute to @RepositoryRestResource. But we thought it is better to use a global detection strategy to handle this feature. The problem with the attribute is, that we must add it to every Repository, or create our own parent AbstractRepository (which does not prevent a developer from using the one from Spring directly). We consider this a security risk because it can be difficult to keep track all repositories.
We implemented a custom detection strategy which allows us to only export explicitly annotated methods. You can have a look at our implementation here: tobiasweiss93@a50113a

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented Jan 15, 2018

Oliver Drotbohm commented

That looks interesting. There's a couple of things I don't quite like about the approach (again: bending the repository detection strategy to now include which methods to expose by default). However, it looks nice enough to try to tweak it and see where this could go. Do you think you could submit that commit as pull request against Spring Data REST so that I could play with it a little?

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented Jan 15, 2018

Tobias Weiss commented

I've created the pull request #286

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented Jan 15, 2018

Oliver Drotbohm commented

I gave this a quick spin and you can find our combined changes here now. I basically expose the new criterion on RepositoryDetectionStrategy with all previously existing enum values returning true. I changed the implementation to eagerly resolve that flag then and use that instead of the strategy instance.

I am still not quite sure about the naming of the enum, as that's where the mismatch of the concepts now becomes most clear. There's just no nice name to mention the additional method exposure defaulting. Yet another different option would be to introduce a flag on RepositoryRestConfiguration and rather hand this into RepositoryResourceMappings instead of RepositoryDetectionStrategy the latter can be obtained via the former. We could then introduce a helper method to allow people to switch to the safe mode (i.e. RepositoryDetectionStrategies.ANNOTATED + methods not exported by default) with a convenience method on RepositoryRestConfiguration.

WDYT?

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented Jan 15, 2018

Johannes Rudolph commented

Let me jump in here and add a few thoughts. I'm on the same team as Tobi. We have an array of micro-services in production with SDR at meshcloud.io

While we found SDR was great for getting an API up and running quickly, we had some issues scaling it to services with more domain complexity than plain CRUD.
For example, our multi-tenant API needs to do properly check user authz and tenant-scoping. Some endpoints may be read-only for some users, others may support POST but no PUT etc.

With SDRs defaults, we repeatedly found that it was too easy to leave a Repository method "exported" (especially inherited save/findAll/delete methods) or accidentlly expose a "priviledged" method that executes a query across tenants in our database.
We considered two options:
a) wrapping the full API in Controllers
b) devise a way to force "opt-in" rather than "opt-out"

We chose the latter option as we do have som plain CRUD that we want to leverage SDR for and we found it painful to wrap SDR manually in a controller while retaining its full capabilities (projections, search, paging etc.).
The "opt-in" strategy implemented here gives us much more confidence in the actually exposed API surface area.

Re. naming: maybe the RepositoryDetectionStrategy should also have a companion "MethodDetectionStrategy" instead? This would split responsibility for finding the right repositories (i.e. based on package, annotations etc.) and then selecting the appropriate methods from the repositories?

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented Jan 16, 2018

Miguel Pereira commented

This would be a great feature. I'd also prefer if the there existed a detection strategy exported no methods by default unless annotated.

Also related. From a multi module approach I've used the default repository detection strategy in Module A and annotated in Module B which depends on module A. This allows me to re-use the repository / services defined in module A in module B without also exporting them. The issues arises when a new developer creates a repository and isn't aware that he/she should not annotate the interface or when we would like to use the annotation to customize the the excerpt projection, path, or when someone decides to make module C and doesn't have as much experience with SDR.

As mentioned above in my use case we also needed more control over the basic operations (save/findOne/delete etc) for complex security / auditing requirements (some of which can be done in the event handlers). I ended up disabling exporting altogether and writing my own controllers that still use core SDR classes like Repositories, RepositoriesEntityLinks, PersistentEntityResourceAssembler... Copying some of the methods in AbstractRepositoryRestController and mimicing RepositoryEntityController since they are package private and I cannot extend them directly. Still not sure how to get access to RootResourceInformation :/

Also I couldn't figure out how to add discoverable links to the RepositorySearchController for repositories that where not exported so I ended up adding my own request handler for /search in my custom controller. I think id like to disable the RepositoryController, RepositorySearchController altogether.

Sorry if I'm mixing issues / topics just wanted to describe how I'm attempting to use SDR. (which I like a lot :)

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented Jan 16, 2018

Oliver Drotbohm commented

I've just pushed a proper feature branch for this issue that already produced binaries for you to test. On the highest level you can now basically call RepositoryRestConfiguration.disableDefaultExposure() and it will set the repository detection strategy to ANNOTATED and disable the default exposure of methods. The move to that approach actually simplified a bit of stuff in the APIs of the mapping subsystem which makes me feel quite comfortable with staying with this approach.

It would be cool if you could give the snapshots a try (use version 3.1.0-DATAREST-1176-SNAPSHOTS) and see if this behaves as expected. If so I'd finalize this by adding the newly introduced knobs to the docs and also create an issue with Spring Boot to expose that new flag via application properties

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented Jan 17, 2018

Tobias Weiss commented

I reviewed your changes. The solution is exactly what I prefer. I think it is a very nice idea to configure the default exposure value on a central place and use the ANNOTATED detection strategy to provide our new feature. I agree with you that this approach simplifies the API stuff.
But currently we can't test your changes with our production system which has a large test base to check the repository behavior. We use spring boot 1.5.9. We cannot upgrade easily to the new major version (several changes would be required and we want to wait for the final version of Spring Boot 2).
Could you perhaps also create a snapshot that is compatible to spring boot 1.5.9?

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented Jan 18, 2018

Oliver Drotbohm commented

There's now a 2.6.10.DATAREST-1176-SNAPSHOT binary that should be usable with Boot 1.5

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented Jan 18, 2018

Tobias Weiss commented

Thank you for the new snapshot. I tested our project with the new feature and it runs perfectly. All our endpoint behavior tests runs correctly. Very nice!

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented Jan 18, 2018

Oliver Drotbohm commented

Thanks for the quick feedback. I'll go ahed and merge the changes then. And then update the docs… 😬

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented Jan 19, 2018

Oliver Drotbohm commented

That's merged, backported and available in the canonical snapshot binaries now. Ingalls SR10 coming next week

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented Jan 19, 2018

Tobias Weiss commented

Awesome :) Thanks a lot to integrate our feature so quickly!

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented Jan 19, 2018

Oliver Drotbohm commented

You're most welcome. Feedback always appreciated

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants