From 3be61c4fa129ccad47ba6f3a2a2ac8474f37abe6 Mon Sep 17 00:00:00 2001 From: Armin Friedl Date: Sat, 11 Jul 2020 18:37:50 +0200 Subject: [PATCH] API Access Configuration, get Artifacts by fling id --- .../fling/controller/FlingController.java | 8 +- .../fling/model/mapper/ArtifactMapper.java | 3 + .../repositories/FlingRepository.java | 4 +- .../security/FlingWebSecurityConfigurer.java | 86 ++++++++++++------- .../fling/service/AuthenticationService.java | 2 +- .../fling/service/AuthorizationService.java | 17 ++++ .../friedl/fling/service/FlingService.java | 12 +++ .../fling/controller/FlingControllerTest.java | 43 +++++++++- .../service/AuthenticationServiceTest.java | 2 +- .../fling/service/FlingServiceTest.java | 48 ++++++++++- 10 files changed, 186 insertions(+), 39 deletions(-) diff --git a/service/fling/src/main/java/net/friedl/fling/controller/FlingController.java b/service/fling/src/main/java/net/friedl/fling/controller/FlingController.java index 43928e2..ba9a11c 100644 --- a/service/fling/src/main/java/net/friedl/fling/controller/FlingController.java +++ b/service/fling/src/main/java/net/friedl/fling/controller/FlingController.java @@ -2,6 +2,7 @@ package net.friedl.fling.controller; import java.io.IOException; import java.util.List; +import java.util.Set; import java.util.UUID; import javax.validation.Valid; import org.springframework.beans.factory.annotation.Autowired; @@ -56,12 +57,17 @@ public class FlingController { return flingService.create(flingDto); } - @PostMapping("/{id}/artifact") + @PostMapping("/{id}/artifacts") public ArtifactDto postArtifact(@PathVariable UUID id, @RequestBody @Valid ArtifactDto artifactDto) { return artifactService.create(id, artifactDto); } + @GetMapping("/{id}/artifacts") + public Set getArtifacts(@PathVariable UUID id) { + return flingService.getArtifacts(id); + } + @GetMapping(path = "/{id}") public FlingDto getFling(@PathVariable UUID id) { return flingService.getById(id); diff --git a/service/fling/src/main/java/net/friedl/fling/model/mapper/ArtifactMapper.java b/service/fling/src/main/java/net/friedl/fling/model/mapper/ArtifactMapper.java index aa917e3..fc2be15 100644 --- a/service/fling/src/main/java/net/friedl/fling/model/mapper/ArtifactMapper.java +++ b/service/fling/src/main/java/net/friedl/fling/model/mapper/ArtifactMapper.java @@ -1,6 +1,7 @@ package net.friedl.fling.model.mapper; import java.util.List; +import java.util.Set; import org.mapstruct.Mapper; import net.friedl.fling.model.dto.ArtifactDto; import net.friedl.fling.persistence.entities.ArtifactEntity; @@ -13,5 +14,7 @@ public interface ArtifactMapper { List mapEntities(List artifactEntities); + Set mapEntities(Set artifactEntities); + List mapDtos(List artifactDtos); } diff --git a/service/fling/src/main/java/net/friedl/fling/persistence/repositories/FlingRepository.java b/service/fling/src/main/java/net/friedl/fling/persistence/repositories/FlingRepository.java index 59d2248..99ce0e6 100644 --- a/service/fling/src/main/java/net/friedl/fling/persistence/repositories/FlingRepository.java +++ b/service/fling/src/main/java/net/friedl/fling/persistence/repositories/FlingRepository.java @@ -11,6 +11,6 @@ public interface FlingRepository extends JpaRepository { FlingEntity findByShareId(String shareId); - @Query("SELECT COUNT(*) FROM ArtifactEntity a, FlingEntity f where a.fling=f.id and f.id=:flingId") - Long countArtifactsById(Long flingId); + @Query("SELECT fe FROM FlingEntity fe JOIN ArtifactEntity ae ON fe.id=ae.id WHERE ae.id=:artifactId") + FlingEntity findByArtifactId(UUID artifactId); } diff --git a/service/fling/src/main/java/net/friedl/fling/security/FlingWebSecurityConfigurer.java b/service/fling/src/main/java/net/friedl/fling/security/FlingWebSecurityConfigurer.java index e3d855c..5658bf1 100644 --- a/service/fling/src/main/java/net/friedl/fling/security/FlingWebSecurityConfigurer.java +++ b/service/fling/src/main/java/net/friedl/fling/security/FlingWebSecurityConfigurer.java @@ -1,5 +1,6 @@ package net.friedl.fling.security; +import static net.friedl.fling.security.FlingAuthorities.FLING_ADMIN; import static org.springframework.security.config.Customizer.withDefaults; import java.util.List; import org.springframework.beans.factory.annotation.Autowired; @@ -10,6 +11,7 @@ import org.springframework.http.HttpMethod; import org.springframework.security.config.annotation.web.builders.HttpSecurity; import org.springframework.security.config.annotation.web.configuration.EnableWebSecurity; import org.springframework.security.config.annotation.web.configuration.WebSecurityConfigurerAdapter; +import org.springframework.security.config.http.SessionCreationPolicy; import org.springframework.security.web.authentication.UsernamePasswordAuthenticationFilter; import org.springframework.web.cors.CorsConfiguration; import org.springframework.web.cors.CorsConfigurationSource; @@ -45,57 +47,79 @@ public class FlingWebSecurityConfigurer extends WebSecurityConfigurerAdapter { @Override protected void configure(HttpSecurity http) throws Exception { //@formatter:off - http + http .csrf().disable() .cors(withDefaults()) + + /**********************************************/ + /** Authentication Interceptor Configuration **/ + /**********************************************/ .addFilterBefore(jwtAuthenticationFilter, UsernamePasswordAuthenticationFilter.class) - // Everybody can try to authenticate + // Do not keep authorization token in session. This would interfere with bearer authentication + // in that it is possible to authenticate without a bearer token if the session is kept. + // Turn off this confusing and non-obvious behavior. + .sessionManagement().sessionCreationPolicy(SessionCreationPolicy.STATELESS).and() + + + /*************************************/ + /** API Authorization Configuration **/ + /*************************************/ + //! Go from most specific to more !// + //! general, as first hit counts !// + + /**********************************/ + /** Authorization for: /api/auth **/ + /**********************************/ .authorizeRequests() .antMatchers("/api/auth/**") .permitAll() .and() - // We need to go from most specific to more general. - // Hence, first define user permissions + + + /***********************************/ + /** Authorization for: /api/fling **/ + /***********************************/ .authorizeRequests() - // TODO: This is still insecure since URLs are not encrypted - // TODO: iframe requests don't send the bearer, use cookie instead - .antMatchers(HttpMethod.GET, "/api/fling/{flingId}/download/{downloadId}") - .permitAll() + .antMatchers(HttpMethod.GET, "/api/fling/{flingId}/**") + .access("@authorizationService.allowFlingAccess(#flingId, authentication)") .and() .authorizeRequests() - .antMatchers(HttpMethod.POST, "/api/artifacts/{flingId}/**") - .access("@authorizationService.allowUpload(#flingId, authentication)") + .antMatchers(HttpMethod.GET, "/api/fling/share/{shareId}") + .access("@authorizationService.allowFlingAccessByShareId(#shareId, authentication)") .and() .authorizeRequests() - .antMatchers(HttpMethod.PATCH, "/api/artifacts/{artifactId}") - .access("@authorizationService.allowPatchingArtifact(#artifactId, authentication)") + .antMatchers(HttpMethod.POST, "/api/fling/{flingId}/artifact") + .access("@authorizationService.allowUpload(#flingId, authentication)") + .and() + // only admin can create, delete and list flings + .authorizeRequests() + .antMatchers(HttpMethod.DELETE, "/api/fling/{flingId}") + .hasAnyAuthority(FLING_ADMIN.getAuthority()) .and() .authorizeRequests() - // TODO: This is still insecure since URLs are not encrypted - // TODO: iframe requests don't send the bearer, use cookie instead - .antMatchers("/api/artifacts/{artifactId}/{downloadId}/download") - .permitAll() + .antMatchers(HttpMethod.POST, "/api/fling") + .hasAuthority(FLING_ADMIN.getAuthority()) .and() .authorizeRequests() - // TODO: Security by request parameters is just not well supported with spring security - // TODO: Change API - .regexMatchers(HttpMethod.GET, "\\/api\\/fling\\?(shareId=|flingId=)[a-zA-Z0-9]+") - .access("@authorizationService.allowFlingAccess(authentication, request)") + .antMatchers(HttpMethod.GET, "/api/fling") + .hasAuthority(FLING_ADMIN.getAuthority()) + .and() + + + /***************************************/ + /** Authorization for: /api/artifacts **/ + /***************************************/ + .authorizeRequests() + .antMatchers(HttpMethod.GET, "/api/artifacts/{artifactId}/**") + .access("@authorizationService.allowArtifactAccess(#artifactId, token)") .and() .authorizeRequests() - // TODO: Security by request parameters is just not well supported with spring security - // TODO: Change API - .regexMatchers(HttpMethod.GET, "\\/api\\/artifacts\\?(shareId=|flingId=)[a-zA-Z0-9]+") - .access("@authorizationService.allowFlingAccess(authentication, request)") + .antMatchers(HttpMethod.POST, "/api/artifacts/{artifactId}/data") + .access("@authorizationService.allowArtifactUpload(#artifactId, token)") .and() .authorizeRequests() - .antMatchers(HttpMethod.GET, "/api/fling/{flingId}/**") - .access("@authorizationService.allowFlingAccess(#flingId, authentication)") - .and() - // And lastly, the owner is allowed everything - .authorizeRequests() - .antMatchers("/api/**") - .hasAuthority(FlingAuthorities.FLING_ADMIN.getAuthority()); + .antMatchers(HttpMethod.DELETE, "/api/artifacts/{artifactId}") + .access("@authorizationService.allowArtifactUpload(#artifactId, token)"); //@formatter:on } diff --git a/service/fling/src/main/java/net/friedl/fling/service/AuthenticationService.java b/service/fling/src/main/java/net/friedl/fling/service/AuthenticationService.java index ec35c5b..afe15f1 100644 --- a/service/fling/src/main/java/net/friedl/fling/service/AuthenticationService.java +++ b/service/fling/src/main/java/net/friedl/fling/service/AuthenticationService.java @@ -91,7 +91,7 @@ public class AuthenticationService { Claims claims = jwtParser.parseClaimsJws(token).getBody(); switch (claims.getSubject()) { - case "owner": + case "admin": return new FlingToken(new FlingAdminAuthority(), token); case "user": UUID grantedFlingId = UUID.fromString(claims.get("id", String.class)); diff --git a/service/fling/src/main/java/net/friedl/fling/service/AuthorizationService.java b/service/fling/src/main/java/net/friedl/fling/service/AuthorizationService.java index 55f6afb..d1a151d 100644 --- a/service/fling/src/main/java/net/friedl/fling/service/AuthorizationService.java +++ b/service/fling/src/main/java/net/friedl/fling/service/AuthorizationService.java @@ -5,6 +5,7 @@ import org.springframework.beans.factory.annotation.Autowired; import org.springframework.security.authentication.AbstractAuthenticationToken; import org.springframework.stereotype.Service; import lombok.extern.slf4j.Slf4j; +import net.friedl.fling.persistence.entities.FlingEntity; import net.friedl.fling.persistence.repositories.FlingRepository; import net.friedl.fling.security.FlingAuthorities; import net.friedl.fling.security.authentication.FlingToken; @@ -65,4 +66,20 @@ public class AuthorizationService { log.info("User not authorized to access fling[.id={}]", flingId); return false; } + + public boolean allowFlingAccessByShareId(String shareId, AbstractAuthenticationToken token) { + FlingEntity flingEntity = flingRepository.findByShareId(shareId); + return allowFlingAccess(flingEntity.getId(), token); + } + + public boolean allowArtifactAccess(UUID artifactId, AbstractAuthenticationToken token) { + FlingEntity flingEntity = flingRepository.findByArtifactId(artifactId); + return allowFlingAccess(flingEntity.getId(), token); + } + + public boolean allowArtifactUpload(UUID artifactId, AbstractAuthenticationToken token) { + FlingEntity flingEntity = flingRepository.findByArtifactId(artifactId); + return allowUpload(flingEntity.getId(), token); + } + } diff --git a/service/fling/src/main/java/net/friedl/fling/service/FlingService.java b/service/fling/src/main/java/net/friedl/fling/service/FlingService.java index 5320f51..2bf418c 100644 --- a/service/fling/src/main/java/net/friedl/fling/service/FlingService.java +++ b/service/fling/src/main/java/net/friedl/fling/service/FlingService.java @@ -3,6 +3,7 @@ package net.friedl.fling.service; import java.io.IOException; import java.util.Base64; import java.util.List; +import java.util.Set; import java.util.UUID; import javax.transaction.Transactional; import org.springframework.beans.factory.annotation.Autowired; @@ -11,7 +12,9 @@ import org.springframework.security.crypto.password.PasswordEncoder; import org.springframework.stereotype.Service; import org.springframework.util.StringUtils; import lombok.extern.slf4j.Slf4j; +import net.friedl.fling.model.dto.ArtifactDto; import net.friedl.fling.model.dto.FlingDto; +import net.friedl.fling.model.mapper.ArtifactMapper; import net.friedl.fling.model.mapper.FlingMapper; import net.friedl.fling.persistence.entities.FlingEntity; import net.friedl.fling.persistence.repositories.FlingRepository; @@ -24,15 +27,18 @@ public class FlingService { private FlingRepository flingRepository; private FlingMapper flingMapper; + private ArtifactMapper artifactMapper; private ArchiveService archiveService; private PasswordEncoder passwordEncoder; @Autowired public FlingService(FlingRepository flingRepository, FlingMapper flingMapper, + ArtifactMapper artifactMapper, ArchiveService archiveService, PasswordEncoder passwordEcoder) { this.flingRepository = flingRepository; this.flingMapper = flingMapper; + this.artifactMapper = artifactMapper; this.archiveService = archiveService; this.passwordEncoder = passwordEcoder; } @@ -92,6 +98,12 @@ public class FlingService { log.debug("Deleted fling {}", id); } + public Set getArtifacts(UUID id) { + FlingEntity flingEntity = flingRepository.getOne(id); + Set artifactDto = artifactMapper.mapEntities(flingEntity.getArtifacts()); + return artifactDto == null ? Set.of() : artifactDto; + } + public boolean validateAuthCode(UUID id, String authCode) { FlingEntity flingEntity = flingRepository.getOne(id); if (StringUtils.hasText(flingEntity.getAuthCode()) != StringUtils.hasText(authCode)) { diff --git a/service/fling/src/test/java/net/friedl/fling/controller/FlingControllerTest.java b/service/fling/src/test/java/net/friedl/fling/controller/FlingControllerTest.java index 36edc34..1860f13 100644 --- a/service/fling/src/test/java/net/friedl/fling/controller/FlingControllerTest.java +++ b/service/fling/src/test/java/net/friedl/fling/controller/FlingControllerTest.java @@ -1,6 +1,7 @@ package net.friedl.fling.controller; import static org.hamcrest.CoreMatchers.equalTo; +import static org.hamcrest.Matchers.anyOf; import static org.hamcrest.Matchers.hasSize; import static org.hamcrest.Matchers.not; import static org.mockito.ArgumentMatchers.any; @@ -20,6 +21,7 @@ import java.io.IOException; import java.nio.file.Path; import java.time.Instant; import java.util.List; +import java.util.Set; import java.util.UUID; import javax.persistence.EntityNotFoundException; import org.hamcrest.Matchers; @@ -108,7 +110,7 @@ public class FlingControllerTest { @Test public void postArtifact_ok() throws Exception { - mockMvc.perform(post("/api/fling/{id}/artifact", flingId) + mockMvc.perform(post("/api/fling/{id}/artifacts", flingId) .content(mapper.writeValueAsString(artifactDto)) .contentType(MediaType.APPLICATION_JSON)) .andExpect(status().isOk()); @@ -118,12 +120,49 @@ public class FlingControllerTest { public void postArtifact_validatesBody_notOk() throws Exception { ArtifactDto invalidArtifactDto = new ArtifactDto(); - mockMvc.perform(post("/api/fling/{id}/artifact", flingId) + mockMvc.perform(post("/api/fling/{id}/artifacts", flingId) .content(mapper.writeValueAsString(invalidArtifactDto)) .contentType(MediaType.APPLICATION_JSON)) .andExpect(status().isBadRequest()); } + @Test + public void getArtifact_noFlingWithId_notFound() throws Exception { + doThrow(EntityNotFoundException.class).when(flingService).getArtifacts(flingId); + + mockMvc.perform(get("/api/fling/{id}/artifacts", flingId)) + .andExpect(status().isNotFound()); + } + + @Test + public void getArtifact_flingFound_noArtifacts_emptySet() throws Exception { + when(flingService.getArtifacts(flingId)).thenReturn(Set.of()); + + mockMvc.perform(get("/api/fling/{id}/artifacts", flingId)) + .andExpect(status().isOk()) + .andExpect(content().string(equalTo("[]"))); + } + + @Test + public void getArtifact_flingFound_hasArtifacts_returnArtifacts() throws Exception { + ArtifactDto artifactDto1 = ArtifactDto.builder() + .id(new UUID(0, 0)) + .build(); + + ArtifactDto artifactDto2 = ArtifactDto.builder() + .id(new UUID(0, 1)) + .build(); + + when(flingService.getArtifacts(flingId)).thenReturn(Set.of(artifactDto1, artifactDto2)); + + mockMvc.perform(get("/api/fling/{id}/artifacts", flingId)) + .andExpect(status().isOk()) + .andExpect(jsonPath("$[0].id", + anyOf(equalTo(new UUID(0, 0).toString()), equalTo(new UUID(0, 1).toString())))) + .andExpect(jsonPath("$[1].id", + anyOf(equalTo(new UUID(0, 0).toString()), equalTo(new UUID(0, 1).toString())))); + } + @Test public void getFling_noFlingWithId_notFound() throws Exception { doThrow(EntityNotFoundException.class).when(flingService).getById(flingId); diff --git a/service/fling/src/test/java/net/friedl/fling/service/AuthenticationServiceTest.java b/service/fling/src/test/java/net/friedl/fling/service/AuthenticationServiceTest.java index 646e675..14181fa 100644 --- a/service/fling/src/test/java/net/friedl/fling/service/AuthenticationServiceTest.java +++ b/service/fling/src/test/java/net/friedl/fling/service/AuthenticationServiceTest.java @@ -118,7 +118,7 @@ public class AuthenticationServiceTest { @Test public void parseAuthentication_owner_AdminAuthority() { Jws jwsClaims = new DefaultJws<>(new DefaultJwsHeader(), - new DefaultClaims(Map.of("sub", "owner")), "signature"); + new DefaultClaims(Map.of("sub", "admin")), "signature"); when(jwtParser.parseClaimsJws(any(String.class))).thenReturn(jwsClaims); FlingToken flingToken = authenticationService.parseAuthentication("any"); diff --git a/service/fling/src/test/java/net/friedl/fling/service/FlingServiceTest.java b/service/fling/src/test/java/net/friedl/fling/service/FlingServiceTest.java index 524f8a2..a765c5a 100644 --- a/service/fling/src/test/java/net/friedl/fling/service/FlingServiceTest.java +++ b/service/fling/src/test/java/net/friedl/fling/service/FlingServiceTest.java @@ -4,8 +4,10 @@ import static org.hamcrest.CoreMatchers.equalTo; import static org.hamcrest.CoreMatchers.hasItems; import static org.hamcrest.CoreMatchers.is; import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.contains; import static org.hamcrest.Matchers.empty; import static org.hamcrest.Matchers.emptyOrNullString; +import static org.hamcrest.Matchers.hasSize; import static org.hamcrest.Matchers.not; import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.verify; @@ -13,7 +15,9 @@ import static org.mockito.Mockito.when; import java.io.IOException; import java.time.Instant; import java.util.List; +import java.util.Set; import java.util.UUID; +import java.util.stream.Collectors; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; @@ -25,9 +29,13 @@ import org.springframework.boot.test.mock.mockito.MockBean; import org.springframework.context.annotation.Bean; import org.springframework.security.crypto.password.PasswordEncoder; import org.springframework.test.context.junit.jupiter.SpringExtension; +import net.friedl.fling.model.dto.ArtifactDto; import net.friedl.fling.model.dto.FlingDto; +import net.friedl.fling.model.mapper.ArtifactMapper; +import net.friedl.fling.model.mapper.ArtifactMapperImpl; import net.friedl.fling.model.mapper.FlingMapper; import net.friedl.fling.model.mapper.FlingMapperImpl; +import net.friedl.fling.persistence.entities.ArtifactEntity; import net.friedl.fling.persistence.entities.FlingEntity; import net.friedl.fling.persistence.repositories.FlingRepository; import net.friedl.fling.service.archive.ArchiveService; @@ -60,10 +68,17 @@ public class FlingServiceTest { return new FlingMapperImpl(); } + @Bean + public ArtifactMapper ArtifactMapper() { + return new ArtifactMapperImpl(); + } + @Bean public FlingService flingService(FlingRepository flingRepository, FlingMapper flingMapper, + ArtifactMapper artifactMapper, ArchiveService archiveService, PasswordEncoder passwordEncoder) { - return new FlingService(flingRepository, flingMapper, archiveService, passwordEncoder); + return new FlingService(flingRepository, flingMapper, artifactMapper, archiveService, + passwordEncoder); } } @@ -158,6 +173,37 @@ public class FlingServiceTest { verify(flingRepository).deleteById(testId); } + @Test + public void getArtifacts_noArtifacts_emptySet() throws IOException { + UUID testId = UUID.randomUUID(); + FlingEntity flingEntity = new FlingEntity(); + flingEntity.setId(testId); + flingEntity.setArtifacts(null); + + when(flingRepository.getOne(testId)).thenReturn(flingEntity); + + assertThat(flingService.getArtifacts(testId), is(empty())); + } + + @Test + public void getArtifacts_flingWithArtifacts_artifactSet() throws Exception { + UUID artifactId = UUID.randomUUID(); + ArtifactEntity artifactEntity = new ArtifactEntity(); + artifactEntity.setId(artifactId); + + UUID flingId = UUID.randomUUID(); + FlingEntity flingEntity = new FlingEntity(); + flingEntity.setId(flingId); + flingEntity.setArtifacts(Set.of(artifactEntity)); + + when(flingRepository.getOne(flingId)).thenReturn(flingEntity); + + Set artifacts = flingService.getArtifacts(flingId); + assertThat(artifacts, hasSize(1)); + assertThat(artifacts.stream().map(ArtifactDto::getId).collect(Collectors.toSet()), + contains(artifactId)); + } + @Test public void validateAuthCode_codesMatch_true() { when(flingRepository.getOne(flingEntity1.getId())).thenReturn(flingEntity1);