Copilot commented on code in PR #864:
URL: https://github.com/apache/ranger/pull/864#discussion_r2870081747
##########
authz-embedded/src/main/java/org/apache/ranger/authz/embedded/RangerEmbeddedAuthorizer.java:
##########
@@ -48,17 +48,19 @@ public class RangerEmbeddedAuthorizer extends
RangerAuthorizer {
private static final Logger LOG =
LoggerFactory.getLogger(RangerEmbeddedAuthorizer.class);
private final RangerAuthzConfig config;
+ private final String appType;
private final Map<String, RangerAuthzPlugin> plugins = new HashMap<>();
public RangerEmbeddedAuthorizer(Properties properties) {
super(properties);
- this.config = new RangerAuthzConfig(properties);
+ this.config = new RangerAuthzConfig(properties);
+ this.appType = config.getAppType();
}
@Override
public void init() throws RangerAuthzException {
- AuditProviderFactory.getInstance().init(config.getAuditProperties(),
"ranger-authz");
+ AuditProviderFactory.getInstance().init(config.getAuditProperties(),
config.getAppType());
Review Comment:
`init()` now passes `config.getAppType()` directly to
`AuditProviderFactory.init()`. If `ranger.authz.app.type` is not set
(blank/null), this changes prior behavior (previously always used
"ranger-authz") and can result in empty `%app-type%` token expansion in audit
destinations and inconsistent audit initialization. Consider defaulting to the
previous value (e.g., "ranger-authz") when the property is blank, and use the
same effective appType consistently (field vs config lookup).
##########
authz-embedded/src/main/java/org/apache/ranger/authz/embedded/RangerEmbeddedAuthorizer.java:
##########
@@ -93,45 +96,28 @@ public RangerAuthzResult authorize(RangerAuthzRequest
request) throws RangerAuth
public RangerMultiAuthzResult authorize(RangerMultiAuthzRequest request)
throws RangerAuthzException {
validateRequest(request);
- RangerAuthzPlugin plugin =
getOrCreatePlugin(request.getContext().getServiceName(),
request.getContext().getServiceType());
- RangerMultiAuthzResult result = new
RangerMultiAuthzResult(request.getRequestId());
+ RangerAuthzPlugin plugin =
getOrCreatePlugin(request.getContext().getServiceName(),
request.getContext().getServiceType());
+ String appType = StringUtils.isNotBlank(this.appType) ?
this.appType : plugin.getPlugin().getAppId();
- if (request.getAccesses() != null) {
- int allowedCount = 0;
- int deniedCount = 0;
- int notDeterminedCount = 0;
+ try (RangerAuthzAuditHandler auditHandler = new
RangerAuthzAuditHandler(appType)) {
+ return authorize(request, plugin, auditHandler);
+ }
+ }
- result.setAccesses(new ArrayList<>(request.getAccesses().size()));
+ public RangerAuthzResult authorize(RangerAuthzRequest request,
RangerAuthzAuditHandler auditHandler) throws RangerAuthzException {
+ validateRequest(request);
- try (RangerAuthzAuditHandler auditHandler = new
RangerAuthzAuditHandler(plugin.getPlugin())) {
- for (RangerAccessInfo accessInfo : request.getAccesses()) {
- RangerAuthzRequest authzRequest = new
RangerAuthzRequest(null, request.getUser(), accessInfo, request.getContext());
- RangerAuthzResult authzResult = authorize(authzRequest,
plugin, auditHandler);
+ RangerAuthzPlugin plugin =
getOrCreatePlugin(request.getContext().getServiceName(),
request.getContext().getServiceType());
- if (authzResult.getDecision() == AccessDecision.ALLOW) {
- allowedCount++;
- } else if (authzResult.getDecision() ==
AccessDecision.DENY) {
- deniedCount++;
- } else if (authzResult.getDecision() ==
AccessDecision.NOT_DETERMINED) {
- notDeterminedCount++;
- }
+ return authorize(request, plugin, auditHandler);
+ }
- result.getAccesses().add(authzResult);
- }
- }
+ public RangerMultiAuthzResult authorize(RangerMultiAuthzRequest request,
RangerAuthzAuditHandler auditHandler) throws RangerAuthzException {
+ validateRequest(request);
- if (allowedCount == request.getAccesses().size()) {
- result.setDecision(AccessDecision.ALLOW);
- } else if (deniedCount == request.getAccesses().size()) {
- result.setDecision(AccessDecision.DENY);
- } else if (notDeterminedCount == request.getAccesses().size()) {
- result.setDecision(AccessDecision.NOT_DETERMINED);
- } else {
- result.setDecision(AccessDecision.PARTIAL);
- }
- }
+ RangerAuthzPlugin plugin =
getOrCreatePlugin(request.getContext().getServiceName(),
request.getContext().getServiceType());
- return result;
+ return authorize(request, plugin, auditHandler);
}
Review Comment:
New overloads `authorize(..., RangerAuthzAuditHandler)` were added, but
existing unit tests only exercise the no-arg audit-handler variants. Please add
tests that call these new overloads to verify the caller-provided handler is
actually used for both single and multi-authorize flows (and that audit events
are flushed/observable when the handler is closed by the caller).
##########
authz-embedded/src/main/java/org/apache/ranger/authz/embedded/RangerAuthzConfig.java:
##########
@@ -46,6 +47,10 @@ public String[] getInitServices() {
return initServices.split(",");
}
+ public String getAppType() {
+ return properties.getProperty(PROP_APP_TYPE);
+ }
Review Comment:
`RangerAuthzConfig.getAppType()` is newly introduced but currently has no
unit test coverage. Adding a small test case that sets `ranger.authz.app.type`
and asserts `getAppType()` returns it would help prevent regressions,
especially since this value now impacts audit initialization.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
