Skip to content

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

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.

Sign up for GitHub

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

Closed
bschuchardt wants to merge 2 commits into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f886b63
GEODE-5074 REST dev client should not access or modify internal regions
bschuchardt Nov 14, 2018
97a01fd
GEODE-5074 REST dev client should not access or modify internal regions
bschuchardt Nov 15, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions ...c/integrationTest/java/org/apache/geode/rest/internal/web/RestServersIntegrationTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,13 @@ public void testGet() throws Exception {
.hasStatusCode(HttpStatus.SC_OK);
}

@Test
public void testGetOnInternalRegion() {
assertResponse(
restClient.doGet("/__PR", null, null))
.hasStatusCode(HttpStatus.SC_NOT_FOUND);
}

@Test
public void testServerStartedOnDefaultPort() throws Exception {
JsonNode jsonArray = assertResponse(restClient.doGet("/servers", null, null))
Expand Down
4 changes: 4 additions & 0 deletions geode-core/src/main/java/org/apache/geode/internal/cache/GemFireCacheImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3149,6 +3149,10 @@ public Region getRegion(String path) {
return getRegion(path, false);
}

public <K, V> Region<K, V> getQueryStore(String path) {
return getRegion(path, false);
}

/**
* returns a set of all current regions in the cache, including buckets
*
Expand Down
2 changes: 2 additions & 0 deletions geode-core/src/main/java/org/apache/geode/internal/cache/InternalCache.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,8 @@ <K, V> RegionAttributes<K, V> invokeRegionBefore(InternalRegion parent, String n

QueryService getLocalQueryService();

<K, V> Region<K, V> getQueryStore(String storeName);
Copy link
Copy Markdown
Member

@jinmeiliao jinmeiliao Nov 15, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
}

Copy link
Copy Markdown
Contributor Author

@bschuchardt bschuchardt Nov 15, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

@bschuchardt bschuchardt Nov 15, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"

Copy link
Copy Markdown
Member

@jinmeiliao jinmeiliao Nov 15, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

@bschuchardt bschuchardt Nov 15, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

@bschuchardt bschuchardt Nov 15, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member

@jinmeiliao jinmeiliao Nov 16, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


void registerInterestStarted();

void registerInterestCompleted();
Expand Down
6 changes: 6 additions & 0 deletions geode-core/src/main/java/org/apache/geode/internal/cache/InternalCacheForClientAccess.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,12 @@ public <K, V> Region<K, V> getRegion(String path) {
return result;
}

@Override
public <K, V> Region<K, V> getQueryStore(String path) {
Region<K, V> result = delegate.getRegion(path);
return result;
}

@Override
public Region getRegion(String path, boolean returnDestroyedRegion) {
Region result = delegate.getRegion(path, returnDestroyedRegion);
Expand Down
5 changes: 5 additions & 0 deletions geode-core/src/main/java/org/apache/geode/internal/cache/xmlcache/CacheCreation.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1126,6 +1126,11 @@ public Region getRegion(String path) {
return this.roots.get(path);
}

@Override
public Region getQueryStore(String path) {
return getRegion(path);
}

@Override
public CacheServer addCacheServer() {
return addCacheServer(false);
Expand Down
4 changes: 2 additions & 2 deletions .../src/main/java/org/apache/geode/rest/internal/web/controllers/AbstractBaseController.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ public void afterPropertiesSet() {
}

protected InternalCache getCache() {
InternalCache cache = cacheProvider.getInternalCache();
InternalCache cache = cacheProvider.getCache();
Assert.state(cache != null, "The Gemfire Cache reference was not properly initialized");
return cache;
}
Expand Down Expand Up @@ -402,7 +402,7 @@ void checkForQueryIdExist(String region, String key) {
}

Region<String, String> getQueryStore(final String namePath) {
return ValidationUtils.returnValueThrowOnNull(getCache().<String, String>getRegion(namePath),
return ValidationUtils.returnValueThrowOnNull(getCache().getQueryStore(namePath),
new GemfireRestException(String.format("Query store does not exist!", namePath)));
}

Expand Down
9 changes: 8 additions & 1 deletion ...pi/src/main/java/org/apache/geode/rest/internal/web/controllers/CommonCrudController.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
package org.apache.geode.rest.internal.web.controllers;

import java.util.ArrayList;
import java.util.HashSet;
import java.util.List;
import java.util.Set;

Expand All @@ -38,6 +39,7 @@
import org.apache.geode.cache.execute.FunctionException;
import org.apache.geode.cache.execute.FunctionService;
import org.apache.geode.cache.execute.ResultCollector;
import org.apache.geode.internal.cache.InternalRegion;
import org.apache.geode.internal.cache.execute.util.FindRestEnabledServersFunction;
import org.apache.geode.internal.logging.LogService;
import org.apache.geode.rest.internal.web.controllers.support.RestServersResultCollector;
Expand Down Expand Up @@ -73,7 +75,12 @@ public ResponseEntity<?> regions() {
logger.debug("Listing all resources (Regions) in Geode...");
final HttpHeaders headers = new HttpHeaders();
headers.setLocation(toUri());
final Set<Region<?, ?>> regions = getCache().rootRegions();
final Set<Region<?, ?>> regions = new HashSet<>();
for (InternalRegion region : getCache().getApplicationRegions()) {
if (region instanceof Region) {
Copy link
Copy Markdown

@agingade agingade Nov 15, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

InternalRegion extends Region, do we need instance check.

Copy link
Copy Markdown
Contributor Author

@bschuchardt bschuchardt Nov 15, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

regions.add(region);
}
} ;
String listRegionsAsJson = JSONUtils.formulateJsonForListRegions(regions, "regions");
return new ResponseEntity<>(listRegionsAsJson, headers, HttpStatus.OK);
}
Expand Down
2 changes: 1 addition & 1 deletion ...i/src/main/java/org/apache/geode/rest/internal/web/controllers/QueryAccessController.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -244,7 +244,7 @@ public ResponseEntity<String> runNamedQuery(@PathVariable("query") String queryI
Query compiledQuery = compiledQueries.get(queryId);
if (compiledQuery == null) {
// This is first time the query is seen by this server.
final String oql = getValue(PARAMETERIZED_QUERIES_REGION, queryId, false);
final String oql = getQueryStore(PARAMETERIZED_QUERIES_REGION).get(queryId);

ValidationUtils.returnValueThrowOnNull(oql, new ResourceNotFoundException(
String.format("No Query with ID (%1$s) was found!", queryId)));
Expand Down
2 changes: 1 addition & 1 deletion ...i/src/main/java/org/apache/geode/rest/internal/web/controllers/support/CacheProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,5 +18,5 @@

public interface CacheProvider {

InternalCache getInternalCache();
InternalCache getCache();
}
9 changes: 7 additions & 2 deletions ...c/main/java/org/apache/geode/rest/internal/web/controllers/support/CacheProviderImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,12 +18,17 @@

import org.apache.geode.internal.cache.GemFireCacheImpl;
import org.apache.geode.internal.cache.InternalCache;
import org.apache.geode.internal.cache.InternalCacheForClientAccess;

@Component("cacheProvider")
public class CacheProviderImpl implements CacheProvider {

@Override
public InternalCache getInternalCache() {
return GemFireCacheImpl.getExisting();
public InternalCache getCache() {
InternalCache result = GemFireCacheImpl.getExisting();
if (result == null) {
return null;
}
return new InternalCacheForClientAccess(result);
}
}