Skip to content

Commit 6e036e0

Browse files
XN137XN137
authored
Fix REST responses for failed Admin operations (#2291)
* Fix REST responses for failed Admin operations the `boolean` return values of many methods in `PolarisAdminService` were often simply not getting used at all, thus the REST api returned success in those cases even though the `PrivilegeResult` was marked as failed. due to this fix a silently failing test now needs to be adjusted. we return the `PrivilegeResult` instead of a `boolean` to give the client at least some indication of what has gone wrong on the server side. note that some of the other operations were throwing Expcetions already, which are already reported back correctly to the client. * review: use http 400 BAD_REQUEST
1 parent c4fc848 commit 6e036e0

File tree

9 files changed

+479
-523
lines changed

9 files changed

+479
-523
lines changed

‎integration-tests/src/main/java/org/apache/polaris/service/it/test/PolarisManagementServiceIntegrationTest.java

Lines changed: 23 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -126,7 +126,9 @@ public static void setup(PolarisApiEndpoints endpoints, ClientCredentials creden
126126

127127
@AfterAll
128128
public static void close() throws Exception {
129-
client.close();
129+
if (client != null) {
130+
client.close();
131+
}
130132
}
131133

132134
@AfterEach
@@ -1723,7 +1725,7 @@ public void testCatalogAdminGrantAndRevokeCatalogRoles() {
17231725
.managementApi(catalogAdminToken)
17241726
.request(
17251727
"v1/principal-roles/"
1726-
+ principalRoleName
1728+
+ principalRoleName2
17271729
+ "/catalog-roles/"
17281730
+ catalogName
17291731
+ "/"
@@ -1738,14 +1740,32 @@ public void testCatalogAdminGrantAndRevokeCatalogRoles() {
17381740
managementApi
17391741
.request(
17401742
"v1/principal-roles/"
1741-
+ principalRoleName
1743+
+ principalRoleName2
17421744
+ "/catalog-roles/"
17431745
+ catalogName
17441746
+ "/"
17451747
+ catalogRoleName)
17461748
.delete()) {
17471749
assertThat(response).returns(Response.Status.NO_CONTENT.getStatusCode(), Response::getStatus);
17481750
}
1751+
1752+
// trigger an internal error by using "principalRoleName" instead of "principalRoleName2"
1753+
try (Response response =
1754+
managementApi
1755+
.request(
1756+
"v1/principal-roles/"
1757+
+ principalRoleName
1758+
+ "/catalog-roles/"
1759+
+ catalogName
1760+
+ "/"
1761+
+ catalogRoleName)
1762+
.delete()) {
1763+
assertThat(response)
1764+
.returns(Response.Status.BAD_REQUEST.getStatusCode(), Response::getStatus);
1765+
assertThat(response.hasEntity()).isTrue();
1766+
ErrorResponse errorResponse = response.readEntity(ErrorResponse.class);
1767+
assertThat(errorResponse.message()).contains("Operation failed: GRANT_NOT_FOUND");
1768+
}
17491769
}
17501770

17511771
@Test

‎runtime/service/src/main/java/org/apache/polaris/service/admin/PolarisAdminService.java

Lines changed: 81 additions & 108 deletions
Large diffs are not rendered by default.

‎runtime/service/src/main/java/org/apache/polaris/service/admin/PolarisServiceImpl.java

Lines changed: 84 additions & 48 deletions
Original file line numberDiff line numberDiff line change
@@ -20,13 +20,15 @@
2020

2121
import jakarta.enterprise.context.RequestScoped;
2222
import jakarta.inject.Inject;
23+
import jakarta.ws.rs.core.MediaType;
2324
import jakarta.ws.rs.core.Response;
2425
import jakarta.ws.rs.core.SecurityContext;
2526
import java.util.List;
2627
import java.util.Locale;
2728
import org.apache.iceberg.catalog.Namespace;
2829
import org.apache.iceberg.catalog.TableIdentifier;
2930
import org.apache.iceberg.exceptions.NotAuthorizedException;
31+
import org.apache.iceberg.rest.responses.ErrorResponse;
3032
import org.apache.polaris.core.admin.model.AddGrantRequest;
3133
import org.apache.polaris.core.admin.model.AuthenticationParameters;
3234
import org.apache.polaris.core.admin.model.Catalog;
@@ -71,6 +73,8 @@
7173
import org.apache.polaris.core.entity.PrincipalRoleEntity;
7274
import org.apache.polaris.core.persistence.MetaStoreManagerFactory;
7375
import org.apache.polaris.core.persistence.PolarisMetaStoreManager;
76+
import org.apache.polaris.core.persistence.dao.entity.BaseResult;
77+
import org.apache.polaris.core.persistence.dao.entity.PrivilegeResult;
7478
import org.apache.polaris.core.persistence.resolver.ResolutionManifestFactory;
7579
import org.apache.polaris.core.secrets.UserSecretsManager;
7680
import org.apache.polaris.core.secrets.UserSecretsManagerFactory;
@@ -134,6 +138,22 @@ private PolarisAdminService newAdminService(
134138
reservedProperties);
135139
}
136140

141+
private static Response toResponse(BaseResult result, Response.Status successStatus) {
142+
if (!result.isSuccess()) {
143+
ErrorResponse icebergErrorResponse =
144+
ErrorResponse.builder()
145+
.responseCode(Response.Status.BAD_REQUEST.getStatusCode())
146+
.withType(result.getReturnStatus().toString())
147+
.withMessage("Operation failed: " + result.getReturnStatus().toString())
148+
.build();
149+
return Response.status(Response.Status.BAD_REQUEST)
150+
.type(MediaType.APPLICATION_JSON_TYPE)
151+
.entity(icebergErrorResponse)
152+
.build();
153+
}
154+
return Response.status(successStatus).build();
155+
}
156+
137157
/** From PolarisCatalogsApiService */
138158
@Override
139159
public Response createCatalog(
@@ -457,8 +477,9 @@ public Response assignPrincipalRole(
457477
request.getPrincipalRole().getName(),
458478
principalName);
459479
PolarisAdminService adminService = newAdminService(realmContext, securityContext);
460-
adminService.assignPrincipalRole(principalName, request.getPrincipalRole().getName());
461-
return Response.status(Response.Status.CREATED).build();
480+
PrivilegeResult result =
481+
adminService.assignPrincipalRole(principalName, request.getPrincipalRole().getName());
482+
return toResponse(result, Response.Status.CREATED);
462483
}
463484

464485
/** From PolarisPrincipalsApiService */
@@ -470,8 +491,8 @@ public Response revokePrincipalRole(
470491
SecurityContext securityContext) {
471492
LOGGER.info("Revoking principalRole {} from principal {}", principalRoleName, principalName);
472493
PolarisAdminService adminService = newAdminService(realmContext, securityContext);
473-
adminService.revokePrincipalRole(principalName, principalRoleName);
474-
return Response.status(Response.Status.NO_CONTENT).build();
494+
PrivilegeResult result = adminService.revokePrincipalRole(principalName, principalRoleName);
495+
return toResponse(result, Response.Status.NO_CONTENT);
475496
}
476497

477498
/** From PolarisPrincipalsApiService */
@@ -503,9 +524,10 @@ public Response assignCatalogRoleToPrincipalRole(
503524
catalogName,
504525
principalRoleName);
505526
PolarisAdminService adminService = newAdminService(realmContext, securityContext);
506-
adminService.assignCatalogRoleToPrincipalRole(
507-
principalRoleName, catalogName, request.getCatalogRole().getName());
508-
return Response.status(Response.Status.CREATED).build();
527+
PrivilegeResult result =
528+
adminService.assignCatalogRoleToPrincipalRole(
529+
principalRoleName, catalogName, request.getCatalogRole().getName());
530+
return toResponse(result, Response.Status.CREATED);
509531
}
510532

511533
/** From PolarisPrincipalRolesApiService */
@@ -522,9 +544,10 @@ public Response revokeCatalogRoleFromPrincipalRole(
522544
catalogName,
523545
principalRoleName);
524546
PolarisAdminService adminService = newAdminService(realmContext, securityContext);
525-
adminService.revokeCatalogRoleFromPrincipalRole(
526-
principalRoleName, catalogName, catalogRoleName);
527-
return Response.status(Response.Status.NO_CONTENT).build();
547+
PrivilegeResult result =
548+
adminService.revokeCatalogRoleFromPrincipalRole(
549+
principalRoleName, catalogName, catalogRoleName);
550+
return toResponse(result, Response.Status.NO_CONTENT);
528551
}
529552

