Limit access to API endpoints that require logged in users to CRUD on…

…ly their information (#90) Limit access to API endpoints when trying to do allowed actions but not for your user, but others. If the id of the user sent as the Path Parameter does not match the one in the authentication token, then a 403 Forbidden is returned. It does not make sense to return a 404 Not Found because the user knows the resource exists
MatiasComercio · Feb 3, 2017 · 3aa15b5 · 3aa15b5
1 parent 756d525
commit 3aa15b5
Show file tree

Hide file tree

Showing 7 changed files with 92 additions and 19 deletions.
diff --git a/services/src/main/java/ar/edu/itba/paw/services/AdminServiceImpl.java b/services/src/main/java/ar/edu/itba/paw/services/AdminServiceImpl.java
@@ -47,7 +47,9 @@ public void update(final Admin admin) {
         admin.setId_seq(oldAdmin.getId_seq());
         admin.setPassword(oldAdmin.getPassword());
         admin.setEmail(oldAdmin.getEmail());
-        admin.getAddress().setId_seq(oldAdmin.getId_seq());
+        if(oldAdmin.getAddress() != null) {
+            admin.getAddress().setId_seq(oldAdmin.getAddress().getId_seq());
+        }
         admin.getAddress().setDni(admin.getDni());
         admin.setRole(oldAdmin.getRole());
 

diff --git a/webapp/src/main/java/ar/edu/itba/paw/webapp/auth/LoggedUser.java b/webapp/src/main/java/ar/edu/itba/paw/webapp/auth/LoggedUser.java
@@ -0,0 +1,17 @@
+package ar.edu.itba.paw.webapp.auth;
+
+import org.springframework.security.core.GrantedAuthority;
+import org.springframework.security.core.context.SecurityContextHolder;
+
+import java.util.Collection;
+
+public class LoggedUser {
+
+	public static int getDni() {
+		return Integer.valueOf(SecurityContextHolder.getContext().getAuthentication().getPrincipal().toString());
+	}
+
+	public static Collection<? extends GrantedAuthority> getAuthorities() {
+		return SecurityContextHolder.getContext().getAuthentication().getAuthorities();
+	}
+}
diff --git a/webapp/src/main/java/ar/edu/itba/paw/webapp/controllers/AdminController.java b/webapp/src/main/java/ar/edu/itba/paw/webapp/controllers/AdminController.java
@@ -3,6 +3,7 @@
 import ar.edu.itba.paw.interfaces.AdminService;
 import ar.edu.itba.paw.models.users.Admin;
 import ar.edu.itba.paw.shared.AdminFilter;
+import ar.edu.itba.paw.webapp.auth.LoggedUser;
 import ar.edu.itba.paw.webapp.models.*;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
@@ -99,8 +100,14 @@ public Response adminsUpdate(@PathParam("dni") final int dni,
     if(oldAdmin == null) {
       return status(Status.NOT_FOUND).build();
     }
+
+    final int loggedDni = LoggedUser.getDni();
+    if(loggedDni != dni) {
+      return status(Status.FORBIDDEN).build();
+    }
+
     final Admin partialAdmin = mapper.convertToAdmin(adminsUpdateDTO);
-    partialAdmin.setDni(dni);
+    partialAdmin.setDni(loggedDni);
     as.update(partialAdmin);
 
     return noContent().build();

diff --git a/webapp/src/main/java/ar/edu/itba/paw/webapp/controllers/SessionController.java b/webapp/src/main/java/ar/edu/itba/paw/webapp/controllers/SessionController.java
@@ -6,12 +6,12 @@
 import ar.edu.itba.paw.models.Role;
 import ar.edu.itba.paw.models.users.Admin;
 import ar.edu.itba.paw.models.users.Student;
+import ar.edu.itba.paw.webapp.auth.LoggedUser;
 import ar.edu.itba.paw.webapp.models.StudentSessionDTO;
 import ar.edu.itba.paw.webapp.models.UserSessionDTO;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 import org.springframework.beans.factory.annotation.Autowired;
-import org.springframework.security.core.context.SecurityContextHolder;
 import org.springframework.stereotype.Component;
 
 import javax.ws.rs.GET;
@@ -41,7 +41,7 @@ public class SessionController {
 	@GET
 	@Produces(MediaType.APPLICATION_JSON)
 	public Response sessionShow() {
-		final int dni = Integer.valueOf(SecurityContextHolder.getContext().getAuthentication().getPrincipal().toString());
+		final int dni = LoggedUser.getDni();
 		final List<Role> roles = us.getRole(dni);
 
 		if(roles.contains(Role.ADMIN)) {

diff --git a/webapp/src/main/java/ar/edu/itba/paw/webapp/controllers/StudentController.java b/webapp/src/main/java/ar/edu/itba/paw/webapp/controllers/StudentController.java
@@ -10,6 +10,7 @@
 import ar.edu.itba.paw.models.users.Student;
 import ar.edu.itba.paw.shared.CourseFilter;
 import ar.edu.itba.paw.shared.StudentFilter;
+import ar.edu.itba.paw.webapp.auth.LoggedUser;
 import ar.edu.itba.paw.webapp.models.*;
 import org.slf4j.LoggerFactory;
 import org.springframework.beans.factory.annotation.Autowired;
@@ -98,6 +99,9 @@ public Response studentsShow(@PathParam("docket") final int docket) {
     if(student == null) {
       return status(Status.NOT_FOUND).build();
     }
+
+
+
     final StudentShowDTO studentShowDTO = mapper.convertToStudentShowDTO(student);
     return ok(studentShowDTO).build();
   }
@@ -113,6 +117,12 @@ public Response studentsUpdate(@PathParam("docket") final int docket,
     if(oldStudent == null) {
       return status(Status.NOT_FOUND).build();
     }
+
+	  final int dni = LoggedUser.getDni();
+	  if(dni != oldStudent.getDni()) {
+		  return status(Status.FORBIDDEN).build();
+	  }
+
     final Student partialStudent = mapper.convertToStudent(studentUpdateDTO);
     partialStudent.setDocket(docket);
     ss.update(partialStudent);
@@ -123,7 +133,7 @@ public Response studentsUpdate(@PathParam("docket") final int docket,
   @DELETE
   @Path("/{docket}")
   public Response studentsDestroy(@PathParam("docket") final int docket) {
-    ss.deleteStudent(docket);
+	  ss.deleteStudent(docket);
     return noContent().build();
   }
 
@@ -161,14 +171,16 @@ public Response studentsCoursesIndex(
   public Response studentsCoursesNew(
           @PathParam("docket") final Integer docket,
           @Valid final InscriptionDTO inscriptionDTO){
-
-    //TODO securityContext.getAuthentication().getAuthorities() ADD_INSCRIPTIONS see if we can put it in WebAuthConfig as an antmatcher
-
     final Student student = ss.getByDocket(docket);
     if(student == null){
       return status(Status.NOT_FOUND).build();
     }
 
+    final int dni = LoggedUser.getDni();
+    if(dni != student.getDni()) {
+    	return status(Status.FORBIDDEN).build();
+    }
+
     final Course course = cs.getByCourseID(inscriptionDTO.getCourseId());
     if(course == null){
       return status(Status.BAD_REQUEST).build();
@@ -190,12 +202,14 @@ public Response studentsCoursesDestroy(
           @PathParam("docket") final Integer docket,
           @Pattern(regexp="\\d{2}\\.\\d{2}")
           @PathParam("courseId") final String courseId){
-
-    //TODO securityContext.getAuthentication().getAuthorities() DELETE_INSCRIPTIONS
-
-    final Student student = ss.getByDocket(docket);
-    if(student == null){
-      return status(Status.NOT_FOUND).build();
+	  final Student student = ss.getByDocket(docket);
+	  if(student == null){
+		  return status(Status.NOT_FOUND).build();
+	  }
+
+    final int dni = LoggedUser.getDni();
+    if(dni != student.getDni()) {
+    	return status(Status.FORBIDDEN).build();
     }
 
     final Course course = cs.getByCourseID(courseId);
@@ -239,12 +253,16 @@ public Response studentsCoursesAvailable(
   @Produces(MediaType.APPLICATION_JSON)
   @Path("/{docket}/grades")
   public Response studentsGradesIndex(@PathParam("docket") final Integer docket){
-
     final Student student = ss.getByDocket(docket);
     if(student == null){
       return status(Status.NOT_FOUND).build();
     }
 
+    final int dni = LoggedUser.getDni();
+    if(dni != student.getDni()) {
+      return status(Status.FORBIDDEN).build();
+    }
+
     final Collection<Collection<TranscriptGrade>> transcript = ss.getTranscript(docket);
 
     final List<List<TranscriptGradeDTO>> transcriptDTO = transcript.stream()
@@ -299,6 +317,11 @@ public Response studentsGradesUpdate(
       return status(Status.NOT_FOUND).build();
     }
 
+	  final int dni = LoggedUser.getDni();
+	  if(dni != student.getDni()) {
+		  return status(Status.FORBIDDEN).build();
+	  }
+
     final Course course = cs.getByCourseID(gradeDTO.getCourseId());
     if(course == null){
       return status(Status.BAD_REQUEST).build();
@@ -341,6 +364,11 @@ public Response studentsFinalInscriptionsNew(
       return status(Status.NOT_FOUND).build();
     }
 
+    final int dni = LoggedUser.getDni();
+    if(dni != student.getDni()) {
+      return status(Status.FORBIDDEN).build();
+    }
+
     final FinalInscription finalInscription = cs.getFinalInscription(inscriptionId);
     if(finalInscription == null){
       return status(Status.NOT_FOUND).build();
@@ -365,6 +393,11 @@ public Response studentsFinalInscriptionsDestroy(
       return status(Status.NOT_FOUND).build();
     }
 
+    final int dni = LoggedUser.getDni();
+    if(dni != student.getDni()) {
+      return status(Status.FORBIDDEN).build();
+    }
+
     final FinalInscription finalInscription = cs.getFinalInscription(inscriptionId);
     if(finalInscription == null){
       return status(Status.NOT_FOUND).build();
@@ -385,6 +418,11 @@ public Response studentsFinalInscriptionsAvailable(@PathParam("docket") final In
       return status(Status.NOT_FOUND).build();
     }
 
+    final int dni = LoggedUser.getDni();
+    if(dni != student.getDni()) {
+      return status(Status.FORBIDDEN).build();
+    }
+
     final List<FinalInscription> finalInscriptions = ss.getAvailableFinalInscriptions(student);
 
     final List<FinalInscriptionIndexDTO> list = finalInscriptions.stream()

diff --git a/webapp/src/main/java/ar/edu/itba/paw/webapp/controllers/UserController.java b/webapp/src/main/java/ar/edu/itba/paw/webapp/controllers/UserController.java
@@ -1,13 +1,13 @@
 package ar.edu.itba.paw.webapp.controllers;
 
 import ar.edu.itba.paw.interfaces.UserService;
+import ar.edu.itba.paw.webapp.auth.LoggedUser;
 import ar.edu.itba.paw.webapp.forms.PasswordDTO;
 import ar.edu.itba.paw.webapp.models.AuthoritiesDTO;
 import ar.edu.itba.paw.webapp.models.AuthorityDTO;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 import org.springframework.beans.factory.annotation.Autowired;
-import org.springframework.security.core.context.SecurityContextHolder;
 import org.springframework.stereotype.Component;
 
 import javax.validation.Valid;
@@ -97,7 +97,7 @@ public class UserController {
   @GET
   @Produces(MediaType.APPLICATION_JSON)
   public Response usersAuthorities(){
-    List<String> actions = SecurityContextHolder.getContext().getAuthentication().getAuthorities().stream().map(o -> o.toString()).collect(Collectors.toList());
+    List<String> actions = LoggedUser.getAuthorities().stream().map(Object::toString).collect(Collectors.toList());
     List<AuthorityDTO> authoritiesList = new LinkedList<>();
 
     for(String action: actions){
@@ -112,9 +112,15 @@ public Response usersAuthorities(){
   @Consumes(MediaType.APPLICATION_JSON)
   public Response usersPasswordChange(@PathParam("dni") final int dni,
                                       @Valid PasswordDTO passwordDTO) {
-    passwordDTO.setDni(dni);
 
-    if(!us.changePassword(dni, passwordDTO.getCurrentPassword(), passwordDTO.getNewPassword())) {
+    final int loggedDni = LoggedUser.getDni();
+    if(loggedDni != dni) {
+      return status(Status.FORBIDDEN).build();
+    }
+
+    passwordDTO.setDni(loggedDni);
+
+    if(!us.changePassword(loggedDni, passwordDTO.getCurrentPassword(), passwordDTO.getNewPassword())) {
       return status(Status.BAD_REQUEST).build();
     }
 

diff --git a/webapp/src/main/java/ar/edu/itba/paw/webapp/models/UserNewDTO.java b/webapp/src/main/java/ar/edu/itba/paw/webapp/models/UserNewDTO.java
@@ -4,6 +4,7 @@
 import org.hibernate.validator.constraints.NotBlank;
 import org.springframework.format.annotation.DateTimeFormat;
 
+import javax.validation.Valid;
 import javax.validation.constraints.Digits;
 import javax.validation.constraints.Min;
 import javax.validation.constraints.NotNull;
@@ -34,6 +35,8 @@ public class UserNewDTO {
   @Size(min=8, max=32)
   private String password;
 
+  @Valid
+  @NotNull
   private AddressDTO address;
 
   public UserNewDTO() {