Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
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
2 changes: 1 addition & 1 deletion compose.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ services:
- "3306:3306"

faf-db-migrations:
image: faforever/faf-db-migrations:v140
image: faforever/faf-db-migrations:v143
command: migrate
environment:
FLYWAY_URL: jdbc:mysql://faf-db/faf?useSSL=false
Expand Down
49 changes: 49 additions & 0 deletions push-branch-image.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
#!/usr/bin/env sh
# Build the current branch into a Docker image and push it as a tag.
# Refuses to run on develop/main/master or in a detached HEAD state.
#
# Builds for linux/amd64 (x86_64) by default so the image runs on the real
# servers even when building from an arm64 machine (Apple Silicon).
#
# The Dockerfile expects a prebuilt jar in build/libs/, so we run the Gradle
# bootJar task first (skip with SKIP_BUILD=1 if you already built it).
#
# Usage: ./push-branch-image.sh
# Env: IMAGE override the destination repo (default: faforever/faf-java-api)
# PLATFORM override the target platform (default: linux/amd64)
# SKIP_BUILD set to 1 to reuse an existing build/libs jar
set -eu

IMAGE="${IMAGE:-faforever/faf-java-api}"
PLATFORM="${PLATFORM:-linux/amd64}"

branch=$(git rev-parse --abbrev-ref HEAD)

case "$branch" in
develop|main|master|HEAD)
echo "Refusing to push '$branch' as a Docker tag." >&2
exit 1
;;
esac

# Docker tags must match [A-Za-z0-9_][A-Za-z0-9_.-]{0,127}.
# Replace anything else (most commonly '/' from branch prefixes) with '-'.
tag=$(printf '%s' "$branch" | sed 's/[^A-Za-z0-9_.-]/-/g')

Comment on lines +29 to +32

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Ensure sanitized tag is always Docker-valid before invoking buildx.

Current sanitization replaces invalid chars, but it can still yield an invalid first character (./-) or exceed 128 chars, causing avoidable late failures in docker buildx.