530553
/** From PolarisPrincipalRolesApiService */
@@ -574,6 +597,7 @@ public Response addGrantToCatalogRole(
574597
catalogRoleName,
575598
catalogName);
576599
PolarisAdminService adminService = newAdminService(realmContext, securityContext);
600+
PrivilegeResult result;
577601
switch (grantRequest.getGrant()) {
578602
// The per-securable-type Privilege enums must be exact String match for a subset of all
579603
// PolarisPrivilege values.
@@ -583,11 +607,12 @@ public Response addGrantToCatalogRole(
583607
PolarisPrivilege.valueOf(viewGrant.getPrivilege().toString());
584608
String viewName = viewGrant.getViewName();
585609
String[] namespaceParts = viewGrant.getNamespace().toArray(new String[0]);
586-
adminService.grantPrivilegeOnViewToRole(
587-
catalogName,
588-
catalogRoleName,
589-
TableIdentifier.of(Namespace.of(namespaceParts), viewName),
590-
privilege);
610+
result =
611+
adminService.grantPrivilegeOnViewToRole(
612+
catalogName,
613+
catalogRoleName,
614+
TableIdentifier.of(Namespace.of(namespaceParts), viewName),
615+
privilege);
591616
break;
592617
}
593618
case TableGrant tableGrant:
@@ -596,27 +621,30 @@ public Response addGrantToCatalogRole(
596621
PolarisPrivilege.valueOf(tableGrant.getPrivilege().toString());
597622
String tableName = tableGrant.getTableName();
598623
String[] namespaceParts = tableGrant.getNamespace().toArray(new String[0]);
599-
adminService.grantPrivilegeOnTableToRole(
600-
catalogName,
601-
catalogRoleName,
602-
TableIdentifier.of(Namespace.of(namespaceParts), tableName),
603-
privilege);
624+
result =
625+
adminService.grantPrivilegeOnTableToRole(
626+
catalogName,
627+
catalogRoleName,
628+
TableIdentifier.of(Namespace.of(namespaceParts), tableName),
629+
privilege);
604630
break;
605631
}
606632
case NamespaceGrant namespaceGrant:
607633
{
608634
PolarisPrivilege privilege =
609635
PolarisPrivilege.valueOf(namespaceGrant.getPrivilege().toString());
610636
String[] namespaceParts = namespaceGrant.getNamespace().toArray(new String[0]);
611-
adminService.grantPrivilegeOnNamespaceToRole(
612-
catalogName, catalogRoleName, Namespace.of(namespaceParts), privilege);
637+
result =
638+
adminService.grantPrivilegeOnNamespaceToRole(
639+
catalogName, catalogRoleName, Namespace.of(namespaceParts), privilege);
613640
break;
614641
}
615642
case CatalogGrant catalogGrant:
616643
{
617644
PolarisPrivilege privilege =
618645
PolarisPrivilege.valueOf(catalogGrant.getPrivilege().toString());
619-
adminService.grantPrivilegeOnCatalogToRole(catalogName, catalogRoleName, privilege);
646+
result =
647+
adminService.grantPrivilegeOnCatalogToRole(catalogName, catalogRoleName, privilege);
620648
break;
621649
}
622650
case PolicyGrant policyGrant:
@@ -625,11 +653,12 @@ public Response addGrantToCatalogRole(
625653
PolarisPrivilege.valueOf(policyGrant.getPrivilege().toString());
626654
String policyName = policyGrant.getPolicyName();
627655
String[] namespaceParts = policyGrant.getNamespace().toArray(new String[0]);
628-
adminService.grantPrivilegeOnPolicyToRole(
629-
catalogName,
630-
catalogRoleName,
631-
new PolicyIdentifier(Namespace.of(namespaceParts), policyName),
632-
privilege);
656+
result =
657+
adminService.grantPrivilegeOnPolicyToRole(
658+
catalogName,
659+
catalogRoleName,
660+
new PolicyIdentifier(Namespace.of(namespaceParts), policyName),
661+
privilege);
633662
break;
634663
}
635664
default:
@@ -640,7 +669,7 @@ public Response addGrantToCatalogRole(
640669
.log("Don't know how to handle privilege grant: {}", grantRequest);
641670
return Response.status(Response.Status.BAD_REQUEST).build();
642671
}
643-
return Response.status(Response.Status.CREATED).build();
672+
return toResponse(result, Response.Status.CREATED);
644673
}
645674

