Skip to content

Commit

Permalink
Add user id validation for create and update API (#533)
Browse files Browse the repository at this point in the history
  • Loading branch information
swaranga-netflix authored Feb 18, 2023
1 parent 7370eee commit a9c816f
Show file tree
Hide file tree
Showing 6 changed files with 281 additions and 121 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -46,11 +46,13 @@
import com.netflix.metacat.main.services.MetacatServiceHelper;
import com.netflix.metacat.main.services.MetacatThriftService;
import com.netflix.metacat.main.services.MetadataService;
import com.netflix.metacat.main.services.OwnerValidationService;
import com.netflix.metacat.main.services.PartitionService;
import com.netflix.metacat.main.services.TableService;
import com.netflix.metacat.main.services.impl.CatalogServiceImpl;
import com.netflix.metacat.main.services.impl.ConnectorTableServiceProxy;
import com.netflix.metacat.main.services.impl.DatabaseServiceImpl;
import com.netflix.metacat.main.services.impl.DefaultOwnerValidationService;
import com.netflix.metacat.main.services.impl.MViewServiceImpl;
import com.netflix.metacat.main.services.impl.PartitionServiceImpl;
import com.netflix.metacat.main.services.impl.TableServiceImpl;
Expand Down Expand Up @@ -105,6 +107,17 @@ public AuthorizationService authorizationService(
return new DefaultAuthorizationService(config);
}

/**
* Owner validation service.
* @param registry the spectator registry
* @return the owner validation service
*/
@Bean
@ConditionalOnMissingBean(OwnerValidationService.class)
public OwnerValidationService ownerValidationService(final Registry registry) {
return new DefaultOwnerValidationService(registry);
}

/**
* Alias service.
*
Expand Down Expand Up @@ -198,6 +211,8 @@ public DatabaseService databaseService(
* @param config configurations
* @param converterUtil converter utilities
* @param authorizationService authorization Service
* @param ownerValidationService owner validation service
*
* @return The table service bean
*/
@Bean
Expand All @@ -212,8 +227,8 @@ public TableService tableService(
final Registry registry,
final Config config,
final ConverterUtil converterUtil,
final AuthorizationService authorizationService
) {
final AuthorizationService authorizationService,
final OwnerValidationService ownerValidationService) {
return new TableServiceImpl(
connectorManager,
connectorTableServiceProxy,
Expand All @@ -225,7 +240,8 @@ public TableService tableService(
registry,
config,
converterUtil,
authorizationService
authorizationService,
ownerValidationService
);
}

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
package com.netflix.metacat.main.services;

import com.netflix.metacat.common.QualifiedName;
import com.netflix.metacat.common.dto.TableDto;
import lombok.NonNull;

import javax.annotation.Nullable;

/**
* Interface for validating table owner attribute.
*/
public interface OwnerValidationService {
/**
* Checks whether the given owner is valid against a registry.
*
* @param user the user
* @return true if the owner is valid, else false
*/
boolean isUserValid(@Nullable String user);

/**
* Enforces valid table owner attribute. Implementations are free to
* handle it as needed - throw exceptions or ignore. The owner attribute
* in the DTO may or may not be valid so implementations should check for validity
* before enforcement.
*
* @param operationName the name of the metacat API, useful for logging
* @param tableName the name of the table
* @param tableDto the table dto containing the owner in the definition metadata field
*/
void enforceOwnerValidation(@NonNull String operationName,
@NonNull QualifiedName tableName,
@NonNull TableDto tableDto);
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,136 @@
package com.netflix.metacat.main.services.impl;

import com.google.common.collect.ImmutableSet;
import com.netflix.metacat.common.MetacatRequestContext;
import com.netflix.metacat.common.QualifiedName;
import com.netflix.metacat.common.dto.TableDto;
import com.netflix.metacat.common.server.util.MetacatContextManager;
import com.netflix.metacat.main.services.OwnerValidationService;
import com.netflix.spectator.api.Registry;
import lombok.NonNull;
import lombok.RequiredArgsConstructor;
import lombok.extern.slf4j.Slf4j;
import org.apache.commons.lang3.StringUtils;
import org.springframework.web.context.request.RequestAttributes;
import org.springframework.web.context.request.RequestContextHolder;
import org.springframework.web.context.request.ServletRequestAttributes;

import javax.annotation.Nullable;
import javax.servlet.http.HttpServletRequest;
import java.util.Enumeration;
import java.util.HashMap;
import java.util.Map;
import java.util.Set;

/**
* A default implementation of Ownership validation service that check for srs against
* known invalid userIds.
*/
@Slf4j
@RequiredArgsConstructor
public class DefaultOwnerValidationService implements OwnerValidationService {
private static final Set<String> KNOWN_INVALID_OWNERS = ImmutableSet.of(
"root", "metacat", "metacat-thrift-interface");

private final Registry registry;

@Override
public boolean isUserValid(@Nullable final String user) {
return !isKnownInvalidUser(user);
}

@Override
public void enforceOwnerValidation(@NonNull final String operationName,
@NonNull final QualifiedName tableName,
@NonNull final TableDto tableDto) {
final String tableOwner = tableDto.getTableOwner().orElse(null);
final MetacatRequestContext context = MetacatContextManager.getContext();
final Map<String, String> requestHeaders = getHttpHeaders();

final boolean tableOwnerValid = isUserValid(tableOwner);

logOwnershipDiagnosticDetails(
operationName, tableName, tableDto, tableOwner,
context, tableOwnerValid, requestHeaders);
}

/**
* Checks if the user is from a know list of invalid users. Subclasses can use
* this method before attempting to check againt remote servies to save on latency.
*
* @param userId the user id
* @return true if the user id is a known invalid user, else false
*/
protected boolean isKnownInvalidUser(@Nullable final String userId) {
return StringUtils.isBlank(userId) || knownInvalidOwners().contains(userId);
}

/**
* Returns set of known invalid users. Subclasses can override to provide
* a list fetched from a dynamic source.
*
* @return set of known invalid users
*/
protected Set<String> knownInvalidOwners() {
return KNOWN_INVALID_OWNERS;
}

/**
* Logs diagnostic data for debugging invalid owners. Subclasses can use this to log
* diagnostic data when owners are found to be invalid.
*/
protected void logOwnershipDiagnosticDetails(final String operationName,
final QualifiedName name,
final TableDto tableDto,
@Nullable final String tableOwner,
final MetacatRequestContext context,
final boolean tableOwnerValid,
final Map<String, String> requestHeaders) {
try {
if (!tableOwnerValid) {
registry.counter(
"metacat.table.owner.invalid",
"operation", operationName,
"catalogAndDb", name.getCatalogName() + "_" + name.getDatabaseName()
).increment();

log.info("Operation: {}, invalid owner: {}. name: {}, table-dto: {}, context: {}, headers: {}",
operationName, tableOwner, name, tableDto, context, requestHeaders);
}
} catch (final Exception ex) {
log.warn("Error when logging diagnostic data for invalid owner for operation: {}, name: {}, table: {}",
operationName, name, tableDto, ex);
}
}

/**
* Returns all the Http headers for the current request. Subclasses can use it to
* log diagnostic data.
*
* @return the Http headers
*/
protected Map<String, String> getHttpHeaders() {
final Map<String, String> requestHeaders = new HashMap<>();

final RequestAttributes requestAttributes = RequestContextHolder.getRequestAttributes();

if (requestAttributes instanceof ServletRequestAttributes) {
final ServletRequestAttributes servletRequestAttributes = (ServletRequestAttributes) requestAttributes;

final HttpServletRequest servletRequest = servletRequestAttributes.getRequest();

if (servletRequest != null) {
final Enumeration<String> headerNames = servletRequest.getHeaderNames();

if (headerNames != null) {
while (headerNames.hasMoreElements()) {
final String header = headerNames.nextElement();
requestHeaders.put(header, servletRequest.getHeader(header));
}
}
}
}

return requestHeaders;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -56,20 +56,14 @@
import com.netflix.metacat.main.services.DatabaseService;
import com.netflix.metacat.main.services.GetTableNamesServiceParameters;
import com.netflix.metacat.main.services.GetTableServiceParameters;
import com.netflix.metacat.main.services.OwnerValidationService;
import com.netflix.metacat.main.services.TableService;
import com.netflix.spectator.api.Registry;
import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
import lombok.RequiredArgsConstructor;
import lombok.extern.slf4j.Slf4j;
import org.apache.commons.lang.StringUtils;
import org.springframework.web.context.request.RequestAttributes;
import org.springframework.web.context.request.RequestContextHolder;
import org.springframework.web.context.request.ServletRequestAttributes;

import javax.annotation.Nullable;
import javax.servlet.http.HttpServletRequest;
import java.util.Enumeration;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.Objects;
Expand All @@ -94,6 +88,7 @@ public class TableServiceImpl implements TableService {
private final Config config;
private final ConverterUtil converterUtil;
private final AuthorizationService authorizationService;
private final OwnerValidationService ownerValidationService;

/**
* {@inheritDoc}
Expand All @@ -106,7 +101,7 @@ public TableDto create(final QualifiedName name, final TableDto tableDto) {
tableDto.getName(), MetacatOperation.CREATE);

setDefaultAttributes(tableDto);
logOwnershipDiagnosticDetails(name, tableDto);
ownerValidationService.enforceOwnerValidation("createTable", name, tableDto);

log.info("Creating table {}", name);
eventBus.post(new MetacatCreateTablePreEvent(name, metacatRequestContext, this, tableDto));
Expand Down Expand Up @@ -150,74 +145,6 @@ private void setDefaultAttributes(final TableDto tableDto) {
setOwnerIfNull(tableDto);
}

/**
* Logs diagnostic data for debugging invalid owners.
*
* @param name the qualified name of the resource
* @param tableDto the table details after all processing has been done on the owner
*/
private void logOwnershipDiagnosticDetails(final QualifiedName name, final TableDto tableDto) {
try {
final String tableOwner = tableDto.getTableOwner().orElse(null);

if (!isOwnerValid(tableOwner)) {
registry.counter(
"unauth.user.create.table",
"catalog", name.getCatalogName(),
"database", name.getDatabaseName(),
"owner", StringUtils.isBlank(tableOwner) ? "null" : tableOwner
).increment();

log.info("Create table with invalid owner: {}. name: {}, table-dto: {}, context: {}, headers: {}",
tableOwner, name, tableDto, MetacatContextManager.getContext(), getHttpHeaders());
} else if (!isOwnerValid(MetacatContextManager.getContext().getUserName())) {
registry.counter(
"unauth.user.createTable.requestContext",
"catalog", name.getCatalogName(),
"database", name.getDatabaseName(),
"owner", StringUtils.isBlank(MetacatContextManager.getContext().getUserName()) ? "null" : tableOwner
).increment();

log.info("Create table called with invalid owner: {} in request context."
+ " name: {}, table-dto: {}, context: {}, headers: {}",
MetacatContextManager.getContext().getUserName(), name, tableDto,
MetacatContextManager.getContext(), getHttpHeaders());
}
} catch (Exception ex) {
log.warn("Error when logging diagnostic data for invalid owner table creation. name: {}, table: {}",
name, tableDto, ex);
}
}

/**
* Returns all the Http headers for the current request.
* @return the Http headers
*/
private Map<String, String> getHttpHeaders() {
final Map<String, String> requestHeaders = new HashMap<>();

final RequestAttributes requestAttributes = RequestContextHolder.getRequestAttributes();

if (requestAttributes instanceof ServletRequestAttributes) {
final ServletRequestAttributes servletRequestAttributes = (ServletRequestAttributes) requestAttributes;

final HttpServletRequest servletRequest = servletRequestAttributes.getRequest();

if (servletRequest != null) {
final Enumeration<String> headerNames = servletRequest.getHeaderNames();

if (headerNames != null) {
while (headerNames.hasMoreElements()) {
final String header = headerNames.nextElement();
requestHeaders.put(header, servletRequest.getHeader(header));
}
}
}
}

return requestHeaders;
}

private void setDefaultDefinitionMetadataIfNull(final TableDto tableDto) {
ObjectNode definitionMetadata = tableDto.getDefinitionMetadata();
if (definitionMetadata == null) {
Expand Down Expand Up @@ -285,10 +212,7 @@ void updateTableOwner(final TableDto tableDto, final String userId) {
}

private boolean isOwnerValid(@Nullable final String userId) {
return StringUtils.isNotBlank(userId)
&& !"metacat".equals(userId)
&& !"root".equals(userId)
&& !"metacat-thrift-interface".equals(userId);
return ownerValidationService.isUserValid(userId);
}

@SuppressFBWarnings
Expand Down Expand Up @@ -548,6 +472,12 @@ public TableDto updateAndReturn(final QualifiedName name, final TableDto tableDt
ignoreErrorsAfterUpdate = connectorTableServiceProxy.update(name, converterUtil.fromTableDto(tableDto));
}

// we do ownership validation and enforcement only if table owner is set in the dto
// because if it is null, we do not update the owner in the existing metadata record
if (tableDto.getTableOwner().isPresent()) {
ownerValidationService.enforceOwnerValidation("updateTable", name, tableDto);
}

try {
// Merge in metadata if the user sent any
if (tableDto.getDataMetadata() != null || tableDto.getDefinitionMetadata() != null) {
Expand Down
Loading

0 comments on commit a9c816f

Please sign in to comment.