Suggested patch
-# Docker tags must match [A-Za-z0-9_][A-Za-z0-9_.-]{0,127}.
-# Replace anything else (most commonly '/' from branch prefixes) with '-'.
-tag=$(printf '%s' "$branch" | sed 's/[^A-Za-z0-9_.-]/-/g')
+# Docker tags must match [A-Za-z0-9_][A-Za-z0-9_.-]{0,127}.
+# Replace invalid chars, normalize leading '.'/'-', and cap length.
+tag=$(
+  printf '%s' "$branch" \
+    | sed 's/[^A-Za-z0-9_.-]/-/g' \
+    | sed 's/^[.-][.-]*/_/' \
+    | cut -c1-128
+)
+
+if [ -z "$tag" ]; then
+  echo "Refusing to push: sanitized tag is empty for branch '$branch'." >&2
+  exit 1
+fi
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
# Docker tags must match [A-Za-z0-9_][A-Za-z0-9_.-]{0,127}.
# Replace anything else (most commonly '/' from branch prefixes) with '-'.
tag=$(printf '%s' "$branch" | sed 's/[^A-Za-z0-9_.-]/-/g')
# Docker tags must match [A-Za-z0-9_][A-Za-z0-9_.-]{0,127}.
# Replace invalid chars, normalize leading '.'/'-', and cap length.
tag=$(
printf '%s' "$branch" \
| sed 's/[^A-Za-z0-9_.-]/-/g' \
| sed 's/^[.-][.-]*/_/' \
| cut -c1-128
)
if [ -z "$tag" ]; then
echo "Refusing to push: sanitized tag is empty for branch '$branch'." >&2
exit 1
fi
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@push-branch-image.sh` around lines 29 - 32, The tag sanitization for the
Docker image does not fully validate Docker tag requirements. The sed
substitution replaces invalid characters but can still produce invalid tags if
the first character becomes a hyphen or dot (Docker requires first character to
be alphanumeric or underscore), or if the resulting tag exceeds 128 characters.
After the sed substitution that creates the tag variable, add logic to remove
any leading hyphens or dots from the tag and truncate the tag to a maximum of
128 characters to ensure complete Docker compliance before passing it to the
docker buildx command.

if [ "${SKIP_BUILD:-0}" != "1" ]; then
echo "Building jar with ./gradlew bootJar"
./gradlew bootJar -x test
fi

echo "Building $IMAGE:$tag from branch $branch for $PLATFORM"
# buildx with --push builds and pushes in one step. --provenance=false keeps
# the registry tag pointing at a plain image manifest rather than a manifest
# list, which some deployment tooling expects.
docker buildx build \
--platform "$PLATFORM" \
--provenance=false \
-t "$IMAGE:$tag" \
--push \
.

echo "Done: $IMAGE:$tag"
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@
@Configuration
public class MainDbTestContainers {
private static final MariaDBContainer<?> fafDBContainer = new MariaDBContainer<>("mariadb:11.7");
private static final GenericContainer<?> flywayMigrationsContainer = new GenericContainer<>("faforever/faf-db-migrations:v140");
private static final GenericContainer<?> flywayMigrationsContainer = new GenericContainer<>("faforever/faf-db-migrations:v143");
private static final Network sharedNetwork = Network.newNetwork();

@Bean
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,12 @@ public class AvatarAssignment extends AbstractEntity<AvatarAssignment> implement
@Relationship(Avatar.TYPE_NAME)
private Avatar avatar;

/**
* @deprecated The selected avatar is now tracked via {@link Player#getCurrentAvatar()}
* (the {@code login.avatar_id} column). Update that relationship instead; this flag is
* kept only for backwards compatibility with older clients and will be removed.
*/
@Deprecated(forRemoval = true)
@Column(name = "selected")
@UpdatePermission(expression = IsEntityOwner.EXPRESSION)
@Audit(action = Action.UPDATE, logStatement = "Avatar ''{0}'' has been selected on player ''{1}''", logExpressions = {"${avatarAssignment.avatar.id}", "${avatarAssignment.player.id}"})
Expand Down
27 changes: 27 additions & 0 deletions src/main/java/com/faforever/api/data/domain/Player.java
Original file line number Diff line number Diff line change
Expand Up @@ -2,16 +2,24 @@

import com.faforever.api.data.checks.IsEntityOwner;
import com.faforever.api.data.checks.Prefab;
import com.faforever.api.data.hook.PlayerAvatarUpdateHook;
import com.faforever.api.data.hook.PlayerAvatarValidationHook;
import com.faforever.api.security.elide.permission.AdminModerationReportCheck;
import com.github.jasminb.jsonapi.annotations.Type;
import com.yahoo.elide.annotation.Audit;
import com.yahoo.elide.annotation.Audit.Action;
import com.yahoo.elide.annotation.Include;
import com.yahoo.elide.annotation.LifeCycleHookBinding;
import com.yahoo.elide.annotation.ReadPermission;
import com.yahoo.elide.annotation.UpdatePermission;
import lombok.Setter;
import org.hibernate.annotations.BatchSize;

import jakarta.persistence.Entity;
import jakarta.persistence.FetchType;
import jakarta.persistence.JoinColumn;
import jakarta.persistence.ManyToMany;
import jakarta.persistence.ManyToOne;
import jakarta.persistence.OneToMany;
import jakarta.persistence.OneToOne;
import jakarta.persistence.Table;
Expand All @@ -29,9 +37,28 @@ public class Player extends Login {
private ClanMembership clanMembership;
private Set<NameRecord> names;
private Set<AvatarAssignment> avatarAssignments;
private Avatar currentAvatar;
private Set<ModerationReport> reporterOnModerationReports;
private Set<ModerationReport> reportedOnModerationReports;

@UpdatePermission(expression = IsEntityOwner.EXPRESSION)
@LifeCycleHookBinding(
operation = LifeCycleHookBinding.Operation.UPDATE,
phase = LifeCycleHookBinding.TransactionPhase.PRECOMMIT,
hook = PlayerAvatarValidationHook.class
)
@LifeCycleHookBinding(
operation = LifeCycleHookBinding.Operation.UPDATE,
phase = LifeCycleHookBinding.TransactionPhase.POSTCOMMIT,
hook = PlayerAvatarUpdateHook.class
)
@Audit(action = Action.UPDATE, logStatement = "Avatar ''{0}'' has been selected on player ''{1}''", logExpressions = {"${player.currentAvatar.id}", "${player.id}"})
@ManyToOne(fetch = FetchType.LAZY)
@JoinColumn(name = "avatar_id")
public Avatar getCurrentAvatar() {
return currentAvatar;
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.

// Permission is managed by ClanMembership class
@UpdatePermission(expression = Prefab.ALL)
@OneToOne(mappedBy = "player")
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
package com.faforever.api.data.hook;

import com.faforever.api.config.RabbitConfiguration;
import com.faforever.api.data.domain.Avatar;
import com.faforever.api.data.domain.Player;
import com.yahoo.elide.annotation.LifeCycleHookBinding.Operation;
import com.yahoo.elide.annotation.LifeCycleHookBinding.TransactionPhase;
import com.yahoo.elide.core.lifecycle.LifeCycleHook;
import com.yahoo.elide.core.security.ChangeSpec;
import com.yahoo.elide.core.security.RequestScope;
import lombok.RequiredArgsConstructor;
import lombok.extern.slf4j.Slf4j;
import org.springframework.amqp.rabbit.core.RabbitTemplate;
import org.springframework.stereotype.Component;

import java.util.HashMap;
import java.util.Map;
import java.util.Optional;

@Slf4j
@Component
@RequiredArgsConstructor
public class PlayerAvatarUpdateHook implements LifeCycleHook<Player> {

public static final String ROUTING_KEY_PLAYER_AVATAR_UPDATE = "success.player_avatar.update";

private final RabbitTemplate rabbitTemplate;

@Override
public void execute(Operation operation, TransactionPhase phase, Player player, RequestScope requestScope, Optional<ChangeSpec> changes) {
final Avatar avatar = player.getCurrentAvatar();
final Integer avatarId = avatar == null ? null : avatar.getId();

Map<String, Object> payload = new HashMap<>();
payload.put("player_id", player.getId());
payload.put("avatar_id", avatarId);

log.debug("Publishing player_avatar update: {}", payload);
rabbitTemplate.convertAndSend(
RabbitConfiguration.EXCHANGE_FAF_LOBBY,
ROUTING_KEY_PLAYER_AVATAR_UPDATE,
payload
);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
package com.faforever.api.data.hook;

import com.faforever.api.avatar.AvatarAssignmentRepository;
import com.faforever.api.data.domain.Avatar;
import com.faforever.api.data.domain.Player;
import com.faforever.api.error.ApiException;
import com.faforever.api.error.Error;
import com.faforever.api.error.ErrorCode;
import com.yahoo.elide.annotation.LifeCycleHookBinding.Operation;
import com.yahoo.elide.annotation.LifeCycleHookBinding.TransactionPhase;
import com.yahoo.elide.core.lifecycle.LifeCycleHook;
import com.yahoo.elide.core.security.ChangeSpec;
import com.yahoo.elide.core.security.RequestScope;
import lombok.RequiredArgsConstructor;
import org.springframework.stereotype.Component;

import java.util.Optional;

/**
* Rejects assigning a {@code currentAvatar} that is not granted to the player. The
* {@code IsEntityOwner} update permission only guarantees the caller edits their own Player row;
* it does not verify the avatar belongs to them. Clearing the avatar (null) is always allowed.
*/
@Component
@RequiredArgsConstructor
public class PlayerAvatarValidationHook implements LifeCycleHook<Player> {

private final AvatarAssignmentRepository avatarAssignmentRepository;

@Override
public void execute(Operation operation, TransactionPhase phase, Player player, RequestScope requestScope, Optional<ChangeSpec> changes) {
final Avatar avatar = player.getCurrentAvatar();
if (avatar == null) {
return;
}

avatarAssignmentRepository.findOneByAvatarIdAndPlayerId(avatar.getId(), player.getId())
.orElseThrow(() -> new ApiException(new Error(ErrorCode.AVATAR_NOT_ASSIGNED, avatar.getId(), player.getId())));
}
}
1 change: 1 addition & 0 deletions src/main/java/com/faforever/api/error/ErrorCode.java
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,7 @@ public enum ErrorCode {
STEAM_LOGIN_VALIDATION_FAILED(210, "Login via Steam failed", "Invalid OpenID redirect code"),
MAP_VERSION_INVALID_RANGE(211, "Invalid map version", "The map version must be a whole number in range {0, number} to {1, number}."),
MOD_VERSION_INVALID_RANGE(212, "Invalid mod version", "The mod version must be a whole number in range {0, number} to {1, number}."),
AVATAR_NOT_ASSIGNED(213, "Avatar not assigned", "Avatar ''{0}'' is not assigned to player ''{1}''."),
;


Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
package com.faforever.api.data.hook;

import com.faforever.api.avatar.AvatarAssignmentRepository;
import com.faforever.api.data.domain.Avatar;
import com.faforever.api.data.domain.AvatarAssignment;
import com.faforever.api.data.domain.Player;
import com.faforever.api.error.ApiException;
import com.faforever.api.error.ErrorCode;
import com.yahoo.elide.annotation.LifeCycleHookBinding.Operation;
import com.yahoo.elide.annotation.LifeCycleHookBinding.TransactionPhase;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.ExtendWith;
import org.mockito.Mock;
import org.mockito.junit.jupiter.MockitoExtension;

import java.util.Optional;

import static com.faforever.api.error.ApiExceptionMatcher.hasErrorCode;
import static org.hamcrest.MatcherAssert.assertThat;
import static org.junit.jupiter.api.Assertions.assertThrows;
import static org.mockito.Mockito.verifyNoInteractions;
import static org.mockito.Mockito.when;

@ExtendWith(MockitoExtension.class)
class PlayerAvatarValidationHookTest {

private static final int PLAYER_ID = 7;
private static final int AVATAR_ID = 42;

@Mock
private AvatarAssignmentRepository avatarAssignmentRepository;

private PlayerAvatarValidationHook instance;

@BeforeEach
void setUp() {
instance = new PlayerAvatarValidationHook(avatarAssignmentRepository);
}

private static Player playerWithAvatar(Integer avatarId) {
Avatar avatar = new Avatar();
avatar.setId(avatarId);
Player player = new Player();
player.setId(PLAYER_ID);
player.setCurrentAvatar(avatar);
return player;
}

@Test
void allowsAssignedAvatar() {
when(avatarAssignmentRepository.findOneByAvatarIdAndPlayerId(AVATAR_ID, PLAYER_ID))
.thenReturn(Optional.of(new AvatarAssignment()));

instance.execute(Operation.UPDATE, TransactionPhase.PRECOMMIT, playerWithAvatar(AVATAR_ID), null, Optional.empty());
}

@Test
void rejectsUnassignedAvatar() {
when(avatarAssignmentRepository.findOneByAvatarIdAndPlayerId(AVATAR_ID, PLAYER_ID))
.thenReturn(Optional.empty());

ApiException result = assertThrows(ApiException.class, () ->
instance.execute(Operation.UPDATE, TransactionPhase.PRECOMMIT, playerWithAvatar(AVATAR_ID), null, Optional.empty()));
assertThat(result, hasErrorCode(ErrorCode.AVATAR_NOT_ASSIGNED));
}

@Test
void allowsClearingAvatar() {
Player player = new Player();
player.setId(PLAYER_ID);

instance.execute(Operation.UPDATE, TransactionPhase.PRECOMMIT, player, null, Optional.empty());

verifyNoInteractions(avatarAssignmentRepository);
}
}
Loading