Add an auditedOfy marker method for allow-listed ofy() calls (#1138)

* Add an auditedOfy marker method for allow-listed ofy() calls

This will allow us to make sure that every usage of ofy() has been
hand-examined and specifically allowed.
This commit is contained in:
gbrodman 2021-05-10 10:55:28 -04:00 committed by GitHub
parent 6115c7eb21
commit b64a49597c
11 changed files with 40 additions and 34 deletions

View file

@ -19,7 +19,7 @@ import static com.google.common.base.Preconditions.checkNotNull;
import static com.google.common.base.Preconditions.checkState; import static com.google.common.base.Preconditions.checkState;
import static google.registry.beam.initsql.BackupPaths.getCommitLogTimestamp; import static google.registry.beam.initsql.BackupPaths.getCommitLogTimestamp;
import static google.registry.beam.initsql.BackupPaths.getExportFilePatterns; import static google.registry.beam.initsql.BackupPaths.getExportFilePatterns;
import static google.registry.model.ofy.ObjectifyService.ofy; import static google.registry.model.ofy.ObjectifyService.auditedOfy;
import static google.registry.util.DateTimeUtils.START_OF_TIME; import static google.registry.util.DateTimeUtils.START_OF_TIME;
import static google.registry.util.DateTimeUtils.isBeforeOrAt; import static google.registry.util.DateTimeUtils.isBeforeOrAt;
import static java.util.Comparator.comparing; import static java.util.Comparator.comparing;
@ -353,7 +353,7 @@ public final class Transforms {
.getEntity() .getEntity()
.filter(Transforms::isMigratable) .filter(Transforms::isMigratable)
.map(Transforms::repairBadData) .map(Transforms::repairBadData)
.map(e -> ofy().toPojo(e)) .map(e -> auditedOfy().toPojo(e))
.map(Transforms::toSqlEntity) .map(Transforms::toSqlEntity)
.orElse(null); .orElse(null);
} }

View file

@ -15,7 +15,7 @@
package google.registry.mapreduce.inputs; package google.registry.mapreduce.inputs;
import static google.registry.model.EntityClasses.ALL_CLASSES; import static google.registry.model.EntityClasses.ALL_CLASSES;
import static google.registry.model.ofy.ObjectifyService.ofy; import static google.registry.model.ofy.ObjectifyService.auditedOfy;
import com.google.appengine.api.datastore.Cursor; import com.google.appengine.api.datastore.Cursor;
import com.google.appengine.api.datastore.QueryResultIterator; import com.google.appengine.api.datastore.QueryResultIterator;
@ -232,7 +232,7 @@ class ChildEntityReader<R extends EppResource, I extends ImmutableObject> extend
/** Query for children of the current resource and of the current child class. */ /** Query for children of the current resource and of the current child class. */
@Override @Override
public QueryResultIterator<I> getQueryIterator(Cursor cursor) { public QueryResultIterator<I> getQueryIterator(Cursor cursor) {
return startQueryAt(ofy().load().type(type).ancestor(ancestor), cursor).iterator(); return startQueryAt(auditedOfy().load().type(type).ancestor(ancestor), cursor).iterator();
} }
@Override @Override

View file

