From 0bcf052a3251a1b56fc4213b21d7c98134dae11a Mon Sep 17 00:00:00 2001 From: sigurdgroneng Date: Thu, 12 Jan 2023 11:14:02 +0100 Subject: [PATCH 1/9] Auth annotation with support for eksterne --- .github/workflows/deploy-feature.yml | 2 +- .../controller/v2/OppfolgingV2Controller.java | 25 ++---- .../service/AuthService.java | 12 ++- .../auth/AuthorizationAnnotationHandler.java | 88 +++++++++++++++++++ .../auth/AuthorizationConfiguration.java | 19 ++++ .../utils/auth/AuthorizationInterceptor.java | 36 ++++++++ .../utils/auth/AuthorizeAktorId.java | 14 +++ .../utils/auth/AuthorizeFnr.java | 14 +++ .../utils/auth/UnauthorizedException.java | 7 ++ .../controller/AdminControllerTest.java | 1 + .../v2/OppfolgingV2ControllerTest.java | 12 ++- 11 files changed, 207 insertions(+), 23 deletions(-) create mode 100644 src/main/java/no/nav/veilarboppfolging/utils/auth/AuthorizationAnnotationHandler.java create mode 100644 src/main/java/no/nav/veilarboppfolging/utils/auth/AuthorizationConfiguration.java create mode 100644 src/main/java/no/nav/veilarboppfolging/utils/auth/AuthorizationInterceptor.java create mode 100644 src/main/java/no/nav/veilarboppfolging/utils/auth/AuthorizeAktorId.java create mode 100644 src/main/java/no/nav/veilarboppfolging/utils/auth/AuthorizeFnr.java create mode 100644 src/main/java/no/nav/veilarboppfolging/utils/auth/UnauthorizedException.java diff --git a/.github/workflows/deploy-feature.yml b/.github/workflows/deploy-feature.yml index 3f793bda7..76a70c77b 100644 --- a/.github/workflows/deploy-feature.yml +++ b/.github/workflows/deploy-feature.yml @@ -26,7 +26,7 @@ jobs: distribution: 'temurin' cache: 'maven' - name: Build maven artifacts - run: mvn -B package + run: mvn -B package -DskipTests - uses: docker/login-action@v2 with: registry: ghcr.io diff --git a/src/main/java/no/nav/veilarboppfolging/controller/v2/OppfolgingV2Controller.java b/src/main/java/no/nav/veilarboppfolging/controller/v2/OppfolgingV2Controller.java index 23c2b4463..ae3d2d3c8 100644 --- a/src/main/java/no/nav/veilarboppfolging/controller/v2/OppfolgingV2Controller.java +++ b/src/main/java/no/nav/veilarboppfolging/controller/v2/OppfolgingV2Controller.java @@ -13,6 +13,8 @@ import no.nav.veilarboppfolging.service.AuthService; import no.nav.veilarboppfolging.service.OppfolgingService; import no.nav.veilarboppfolging.utils.DtoMappers; +import no.nav.veilarboppfolging.utils.auth.AuthorizeAktorId; +import no.nav.veilarboppfolging.utils.auth.AuthorizeFnr; import org.springframework.http.HttpStatus; import org.springframework.http.ResponseEntity; import org.springframework.web.bind.annotation.*; @@ -27,17 +29,13 @@ @RequestMapping("/api/v2/oppfolging") @RequiredArgsConstructor public class OppfolgingV2Controller { - private final static List ALLOWLIST = List.of("veilarbvedtaksstotte", "veilarbdialog", "veilarbaktivitet"); private final OppfolgingService oppfolgingService; private final AuthService authService; + @AuthorizeFnr(allowlist = {"veilarbvedtaksstotte", "veilarbdialog", "veilarbaktivitet"}) @GetMapping(params = "fnr") public UnderOppfolgingV2Response underOppfolging(@RequestParam(value = "fnr") Fnr fnr) { - boolean harTilgangSomAADSystembruker = authService.erSystemBrukerFraAzureAd(); - if (!harTilgangSomAADSystembruker) { - authService.sjekkLesetilgangMedFnr(fnr); - } return new UnderOppfolgingV2Response(oppfolgingService.erUnderOppfolging(fnr)); } @@ -52,9 +50,7 @@ public UnderOppfolgingV2Response underOppfolging() { if (!authService.erEksternBruker()) { throw new ResponseStatusException(HttpStatus.BAD_REQUEST, "Som internbruker/systembruker er aktorId eller fnr påkrevd"); } - Fnr fnr = Fnr.of(authService.getInnloggetBrukerIdent()); - return new UnderOppfolgingV2Response(oppfolgingService.erUnderOppfolging(fnr)); } @@ -62,7 +58,6 @@ public UnderOppfolgingV2Response underOppfolging() { public ResponseEntity startOppfolging(@RequestParam("fnr") Fnr fnr) { authService.skalVereInternBruker(); oppfolgingService.startOppfolging(fnr); - return ResponseEntity.status(HttpStatus.NO_CONTENT).build(); } @@ -70,7 +65,6 @@ public ResponseEntity startOppfolging(@RequestParam("fnr") Fnr fnr) { public ResponseEntity avsluttOppfolging(@RequestBody AvsluttOppfolgingV2Request request) { authService.skalVereInternBruker(); oppfolgingService.avsluttOppfolging(request.getFnr(), request.getVeilederId().get(), request.getBegrunnelse()); - return ResponseEntity.status(HttpStatus.NO_CONTENT).build(); } @@ -95,14 +89,9 @@ public OppfolgingPeriodeMinimalDTO hentOppfolgingsPeriode(@PathVariable("uuid") return tilOppfolgingPeriodeMinimalDTO(periode); } + @AuthorizeFnr(allowlist = {"veilarbvedtaksstotte", "veilarbdialog", "veilarbaktivitet"}) @GetMapping("/periode/gjeldende") public ResponseEntity hentGjeldendePeriode(@RequestParam("fnr") Fnr fnr) { - if (authService.erSystemBrukerFraAzureAd()) { - authService.sjekkAtApplikasjonErIAllowList(ALLOWLIST); - } else { - authService.sjekkLesetilgangMedFnr(fnr); - } - return oppfolgingService.hentGjeldendeOppfolgingsperiode(fnr) .map(DtoMappers::tilOppfolgingPeriodeMinimalDTO) .map(op -> new ResponseEntity<>(op, HttpStatus.OK)) @@ -115,13 +104,9 @@ public List hentOppfolgingsperioder(@RequestParam("fnr") F return hentOppfolgingsperioder(aktorId); } + @AuthorizeAktorId(allowlist = {"veilarbvedtaksstotte", "veilarbdialog", "veilarbaktivitet"}) @GetMapping(value = "/perioder", params = "aktorId") public List hentOppfolgingsperioder(@RequestParam("aktorId") AktorId aktorId) { - if (authService.erSystemBrukerFraAzureAd()) { - authService.sjekkAtApplikasjonErIAllowList(ALLOWLIST); - } else { - authService.sjekkLesetilgangMedAktorId(aktorId); - } return oppfolgingService.hentOppfolgingsperioder(aktorId) .stream() .map(this::filtrerKvpPerioder) diff --git a/src/main/java/no/nav/veilarboppfolging/service/AuthService.java b/src/main/java/no/nav/veilarboppfolging/service/AuthService.java index 1316ff047..aa9585062 100644 --- a/src/main/java/no/nav/veilarboppfolging/service/AuthService.java +++ b/src/main/java/no/nav/veilarboppfolging/service/AuthService.java @@ -151,7 +151,13 @@ public boolean harVeilederSkriveTilgangTilFnr(String veilederId, Fnr fnr) { } public void sjekkLesetilgangMedFnr(Fnr fnr) { - sjekkLesetilgangMedAktorId(getAktorIdOrThrow(fnr)); + if (authContextHolder.erEksternBruker()) { + if (!harEksternBrukerTilgang(fnr)) { + throw new ResponseStatusException(HttpStatus.FORBIDDEN, "Ekstern bruker har ikke tilgang på andre brukere enn seg selv"); + } + } else { + sjekkLesetilgangMedAktorId(getAktorIdOrThrow(fnr)); + } } public void sjekkLesetilgangMedAktorId(AktorId aktorId) { @@ -306,6 +312,10 @@ private Optional getNavIdentClaimHvisTilgjengelig() { return empty(); } + + public void sjekkAtApplikasjonErIAllowList(String[] allowlist) { + sjekkAtApplikasjonErIAllowList(List.of(allowlist)); + } @SneakyThrows public void sjekkAtApplikasjonErIAllowList(List allowlist) { String appname = hentApplikasjonFraContext(); diff --git a/src/main/java/no/nav/veilarboppfolging/utils/auth/AuthorizationAnnotationHandler.java b/src/main/java/no/nav/veilarboppfolging/utils/auth/AuthorizationAnnotationHandler.java new file mode 100644 index 000000000..023c48344 --- /dev/null +++ b/src/main/java/no/nav/veilarboppfolging/utils/auth/AuthorizationAnnotationHandler.java @@ -0,0 +1,88 @@ +package no.nav.veilarboppfolging.utils.auth; + +import lombok.RequiredArgsConstructor; +import no.nav.common.types.identer.AktorId; +import no.nav.common.types.identer.Fnr; +import no.nav.veilarboppfolging.service.AuthService; +import org.springframework.http.HttpStatus; +import org.springframework.web.server.ResponseStatusException; + +import javax.servlet.http.HttpServletRequest; +import java.lang.annotation.Annotation; +import java.lang.reflect.Method; +import java.util.Arrays; +import java.util.List; +import java.util.Optional; + +@RequiredArgsConstructor +public class AuthorizationAnnotationHandler { + + private final AuthService authService; + + private static final List> SUPPORTED_ANNOTATIONS = List.of( + AuthorizeFnr.class, + AuthorizeAktorId.class + ); + + private void authorizeRequest(Annotation annotation, HttpServletRequest request) { + var idToken = authService.getInnloggetBrukerToken(); + if (idToken == null || idToken.isEmpty()) { + throw new UnauthorizedException("Missing token"); + } + if (annotation instanceof AuthorizeFnr) { + var fnr = Fnr.of(getFnr(request)); + var allowlist = ((AuthorizeFnr) annotation).allowlist(); + handleAuthorizeFnr(fnr, allowlist); + } else if (annotation instanceof AuthorizeAktorId) { + var allowlist = ((AuthorizeFnr) annotation).allowlist(); + var aktorId = AktorId.of(getAktorId(request)); + handleAuthorizeAktorId(aktorId, allowlist); + } + } + + private void handleAuthorizeFnr(Fnr fnr, String[] allowlist) { + authService.sjekkAtApplikasjonErIAllowList(allowlist); + if (authService.erEksternBruker()) { + authService.harEksternBrukerTilgang(fnr); + } else if (authService.erInternBruker()) { + authService.sjekkLesetilgangMedFnr(fnr); + } + } + + private void handleAuthorizeAktorId(AktorId aktorId, String[] allowlist) { + authService.sjekkAtApplikasjonErIAllowList(allowlist); + if (authService.erInternBruker()) { + authService.sjekkLesetilgangMedAktorId(aktorId); + } else if (authService.erEksternBruker()) { + throw new ResponseStatusException(HttpStatus.BAD_REQUEST, "Eksternbruker ikke tillatt"); + } + } + + public void doAuthorizationCheckIfTagged(Method handlerMethod, HttpServletRequest request) { + Optional.ofNullable(getAnnotation(handlerMethod, SUPPORTED_ANNOTATIONS)) + // Skip if not tagged + .ifPresent((Annotation annotation) -> { + authorizeRequest(annotation, request); + }); + } + + protected Annotation getAnnotation(Method method, List> types) { + return Optional.ofNullable(findAnnotation(types, method.getAnnotations())) + .orElseGet(() -> findAnnotation(types, method.getDeclaringClass().getAnnotations())); + } + + private static Annotation findAnnotation(List> types, Annotation... annotations) { + return Arrays.stream(annotations) + .filter(a -> types.contains(a.annotationType())) + .findFirst() + .orElse(null); + } + private String getFnr(HttpServletRequest request) { + /* Get fnr from headers instead of query when supported by clients */ + return request.getParameter("fnr"); + } + private String getAktorId(HttpServletRequest request) { + return request.getParameter("aktorId"); + } + +} diff --git a/src/main/java/no/nav/veilarboppfolging/utils/auth/AuthorizationConfiguration.java b/src/main/java/no/nav/veilarboppfolging/utils/auth/AuthorizationConfiguration.java new file mode 100644 index 000000000..5f1e7aef3 --- /dev/null +++ b/src/main/java/no/nav/veilarboppfolging/utils/auth/AuthorizationConfiguration.java @@ -0,0 +1,19 @@ +package no.nav.veilarboppfolging.utils.auth; + +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.context.annotation.Configuration; +import org.springframework.web.servlet.config.annotation.InterceptorRegistry; +import org.springframework.web.servlet.config.annotation.WebMvcConfigurer; + +@Configuration +public class AuthorizationConfiguration implements WebMvcConfigurer { + + @Autowired + private AuthorizationInterceptor authorizationInterceptor; + + @Override + public void addInterceptors(InterceptorRegistry registry) { + registry.addInterceptor(authorizationInterceptor); + } + +} diff --git a/src/main/java/no/nav/veilarboppfolging/utils/auth/AuthorizationInterceptor.java b/src/main/java/no/nav/veilarboppfolging/utils/auth/AuthorizationInterceptor.java new file mode 100644 index 000000000..ee2adc739 --- /dev/null +++ b/src/main/java/no/nav/veilarboppfolging/utils/auth/AuthorizationInterceptor.java @@ -0,0 +1,36 @@ +package no.nav.veilarboppfolging.utils.auth; + +import lombok.extern.slf4j.Slf4j; +import no.nav.veilarboppfolging.service.AuthService; +import org.springframework.http.HttpStatus; +import org.springframework.stereotype.Component; +import org.springframework.web.method.HandlerMethod; +import org.springframework.web.server.ResponseStatusException; +import org.springframework.web.servlet.HandlerInterceptor; + +import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.HttpServletResponse; + +@Slf4j +@Component +public class AuthorizationInterceptor implements HandlerInterceptor { + private final AuthorizationAnnotationHandler annotationHandler; + + public AuthorizationInterceptor(AuthService authService) { + this.annotationHandler = new AuthorizationAnnotationHandler(authService); + } + + @Override + public boolean preHandle(HttpServletRequest request, HttpServletResponse response, Object handler) throws Exception { + if (handler instanceof HandlerMethod) { + try { + annotationHandler.doAuthorizationCheckIfTagged(((HandlerMethod) handler).getMethod(), request); + } catch (Exception e) { + log.error("Failed to process annotation", e); + throw new ResponseStatusException(HttpStatus.INTERNAL_SERVER_ERROR); + } + } + return true; + } + +} diff --git a/src/main/java/no/nav/veilarboppfolging/utils/auth/AuthorizeAktorId.java b/src/main/java/no/nav/veilarboppfolging/utils/auth/AuthorizeAktorId.java new file mode 100644 index 000000000..b19ac7e0d --- /dev/null +++ b/src/main/java/no/nav/veilarboppfolging/utils/auth/AuthorizeAktorId.java @@ -0,0 +1,14 @@ +package no.nav.veilarboppfolging.utils.auth; + +import java.lang.annotation.Retention; +import java.lang.annotation.Target; + +import static java.lang.annotation.ElementType.METHOD; +import static java.lang.annotation.ElementType.TYPE; +import static java.lang.annotation.RetentionPolicy.RUNTIME; + +@Retention(RUNTIME) +@Target({ METHOD, TYPE }) +public @interface AuthorizeAktorId { + String[] allowlist() default {}; +} diff --git a/src/main/java/no/nav/veilarboppfolging/utils/auth/AuthorizeFnr.java b/src/main/java/no/nav/veilarboppfolging/utils/auth/AuthorizeFnr.java new file mode 100644 index 000000000..409cdb5a2 --- /dev/null +++ b/src/main/java/no/nav/veilarboppfolging/utils/auth/AuthorizeFnr.java @@ -0,0 +1,14 @@ +package no.nav.veilarboppfolging.utils.auth; + +import java.lang.annotation.Retention; +import java.lang.annotation.Target; + +import static java.lang.annotation.ElementType.METHOD; +import static java.lang.annotation.ElementType.TYPE; +import static java.lang.annotation.RetentionPolicy.RUNTIME; + +@Retention(RUNTIME) +@Target({ METHOD, TYPE }) +public @interface AuthorizeFnr { + String[] allowlist() default {}; +} diff --git a/src/main/java/no/nav/veilarboppfolging/utils/auth/UnauthorizedException.java b/src/main/java/no/nav/veilarboppfolging/utils/auth/UnauthorizedException.java new file mode 100644 index 000000000..d8559f990 --- /dev/null +++ b/src/main/java/no/nav/veilarboppfolging/utils/auth/UnauthorizedException.java @@ -0,0 +1,7 @@ +package no.nav.veilarboppfolging.utils.auth; + +public class UnauthorizedException extends RuntimeException { + UnauthorizedException(String message) { + super(message); + } +} diff --git a/src/test/java/no/nav/veilarboppfolging/controller/AdminControllerTest.java b/src/test/java/no/nav/veilarboppfolging/controller/AdminControllerTest.java index f30357081..e4aed0832 100644 --- a/src/test/java/no/nav/veilarboppfolging/controller/AdminControllerTest.java +++ b/src/test/java/no/nav/veilarboppfolging/controller/AdminControllerTest.java @@ -7,6 +7,7 @@ import no.nav.veilarboppfolging.service.AuthService; import no.nav.veilarboppfolging.service.KafkaRepubliseringService; import no.nav.veilarboppfolging.service.ManuellStatusService; +import no.nav.veilarboppfolging.utils.auth.AuthorizationInterceptor; import org.junit.jupiter.api.Test; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.boot.test.autoconfigure.web.servlet.WebMvcTest; diff --git a/src/test/java/no/nav/veilarboppfolging/controller/v2/OppfolgingV2ControllerTest.java b/src/test/java/no/nav/veilarboppfolging/controller/v2/OppfolgingV2ControllerTest.java index bd39451a7..6583b0289 100644 --- a/src/test/java/no/nav/veilarboppfolging/controller/v2/OppfolgingV2ControllerTest.java +++ b/src/test/java/no/nav/veilarboppfolging/controller/v2/OppfolgingV2ControllerTest.java @@ -1,5 +1,6 @@ package no.nav.veilarboppfolging.controller.v2; +import no.nav.common.auth.oidc.filter.OidcAuthenticationFilter; import no.nav.common.json.JsonUtils; import no.nav.common.types.identer.Fnr; import no.nav.veilarboppfolging.controller.response.OppfolgingPeriodeMinimalDTO; @@ -8,11 +9,15 @@ import no.nav.veilarboppfolging.service.AuthService; import no.nav.veilarboppfolging.service.OppfolgingService; +import no.nav.veilarboppfolging.utils.auth.AuthorizationInterceptor; import org.junit.jupiter.api.Test; import org.mockito.Mockito; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.boot.test.autoconfigure.web.servlet.WebMvcTest; import org.springframework.boot.test.mock.mockito.MockBean; +import org.springframework.context.annotation.ComponentScan; +import org.springframework.context.annotation.FilterType; +import org.springframework.stereotype.Component; import org.springframework.test.web.servlet.MockMvc; import wiremock.org.eclipse.jetty.http.HttpStatus; @@ -29,7 +34,7 @@ import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.content; import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status; -@WebMvcTest(controllers = OppfolgingV2Controller.class) +@WebMvcTest(controllers = OppfolgingV2Controller.class, includeFilters = @ComponentScan.Filter(type = FilterType.ANNOTATION)) class OppfolgingV2ControllerTest { @Autowired @@ -38,11 +43,15 @@ class OppfolgingV2ControllerTest { @MockBean private AuthService authService; + @MockBean + AuthorizationInterceptor authorizationInterceptor; + @MockBean private OppfolgingService oppfolgingService; @Test void hentGjeldendeOppfolginsperiode_should_return_gjeldende() throws Exception { + when(authorizationInterceptor.preHandle(any(), any(), any())).thenReturn(true); ZonedDateTime startDato = ZonedDateTime.of(2021, 8, 27, 13, 44, 26, 356299000, ZoneId.of("Europe/Paris")); UUID uuid = UUID.fromString("e3e7f94b-d08d-464b-bdf5-e219207e915f"); OppfolgingsperiodeEntity gjeldendePeriode = OppfolgingsperiodeEntity.builder() @@ -66,6 +75,7 @@ void hentGjeldendeOppfolginsperiode_should_return_gjeldende() throws Exception { @Test void hentGJeldendeOppfolgingsPeriode_should_return_204_on_empty_result() throws Exception { + when(authorizationInterceptor.preHandle(any(), any(), any())).thenReturn(true); Fnr fnr = Fnr.of("1234"); when(oppfolgingService.hentGjeldendeOppfolgingsperiode(fnr)).thenReturn(Optional.empty()); mockMvc.perform(get("/api/v2/oppfolging/periode/gjeldende").queryParam("fnr", fnr.get())) From 965c58af0a65a9750e1ad62a9582f9fc04354c4b Mon Sep 17 00:00:00 2001 From: sigurdgroneng Date: Mon, 16 Jan 2023 12:20:27 +0100 Subject: [PATCH 2/9] Fix test --- .../no/nav/veilarboppfolging/service/AuthService.java | 2 +- .../veilarboppfolging/service/OppfolgingServiceTest.java | 8 +++++--- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/src/main/java/no/nav/veilarboppfolging/service/AuthService.java b/src/main/java/no/nav/veilarboppfolging/service/AuthService.java index aa9585062..e2edfbd04 100644 --- a/src/main/java/no/nav/veilarboppfolging/service/AuthService.java +++ b/src/main/java/no/nav/veilarboppfolging/service/AuthService.java @@ -151,7 +151,7 @@ public boolean harVeilederSkriveTilgangTilFnr(String veilederId, Fnr fnr) { } public void sjekkLesetilgangMedFnr(Fnr fnr) { - if (authContextHolder.erEksternBruker()) { + if (erEksternBruker()) { if (!harEksternBrukerTilgang(fnr)) { throw new ResponseStatusException(HttpStatus.FORBIDDEN, "Ekstern bruker har ikke tilgang på andre brukere enn seg selv"); } diff --git a/src/test/java/no/nav/veilarboppfolging/service/OppfolgingServiceTest.java b/src/test/java/no/nav/veilarboppfolging/service/OppfolgingServiceTest.java index be657ce11..6208626bd 100644 --- a/src/test/java/no/nav/veilarboppfolging/service/OppfolgingServiceTest.java +++ b/src/test/java/no/nav/veilarboppfolging/service/OppfolgingServiceTest.java @@ -2,6 +2,7 @@ import lombok.SneakyThrows; import lombok.val; +import no.nav.common.client.aktorregister.IngenGjeldendeIdentException; import no.nav.common.types.identer.AktorId; import no.nav.common.types.identer.Fnr; import no.nav.pto_schema.kafka.json.topic.SisteOppfolgingsperiodeV1; @@ -181,10 +182,11 @@ public void utenEnhetTilgang() { } @Test - public void ukjentAktor() { + public void skal_krasje_nar_aktorId_er_ukjent() { doCallRealMethod().when(authService).sjekkLesetilgangMedFnr(any()); - doThrow(new IllegalArgumentException()).when(authService).getAktorIdOrThrow(FNR); - assertThrows(IllegalArgumentException.class, this::hentOppfolgingStatus); + when(authService.erEksternBruker()).thenReturn(false); + doThrow(new IngenGjeldendeIdentException()).when(authService).getAktorIdOrThrow(FNR); + assertThrows(IngenGjeldendeIdentException.class, this::hentOppfolgingStatus); } @Test From e2f00a636481a657d96aca07046563ed5c4b6c64 Mon Sep 17 00:00:00 2001 From: sigurdgroneng Date: Mon, 16 Jan 2023 12:32:35 +0100 Subject: [PATCH 3/9] Remove unused filter --- .../controller/v2/OppfolgingV2ControllerTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/test/java/no/nav/veilarboppfolging/controller/v2/OppfolgingV2ControllerTest.java b/src/test/java/no/nav/veilarboppfolging/controller/v2/OppfolgingV2ControllerTest.java index 6583b0289..20c1b8176 100644 --- a/src/test/java/no/nav/veilarboppfolging/controller/v2/OppfolgingV2ControllerTest.java +++ b/src/test/java/no/nav/veilarboppfolging/controller/v2/OppfolgingV2ControllerTest.java @@ -34,7 +34,7 @@ import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.content; import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status; -@WebMvcTest(controllers = OppfolgingV2Controller.class, includeFilters = @ComponentScan.Filter(type = FilterType.ANNOTATION)) +@WebMvcTest(controllers = OppfolgingV2Controller.class) class OppfolgingV2ControllerTest { @Autowired From d733396292a9172069f97b5d4f505efe9fb90163 Mon Sep 17 00:00:00 2001 From: sigurdgroneng Date: Mon, 16 Jan 2023 13:15:20 +0100 Subject: [PATCH 4/9] Dont catch ResponseStatusExceptions --- .../controller/v2/OppfolgingV2Controller.java | 2 +- .../utils/auth/AuthorizationInterceptor.java | 4 ++++ 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/src/main/java/no/nav/veilarboppfolging/controller/v2/OppfolgingV2Controller.java b/src/main/java/no/nav/veilarboppfolging/controller/v2/OppfolgingV2Controller.java index ae3d2d3c8..eb741d666 100644 --- a/src/main/java/no/nav/veilarboppfolging/controller/v2/OppfolgingV2Controller.java +++ b/src/main/java/no/nav/veilarboppfolging/controller/v2/OppfolgingV2Controller.java @@ -33,7 +33,7 @@ public class OppfolgingV2Controller { private final AuthService authService; - @AuthorizeFnr(allowlist = {"veilarbvedtaksstotte", "veilarbdialog", "veilarbaktivitet"}) + @AuthorizeFnr(allowlist = {"veilarbvedtaksstotte", "veilarbdialog", "veilarbaktivitet", "veilarbregistrering"}) @GetMapping(params = "fnr") public UnderOppfolgingV2Response underOppfolging(@RequestParam(value = "fnr") Fnr fnr) { return new UnderOppfolgingV2Response(oppfolgingService.erUnderOppfolging(fnr)); diff --git a/src/main/java/no/nav/veilarboppfolging/utils/auth/AuthorizationInterceptor.java b/src/main/java/no/nav/veilarboppfolging/utils/auth/AuthorizationInterceptor.java index ee2adc739..61a072588 100644 --- a/src/main/java/no/nav/veilarboppfolging/utils/auth/AuthorizationInterceptor.java +++ b/src/main/java/no/nav/veilarboppfolging/utils/auth/AuthorizationInterceptor.java @@ -26,6 +26,10 @@ public boolean preHandle(HttpServletRequest request, HttpServletResponse respons try { annotationHandler.doAuthorizationCheckIfTagged(((HandlerMethod) handler).getMethod(), request); } catch (Exception e) { + // Catch all exception except status-exceptions + if (e instanceof ResponseStatusException) { + return true; + } log.error("Failed to process annotation", e); throw new ResponseStatusException(HttpStatus.INTERNAL_SERVER_ERROR); } From ce27dcac56a3a5330138fcb8fc67dd0c29ce7ea0 Mon Sep 17 00:00:00 2001 From: sigurdgroneng Date: Mon, 16 Jan 2023 14:13:21 +0100 Subject: [PATCH 5/9] fallback to sub on old tokens --- src/main/java/no/nav/veilarboppfolging/service/AuthService.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/no/nav/veilarboppfolging/service/AuthService.java b/src/main/java/no/nav/veilarboppfolging/service/AuthService.java index e2edfbd04..6710e3fc2 100644 --- a/src/main/java/no/nav/veilarboppfolging/service/AuthService.java +++ b/src/main/java/no/nav/veilarboppfolging/service/AuthService.java @@ -345,7 +345,7 @@ private Optional getFullAppName() { // "cluster:team:app" } else if (isTokenX(maybeClaims)) { return maybeClaims.flatMap(claims -> getStringClaimOrEmpty(claims, "client_id")); } else { - return Optional.empty(); + return authContextHolder.getSubject(); } } From 818ccd736b41ae6d01a98db44812f352a250ccb1 Mon Sep 17 00:00:00 2001 From: sigurdgroneng Date: Mon, 16 Jan 2023 15:09:36 +0100 Subject: [PATCH 6/9] Only allowlist for azure-ad clients --- .../utils/auth/AuthorizationAnnotationHandler.java | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/main/java/no/nav/veilarboppfolging/utils/auth/AuthorizationAnnotationHandler.java b/src/main/java/no/nav/veilarboppfolging/utils/auth/AuthorizationAnnotationHandler.java index 023c48344..0fa13a2dc 100644 --- a/src/main/java/no/nav/veilarboppfolging/utils/auth/AuthorizationAnnotationHandler.java +++ b/src/main/java/no/nav/veilarboppfolging/utils/auth/AuthorizationAnnotationHandler.java @@ -34,25 +34,27 @@ private void authorizeRequest(Annotation annotation, HttpServletRequest request) var allowlist = ((AuthorizeFnr) annotation).allowlist(); handleAuthorizeFnr(fnr, allowlist); } else if (annotation instanceof AuthorizeAktorId) { - var allowlist = ((AuthorizeFnr) annotation).allowlist(); + var allowlist = ((AuthorizeAktorId) annotation).allowlist(); var aktorId = AktorId.of(getAktorId(request)); handleAuthorizeAktorId(aktorId, allowlist); } } private void handleAuthorizeFnr(Fnr fnr, String[] allowlist) { - authService.sjekkAtApplikasjonErIAllowList(allowlist); if (authService.erEksternBruker()) { authService.harEksternBrukerTilgang(fnr); + } else if (authService.erSystemBrukerFraAzureAd()) { + authService.sjekkAtApplikasjonErIAllowList(allowlist); } else if (authService.erInternBruker()) { authService.sjekkLesetilgangMedFnr(fnr); } } private void handleAuthorizeAktorId(AktorId aktorId, String[] allowlist) { - authService.sjekkAtApplikasjonErIAllowList(allowlist); if (authService.erInternBruker()) { authService.sjekkLesetilgangMedAktorId(aktorId); + } else if (authService.erSystemBrukerFraAzureAd()) { + authService.sjekkAtApplikasjonErIAllowList(allowlist); } else if (authService.erEksternBruker()) { throw new ResponseStatusException(HttpStatus.BAD_REQUEST, "Eksternbruker ikke tillatt"); } From 11eaeebd896187009446879b17cddfc17572ce3e Mon Sep 17 00:00:00 2001 From: sigurdgroneng Date: Tue, 17 Jan 2023 11:04:25 +0100 Subject: [PATCH 7/9] Add audit-logging for ekstern bruker --- pom.xml | 7 +++++- .../config/ApplicationConfig.java | 7 ++++++ .../controller/KvpController.java | 24 +++++++++---------- .../controller/v2/KvpV2Controller.java | 14 ++++++----- .../service/AuthService.java | 24 +++++++++++++++++-- .../config/ApplicationTestConfig.java | 17 +++++++++++++ .../service/AuthServiceTest.java | 7 +++++- 7 files changed, 78 insertions(+), 22 deletions(-) diff --git a/pom.xml b/pom.xml index f3e8013cf..f539f92c9 100644 --- a/pom.xml +++ b/pom.xml @@ -19,7 +19,7 @@ 11 - 2.2022.11.16_08.36-35c94368bc44 + 2.2023.01.10_13.49-81ddc732df3a 1.2019.09.25-00.21-49b69f0625e0 1.16.3 @@ -188,6 +188,11 @@ auth ${common.version} + + no.nav.common + audit-log + ${common.version} + no.nav.common token-client diff --git a/src/main/java/no/nav/veilarboppfolging/config/ApplicationConfig.java b/src/main/java/no/nav/veilarboppfolging/config/ApplicationConfig.java index 3239fb611..6dfec7e77 100644 --- a/src/main/java/no/nav/veilarboppfolging/config/ApplicationConfig.java +++ b/src/main/java/no/nav/veilarboppfolging/config/ApplicationConfig.java @@ -8,6 +8,8 @@ import no.nav.common.abac.audit.AuditLogFilterUtils; import no.nav.common.abac.audit.SpringAuditRequestInfoSupplier; import no.nav.common.abac.constants.NavAttributter; +import no.nav.common.audit_log.log.AuditLogger; +import no.nav.common.audit_log.log.AuditLoggerImpl; import no.nav.common.auth.context.AuthContextHolder; import no.nav.common.auth.context.AuthContextHolderThreadLocal; import no.nav.common.cxf.StsConfig; @@ -107,4 +109,9 @@ serviceUserCredentials.password, new SpringAuditRequestInfoSupplier(), ); } + @Bean + AuditLogger auditLogger() { + return new AuditLoggerImpl(); + } + } diff --git a/src/main/java/no/nav/veilarboppfolging/controller/KvpController.java b/src/main/java/no/nav/veilarboppfolging/controller/KvpController.java index 43add8181..703f7acbc 100644 --- a/src/main/java/no/nav/veilarboppfolging/controller/KvpController.java +++ b/src/main/java/no/nav/veilarboppfolging/controller/KvpController.java @@ -1,8 +1,9 @@ package no.nav.veilarboppfolging.controller; -import io.swagger.annotations.ApiOperation; -import io.swagger.annotations.ApiResponse; -import io.swagger.annotations.ApiResponses; +import io.swagger.v3.oas.annotations.Operation; +import io.swagger.v3.oas.annotations.media.Content; +import io.swagger.v3.oas.annotations.media.Schema; +import io.swagger.v3.oas.annotations.responses.ApiResponse; import lombok.RequiredArgsConstructor; import no.nav.common.auth.context.AuthContextHolder; import no.nav.common.types.identer.AktorId; @@ -37,16 +38,15 @@ public class KvpController { @GetMapping("/{aktorId}/currentStatus") - @ApiOperation( - value = "Extract KVP status for an actor", - notes = "This API endpoint is only available for system users" + @Operation( + summary = "Extract KVP status for an actor. This API endpoint is only available for system users", + responses = { + @ApiResponse(responseCode = "200", description = "Actor is currently in a KVP period.", content = @Content(schema = @Schema(implementation = KvpDTO.class))), + @ApiResponse(responseCode = "204", description = "Actor is currently not in a KVP period."), + @ApiResponse(responseCode = "403", description = "The API endpoint is requested by a user which is not in the allowed users list."), + @ApiResponse(responseCode = "500", description = "There is a server-side bug which should be fixed.") + } ) - @ApiResponses({ - @ApiResponse(code = 200, message = "Actor is currently in a KVP period.", response = KvpDTO.class), - @ApiResponse(code = 204, message = "Actor is currently not in a KVP period."), - @ApiResponse(code = 403, message = "The API endpoint is requested by a user which is not in the allowed users list."), - @ApiResponse(code = 500, message = "There is a server-side bug which should be fixed.") - }) public ResponseEntity getKvpStatus(@PathVariable("aktorId") AktorId aktorId) { // KVP information is only available to certain system users. We trust these users here, // so that we can avoid doing an ABAC query on each request. diff --git a/src/main/java/no/nav/veilarboppfolging/controller/v2/KvpV2Controller.java b/src/main/java/no/nav/veilarboppfolging/controller/v2/KvpV2Controller.java index 1c5833fdd..6fbe1b9ac 100644 --- a/src/main/java/no/nav/veilarboppfolging/controller/v2/KvpV2Controller.java +++ b/src/main/java/no/nav/veilarboppfolging/controller/v2/KvpV2Controller.java @@ -1,7 +1,9 @@ package no.nav.veilarboppfolging.controller.v2; -import io.swagger.annotations.ApiResponse; -import io.swagger.annotations.ApiResponses; +import io.swagger.v3.oas.annotations.Operation; +import io.swagger.v3.oas.annotations.media.Content; +import io.swagger.v3.oas.annotations.media.Schema; +import io.swagger.v3.oas.annotations.responses.ApiResponse; import lombok.RequiredArgsConstructor; import no.nav.common.auth.context.AuthContextHolder; import no.nav.common.types.identer.AktorId; @@ -34,10 +36,10 @@ public class KvpV2Controller { private final AuthContextHolder authContextHolder; @GetMapping - @ApiResponses({ - @ApiResponse(code = 200, message = "Actor is currently in a KVP period.", response = KvpDTO.class), - @ApiResponse(code = 204, message = "Actor is currently not in a KVP period."), - @ApiResponse(code = 403, message = "The API endpoint is requested by a user which is not in the allowed users list.") + @Operation(responses = { + @ApiResponse(responseCode = "200", description = "Actor is currently in a KVP period.", content = @Content(schema = @Schema(implementation = KvpDTO.class))), + @ApiResponse(responseCode = "204", description = "Actor is currently not in a KVP period."), + @ApiResponse(responseCode = "403", description = "The API endpoint is requested by a user which is not in the allowed users list.") }) public ResponseEntity getKvpStatus(@RequestParam("aktorId") AktorId aktorId) { // KVP information is only available to certain system users. We trust these users here, diff --git a/src/main/java/no/nav/veilarboppfolging/service/AuthService.java b/src/main/java/no/nav/veilarboppfolging/service/AuthService.java index 6710e3fc2..a0a4f8674 100644 --- a/src/main/java/no/nav/veilarboppfolging/service/AuthService.java +++ b/src/main/java/no/nav/veilarboppfolging/service/AuthService.java @@ -12,6 +12,11 @@ import no.nav.common.abac.domain.Attribute; import no.nav.common.abac.domain.request.*; import no.nav.common.abac.domain.response.XacmlResponse; +import no.nav.common.audit_log.cef.CefMessage; +import no.nav.common.audit_log.cef.CefMessageEvent; +import no.nav.common.audit_log.cef.CefMessageSeverity; +import no.nav.common.audit_log.log.AuditLogger; +import no.nav.common.audit_log.log.AuditLoggerImpl; import no.nav.common.auth.context.AuthContextHolder; import no.nav.common.auth.context.UserRole; import no.nav.common.auth.utils.IdentUtils; @@ -48,6 +53,7 @@ public class AuthService { private final AuthContextHolder authContextHolder; + private final AuditLogger auditLogger; private final Pep veilarbPep; @@ -60,13 +66,14 @@ public class AuthService { private final EnvironmentProperties environmentProperties; @Autowired - public AuthService(AuthContextHolder authContextHolder, Pep veilarbPep, AktorOppslagClient aktorOppslagClient, Credentials serviceUserCredentials, AzureAdOnBehalfOfTokenClient aadOboTokenClient, EnvironmentProperties environmentProperties) { + public AuthService(AuthContextHolder authContextHolder, Pep veilarbPep, AktorOppslagClient aktorOppslagClient, Credentials serviceUserCredentials, AzureAdOnBehalfOfTokenClient aadOboTokenClient, EnvironmentProperties environmentProperties, AuditLogger auditLogger) { this.authContextHolder = authContextHolder; this.veilarbPep = veilarbPep; this.aktorOppslagClient = aktorOppslagClient; this.serviceUserCredentials = serviceUserCredentials; this.aadOboTokenClient = aadOboTokenClient; this.environmentProperties = environmentProperties; + this.auditLogger = auditLogger; } public void skalVereEnAv(List roller) { @@ -90,7 +97,20 @@ public void skalVereInternEllerSystemBruker() { } public boolean harEksternBrukerTilgang(Fnr fnr) { - return getInnloggetBrukerIdent().equals(fnr.get()); + // Når man ikke bruker Pep så må man gjøre auditlogging selv + var subjectUser = getInnloggetBrukerIdent(); + var isAllowed = subjectUser.equals(fnr.get()); + auditLogger.log(CefMessage.builder() + .timeEnded(System.currentTimeMillis()) + .applicationName("veilarboppfolging") + .sourceUserId(subjectUser) + .event(CefMessageEvent.ACCESS) + .severity(CefMessageSeverity.INFO) + .name("veilarboppfolging-audit-log") + .destinationUserId(fnr.get()) + .extension("msg", "Ekstern bruker har gjort oppslag på bruker") + .build()); + return isAllowed; } public void skalVereSystemBruker() { diff --git a/src/test/java/no/nav/veilarboppfolging/config/ApplicationTestConfig.java b/src/test/java/no/nav/veilarboppfolging/config/ApplicationTestConfig.java index 862b4590f..fdf6491be 100644 --- a/src/test/java/no/nav/veilarboppfolging/config/ApplicationTestConfig.java +++ b/src/test/java/no/nav/veilarboppfolging/config/ApplicationTestConfig.java @@ -6,6 +6,8 @@ import net.javacrumbs.shedlock.provider.jdbctemplate.JdbcTemplateLockProvider; import no.finn.unleash.UnleashContext; import no.nav.common.abac.Pep; +import no.nav.common.audit_log.cef.CefMessage; +import no.nav.common.audit_log.log.AuditLogger; import no.nav.common.auth.context.AuthContextHolder; import no.nav.common.auth.context.AuthContextHolderThreadLocal; import no.nav.common.featuretoggle.UnleashClient; @@ -114,5 +116,20 @@ public LeaderElectionClient leaderElectionClient() { return () -> true; } + @Bean + public AuditLogger auditLogger() { + return new AuditLogger() { + @Override + public void log(CefMessage message) { + return; + } + + @Override + public void log(String message) { + return; + } + }; + } + } diff --git a/src/test/java/no/nav/veilarboppfolging/service/AuthServiceTest.java b/src/test/java/no/nav/veilarboppfolging/service/AuthServiceTest.java index 1991e9adb..b0fb0c480 100644 --- a/src/test/java/no/nav/veilarboppfolging/service/AuthServiceTest.java +++ b/src/test/java/no/nav/veilarboppfolging/service/AuthServiceTest.java @@ -2,6 +2,8 @@ import com.nimbusds.jwt.JWTClaimsSet; import no.nav.common.abac.Pep; +import no.nav.common.audit_log.log.AuditLogger; +import no.nav.common.audit_log.log.AuditLoggerImpl; import no.nav.common.auth.context.AuthContextHolder; import no.nav.common.auth.context.UserRole; import no.nav.common.client.aktoroppslag.AktorOppslagClient; @@ -33,13 +35,16 @@ public class AuthServiceTest { private final EnvironmentProperties environmentProperties = mock(EnvironmentProperties.class); + private final AuditLogger auditLogger = mock(AuditLoggerImpl.class); + private AuthService authService = new AuthService( authContextHolder, veilarbPep, aktorOppslagClient, serviceUserCredentials, azureAdOnBehalfOfTokenClient, - environmentProperties + environmentProperties, + auditLogger ); @Test From 6f610e3812d8ff120c6fb1903da5320f89265f56 Mon Sep 17 00:00:00 2001 From: sigurdgroneng Date: Tue, 17 Jan 2023 12:20:38 +0100 Subject: [PATCH 8/9] Throw exception when not allowed --- .../auth/AuthorizationAnnotationHandler.java | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/src/main/java/no/nav/veilarboppfolging/utils/auth/AuthorizationAnnotationHandler.java b/src/main/java/no/nav/veilarboppfolging/utils/auth/AuthorizationAnnotationHandler.java index 0fa13a2dc..2edd4c4a3 100644 --- a/src/main/java/no/nav/veilarboppfolging/utils/auth/AuthorizationAnnotationHandler.java +++ b/src/main/java/no/nav/veilarboppfolging/utils/auth/AuthorizationAnnotationHandler.java @@ -32,31 +32,29 @@ private void authorizeRequest(Annotation annotation, HttpServletRequest request) if (annotation instanceof AuthorizeFnr) { var fnr = Fnr.of(getFnr(request)); var allowlist = ((AuthorizeFnr) annotation).allowlist(); - handleAuthorizeFnr(fnr, allowlist); + authorizeFnr(fnr, allowlist); } else if (annotation instanceof AuthorizeAktorId) { var allowlist = ((AuthorizeAktorId) annotation).allowlist(); var aktorId = AktorId.of(getAktorId(request)); - handleAuthorizeAktorId(aktorId, allowlist); + authorizeAktorId(aktorId, allowlist); } } - private void handleAuthorizeFnr(Fnr fnr, String[] allowlist) { - if (authService.erEksternBruker()) { - authService.harEksternBrukerTilgang(fnr); - } else if (authService.erSystemBrukerFraAzureAd()) { + private void authorizeFnr(Fnr fnr, String[] allowlist) { + if (authService.erSystemBrukerFraAzureAd()) { authService.sjekkAtApplikasjonErIAllowList(allowlist); - } else if (authService.erInternBruker()) { + } else { authService.sjekkLesetilgangMedFnr(fnr); } } - private void handleAuthorizeAktorId(AktorId aktorId, String[] allowlist) { + private void authorizeAktorId(AktorId aktorId, String[] allowlist) { if (authService.erInternBruker()) { authService.sjekkLesetilgangMedAktorId(aktorId); } else if (authService.erSystemBrukerFraAzureAd()) { authService.sjekkAtApplikasjonErIAllowList(allowlist); } else if (authService.erEksternBruker()) { - throw new ResponseStatusException(HttpStatus.BAD_REQUEST, "Eksternbruker ikke tillatt"); + throw new ResponseStatusException(HttpStatus.FORBIDDEN, "Eksternbruker ikke tillatt"); } } From 0867eb28824b2e870af328d2369d7bd23845b274 Mon Sep 17 00:00:00 2001 From: sigurdgroneng Date: Tue, 17 Jan 2023 14:27:35 +0100 Subject: [PATCH 9/9] Auditlog av resultat --- src/main/java/no/nav/veilarboppfolging/service/AuthService.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/no/nav/veilarboppfolging/service/AuthService.java b/src/main/java/no/nav/veilarboppfolging/service/AuthService.java index a0a4f8674..1352665ad 100644 --- a/src/main/java/no/nav/veilarboppfolging/service/AuthService.java +++ b/src/main/java/no/nav/veilarboppfolging/service/AuthService.java @@ -108,7 +108,7 @@ public boolean harEksternBrukerTilgang(Fnr fnr) { .severity(CefMessageSeverity.INFO) .name("veilarboppfolging-audit-log") .destinationUserId(fnr.get()) - .extension("msg", "Ekstern bruker har gjort oppslag på bruker") + .extension("msg", isAllowed ? "Ekstern bruker har gjort oppslag på seg selv" : "Ekstern bruker ble nektet innsyn") .build()); return isAllowed; }