646675
/** From PolarisCatalogsApiService */
@@ -663,6 +692,7 @@ public Response revokeGrantFromCatalogRole(
663692
}
664693

665694
PolarisAdminService adminService = newAdminService(realmContext, securityContext);
695+
PrivilegeResult result;
666696
switch (grantRequest.getGrant()) {
667697
// The per-securable-type Privilege enums must be exact String match for a subset of all
668698
// PolarisPrivilege values.
@@ -672,11 +702,12 @@ public Response revokeGrantFromCatalogRole(
672702
PolarisPrivilege.valueOf(viewGrant.getPrivilege().toString());
673703
String viewName = viewGrant.getViewName();
674704
String[] namespaceParts = viewGrant.getNamespace().toArray(new String[0]);
675-
adminService.revokePrivilegeOnViewFromRole(
676-
catalogName,
677-
catalogRoleName,
678-
TableIdentifier.of(Namespace.of(namespaceParts), viewName),
679-
privilege);
705+
result =
706+
adminService.revokePrivilegeOnViewFromRole(
707+
catalogName,
708+
catalogRoleName,
709+
TableIdentifier.of(Namespace.of(namespaceParts), viewName),
710+
privilege);
680711
break;
681712
}
682713
case TableGrant tableGrant:
@@ -685,27 +716,31 @@ public Response revokeGrantFromCatalogRole(
685716
PolarisPrivilege.valueOf(tableGrant.getPrivilege().toString());
686717
String tableName = tableGrant.getTableName();
687718
String[] namespaceParts = tableGrant.getNamespace().toArray(new String[0]);
688-
adminService.revokePrivilegeOnTableFromRole(
689-
catalogName,
690-
catalogRoleName,
691-
TableIdentifier.of(Namespace.of(namespaceParts), tableName),
692-
privilege);
719+
result =
720+
adminService.revokePrivilegeOnTableFromRole(
721+
catalogName,
722+
catalogRoleName,
723+
TableIdentifier.of(Namespace.of(namespaceParts), tableName),
724+
privilege);
693725
break;
694726
}
695727
case NamespaceGrant namespaceGrant:
696728
{
697729
PolarisPrivilege privilege =
698730
PolarisPrivilege.valueOf(namespaceGrant.getPrivilege().toString());
699731
String[] namespaceParts = namespaceGrant.getNamespace().toArray(new String[0]);
700-
adminService.revokePrivilegeOnNamespaceFromRole(
701-
catalogName, catalogRoleName, Namespace.of(namespaceParts), privilege);
732+
result =
733+
adminService.revokePrivilegeOnNamespaceFromRole(
734+
catalogName, catalogRoleName, Namespace.of(namespaceParts), privilege);
702735
break;
703736
}
704737
case CatalogGrant catalogGrant:
705738
{
706739
PolarisPrivilege privilege =
707740
PolarisPrivilege.valueOf(catalogGrant.getPrivilege().toString());
708-
adminService.revokePrivilegeOnCatalogFromRole(catalogName, catalogRoleName, privilege);
741+
result =
742+
adminService.revokePrivilegeOnCatalogFromRole(
743+
catalogName, catalogRoleName, privilege);
709744
break;
710745
}
711746
case PolicyGrant policyGrant:
@@ -714,11 +749,12 @@ public Response revokeGrantFromCatalogRole(
714749
PolarisPrivilege.valueOf(policyGrant.getPrivilege().toString());
715750
String policyName = policyGrant.getPolicyName();
716751
String[] namespaceParts = policyGrant.getNamespace().toArray(new String[0]);
717-
adminService.revokePrivilegeOnPolicyFromRole(
718-
catalogName,
719-
catalogRoleName,
720-
new PolicyIdentifier(Namespace.of(namespaceParts), policyName),
721-
privilege);
752+
result =
753+
adminService.revokePrivilegeOnPolicyFromRole(
754+
catalogName,
755+
catalogRoleName,
756+
new PolicyIdentifier(Namespace.of(namespaceParts), policyName),
757+
privilege);
722758
break;
723759
}
724760
default:
@@ -729,7 +765,7 @@ public Response revokeGrantFromCatalogRole(
729765
.log("Don't know how to handle privilege revocation: {}", grantRequest);
730766
return Response.status(Response.Status.BAD_REQUEST).build();
731767
}
732-
return Response.status(Response.Status.CREATED).build();
768+
return toResponse(result, Response.Status.CREATED);
733769
}
734770

735771
/** From PolarisCatalogsApiService */

0 commit comments

Comments
 (0)