From 334bd779e9f7fd74f0a11a8fdcee05ac6c054c9c Mon Sep 17 00:00:00 2001 From: Yury Brigadirenko Date: Fri, 3 Jan 2025 15:13:14 -0500 Subject: [PATCH] concord-server: allow tokens without users, remove user from default agent token (#1054) --- .../concord/server/db/liquibase.xml | 1 + .../walmartlabs/concord/server/db/v0.69.0.xml | 34 ++++---- .../walmartlabs/concord/server/db/v0.70.0.xml | 28 +++---- .../walmartlabs/concord/server/db/v0.79.0.xml | 3 + .../walmartlabs/concord/server/db/v0.80.0.xml | 4 + .../walmartlabs/concord/server/db/v1.45.0.xml | 8 +- .../walmartlabs/concord/server/db/v1.86.0.xml | 11 ++- .../walmartlabs/concord/server/db/v2.21.0.xml | 47 +++++++++++ .../server/security/apikey/ApiKeyRealm.java | 17 ++-- .../security/apikey/ApiKeyResource.java | 77 +++++++++++++------ .../server/liquibase/ext/ApiTokenCreator.java | 42 +++++++--- 11 files changed, 200 insertions(+), 72 deletions(-) create mode 100644 server/db/src/main/resources/com/walmartlabs/concord/server/db/v2.21.0.xml diff --git a/server/db/src/main/resources/com/walmartlabs/concord/server/db/liquibase.xml b/server/db/src/main/resources/com/walmartlabs/concord/server/db/liquibase.xml index e0f1f06686..23dde8d830 100644 --- a/server/db/src/main/resources/com/walmartlabs/concord/server/db/liquibase.xml +++ b/server/db/src/main/resources/com/walmartlabs/concord/server/db/liquibase.xml @@ -115,5 +115,6 @@ + diff --git a/server/db/src/main/resources/com/walmartlabs/concord/server/db/v0.69.0.xml b/server/db/src/main/resources/com/walmartlabs/concord/server/db/v0.69.0.xml index aab2a816f4..f1d95bb7fd 100644 --- a/server/db/src/main/resources/com/walmartlabs/concord/server/db/v0.69.0.xml +++ b/server/db/src/main/resources/com/walmartlabs/concord/server/db/v0.69.0.xml @@ -4,24 +4,32 @@ xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://www.liquibase.org/xml/ns/dbchangelog http://www.liquibase.org/xml/ns/dbchangelog/dbchangelog-3.3.xsd"> + + - + + + <!– "O+JMYwBsU797EKtlRQYu+Q" –> + 1sw9eLZ41EOK4w/iV3jFnn6cqeAMeFtxfazqVY04koY + ${concordAgentUserId} + + + --> + + diff --git a/server/db/src/main/resources/com/walmartlabs/concord/server/db/v0.70.0.xml b/server/db/src/main/resources/com/walmartlabs/concord/server/db/v0.70.0.xml index 068d614af3..f83f4e3193 100644 --- a/server/db/src/main/resources/com/walmartlabs/concord/server/db/v0.70.0.xml +++ b/server/db/src/main/resources/com/walmartlabs/concord/server/db/v0.70.0.xml @@ -4,21 +4,23 @@ xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://www.liquibase.org/xml/ns/dbchangelog http://www.liquibase.org/xml/ns/dbchangelog/dbchangelog-3.3.xsd"> - + + <!– "Gz0q/DeGlH8Zs7QJMj1v8g" –> + DrRt3j6G7b6GHY/Prddu4voyKyZa17iFkEj99ac0q/A + ${concordRunnerUserId} + + + --> diff --git a/server/db/src/main/resources/com/walmartlabs/concord/server/db/v0.79.0.xml b/server/db/src/main/resources/com/walmartlabs/concord/server/db/v0.79.0.xml index 5ae08bc7b1..ad62e33bc1 100644 --- a/server/db/src/main/resources/com/walmartlabs/concord/server/db/v0.79.0.xml +++ b/server/db/src/main/resources/com/walmartlabs/concord/server/db/v0.79.0.xml @@ -4,12 +4,15 @@ xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://www.liquibase.org/xml/ns/dbchangelog http://www.liquibase.org/xml/ns/dbchangelog/dbchangelog-3.3.xsd"> + + diff --git a/server/db/src/main/resources/com/walmartlabs/concord/server/db/v0.80.0.xml b/server/db/src/main/resources/com/walmartlabs/concord/server/db/v0.80.0.xml index a44d519aa3..94555ad3a8 100644 --- a/server/db/src/main/resources/com/walmartlabs/concord/server/db/v0.80.0.xml +++ b/server/db/src/main/resources/com/walmartlabs/concord/server/db/v0.80.0.xml @@ -7,16 +7,20 @@ + ANY ${concordSystemWriterRoleId} concordSystemWriter true + + - + + + --> diff --git a/server/db/src/main/resources/com/walmartlabs/concord/server/db/v1.86.0.xml b/server/db/src/main/resources/com/walmartlabs/concord/server/db/v1.86.0.xml index 3e122acf6f..2e17a7f069 100644 --- a/server/db/src/main/resources/com/walmartlabs/concord/server/db/v1.86.0.xml +++ b/server/db/src/main/resources/com/walmartlabs/concord/server/db/v1.86.0.xml @@ -5,7 +5,10 @@ xsi:schemaLocation="http://www.liquibase.org/xml/ns/dbchangelog http://www.liquibase.org/xml/ns/dbchangelog/dbchangelog-3.3.xsd"> + + @@ -38,6 +41,8 @@ + + + + - + --> diff --git a/server/db/src/main/resources/com/walmartlabs/concord/server/db/v2.21.0.xml b/server/db/src/main/resources/com/walmartlabs/concord/server/db/v2.21.0.xml new file mode 100644 index 0000000000..5e6ed21a73 --- /dev/null +++ b/server/db/src/main/resources/com/walmartlabs/concord/server/db/v2.21.0.xml @@ -0,0 +1,47 @@ + + + + + + + + + + + + create unique index IDX_API_KEYS_NAME_USER_NULL on API_KEYS (KEY_NAME) where USER_ID is null + + + create unique index IDX_API_KEYS_NAME_USER_NOT_NULL on API_KEYS (KEY_NAME, USER_ID) where USER_ID is not null + + + + + ANY + + + + select count(key_id) + from API_KEYS + where KEY_NAME = 'concordAgentKey_autogenerated'; + + + + select count(key_id) + from API_KEYS + where USER_ID = 'd4f123c1-f8d4-40b2-8a12-b8947b9ce2d8'; + + + + + + + + + + + + diff --git a/server/impl/src/main/java/com/walmartlabs/concord/server/security/apikey/ApiKeyRealm.java b/server/impl/src/main/java/com/walmartlabs/concord/server/security/apikey/ApiKeyRealm.java index 609893efc8..e3398c360c 100644 --- a/server/impl/src/main/java/com/walmartlabs/concord/server/security/apikey/ApiKeyRealm.java +++ b/server/impl/src/main/java/com/walmartlabs/concord/server/security/apikey/ApiKeyRealm.java @@ -62,17 +62,20 @@ public boolean supports(AuthenticationToken token) { protected AuthenticationInfo doGetAuthenticationInfo(AuthenticationToken token) throws AuthenticationException { ApiKey t = (ApiKey) token; - UserEntry u = userManager.get(t.getUserId()).orElse(null); - if (u == null) { - return null; - } + UserEntry u = null; + if (t.getUserId() != null) { + u = userManager.get(t.getUserId()).orElse(null); + if (u == null) { + return null; + } - if (u.isDisabled()) { - throw new AuthenticationException("User account '" + u.getName() + "' is disabled"); + if (u.isDisabled()) { + throw new AuthenticationException("User account '" + u.getName() + "' is disabled"); + } } auditLog.add(AuditObject.SYSTEM, AuditAction.ACCESS) - .userId(u.getId()) + .userId(u != null ? u.getId() : null) .field("realm", REALM_NAME) .field("apiKeyId", t.getKeyId()) .log(); diff --git a/server/impl/src/main/java/com/walmartlabs/concord/server/security/apikey/ApiKeyResource.java b/server/impl/src/main/java/com/walmartlabs/concord/server/security/apikey/ApiKeyResource.java index da0d664385..a33be0a717 100644 --- a/server/impl/src/main/java/com/walmartlabs/concord/server/security/apikey/ApiKeyResource.java +++ b/server/impl/src/main/java/com/walmartlabs/concord/server/security/apikey/ApiKeyResource.java @@ -20,6 +20,7 @@ * ===== */ +import com.walmartlabs.concord.common.validation.ConcordKey; import com.walmartlabs.concord.db.PgUtils; import com.walmartlabs.concord.server.GenericOperationResult; import com.walmartlabs.concord.server.OperationResult; @@ -38,6 +39,7 @@ import com.walmartlabs.concord.server.user.UserType; import io.swagger.v3.oas.annotations.Operation; import io.swagger.v3.oas.annotations.tags.Tag; +import org.apache.shiro.authz.AuthorizationException; import org.jooq.exception.DataAccessException; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -47,7 +49,6 @@ import javax.ws.rs.*; import javax.ws.rs.core.MediaType; import java.time.OffsetDateTime; -import java.time.temporal.ChronoUnit; import java.util.List; import java.util.UUID; @@ -85,6 +86,22 @@ public List list(@QueryParam("userId") UUID requestUserId) { return apiKeyDao.list(userId); } + @POST + @Path("/{name}") + @Consumes(MediaType.APPLICATION_JSON) + @Produces(MediaType.APPLICATION_JSON) + @Validate + @Operation(description = "Create a new API key", operationId = "createApiKey") + public CreateApiKeyResponse create(@PathParam("name") @ConcordKey String name) { + assertAdmin(); + + if (apiKeyDao.getId(null, name) != null) { + throw new ValidationErrorsException("API Token with name '" + name + "' already exists"); + } + + return createApiKey(null, name); + } + @POST @Consumes(MediaType.APPLICATION_JSON) @Produces(MediaType.APPLICATION_JSON) @@ -105,18 +122,44 @@ public CreateApiKeyResponse create(@Valid CreateApiKeyRequest req) { String name = trim(req.getName()); if (name == null || name.isEmpty()) { // auto generate the name - name = "key#" + UUID.randomUUID().toString(); + name = "key#" + UUID.randomUUID(); } if (apiKeyDao.getId(userId, name) != null) { throw new ValidationErrorsException("API Token with name '" + name + "' already exists"); } + return createApiKey(userId, name); + } + + @DELETE + @Path("/{id}") + @Produces(MediaType.APPLICATION_JSON) + @Validate + @Operation(description = "Delete an existing API key", operationId = "deleteUserApiKeyById") + public GenericOperationResult deleteKeyById(@PathParam("id") UUID id) { + UUID userId = apiKeyDao.getUserId(id); + if (userId == null) { + throw new ValidationErrorsException("API key not found: " + id); + } + + assertOwner(userId); + + apiKeyDao.delete(id); + + auditLog.add(AuditObject.API_KEY, AuditAction.DELETE) + .field("id", id) + .log(); + + return new GenericOperationResult(OperationResult.DELETED); + } + + private CreateApiKeyResponse createApiKey(UUID userId, String name) { String key = apiKeyDao.newApiKey(); OffsetDateTime expiredAt = null; if (cfg.isExpirationEnabled()) { - expiredAt = OffsetDateTime.now().plus(cfg.getExpirationPeriod().toDays(), ChronoUnit.DAYS); + expiredAt = OffsetDateTime.now().plusDays(cfg.getExpirationPeriod().toDays()); } UUID id; @@ -141,28 +184,6 @@ public CreateApiKeyResponse create(@Valid CreateApiKeyRequest req) { return new CreateApiKeyResponse(id, key); } - @DELETE - @Path("/{id}") - @Produces(MediaType.APPLICATION_JSON) - @Validate - @Operation(description = "Delete an existing API key", operationId = "deleteUserApiKeyById") - public GenericOperationResult deleteKeyById(@PathParam("id") UUID id) { - UUID userId = apiKeyDao.getUserId(id); - if (userId == null) { - throw new ValidationErrorsException("API key not found: " + id); - } - - assertOwner(userId); - - apiKeyDao.delete(id); - - auditLog.add(AuditObject.API_KEY, AuditAction.DELETE) - .field("id", id) - .log(); - - return new GenericOperationResult(OperationResult.DELETED); - } - private UUID assertUsername(String username, String domain, UserType type) { if (username == null) { return null; @@ -200,6 +221,12 @@ private static void assertOwner(UUID userId) { } } + private static void assertAdmin() { + if (!Roles.isAdmin()) { + throw new AuthorizationException("Only admins are allowed to update organizations"); + } + } + private static String trim(String s) { if (s == null) { return null; diff --git a/server/liquibase-ext/src/main/java/com/walmartlabs/concord/server/liquibase/ext/ApiTokenCreator.java b/server/liquibase-ext/src/main/java/com/walmartlabs/concord/server/liquibase/ext/ApiTokenCreator.java index 4f7d9d477a..c1ee5c0712 100644 --- a/server/liquibase-ext/src/main/java/com/walmartlabs/concord/server/liquibase/ext/ApiTokenCreator.java +++ b/server/liquibase-ext/src/main/java/com/walmartlabs/concord/server/liquibase/ext/ApiTokenCreator.java @@ -43,12 +43,13 @@ public class ApiTokenCreator implements CustomSqlChange, CustomSqlRollback { private static final Logger log = LoggerFactory.getLogger(ApiTokenCreator.class); - private static final String KEY_NAME = "autogenerated"; + private static final String DEFAULT_KEY_NAME = "autogenerated"; private String token; private String userId; private String username; private String skip; + private String keyName; public void setUserId(String userId) { this.userId = userId; @@ -66,6 +67,10 @@ public void setSkip(String skip) { this.skip = skip; } + public void setKeyName(String keyName) { + this.keyName = keyName; + } + @Override public SqlStatement[] generateStatements(Database database) { if (this.token == null) { @@ -77,7 +82,7 @@ public SqlStatement[] generateStatements(Database database) { new InsertStatement(null, null, "API_KEYS") .addColumnValue("API_KEY", hash(token)) .addColumnValue("USER_ID", userId) - .addColumnValue("KEY_NAME", KEY_NAME) + .addColumnValue("KEY_NAME", getKeyName()) }; } @@ -87,12 +92,20 @@ public SqlStatement[] generateRollbackStatements(Database database) { return new SqlStatement[0]; } - return new SqlStatement[]{ - new DeleteStatement(null, null, "API_KEYS") - .setWhere("USER_ID=? and KEY_NAME=?") - .addWhereParameter(userId) - .addWhereParameter(KEY_NAME) - }; + if (userId != null) { + return new SqlStatement[]{ + new DeleteStatement(null, null, "API_KEYS") + .setWhere("USER_ID=? and KEY_NAME=?") + .addWhereParameter(userId) + .addWhereParameter(getKeyName()) + }; + } else { + return new SqlStatement[]{ + new DeleteStatement(null, null, "API_KEYS") + .setWhere("KEY_NAME=?") + .addWhereParameter(getKeyName()) + }; + } } @Override @@ -105,7 +118,11 @@ public String getConfirmationMessage() { System.out.println(">>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>"); System.out.println(">>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>"); System.out.println(); - System.out.println("API token created for user '" + username + "': " + token); + if (username == null) { + System.out.println("API token created (without a user): " + token); + } else { + System.out.println("API token created for user '" + username + "': " + token); + } System.out.println(); System.out.println("(don't forget to remove it in production)"); System.out.println(); @@ -137,6 +154,13 @@ public ValidationErrors validate(Database database) { return null; } + private String getKeyName() { + if (this.keyName != null) { + return this.keyName; + } + return DEFAULT_KEY_NAME; + } + private static String newApiKey() { try { byte[] ab = new byte[16];