@ -14,7 +14,7 @@
package google.registry.mapreduce.inputs; package google.registry.mapreduce.inputs;
import static google.registry.model.ofy.ObjectifyService.ofy; import static google.registry.model.ofy.ObjectifyService.auditedOfy;
import com.google.appengine.api.datastore.Cursor; import com.google.appengine.api.datastore.Cursor;
import com.google.appengine.api.datastore.QueryResultIterator; import com.google.appengine.api.datastore.QueryResultIterator;
@ -68,7 +68,8 @@ class CommitLogManifestReader
/** Query for children of this bucket. */ /** Query for children of this bucket. */
Query<CommitLogManifest> createBucketQuery() { Query<CommitLogManifest> createBucketQuery() {
Query<CommitLogManifest> query = ofy().load().type(CommitLogManifest.class).ancestor(bucketKey); Query<CommitLogManifest> query =
auditedOfy().load().type(CommitLogManifest.class).ancestor(bucketKey);
if (olderThan != null) { if (olderThan != null) {
query = query.filterKey( query = query.filterKey(
"<", "<",

View file

@ -15,7 +15,7 @@
package google.registry.mapreduce.inputs; package google.registry.mapreduce.inputs;
import static com.google.common.collect.ImmutableSet.toImmutableSet; import static com.google.common.collect.ImmutableSet.toImmutableSet;
import static google.registry.model.ofy.ObjectifyService.ofy; import static google.registry.model.ofy.ObjectifyService.auditedOfy;
import com.google.appengine.api.datastore.Cursor; import com.google.appengine.api.datastore.Cursor;
import com.google.appengine.api.datastore.QueryResultIterator; import com.google.appengine.api.datastore.QueryResultIterator;
@ -68,7 +68,8 @@ abstract class EppResourceBaseReader<T> extends RetryingInputReader<EppResourceI
/** Query for children of this bucket. */ /** Query for children of this bucket. */
Query<EppResourceIndex> query() { Query<EppResourceIndex> query() {
Query<EppResourceIndex> query = ofy().load().type(EppResourceIndex.class).ancestor(bucketKey); Query<EppResourceIndex> query =
auditedOfy().load().type(EppResourceIndex.class).ancestor(bucketKey);
return filterKinds.isEmpty() ? query : query.filter("kind in", filterKinds); return filterKinds.isEmpty() ? query : query.filter("kind in", filterKinds);
} }

View file

@ -14,7 +14,7 @@
package google.registry.mapreduce.inputs; package google.registry.mapreduce.inputs;
import static google.registry.model.ofy.ObjectifyService.ofy; import static google.registry.model.ofy.ObjectifyService.auditedOfy;
import com.google.appengine.tools.mapreduce.InputReader; import com.google.appengine.tools.mapreduce.InputReader;
import com.google.common.collect.ImmutableSet; import com.google.common.collect.ImmutableSet;
@ -62,7 +62,7 @@ class EppResourceEntityReader<R extends EppResource> extends EppResourceBaseRead
// Loop until we find a value, or nextQueryResult() throws a NoSuchElementException. // Loop until we find a value, or nextQueryResult() throws a NoSuchElementException.
while (true) { while (true) {
Key<? extends EppResource> key = nextQueryResult().getKey(); Key<? extends EppResource> key = nextQueryResult().getKey();
EppResource resource = ofy().load().key(key).now(); EppResource resource = auditedOfy().load().key(key).now();
if (resource == null) { if (resource == null) {
logger.atSevere().log("EppResourceIndex key %s points at a missing resource", key); logger.atSevere().log("EppResourceIndex key %s points at a missing resource", key);
continue; continue;

View file

@ -15,7 +15,7 @@
package google.registry.mapreduce.inputs; package google.registry.mapreduce.inputs;
import static com.google.common.base.Preconditions.checkNotNull; import static com.google.common.base.Preconditions.checkNotNull;
import static google.registry.model.ofy.ObjectifyService.ofy; import static google.registry.model.ofy.ObjectifyService.auditedOfy;
import com.google.appengine.api.datastore.Cursor; import com.google.appengine.api.datastore.Cursor;
import com.google.appengine.api.datastore.DatastoreTimeoutException; import com.google.appengine.api.datastore.DatastoreTimeoutException;
@ -150,7 +150,7 @@ abstract class RetryingInputReader<I, T> extends InputReader<T> {
String.format("Got an unrecoverable failure while reading item %d/%d.", loaded, total), String.format("Got an unrecoverable failure while reading item %d/%d.", loaded, total),
e); e);
} finally { } finally {
ofy().clearSessionCache(); auditedOfy().clearSessionCache();
} }
} }

View file

@ -66,15 +66,19 @@ public class ObjectifyService {
/** A singleton instance of our Ofy wrapper. */ /** A singleton instance of our Ofy wrapper. */
private static final Ofy OFY = new Ofy(null); private static final Ofy OFY = new Ofy(null);
/** /** Returns the singleton {@link Ofy} instance. */
* Returns a singleton {@link Ofy} instance.
*
* <p><b>Deprecated:</b> This will go away once everything injects {@code Ofy}.
*/
public static Ofy ofy() { public static Ofy ofy() {
return OFY; return OFY;
} }
/**
* Returns the singleton {@link Ofy} instance, signifying that the caller has been audited for the
* Registry 3.0 conversion.
*/
public static Ofy auditedOfy() {
return OFY;
}
static { static {
initOfyOnce(); initOfyOnce();
} }

View file

@ -16,6 +16,7 @@ package google.registry.persistence.transaction;
import static com.google.common.base.Preconditions.checkArgument; import static com.google.common.base.Preconditions.checkArgument;
import static com.google.common.base.Preconditions.checkNotNull; import static com.google.common.base.Preconditions.checkNotNull;
import static google.registry.model.ofy.ObjectifyService.auditedOfy;
import static google.registry.persistence.transaction.TransactionManagerFactory.ofyTm; import static google.registry.persistence.transaction.TransactionManagerFactory.ofyTm;
import static google.registry.persistence.transaction.TransactionManagerFactory.tm; import static google.registry.persistence.transaction.TransactionManagerFactory.tm;
@ -25,7 +26,6 @@ import com.google.common.collect.ImmutableList;
import com.google.storage.onestore.v3.OnestoreEntity.EntityProto; import com.google.storage.onestore.v3.OnestoreEntity.EntityProto;
import google.registry.model.Buildable; import google.registry.model.Buildable;
import google.registry.model.ImmutableObject; import google.registry.model.ImmutableObject;
import google.registry.model.ofy.ObjectifyService;
import google.registry.persistence.VKey; import google.registry.persistence.VKey;
import java.io.ByteArrayInputStream; import java.io.ByteArrayInputStream;
import java.io.ByteArrayOutputStream; import java.io.ByteArrayOutputStream;
@ -214,7 +214,7 @@ public class Transaction extends ImmutableObject implements Buildable {
@Override @Override
public void serializeTo(ObjectOutputStream out) throws IOException { public void serializeTo(ObjectOutputStream out) throws IOException {
out.writeObject(Type.UPDATE); out.writeObject(Type.UPDATE);
Entity realEntity = ObjectifyService.ofy().toEntity(entity); Entity realEntity = auditedOfy().toEntity(entity);
EntityProto proto = EntityTranslator.convertToPb(realEntity); EntityProto proto = EntityTranslator.convertToPb(realEntity);
out.write(VERSION_ID); out.write(VERSION_ID);
proto.writeDelimitedTo(out); proto.writeDelimitedTo(out);
@ -223,7 +223,7 @@ public class Transaction extends ImmutableObject implements Buildable {
public static Update deserializeFrom(ObjectInputStream in) throws IOException { public static Update deserializeFrom(ObjectInputStream in) throws IOException {
EntityProto proto = new EntityProto(); EntityProto proto = new EntityProto();
proto.parseDelimitedFrom(in); proto.parseDelimitedFrom(in);
return new Update(ObjectifyService.ofy().toPojo(EntityTranslator.createFromPb(proto))); return new Update(auditedOfy().toPojo(EntityTranslator.createFromPb(proto)));
} }
} }

View file

@ -163,9 +163,9 @@ public interface TransactionManager {
* Updates an entity in the database without writing commit logs if the underlying database is * Updates an entity in the database without writing commit logs if the underlying database is
* Datastore. * Datastore.
* *
* <p>This method is for the sake of keeping a single code path when replacing ofy() with tm() in * <p>This method is for the sake of keeping a single code path when replacing ofy calls with tm()
* the application code. When the method is invoked with Datastore, it won't write the commit log * in the application code. When the method is invoked with Datastore, it won't write the commit
* backup; when invoked with Cloud SQL, it behaves the same as the method which doesn't have * log backup; when invoked with Cloud SQL, it behaves the same as the method which doesn't have
* "WithoutBackup" in its method name because it is not necessary to have the backup mechanism in * "WithoutBackup" in its method name because it is not necessary to have the backup mechanism in
* SQL. * SQL.
*/ */
@ -175,9 +175,9 @@ public interface TransactionManager {
* Updates all entities in the database without writing any backup if the underlying database is * Updates all entities in the database without writing any backup if the underlying database is
* Datastore. * Datastore.
* *
* <p>This method is for the sake of keeping a single code path when replacing ofy() with tm() in * <p>This method is for the sake of keeping a single code path when replacing ofy calls with tm()
* the application code. When the method is invoked with Datastore, it won't write the commit log * in the application code. When the method is invoked with Datastore, it won't write the commit
* backup; when invoked with Cloud SQL, it behaves the same as the method which doesn't have * log backup; when invoked with Cloud SQL, it behaves the same as the method which doesn't have
* "WithoutBackup" in its method name because it is not necessary to have the backup mechanism in * "WithoutBackup" in its method name because it is not necessary to have the backup mechanism in
* SQL. * SQL.
*/ */

View file

@ -17,7 +17,7 @@ package google.registry.rdap;
import static com.google.common.collect.ImmutableSet.toImmutableSet; import static com.google.common.collect.ImmutableSet.toImmutableSet;
import static google.registry.model.EppResourceUtils.loadByForeignKey; import static google.registry.model.EppResourceUtils.loadByForeignKey;
import static google.registry.model.index.ForeignKeyIndex.loadAndGetKey; import static google.registry.model.index.ForeignKeyIndex.loadAndGetKey;
import static google.registry.model.ofy.ObjectifyService.ofy; import static google.registry.model.ofy.ObjectifyService.auditedOfy;
import static google.registry.persistence.transaction.TransactionManagerFactory.jpaTm; import static google.registry.persistence.transaction.TransactionManagerFactory.jpaTm;
import static google.registry.request.Action.Method.GET; import static google.registry.request.Action.Method.GET;
import static google.registry.request.Action.Method.HEAD; import static google.registry.request.Action.Method.HEAD;
@ -206,7 +206,7 @@ public class RdapDomainSearchAction extends RdapSearchActionBase {
RdapResultSet<DomainBase> resultSet; RdapResultSet<DomainBase> resultSet;
if (isDatastore()) { if (isDatastore()) {
Query<DomainBase> query = Query<DomainBase> query =
ofy() auditedOfy()
.load() .load()
.type(DomainBase.class) .type(DomainBase.class)
.filter("fullyQualifiedDomainName <", partialStringQuery.getNextInitialString()) .filter("fullyQualifiedDomainName <", partialStringQuery.getNextInitialString())
@ -261,7 +261,7 @@ public class RdapDomainSearchAction extends RdapSearchActionBase {
int querySizeLimit = RESULT_SET_SIZE_SCALING_FACTOR * rdapResultSetMaxSize; int querySizeLimit = RESULT_SET_SIZE_SCALING_FACTOR * rdapResultSetMaxSize;
RdapResultSet<DomainBase> resultSet; RdapResultSet<DomainBase> resultSet;
if (isDatastore()) { if (isDatastore()) {
Query<DomainBase> query = ofy().load().type(DomainBase.class).filter("tld", tld); Query<DomainBase> query = auditedOfy().load().type(DomainBase.class).filter("tld", tld);
if (cursorString.isPresent()) { if (cursorString.isPresent()) {
query = query.filter("fullyQualifiedDomainName >", cursorString.get()); query = query.filter("fullyQualifiedDomainName >", cursorString.get());
} }
@ -546,7 +546,7 @@ public class RdapDomainSearchAction extends RdapSearchActionBase {
numHostKeysSearched += chunk.size(); numHostKeysSearched += chunk.size();
if (isDatastore()) { if (isDatastore()) {
Query<DomainBase> query = Query<DomainBase> query =
ofy() auditedOfy()
.load() .load()
.type(DomainBase.class) .type(DomainBase.class)
.filter( .filter(

View file

@ -15,7 +15,7 @@
package google.registry.rdap; package google.registry.rdap;
import static com.google.common.base.Charsets.UTF_8; import static com.google.common.base.Charsets.UTF_8;
import static google.registry.model.ofy.ObjectifyService.ofy; import static google.registry.model.ofy.ObjectifyService.auditedOfy;
import static google.registry.persistence.transaction.TransactionManagerFactory.jpaTm; import static google.registry.persistence.transaction.TransactionManagerFactory.jpaTm;
import static google.registry.util.DateTimeUtils.END_OF_TIME; import static google.registry.util.DateTimeUtils.END_OF_TIME;
@ -358,7 +358,7 @@ public abstract class RdapSearchActionBase extends RdapActionBase {
"Initial search string must be at least %d characters", "Initial search string must be at least %d characters",
RdapSearchPattern.MIN_INITIAL_STRING_LENGTH)); RdapSearchPattern.MIN_INITIAL_STRING_LENGTH));
} }
Query<T> query = ofy().load().type(clazz); Query<T> query = auditedOfy().load().type(clazz);
if (!partialStringQuery.getHasWildcard()) { if (!partialStringQuery.getHasWildcard()) {
query = query.filter(filterField, partialStringQuery.getInitialString()); query = query.filter(filterField, partialStringQuery.getInitialString());
} else { } else {
@ -457,7 +457,7 @@ public abstract class RdapSearchActionBase extends RdapActionBase {
"Initial search string must be at least %d characters", "Initial search string must be at least %d characters",
RdapSearchPattern.MIN_INITIAL_STRING_LENGTH)); RdapSearchPattern.MIN_INITIAL_STRING_LENGTH));
} }
Query<T> query = ofy().load().type(clazz).filter(filterField, queryString); Query<T> query = auditedOfy().load().type(clazz).filter(filterField, queryString);
if (cursorString.isPresent()) { if (cursorString.isPresent()) {
if (cursorField.isPresent()) { if (cursorField.isPresent()) {
query = query.filter(cursorField.get() + " >", cursorString.get()); query = query.filter(cursorField.get() + " >", cursorString.get());
@ -526,7 +526,7 @@ public abstract class RdapSearchActionBase extends RdapActionBase {
"Initial search string must be at least %d characters", "Initial search string must be at least %d characters",
RdapSearchPattern.MIN_INITIAL_STRING_LENGTH)); RdapSearchPattern.MIN_INITIAL_STRING_LENGTH));
} }
Query<T> query = ofy().load().type(clazz); Query<T> query = auditedOfy().load().type(clazz);
if (!partialStringQuery.getHasWildcard()) { if (!partialStringQuery.getHasWildcard()) {
query = query.filterKey("=", Key.create(clazz, partialStringQuery.getInitialString())); query = query.filterKey("=", Key.create(clazz, partialStringQuery.getInitialString()));
} else { } else {