GEODE-5074 REST dev client should not access or modify internal regions

[bschuchardt]

The Developer REST API was accessing a raw cache for all operations.
I've switched it to use a filtered InternalCacheForClientAccess cache
instead for most operations. It needed to have the unfiltered cache in
order to find the ParameterizedQueries region for accessing saved
queries.

Thank you for submitting a contribution to Apache Geode.

In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:

For all changes:

• Is there a JIRA ticket associated with this PR? Is it referenced in the commit message?

• Has your PR been rebased against the latest commit within the target branch (typically develop)?

• Is your initial contribution a single, squashed commit?

• Does gradlew build run cleanly?

• Have you written or updated unit tests to verify your changes?

• If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under ASF 2.0?

Note:

Please ensure that once the PR is submitted, you check travis-ci for build issues and
submit an update to your PR as soon as possible. If you need help, please send an
email to dev@geode.apache.org.

[agingade]

InternalRegion extends Region, do we need instance check.

[bschuchardt]

The java compiler would not let me add the InternalRegions to the "regions" HashSet without casting to Region first. Since that was required I felt obliged to perform an instanceof check.

[jinmeiliao]

Instead of adding an interface method here, can we just have the getInternalCache return the InternalCacheForClientAccess instance?

[bschuchardt]

We can't do that because we need to be able to get the ParameterizedQueries internal Region and InternalCacheForClientAccess won't allow that.

[jinmeiliao]

so here, we can return the InternalCacheForClientAccess object. Looks like all the caller of this method needs it for client access in the controller except for getting the querystore. We can add a method in the InternalCacheForClientAccess object to get the queryStore since looks like we should allow that for client access.

[bschuchardt]

I like that idea

[jinmeiliao]

I don't think we need to add this interface here. Just add it as a method in CacheForClientAccess object. The CacheProvider is an internal interface and is only used in the geode-web-api module, we can have it return CacheForClientAccess object instead of InternalCache object. This method doesn't belong to the regular cache, in my opinion.

Also, the getQueryStore method doesn't need a region path passed to it, right? It's always going to be the " "ParameterizedQueries" static string. You can't pass other string to it to get the query store at all. So we can just do this:

public <K, V> Region<K, V> getQueryStore() {
Region<K, V> result = delegate.getRegion(PARAMETERIZED_QUERIES_REGION);
return result;
}

[bschuchardt]

I disagree. InternalCacheForClientAccess implements InternalCache and should be indistinguishable from other InternalCache implementations. I liked your first idea because it removed knowledge of server vs client cache access from the REST code.
Having getQueryStore take an argument is more flexible and leaves it open to allowing there to be multiple query stores. Since that's the way the REST code was written I think it should stay like that.

[bschuchardt]

I would consider changing the method name from getQueryStore to getInternalRegion though. I think that would make the JMX code that I'm now working on cleaner because I could install an InternalCacheForClientAccess into it and then use getInternalRegion in specific places where it needs to get its "monitoring regions"

[jinmeiliao]

but then wouldn't the client just pass in any internal region name to that method and get access to any internal region? I believe in the REST code, any caller to that method is passing in the same string... (but I may have missed something).

In my thought, CacheForClientAccess is a child of InternalCache which has restricted access to the regions, but aside from that, it needs to access some "internal regions" for legacy reasons. InternalCache already has that capability in the "getRegion" method, so it doesn't need anymore method to get a special internal region. But for CacheClientAccess access, some special internal region will be allowed access by adding some additional method. My 2 cents.

[bschuchardt]

This isn't code that's accessible from a client. This is server-side code and it's okay for that code to intentionally get an internal region. What we don't want is someone using the REST API to be able to send a request that modifies an internal region or returns privileged information from an internal region.

[bschuchardt]

Ah, I think if I have the REST code assume it has an InternalCacheForClientAccess instead of an InternalCache I can remove the getQueryStore method from InternalCache and GemFireCacheImpl. Yeah, that will work. Thanks Jinmei

[jinmeiliao]

Yes, that's what I meant as well. The rest code should be working with InternalCacheForClientAccess alone because that is what it needs. I like the renaming that method to getInterRegion for a more general purpose.

[bschuchardt]

I've removed the getQueryStore method from InternalCache and GemFireCacheImpl and modified the REST code to know that it's working with an InternalCacheForClientAccess so it can use the getQueryStore method (which I've renamed to getInternalRegion for upcoming JMX work). With that I'm closing this PR.