Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New configuration to (dis-)allow external users create access tokens #21438

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
Introduce new configuration for allowing external users to create an …
…access token (or not). Starting with graylog 6.2, this is set to not allow.
fpetersen-gl committed Jan 23, 2025
commit b84922ff2a9266fd9696deaec4971615459f516c
Original file line number Diff line number Diff line change
@@ -27,6 +27,27 @@
import io.swagger.annotations.ApiParam;
import io.swagger.annotations.ApiResponse;
import io.swagger.annotations.ApiResponses;
import jakarta.inject.Inject;
import jakarta.validation.Valid;
import jakarta.validation.constraints.NotBlank;
import jakarta.validation.constraints.NotNull;
import jakarta.ws.rs.BadRequestException;
import jakarta.ws.rs.Consumes;
import jakarta.ws.rs.DELETE;
import jakarta.ws.rs.DefaultValue;
import jakarta.ws.rs.ForbiddenException;
import jakarta.ws.rs.GET;
import jakarta.ws.rs.InternalServerErrorException;
import jakarta.ws.rs.NotFoundException;
import jakarta.ws.rs.POST;
import jakarta.ws.rs.PUT;
import jakarta.ws.rs.Path;
import jakarta.ws.rs.PathParam;
import jakarta.ws.rs.Produces;
import jakarta.ws.rs.QueryParam;
import jakarta.ws.rs.core.Context;
import jakarta.ws.rs.core.MediaType;
import jakarta.ws.rs.core.Response;
import org.apache.commons.lang.StringUtils;
import org.apache.shiro.authz.annotation.RequiresAuthentication;
import org.apache.shiro.authz.annotation.RequiresPermissions;
@@ -42,6 +63,7 @@
import org.graylog2.audit.AuditEventTypes;
import org.graylog2.audit.jersey.AuditEvent;
import org.graylog2.database.PaginatedList;
import org.graylog2.plugin.cluster.ClusterConfigService;
import org.graylog2.plugin.database.ValidationException;
import org.graylog2.plugin.database.users.User;
import org.graylog2.rest.models.PaginatedResponse;
@@ -73,37 +95,13 @@
import org.graylog2.users.PaginatedUserService;
import org.graylog2.users.RoleService;
import org.graylog2.users.RoleServiceImpl;
import org.graylog2.users.UserConfiguration;
import org.graylog2.users.UserOverviewDTO;
import org.joda.time.DateTimeZone;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

import javax.annotation.Nullable;

import jakarta.inject.Inject;

import jakarta.validation.Valid;
import jakarta.validation.constraints.NotBlank;
import jakarta.validation.constraints.NotNull;

import jakarta.ws.rs.BadRequestException;
import jakarta.ws.rs.Consumes;
import jakarta.ws.rs.DELETE;
import jakarta.ws.rs.DefaultValue;
import jakarta.ws.rs.ForbiddenException;
import jakarta.ws.rs.GET;
import jakarta.ws.rs.InternalServerErrorException;
import jakarta.ws.rs.NotFoundException;
import jakarta.ws.rs.POST;
import jakarta.ws.rs.PUT;
import jakarta.ws.rs.Path;
import jakarta.ws.rs.PathParam;
import jakarta.ws.rs.Produces;
import jakarta.ws.rs.QueryParam;
import jakarta.ws.rs.core.Context;
import jakarta.ws.rs.core.MediaType;
import jakarta.ws.rs.core.Response;

