Check registrar existence prior to verifying access

This way the error messages are more sensible when a registrar doesn't exist
(which realistically shouldn't happen in the typical case anyway).

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=240376239
This commit is contained in:
mcilwain 2019-03-26 10:30:39 -07:00 committed by jianglai
parent bb09f259b3
commit 4240be268a
3 changed files with 6 additions and 8 deletions

View file

@ -220,16 +220,13 @@ public class AuthenticatedRegistrarAccessor {
* @param clientId ID of the registrar we request * @param clientId ID of the registrar we request
*/ */
public Registrar getRegistrar(String clientId) throws RegistrarAccessDeniedException { public Registrar getRegistrar(String clientId) throws RegistrarAccessDeniedException {
// Verify access before checking if the registrar exists, in order to not leak information
// about objects in the system the user doesn't have permissions on.
verifyAccess(clientId);
Registrar registrar = Registrar registrar =
Registrar.loadByClientId(clientId) Registrar.loadByClientId(clientId)
.orElseThrow( .orElseThrow(
() -> () ->
new RegistrarAccessDeniedException( new RegistrarAccessDeniedException(
String.format("Registrar %s not found", clientId))); String.format("Registrar %s does not exist", clientId)));
verifyAccess(clientId);
if (!clientId.equals(registrar.getClientId())) { if (!clientId.equals(registrar.getClientId())) {
logger.atSevere().log( logger.atSevere().log(

View file

@ -349,7 +349,7 @@ public class AuthenticatedRegistrarAccessorTest {
expectGetRegistrarFailure( expectGetRegistrarFailure(
"BadClientId", "BadClientId",
GAE_ADMIN, GAE_ADMIN,
"admin admin@gmail.com doesn't have access to registrar BadClientId"); "Registrar BadClientId does not exist");
verifyZeroInteractions(lazyGroupsConnection); verifyZeroInteractions(lazyGroupsConnection);
} }

View file

@ -113,14 +113,15 @@ public final class OteStatusActionTest {
} }
@Test @Test
public void testFailure_noRegistrar() { public void testFailure_registrarDoesntExist() {
assertThat(action.handleJsonRequest(ImmutableMap.of("clientId", "nonexistent-3"))) assertThat(action.handleJsonRequest(ImmutableMap.of("clientId", "nonexistent-3")))
.containsExactlyEntriesIn( .containsExactlyEntriesIn(
errorResultWithMessage("TestUserId doesn't have access to registrar nonexistent-3")); errorResultWithMessage("Registrar nonexistent-3 does not exist"));
} }
@Test @Test
public void testFailure_notAuthorized() { public void testFailure_notAuthorized() {
persistNewRegistrar(CLIENT_ID, "blobio-1", Type.REAL, 1L);
action.registrarAccessor = action.registrarAccessor =
AuthenticatedRegistrarAccessor.createForTesting(ImmutableSetMultimap.of()); AuthenticatedRegistrarAccessor.createForTesting(ImmutableSetMultimap.of());
assertThat(action.handleJsonRequest(ImmutableMap.of("clientId", CLIENT_ID))) assertThat(action.handleJsonRequest(ImmutableMap.of("clientId", CLIENT_ID)))