Skip to content

Commit

Permalink
If cannot access any message streams, other streams are tried as defa… (
Browse files Browse the repository at this point in the history
  • Loading branch information
luk-kaminski authored Dec 5, 2023
1 parent 612c4f9 commit 48480f8
Show file tree
Hide file tree
Showing 12 changed files with 49 additions and 25 deletions.
7 changes: 7 additions & 0 deletions changelog/unreleased/issue-4333.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
type = "fixed"
message = "Error page was shown when navigating to search page with permissions to some streams (usually non-mesage streams, like events), but no permission for default stream. It has been fixed."

issues = ["Graylog2/graylog-plugin-enterprise#4333"]
pulls = ["17548","Graylog2/graylog-plugin-enterprise#6245"]


Original file line number Diff line number Diff line change
Expand Up @@ -226,7 +226,7 @@ private void filterSearch(EventFactory eventFactory, AggregationEventProcessorPa
EventConsumer<List<EventWithContext>> eventsConsumer) throws EventProcessorException {
Set<String> streams = getStreams(parameters);
if (streams.isEmpty()) {
streams = new HashSet<>(permittedStreams.load(streamId -> true));
streams = new HashSet<>(permittedStreams.loadAllMessageStreams(streamId -> true));
}

final AtomicInteger messageCount = new AtomicInteger(0);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@
import org.graylog.events.processor.EventProcessorException;
import org.graylog.events.search.MoreSearch;
import org.graylog.plugins.views.search.Filter;
import org.graylog.plugins.views.search.Parameter;
import org.graylog.plugins.views.search.ParameterProvider;
import org.graylog.plugins.views.search.Query;
import org.graylog.plugins.views.search.QueryResult;
Expand All @@ -38,7 +37,6 @@
import org.graylog.plugins.views.search.elasticsearch.QueryStringDecorators;
import org.graylog.plugins.views.search.engine.BackendQuery;
import org.graylog.plugins.views.search.engine.QueryEngine;
import org.graylog.plugins.views.search.engine.normalization.SearchNormalization;
import org.graylog.plugins.views.search.errors.EmptyParameterError;
import org.graylog.plugins.views.search.errors.QueryError;
import org.graylog.plugins.views.search.errors.SearchError;
Expand Down Expand Up @@ -369,7 +367,7 @@ private SearchJob getSearchJob(AggregationEventProcessorParameters parameters, S
// This adds all streams if none were provided
// TODO: Once we introduce "EventProcessor owners" this should only load the permitted streams of the
// user who created this EventProcessor.
search = search.addStreamsToQueriesWithoutStreams(() -> permittedStreams.load((streamId) -> true));
search = search.addStreamsToQueriesWithoutStreams(() -> permittedStreams.loadAllMessageStreams((streamId) -> true));
final SearchJob searchJob = queryEngine.execute(searchJobService.create(search, username), Collections.emptySet());
try {
Uninterruptibles.getUninterruptibly(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ private Query normalize(final Query query,

@Override
public Search preValidation(Search search, SearchUser searchUser, ExecutionState executionState) {
final Search searchWithStreams = search.addStreamsToQueriesWithoutStreams(() -> searchUser.streams().loadAll());
final Search searchWithStreams = search.addStreamsToQueriesWithoutStreams(() -> searchUser.streams().loadMessageStreamsWithFallback());
Search normalizedSearch = searchWithStreams.applyExecutionState(firstNonNull(executionState, ExecutionState.empty()));

return normalize(normalizedSearch, pluggableNormalizers);
Expand All @@ -79,7 +79,7 @@ public Search postValidation(Search search, SearchUser searchUser, ExecutionStat
public Query preValidation(final Query query, final ParameterProvider parameterProvider, SearchUser searchUser, ExecutionState executionState) {
Query normalizedQuery = query;
if (!query.hasStreams()) {
normalizedQuery = query.addStreamsToFilter(searchUser.streams().loadAll());
normalizedQuery = query.addStreamsToFilter(searchUser.streams().loadMessageStreamsWithFallback());
}

if (!executionState.equals(ExecutionState.empty())) {
Expand All @@ -93,4 +93,6 @@ public Query preValidation(final Query query, final ParameterProvider parameterP
public Query postValidation(final Query query, final ParameterProvider parameterProvider) {
return normalize(query, parameterProvider, postValidationNormalizers);
}


}
Original file line number Diff line number Diff line change
Expand Up @@ -40,8 +40,22 @@ public UserStreams(StreamPermissions streamPermissions, PermittedStreams permitt
this.permittedStreams = permittedStreams;
}

public ImmutableSet<String> loadAll() {
return permittedStreams.load(streamPermissions);
public ImmutableSet<String> loadAllMessageStreams() {
return permittedStreams.loadAllMessageStreams(streamPermissions);
}

public ImmutableSet<String> loadAllStreams() {
return permittedStreams.loadAll(streamPermissions);
}

public ImmutableSet<String> loadMessageStreamsWithFallback() {
final ImmutableSet<String> messageStreams = loadAllMessageStreams();
if (!messageStreams.isEmpty()) {
return messageStreams;
} else {
//if a user cannot access any message streams, load others (non-message) as default
return loadAllStreams();
}
}

/**
Expand All @@ -53,7 +67,7 @@ public ImmutableSet<String> loadAll() {
*/
public ImmutableSet<String> readableOrAllIfEmpty(@Nullable final Set<String> requestedStreams) {
if (requestedStreams == null || requestedStreams.isEmpty()) {
return loadAll();
return loadMessageStreamsWithFallback();
} else {

final Set<String> notPermittedStreams = requestedStreams.stream()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ public FieldTypesResource(MappedFieldTypesService mappedFieldTypesService) {
@GET
@ApiOperation(value = "Retrieve the list of all fields present in the system")
public Set<MappedFieldTypeDTO> allFieldTypes(@Context SearchUser searchUser) {
final ImmutableSet<String> streams = searchUser.streams().loadAll();
final ImmutableSet<String> streams = searchUser.streams().loadAllMessageStreams();
return mappedFieldTypesService.fieldTypesByStreamIds(streams, RelativeRange.allTime());
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,7 @@ private MessagesRequest fillInIfNecessary(MessagesRequest requestFromClient, Sea
MessagesRequest request = requestFromClient != null ? requestFromClient : MessagesRequest.withDefaults();

if (request.streams().isEmpty()) {
request = request.withStreams(searchUser.streams().loadAll());
request = request.withStreams(searchUser.streams().loadMessageStreamsWithFallback());
}

if (!request.timeZone().isPresent()) {
Expand Down Expand Up @@ -242,7 +242,7 @@ private Search loadSearch(String searchId, ExecutionState executionState, Search
Search search = searchDomain.getForUser(searchId, searchUser)
.orElseThrow(() -> new NotFoundException("Search with id " + searchId + " does not exist"));

search = search.addStreamsToQueriesWithoutStreams(() -> searchUser.streams().loadAll());
search = search.addStreamsToQueriesWithoutStreams(() -> searchUser.streams().loadMessageStreamsWithFallback());

search = search.applyExecutionState(executionState);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,10 +21,7 @@
import org.graylog2.streams.StreamService;

import javax.inject.Inject;

import java.util.List;
import java.util.function.Supplier;
import java.util.stream.Collectors;
import java.util.stream.Stream;

import static org.graylog2.plugin.streams.Stream.NON_MESSAGE_STREAM_IDS;
Expand All @@ -42,7 +39,7 @@ public PermittedStreams(StreamService streamService) {
this(() -> streamService.loadAll().stream().map(org.graylog2.plugin.streams.Stream::getId));
}

public ImmutableSet<String> load(StreamPermissions streamPermissions) {
public ImmutableSet<String> loadAllMessageStreams(final StreamPermissions streamPermissions) {
return allStreamsProvider.get()
// Unless explicitly queried, exclude event and failure indices by default
// Having these indices in every search, makes sorting almost impossible
Expand All @@ -52,4 +49,10 @@ public ImmutableSet<String> load(StreamPermissions streamPermissions) {
.filter(streamPermissions::canReadStream)
.collect(ImmutableSet.toImmutableSet());
}

public ImmutableSet<String> loadAll(final StreamPermissions streamPermissions) {
return allStreamsProvider.get()
.filter(streamPermissions::canReadStream)
.collect(ImmutableSet.toImmutableSet());
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -180,7 +180,7 @@ private RelativeRange defaultTimeRange() {
}

private ImmutableSet<String> loadAllAllowedStreamsForUser(SearchUser searchUser) {
return permittedStreams.load(searchUser);
return permittedStreams.loadAllMessageStreams(searchUser);
}

private SuggestionFieldType getFieldType(Set<String> streams, TimeRange timerange, final String fieldName) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -153,7 +153,7 @@ public PageListResponse<IndexSetFieldTypeSummary> fieldTypeSummaries(@ApiParam(n

private Set<String> normalizeStreamIds(Set<String> streams, SearchUser searchUser) {
return (streams == null || streams.isEmpty())
? permittedStreams.load(searchUser)
? permittedStreams.loadAllMessageStreams(searchUser)
: streams;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -28,28 +28,28 @@ public class PermittedStreamsTest {
@Test
public void findsStreams() {
final PermittedStreams sut = new PermittedStreams(() -> java.util.stream.Stream.of("oans", "zwoa", "gsuffa"));
ImmutableSet<String> result = sut.load(id -> true);
ImmutableSet<String> result = sut.loadAllMessageStreams(id -> true);
assertThat(result).containsExactlyInAnyOrder("oans", "zwoa", "gsuffa");
}

@Test
public void filtersOutNonPermittedStreams() {
final PermittedStreams sut = new PermittedStreams(() -> java.util.stream.Stream.of("oans", "zwoa", "gsuffa"));
ImmutableSet<String> result = sut.load(id -> id.equals("gsuffa"));
ImmutableSet<String> result = sut.loadAllMessageStreams(id -> id.equals("gsuffa"));
assertThat(result).containsExactly("gsuffa");
}

@Test
public void returnsEmptyListIfNoStreamsFound() {
final PermittedStreams sut = new PermittedStreams(() -> java.util.stream.Stream.of("oans", "zwoa", "gsuffa"));
ImmutableSet<String> result = sut.load(id -> false);
ImmutableSet<String> result = sut.loadAllMessageStreams(id -> false);
assertThat(result).isEmpty();
}

@Test
public void filtersDefaultStreams() {
final PermittedStreams sut = new PermittedStreams(() -> Streams.concat(NON_MESSAGE_STREAM_IDS.stream(), java.util.stream.Stream.of("i'm ok")));
ImmutableSet<String> result = sut.load(id -> true);
ImmutableSet<String> result = sut.loadAllMessageStreams(id -> true);
assertThat(result).containsExactly("i'm ok");
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,7 @@ protected User getCurrentUser() {
}
};

when(searchUser.streams().loadAll()).thenReturn(ImmutableSet.of("Default Stream"));
when(searchUser.streams().loadMessageStreamsWithFallback()).thenReturn(ImmutableSet.of("Default Stream"));
}

@Test
Expand Down Expand Up @@ -234,7 +234,7 @@ public void failureToAddDefaultStreamsInAsyncSearchLeadsTo403() {
final Search search = makeNewSearch("search1");
persistSearch(search);

when(searchUser.streams().loadAll()).thenReturn(ImmutableSet.of());
when(searchUser.streams().loadMessageStreamsWithFallback()).thenReturn(ImmutableSet.of());

assertThatExceptionOfType(MissingStreamPermissionException.class)
.isThrownBy(() -> this.searchResource.executeQuery(search.id(), null, searchUser));
Expand All @@ -245,7 +245,7 @@ public void failureToAddDefaultStreamsInSynchronousSearchLeadsTo403() {
final Search search = makeNewSearch("search1");
final SearchDTO searchDTO = SearchDTO.fromSearch(search);

when(searchUser.streams().loadAll()).thenReturn(ImmutableSet.of());
when(searchUser.streams().loadMessageStreamsWithFallback()).thenReturn(ImmutableSet.of());

assertThatExceptionOfType(MissingStreamPermissionException.class)
.isThrownBy(() -> this.searchResource.executeSyncJob(searchDTO, 0, searchUser));
Expand Down

0 comments on commit 48480f8

Please sign in to comment.