import java.net.URI;
import java.util.ArrayList;
import java.util.Collection;
@@ -147,6 +145,7 @@ public class UsersResource extends RestResource {
private final UserSessionTerminationService sessionTerminationService;
private final DefaultSecurityManager securityManager;
private final GlobalAuthServiceConfig globalAuthServiceConfig;
private final ClusterConfigService clusterConfigService;

protected static final ImmutableMap<String, SearchQueryField> SEARCH_FIELD_MAPPING = ImmutableMap.<String, SearchQueryField>builder()
.put(UserOverviewDTO.FIELD_ID, SearchQueryField.create("_id", SearchQueryField.Type.OBJECT_ID))
@@ -162,7 +161,7 @@ public UsersResource(UserManagementService userManagementService,
RoleService roleService,
MongoDBSessionService sessionService,
UserSessionTerminationService sessionTerminationService, DefaultSecurityManager securityManager,
GlobalAuthServiceConfig globalAuthServiceConfig) {
GlobalAuthServiceConfig globalAuthServiceConfig, ClusterConfigService clusterConfigService) {
this.userManagementService = userManagementService;
this.accessTokenService = accessTokenService;
this.roleService = roleService;
@@ -172,6 +171,7 @@ public UsersResource(UserManagementService userManagementService,
this.securityManager = securityManager;
this.searchQueryParser = new SearchQueryParser(UserOverviewDTO.FIELD_FULL_NAME, SEARCH_FIELD_MAPPING);
this.globalAuthServiceConfig = globalAuthServiceConfig;
this.clusterConfigService = clusterConfigService;
}

/**
@@ -720,7 +720,9 @@ public Token generateNewToken(
@ApiParam(name = "JSON Body", value = "Placeholder because POST requests should have a body. Set to '{}', the content will be ignored.", defaultValue = "{}") String body) {
final User user = loadUserById(userId);
final String username = user.getName();
if (!isPermitted(USERS_TOKENCREATE, username)) {

if (!isPermitted(USERS_TOKENCREATE, username) ||
isExternalUserDenied(user)) {
throw new ForbiddenException("Not allowed to create tokens for user " + username);
}
final AccessToken accessToken = accessTokenService.create(user.getName(), name);
@@ -755,6 +757,11 @@ public void revokeToken(
}
}

private boolean isExternalUserDenied(User user) {
return user.isExternalUser()
&& !clusterConfigService.getOrDefault(UserConfiguration.class, UserConfiguration.DEFAULT_VALUES).allowAccessTokenForExternalUsers();
}

private User loadUserById(String userId) {
final User user = userManagementService.loadById(userId);
if (user == null) {
Original file line number Diff line number Diff line change
@@ -21,6 +21,7 @@
import com.fasterxml.jackson.annotation.JsonProperty;
import com.google.auto.value.AutoValue;
import org.graylog.autovalue.WithBeanGetter;
import org.graylog2.plugin.Version;

import java.time.Duration;
import java.time.temporal.ChronoUnit;
@@ -29,18 +30,26 @@
@AutoValue
@WithBeanGetter
public abstract class UserConfiguration {
public static final UserConfiguration DEFAULT_VALUES = create(false, Duration.of(8, ChronoUnit.HOURS));
private static final boolean IS_BEFORE_VERSION_6_2 = !Version.CURRENT_CLASSPATH.sameOrHigher(Version.from(6, 2, 0));

//Starting with graylog version 6.2, external users are not allowed to own access tokens by default.
// Before this version, it is allowed, to not introduce a breaking change:
public static final UserConfiguration DEFAULT_VALUES = create(false, Duration.of(8, ChronoUnit.HOURS), IS_BEFORE_VERSION_6_2);

@JsonProperty("enable_global_session_timeout")
public abstract boolean enableGlobalSessionTimeout();

@JsonProperty("global_session_timeout_interval")
public abstract java.time.Duration globalSessionTimeoutInterval();
public abstract Duration globalSessionTimeoutInterval();

@JsonProperty("allow_access_token_for_external_user")
public abstract boolean allowAccessTokenForExternalUsers();

@JsonCreator
public static UserConfiguration create(
@JsonProperty("enable_global_session_timeout") boolean enableGlobalSessionTimeout,
@JsonProperty("global_session_timeout_interval") java.time.Duration globalSessionTimeoutInterval) {
return new AutoValue_UserConfiguration(enableGlobalSessionTimeout, globalSessionTimeoutInterval);
@JsonProperty("global_session_timeout_interval") Duration globalSessionTimeoutInterval,
@JsonProperty("allow_access_token_for_external_user") boolean allowAccessTokenForExternalUsers) {
return new AutoValue_UserConfiguration(enableGlobalSessionTimeout, globalSessionTimeoutInterval, allowAccessTokenForExternalUsers);
}
}
Original file line number Diff line number Diff line change
@@ -17,18 +17,24 @@
package org.graylog2.rest.resources.users;

import com.google.common.collect.ImmutableSet;
import jakarta.ws.rs.ForbiddenException;
import jakarta.ws.rs.core.Response;
import org.apache.shiro.mgt.DefaultSecurityManager;
import org.apache.shiro.subject.Subject;
import org.bson.types.ObjectId;
import org.graylog.security.authservice.GlobalAuthServiceConfig;
import org.graylog.testing.mongodb.MongoDBInstance;
import org.graylog2.Configuration;
import org.graylog2.configuration.HttpConfiguration;
import org.graylog2.plugin.Tools;
import org.graylog2.plugin.cluster.ClusterConfigService;
import org.graylog2.plugin.database.ValidationException;
import org.graylog2.rest.models.users.requests.CreateUserRequest;
import org.graylog2.rest.models.users.requests.Startpage;
import org.graylog2.rest.models.users.requests.UpdateUserPreferences;
import org.graylog2.rest.models.users.responses.Token;
import org.graylog2.security.AccessToken;
import org.graylog2.security.AccessTokenImpl;
import org.graylog2.security.AccessTokenService;
import org.graylog2.security.MongoDBSessionService;
import org.graylog2.security.PasswordAlgorithmFactory;
@@ -39,7 +45,9 @@
import org.graylog2.shared.users.UserManagementService;
import org.graylog2.users.PaginatedUserService;
import org.graylog2.users.RoleService;
import org.graylog2.users.UserConfiguration;
import org.graylog2.users.UserImpl;
import org.joda.time.DateTime;
import org.junit.Assert;
import org.junit.Before;
import org.junit.Rule;
@@ -48,18 +56,21 @@
import org.mockito.junit.MockitoJUnit;
import org.mockito.junit.MockitoRule;

import jakarta.ws.rs.core.Response;

import java.time.Duration;
import java.time.temporal.ChronoUnit;
import java.util.Collections;
import java.util.HashMap;
import java.util.Map;

import static org.graylog2.shared.security.RestPermissions.USERS_TOKENCREATE;
import static org.junit.Assert.assertEquals;
import static org.mockito.ArgumentMatchers.anyString;
import static org.mockito.ArgumentMatchers.eq;
import static org.mockito.ArgumentMatchers.isA;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.times;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.verifyNoMoreInteractions;
import static org.mockito.Mockito.when;

public class UsersResourceTest {
@@ -100,6 +111,8 @@ public class UsersResourceTest {
private DefaultSecurityManager securityManager;
@Mock
private GlobalAuthServiceConfig globalAuthServiceConfig;
@Mock
private ClusterConfigService clusterConfigService;

UserImplFactory userImplFactory;

@@ -109,7 +122,7 @@ public void setUp() throws Exception {
new Permissions(ImmutableSet.of(new RestPermissions())));
usersResource = new TestUsersResource(userManagementService, paginatedUserService, accessTokenService,
roleService, sessionService, new HttpConfiguration(), subject,
sessionTerminationService, securityManager, globalAuthServiceConfig);
sessionTerminationService, securityManager, globalAuthServiceConfig, clusterConfigService);
}

/**
@@ -132,6 +145,78 @@ public void savePreferencesSuccess() throws ValidationException {
verify(userManagementService, times(1)).save(isA(UserImpl.class));
}

@Test
public void createTokenForInternalUserSucceeds() {
final Map<String, Object> userProps = Map.of(UserImpl.USERNAME, USERNAME);
final String tokenName = "tokenName";
final String token = "someToken";
final DateTime lastAccess = Tools.nowUTC();
final Map<String, Object> tokenProps = Map.of(AccessTokenImpl.NAME, tokenName, AccessTokenImpl.TOKEN, token, AccessTokenImpl.LAST_ACCESS, lastAccess);
final ObjectId tokenId = new ObjectId();
final AccessToken accessToken = new AccessTokenImpl(tokenId, tokenProps);
final Token expected = Token.create(tokenId.toHexString(), tokenName, token, lastAccess);

when(userManagementService.loadById(eq(USERNAME))).thenReturn(userImplFactory.create(userProps));
when(subject.isPermitted(eq(USERS_TOKENCREATE + ":" + USERNAME))).thenReturn(true);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove unnecessary eq():

Suggested change
when(userManagementService.loadById(eq(USERNAME))).thenReturn(userImplFactory.create(userProps));
when(subject.isPermitted(eq(USERS_TOKENCREATE + ":" + USERNAME))).thenReturn(true);
when(userManagementService.loadById(USERNAME)).thenReturn(userImplFactory.create(userProps));
when(subject.isPermitted(USERS_TOKENCREATE + ":" + USERNAME)).thenReturn(true);

Do the same for other instances in this file.

when(accessTokenService.create(USERNAME, tokenName)).thenReturn(accessToken);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider factoring out token creation into a separate method to improve clarity and reduce code size.

private Token setupTokenCreation(String username, boolean isExternal, boolean allowExternalTokens) {
    final Map<String, Object> userProps = Map.of(UserImpl.USERNAME, username, UserImpl.EXTERNAL_USER, String.valueOf(isExternal));
    final String tokenName = "tokenName";
    final String token = "someToken";
    final DateTime lastAccess = Tools.nowUTC();
    final Map<String, Object> tokenProps = Map.of(AccessTokenImpl.NAME, tokenName, AccessTokenImpl.TOKEN, token, AccessTokenImpl.LAST_ACCESS, lastAccess);
    final ObjectId tokenId = new ObjectId();
    final AccessToken accessToken = new AccessTokenImpl(tokenId, tokenProps);
    final Token expected = Token.create(tokenId.toHexString(), tokenName, token, lastAccess);

    when(userManagementService.loadById(eq(username))).thenReturn(userImplFactory.create(userProps));
    when(subject.isPermitted(eq(USERS_TOKENCREATE + ":" + username))).thenReturn(true);
    when(clusterConfigService.getOrDefault(UserConfiguration.class, UserConfiguration.DEFAULT_VALUES))
            .thenReturn(UserConfiguration.create(false, Duration.of(8, ChronoUnit.HOURS), allowExternalTokens));
    when(accessTokenService.create(username, tokenName)).thenReturn(accessToken);

    return expected;
}


try {
final Token actual = usersResource.generateNewToken(USERNAME, tokenName, "{}");
assertEquals(expected, actual);
} finally {
verify(subject).isPermitted(USERS_TOKENCREATE + ":" + USERNAME);
verify(accessTokenService).create(USERNAME, tokenName);
verifyNoMoreInteractions(clusterConfigService, accessTokenService);
}
}

@Test(expected = ForbiddenException.class)
public void createTokenForExternalUserFailsIfConfigured() {
final Map<String, Object> userProps = Map.of(UserImpl.USERNAME, USERNAME, UserImpl.EXTERNAL_USER, "TRUE");
final String tokenName = "tokenName";

when(userManagementService.loadById(eq(USERNAME))).thenReturn(userImplFactory.create(userProps));
when(subject.isPermitted(eq(USERS_TOKENCREATE + ":" + USERNAME))).thenReturn(true);
when(clusterConfigService.getOrDefault(UserConfiguration.class, UserConfiguration.DEFAULT_VALUES))
.thenReturn(UserConfiguration.create(false, Duration.of(8, ChronoUnit.HOURS), false));

try {
usersResource.generateNewToken(USERNAME, tokenName, "{}");
} finally {
verify(subject).isPermitted(USERS_TOKENCREATE + ":" + USERNAME);
verify(clusterConfigService).getOrDefault(UserConfiguration.class, UserConfiguration.DEFAULT_VALUES);
verifyNoMoreInteractions(clusterConfigService, accessTokenService);
}
}

@Test
public void createTokenForExternalUserSucceedsIfConfigured() {
final Map<String, Object> userProps = Map.of(UserImpl.USERNAME, USERNAME, UserImpl.EXTERNAL_USER, "TRUE");
final String tokenName = "tokenName";
final String token = "someToken";
final DateTime lastAccess = Tools.nowUTC();
final Map<String, Object> tokenProps = Map.of(AccessTokenImpl.NAME, tokenName, AccessTokenImpl.TOKEN, token, AccessTokenImpl.LAST_ACCESS, lastAccess);
final ObjectId tokenId = new ObjectId();
final AccessToken accessToken = new AccessTokenImpl(tokenId, tokenProps);
final Token expected = Token.create(tokenId.toHexString(), tokenName, token, lastAccess);

when(userManagementService.loadById(eq(USERNAME))).thenReturn(userImplFactory.create(userProps));
when(subject.isPermitted(eq(USERS_TOKENCREATE + ":" + USERNAME))).thenReturn(true);
when(clusterConfigService.getOrDefault(UserConfiguration.class, UserConfiguration.DEFAULT_VALUES))
.thenReturn(UserConfiguration.create(false, Duration.of(8, ChronoUnit.HOURS), true));
when(accessTokenService.create(USERNAME, tokenName)).thenReturn(accessToken);

try {
final Token actual = usersResource.generateNewToken(USERNAME, tokenName, "{}");
assertEquals(expected, actual);
} finally {
verify(subject).isPermitted(USERS_TOKENCREATE + ":" + USERNAME);
verify(clusterConfigService).getOrDefault(UserConfiguration.class, UserConfiguration.DEFAULT_VALUES);
verify(accessTokenService).create(USERNAME, tokenName);
verifyNoMoreInteractions(clusterConfigService, accessTokenService);
}
}

private CreateUserRequest buildCreateUserRequest() {
return CreateUserRequest.create(USERNAME, PASSWORD, EMAIL,
FIRST_NAME, LAST_NAME, Collections.singletonList(""),
@@ -151,9 +236,10 @@ public TestUsersResource(UserManagementService userManagementService, PaginatedU
AccessTokenService accessTokenService, RoleService roleService,
MongoDBSessionService sessionService, HttpConfiguration configuration,
Subject subject, UserSessionTerminationService sessionTerminationService,
DefaultSecurityManager securityManager, GlobalAuthServiceConfig globalAuthServiceConfig) {
DefaultSecurityManager securityManager, GlobalAuthServiceConfig globalAuthServiceConfig,
ClusterConfigService clusterConfigService) {
super(userManagementService, paginatedUserService, accessTokenService, roleService, sessionService,
sessionTerminationService, securityManager, globalAuthServiceConfig);
sessionTerminationService, securityManager, globalAuthServiceConfig, clusterConfigService);
this.subject = subject;
super.configuration = configuration;
}