Split User and Player class properly to avoid side filter attacks

FAForever · Mar 21, 2020 · bddf852 · bddf852
1 parent 0c54fe3
commit bddf852
Show file tree

Hide file tree

Showing 13 changed files with 161 additions and 164 deletions.
diff --git a/src/inttest/java/com/faforever/api/data/AvatarAssignmentElideTest.java b/src/inttest/java/com/faforever/api/data/AvatarAssignmentElideTest.java
@@ -6,7 +6,6 @@
 import com.faforever.commons.api.dto.Avatar;
 import com.faforever.commons.api.dto.AvatarAssignment;
 import com.faforever.commons.api.dto.Player;
-import org.junit.jupiter.api.Disabled;
 import org.junit.jupiter.api.Test;
 import org.springframework.http.HttpHeaders;
 import org.springframework.security.test.context.support.WithUserDetails;
@@ -53,7 +52,6 @@ public void getAvatarWithPlayer() throws Exception {
   }
 
   @Test
-  @Disabled("Temporary disabled due to security changes in Player class")
   public void canAssignAvatarWithScopeAndRole() throws Exception {
     final Avatar avatar = (Avatar) new Avatar().setId("1");
     final Player player = (Player) new Player().setId("1");

diff --git a/src/inttest/java/com/faforever/api/data/BanTest.java b/src/inttest/java/com/faforever/api/data/BanTest.java
@@ -7,7 +7,6 @@
 import com.faforever.commons.api.dto.BanLevel;
 import com.faforever.commons.api.dto.ModerationReport;
 import com.faforever.commons.api.dto.Player;
-import org.junit.jupiter.api.Disabled;
 import org.junit.jupiter.api.Test;
 import org.springframework.http.HttpHeaders;
 import org.springframework.test.context.jdbc.Sql;
@@ -124,7 +123,6 @@ public void cannotCreateBanWithoutRole() throws Exception {
   }
 
   @Test
-  @Disabled("Temporary disabled due to security changes in Player class")
   public void canCreateBanWithScopeAndRole() throws Exception {
     mockMvc.perform(post("/data/banInfo")
       .with(getOAuthTokenWithTestUser(OAuthScope._ADMINISTRATIVE_ACTION, GroupPermission.ROLE_ADMIN_ACCOUNT_BAN))
@@ -134,7 +132,6 @@ public void canCreateBanWithScopeAndRole() throws Exception {
   }
 
   @Test
-  @Disabled("Temporary disabled due to security changes in Player class")
   public void canCreateBanWithModerationWithScopeAndRole() throws Exception {
 
     final BanInfo ban = new BanInfo()

diff --git a/src/inttest/java/com/faforever/api/data/ClanElideTest.java b/src/inttest/java/com/faforever/api/data/ClanElideTest.java
@@ -8,7 +8,6 @@
 import com.faforever.api.player.PlayerRepository;
 import lombok.SneakyThrows;
 import org.json.JSONObject;
-import org.junit.jupiter.api.Disabled;
 import org.junit.jupiter.api.Test;
 import org.springframework.beans.factory.annotation.Autowired;
 import org.springframework.http.HttpHeaders;
@@ -96,7 +95,6 @@ public void cannotDeleteAsMember() throws Exception {
   }
 
   @Test
-  @Disabled("Temporary disabled due to security changes in Player class")
   public void getFilteredPlayerForClanInvite() throws Exception {
     mockMvc.perform(get("/data/player?filter=login==*MEMBER*&sort=login"))
       .andExpect(status().isOk())
@@ -107,7 +105,6 @@ public void getFilteredPlayerForClanInvite() throws Exception {
 
   @Test
   @WithUserDetails(AUTH_CLAN_LEADER)
-  @Disabled("Temporary disabled due to security changes in Player class")
   public void canTransferLeadershipAsLeader() throws Exception {
     assertThat(clanRepository.getOne(1).getLeader().getLogin(), is(AUTH_CLAN_LEADER));
 

diff --git a/src/inttest/java/com/faforever/api/data/UserNoteTest.java b/src/inttest/java/com/faforever/api/data/UserNoteTest.java
@@ -2,19 +2,20 @@
 
 import com.faforever.api.AbstractIntegrationTest;
 import com.faforever.api.data.domain.GroupPermission;
-import com.faforever.api.player.PlayerRepository;
 import com.faforever.api.security.OAuthScope;
-import org.junit.jupiter.api.Disabled;
+import com.faforever.api.user.UserRepository;
 import org.junit.jupiter.api.Test;
 import org.springframework.beans.factory.annotation.Autowired;
 import org.springframework.http.HttpHeaders;
 import org.springframework.test.context.jdbc.Sql;
 import org.springframework.test.context.jdbc.Sql.ExecutionPhase;
 
+import java.util.Set;
+
 import static com.faforever.api.data.JsonApiMediaType.JSON_API_MEDIA_TYPE;
+import static org.hamcrest.CoreMatchers.is;
+import static org.hamcrest.MatcherAssert.assertThat;
 import static org.hamcrest.Matchers.hasSize;
-import static org.hamcrest.core.Is.is;
-import static org.junit.Assert.assertThat;
 import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.get;
 import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.post;
 import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.jsonPath;
@@ -35,23 +36,23 @@ public class UserNoteTest extends AbstractIntegrationTest {
           "relationships": {
               "author": {
                   "data": {
-                      "type": "player",
+                      "type": "user",
                       "id": "1"
                   }
               },
               "player": {
                   "data": {
-                      "type": "player",
+                      "type": "user",
                       "id": "3"
                   }
               }
           }
       }
   }
    */
-  private static final String testPost = "{\"data\":{\"type\":\"userNote\",\"attributes\":{\"watched\":false,\"note\":\"This note will be posted\"},\"relationships\":{\"author\":{\"data\":{\"type\":\"player\",\"id\":\"1\"}},\"player\":{\"data\":{\"type\":\"player\",\"id\":\"3\"}}}}}";
+  private static final String testPost = "{\"data\":{\"type\":\"userNote\",\"attributes\":{\"watched\":false,\"note\":\"This note will be posted\"},\"relationships\":{\"author\":{\"data\":{\"type\":\"user\",\"id\":\"1\"}},\"player\":{\"data\":{\"type\":\"user\",\"id\":\"3\"}}}}}";
   @Autowired
-  PlayerRepository playerRepository;
+  UserRepository userRepository;
 
   @Test
   public void emptyResultWithoutScope() throws Exception {
@@ -117,16 +118,16 @@ public void cannotCreateUserNoteWithoutRole() throws Exception {
   }
 
   @Test
-  @Disabled("Temporary disabled due to security changes in Player class")
   public void canCreateUserNoteWithScopeAndRole() throws Exception {
-    assertThat(playerRepository.getOne(3).getUserNotes().size(), is(0));
+    assertThat(userRepository.getOne(3).getUserNotes().size(), is(0));
 
     mockMvc.perform(post("/data/userNote")
-      .with(getOAuthTokenWithTestUser(OAuthScope._READ_SENSIBLE_USERDATA, GroupPermission.ROLE_ADMIN_ACCOUNT_NOTE))
+      .with(getOAuthTokenWithTestUser(Set.of(OAuthScope._READ_SENSIBLE_USERDATA),
+        Set.of(GroupPermission.ROLE_READ_ACCOUNT_PRIVATE_DETAILS, GroupPermission.ROLE_ADMIN_ACCOUNT_NOTE)))
       .header(HttpHeaders.CONTENT_TYPE, JSON_API_MEDIA_TYPE)
       .content(testPost))
       .andExpect(status().isCreated());
 
-    assertThat(playerRepository.getOne(3).getUserNotes().size(), is(1));
+    assertThat(userRepository.getOne(3).getUserNotes().size(), is(1));
   }
 }
diff --git a/src/inttest/java/com/faforever/api/moderationreport/ModerationReportTest.java b/src/inttest/java/com/faforever/api/moderationreport/ModerationReportTest.java
@@ -9,7 +9,6 @@
 import com.faforever.commons.api.dto.Player;
 import com.google.common.collect.Sets;
 import org.junit.jupiter.api.BeforeEach;
-import org.junit.jupiter.api.Disabled;
 import org.junit.jupiter.api.Test;
 import org.springframework.http.HttpHeaders;
 import org.springframework.security.test.context.support.WithAnonymousUser;
@@ -71,7 +70,6 @@ public void anonymousUserCannotCreateValidModerationReport() throws Exception {
   }
 
   @Test
-  @Disabled("Temporary disabled due to security changes in Player class")
   public void canCreateValidModerationReportWithoutScopeAndRole() throws Exception {
     mockMvc.perform(get("/data/account"));
     mockMvc.perform(
@@ -159,7 +157,6 @@ public void cannotUpdateSomeoneElsesReport() throws Exception {
   }
 
   @Test
-  @Disabled("Temporary disabled due to security changes in Player class")
   public void canUpdateOwnReport() throws Exception {
     reportedUsers.add((Player) new Player().setId("1"));
 

diff --git a/src/main/java/com/faforever/api/ban/BanService.java b/src/main/java/com/faforever/api/ban/BanService.java
@@ -1,22 +1,22 @@
 package com.faforever.api.ban;
 
-import com.faforever.api.data.domain.Player;
-import com.faforever.api.player.PlayerRepository;
+import com.faforever.api.data.domain.User;
+import com.faforever.api.user.UserRepository;
+import lombok.RequiredArgsConstructor;
 import org.springframework.stereotype.Service;
 
 @Service
+@RequiredArgsConstructor
 public class BanService {
-  private final PlayerRepository playerRepository;
+  private final UserRepository userRepository;
 
-  public BanService(PlayerRepository playerRepository) {
-    this.playerRepository = playerRepository;
-  }
-
-  public boolean hasActiveGlobalBan(Player player) {
-    return player.isGlobalBanned();
+  public boolean hasActiveGlobalBan(User user) {
+    return user.isGlobalBanned();
   }
 
   public boolean hasActiveGlobalBan(String username) {
-    return hasActiveGlobalBan(playerRepository.findOneByLogin(username));
+    return userRepository.findOneByLogin(username)
+      .map(this::hasActiveGlobalBan)
+      .orElse(false);
   }
 }
diff --git a/src/main/java/com/faforever/api/data/domain/Login.java b/src/main/java/com/faforever/api/data/domain/Login.java
@@ -1,108 +1,37 @@
 package com.faforever.api.data.domain;
 
-import com.faforever.api.data.checks.IsEntityOwner;
 import com.faforever.api.data.checks.Prefab;
-import com.faforever.api.security.elide.permission.AdminAccountBanCheck;
-import com.faforever.api.security.elide.permission.AdminAccountNoteCheck;
-import com.faforever.api.security.elide.permission.ReadAccountPrivateDetailsCheck;
-import com.faforever.api.security.elide.permission.ReadUserGroupCheck;
 import com.fasterxml.jackson.annotation.JsonIgnore;
-import com.yahoo.elide.annotation.ReadPermission;
 import com.yahoo.elide.annotation.UpdatePermission;
 import lombok.Setter;
 
 import javax.persistence.Column;
-import javax.persistence.FetchType;
-import javax.persistence.ManyToMany;
 import javax.persistence.MappedSuperclass;
-import javax.persistence.OneToMany;
+import javax.persistence.OneToOne;
 import javax.persistence.Transient;
-import java.time.OffsetDateTime;
-import java.util.HashSet;
-import java.util.Set;
-import java.util.stream.Collectors;
 
 @MappedSuperclass
 @Setter
 public abstract class Login extends AbstractEntity implements OwnableEntity {
 
   private String login;
-  private String email;
-  private String steamId;
-  private String userAgent;
-  private Set<BanInfo> bans;
-  private Set<UserNote> userNotes;
-  private Set<UserGroup> userGroups;
-  private String recentIpAddress;
-  private OffsetDateTime lastLogin;
-
-  public Login() {
-    this.bans = new HashSet<>(0);
-    this.userNotes = new HashSet<>(0);
-  }
+  private ClanMembership clanMembership;
 
   @Column(name = "login")
   public String getLogin() {
     return login;
   }
 
-  @Column(name = "email")
-  @ReadPermission(expression = IsEntityOwner.EXPRESSION + " OR " + ReadAccountPrivateDetailsCheck.EXPRESSION)
-  public String getEmail() {
-    return email;
-  }
-
-  @Column(name = "steamid")
-  @ReadPermission(expression = IsEntityOwner.EXPRESSION + " OR " + ReadAccountPrivateDetailsCheck.EXPRESSION)
-  public String getSteamId() {
-    return steamId;
-  }
-
-  @Column(name = "ip")
-  @ReadPermission(expression = IsEntityOwner.EXPRESSION + " OR " + ReadAccountPrivateDetailsCheck.EXPRESSION)
-  public String getRecentIpAddress() {
-    return recentIpAddress;
-  }
-
-  @Column(name = "last_login")
-  @ReadPermission(expression = IsEntityOwner.EXPRESSION + " OR " + ReadAccountPrivateDetailsCheck.EXPRESSION)
-  public OffsetDateTime getLastLogin() {
-    return lastLogin;
-  }
-
-  @Column(name = "user_agent")
-  public String getUserAgent() {
-    return userAgent;
-  }
-
-  @OneToMany(mappedBy = "player", fetch = FetchType.EAGER)
-  // Permission is managed by BanInfo class
-  @UpdatePermission(expression = AdminAccountBanCheck.EXPRESSION)
-  public Set<BanInfo> getBans() {
-    return this.bans;
-  }
-
-  @OneToMany(mappedBy = "player", fetch = FetchType.EAGER)
-  @UpdatePermission(expression = AdminAccountNoteCheck.EXPRESSION)
-  public Set<UserNote> getUserNotes() {
-    return this.userNotes;
-  }
-
-  @Transient
-  public Set<BanInfo> getActiveBans() {
-    return getBans().stream().filter(ban -> ban.getBanStatus() == BanStatus.BANNED).collect(Collectors.toSet());
+  // Permission is managed by ClanMembership class
+  @UpdatePermission(expression = Prefab.ALL)
+  @OneToOne(mappedBy = "player")
+  public ClanMembership getClanMembership() {
+    return this.clanMembership;
   }
 
   @Transient
-  public boolean isGlobalBanned() {
-    return getActiveBans().stream().anyMatch(ban -> ban.getLevel() == BanLevel.GLOBAL);
-  }
-
-  @ReadPermission(expression = IsEntityOwner.EXPRESSION + " OR " + ReadUserGroupCheck.EXPRESSION)
-  @UpdatePermission(expression = Prefab.ALL)
-  @ManyToMany(mappedBy = "members")
-  public Set<UserGroup> getUserGroups() {
-    return userGroups;
+  public Clan getClan() {
+    return clanMembership == null ? null : clanMembership.getClan();
   }
 
   @Override

diff --git a/src/main/java/com/faforever/api/data/domain/Player.java b/src/main/java/com/faforever/api/data/domain/Player.java
@@ -1,23 +1,18 @@
 package com.faforever.api.data.domain;
 
-import com.faforever.api.data.checks.IsEntityOwner;
 import com.faforever.api.data.checks.Prefab;
-import com.faforever.api.security.elide.permission.AdminModerationReportCheck;
 import com.github.jasminb.jsonapi.annotations.Type;
 import com.yahoo.elide.annotation.Include;
-import com.yahoo.elide.annotation.ReadPermission;
 import com.yahoo.elide.annotation.SharePermission;
 import com.yahoo.elide.annotation.UpdatePermission;
 import lombok.Setter;
 import org.hibernate.annotations.BatchSize;
 
 import javax.persistence.Entity;
 import javax.persistence.FetchType;
-import javax.persistence.ManyToMany;
 import javax.persistence.OneToMany;
 import javax.persistence.OneToOne;
 import javax.persistence.Table;
-import javax.persistence.Transient;
 import java.util.Set;
 
 @Entity
@@ -27,17 +22,13 @@
 @SharePermission
 @Setter
 @Type(Player.TYPE_NAME)
-@ReadPermission(expression = AdminModerationReportCheck.EXPRESSION + " OR " + IsEntityOwner.EXPRESSION)
 public class Player extends Login {
-
   public static final String TYPE_NAME = "player";
+
   private Ladder1v1Rating ladder1v1Rating;
   private GlobalRating globalRating;
-  private ClanMembership clanMembership;
   private Set<NameRecord> names;
   private Set<AvatarAssignment> avatarAssignments;
-  private Set<ModerationReport> reporterOnModerationReports;
-  private Set<ModerationReport> reportedOnModerationReports;
 
   @OneToOne(mappedBy = "player", fetch = FetchType.LAZY)
   @BatchSize(size = 1000)
@@ -51,17 +42,6 @@ public GlobalRating getGlobalRating() {
     return globalRating;
   }
 
-  // Permission is managed by ClanMembership class
-  @UpdatePermission(expression = Prefab.ALL)
-  @OneToOne(mappedBy = "player")
-  public ClanMembership getClanMembership() {
-    return this.clanMembership;
-  }
-
-  @Transient
-  public Clan getClan() {
-    return clanMembership == null ? null : clanMembership.getClan();
-  }
 
   // Permission is managed by NameRecord class
   @UpdatePermission(expression = Prefab.ALL)
@@ -78,23 +58,6 @@ public Set<AvatarAssignment> getAvatarAssignments() {
     return avatarAssignments;
   }
 
-  @ReadPermission(expression = AdminModerationReportCheck.EXPRESSION + " OR " + IsEntityOwner.EXPRESSION)
-  // Permission is managed by Moderation reports class
-  @UpdatePermission(expression = Prefab.ALL)
-  @OneToMany(mappedBy = "reporter")
-  @BatchSize(size = 1000)
-  public Set<ModerationReport> getReporterOnModerationReports() {
-    return reporterOnModerationReports;
-  }
-
-  // Permission is managed by Moderation reports class
-  @ReadPermission(expression = AdminModerationReportCheck.EXPRESSION)
-  @UpdatePermission(expression = Prefab.ALL)
-  @ManyToMany(mappedBy = "reportedUsers")
-  public Set<ModerationReport> getReportedOnModerationReports() {
-    return reportedOnModerationReports;
-  }
-
   @Override
   public String toString() {
     return "Player(" + getId() + ", " + getLogin() + ")";