From 21172502fd4b29c1b8919e804f388da2b62b036c Mon Sep 17 00:00:00 2001 From: soyabeen Date: Wed, 2 Oct 2019 21:46:32 +0200 Subject: [PATCH 01/36] Add admin auth group to course and keycloak client (draft) #326 --- .../ifi/access/course/model/CourseConfig.java | 5 +++- .../ch/uzh/ifi/access/keycloak/Group.java | 24 +++++++++++++++---- .../ifi/access/keycloak/KeycloakClient.java | 2 ++ 3 files changed, 25 insertions(+), 6 deletions(-) diff --git a/src/main/java/ch/uzh/ifi/access/course/model/CourseConfig.java b/src/main/java/ch/uzh/ifi/access/course/model/CourseConfig.java index cdff4439..b334d29d 100644 --- a/src/main/java/ch/uzh/ifi/access/course/model/CourseConfig.java +++ b/src/main/java/ch/uzh/ifi/access/course/model/CourseConfig.java @@ -10,12 +10,14 @@ @Data @AllArgsConstructor public class CourseConfig { + protected String title; protected String description; protected String owner; protected LocalDateTime startDate; protected LocalDateTime endDate; + protected List admins = List.of(); protected List assistants = List.of(); protected List students = List.of(); @@ -23,4 +25,5 @@ public CourseConfig(){ this.description = ""; this.owner = ""; } -} + +} \ No newline at end of file diff --git a/src/main/java/ch/uzh/ifi/access/keycloak/Group.java b/src/main/java/ch/uzh/ifi/access/keycloak/Group.java index e20f71c4..cd371c26 100644 --- a/src/main/java/ch/uzh/ifi/access/keycloak/Group.java +++ b/src/main/java/ch/uzh/ifi/access/keycloak/Group.java @@ -10,6 +10,7 @@ public class Group { private static final String STUDENTS_GROUP_NAME = "students"; private static final String AUTHORS_GROUP_NAME = "authors"; + private static final String ADMINS_GROUP_NAME = "admins"; private GroupRepresentation course; @@ -17,13 +18,15 @@ public class Group { private GroupRepresentation authors; + private GroupRepresentation admins; + private GroupsResource resource; - public Group(GroupRepresentation course, GroupRepresentation students, GroupRepresentation authors, GroupsResource resource) { + public Group(GroupRepresentation course, GroupRepresentation students, GroupRepresentation authors, GroupRepresentation admins, GroupsResource resource) { this.course = course; this.students = students; this.authors = authors; - + this.admins = admins; this.resource = resource; } @@ -35,9 +38,9 @@ public String getStudentsGroupId() { return students.getId(); } - public String getAuthorsGroupId() { - return authors.getId(); - } + public String getAuthorsGroupId() { return authors.getId(); } + + public String getAdminsGroupId() { return admins.getId(); } public Users getStudents() { return new Users(resource.group(students.getId()).members(), List.of()); @@ -47,6 +50,10 @@ public Users getAuthors() { return new Users(resource.group(authors.getId()).members(), List.of()); } + public Users getAdmins() { + return new Users(resource.group(admins.getId()).members(), List.of()); + } + static Group create(final String courseId, String title, GroupsResource resource) { GroupRepresentation courseRepresentation = new GroupRepresentation(); courseRepresentation.setName(courseId); @@ -63,14 +70,21 @@ static Group create(final String courseId, String title, GroupsResource resource authors.setName(String.format("%s - %s", title.replace(AUTHORS_GROUP_NAME, ""), AUTHORS_GROUP_NAME)); authors.singleAttribute("Title", title); + GroupRepresentation admins = new GroupRepresentation(); + admins.setName(String.format("%s - %s", title.replace(ADMINS_GROUP_NAME, ""), ADMINS_GROUP_NAME)); + admins.singleAttribute("Title", title); + String studentsGroupId = Utils.getCreatedId(course.subGroup(students)); String authorsGroupId = Utils.getCreatedId(course.subGroup(authors)); + String adminsGroupId = Utils.getCreatedId(course.subGroup(admins)); GroupRepresentation studentsGroup = resource.group(studentsGroupId).toRepresentation(); GroupRepresentation authorsGroup = resource.group(authorsGroupId).toRepresentation(); + GroupRepresentation adminsGroup = resource.group(adminsGroupId).toRepresentation(); return new Group(course.toRepresentation(), studentsGroup, authorsGroup, + adminsGroup, resource); } diff --git a/src/main/java/ch/uzh/ifi/access/keycloak/KeycloakClient.java b/src/main/java/ch/uzh/ifi/access/keycloak/KeycloakClient.java index 4ff824ca..d8c0eba4 100644 --- a/src/main/java/ch/uzh/ifi/access/keycloak/KeycloakClient.java +++ b/src/main/java/ch/uzh/ifi/access/keycloak/KeycloakClient.java @@ -70,6 +70,7 @@ public UserRepresentation getUserById(String id) { public Group enrollUsersInCourse(Course course) { Users students = getUsersIfExistOrCreateUsers(course.getStudents()); Users assistants = getUsersIfExistOrCreateUsers(course.getAssistants()); + Users admins = getUsersIfExistOrCreateUsers(course.getAdmins()); if (course.getTitle() != null && !course.getTitle().isEmpty()) { // A group can only be initialized with a title @@ -78,6 +79,7 @@ public Group enrollUsersInCourse(Course course) { UsersResource usersResource = realmResource.users(); students.enrollUsersInGroup(courseGroup.getStudentsGroupId(), usersResource); assistants.enrollUsersInGroup(courseGroup.getAuthorsGroupId(), usersResource); + admins.enrollUsersInGroup(courseGroup.getAdminsGroupId(), usersResource); return courseGroup; } else { From 64178794e303d3163bbee122f5d77bf1b8b88767 Mon Sep 17 00:00:00 2001 From: soyabeen Date: Wed, 2 Oct 2019 21:52:15 +0200 Subject: [PATCH 02/36] Set admins in course using set() method #326 --- src/main/java/ch/uzh/ifi/access/course/model/Course.java | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/main/java/ch/uzh/ifi/access/course/model/Course.java b/src/main/java/ch/uzh/ifi/access/course/model/Course.java index d835a965..6387d05c 100644 --- a/src/main/java/ch/uzh/ifi/access/course/model/Course.java +++ b/src/main/java/ch/uzh/ifi/access/course/model/Course.java @@ -35,8 +35,8 @@ public Course(String name) { } @Builder - public Course(String title, String description, String owner, LocalDateTime startDate, LocalDateTime endDate, List assistants, List students, String id, String gitHash, String gitURL, String directory, List assignments) { - super(title, description, owner, startDate, endDate, assistants, students); + public Course(String title, String description, String owner, LocalDateTime startDate, LocalDateTime endDate, List admins, List assistants, List students, String id, String gitHash, String gitURL, String directory, List assignments) { + super(title, description, owner, startDate, endDate, admins, assistants, students); this.id = id; this.gitHash = gitHash; this.gitURL = gitURL; @@ -51,6 +51,7 @@ public void set(CourseConfig other) { this.owner = other.getOwner(); this.startDate = other.getStartDate(); this.endDate = other.getEndDate(); + this.admins = other.getAdmins(); this.assistants = other.getAssistants(); this.students = other.getStudents(); } From 751bd4f3e64ef3ebdb1b6594280feee3bc4da0d9 Mon Sep 17 00:00:00 2001 From: soyabeen Date: Wed, 2 Oct 2019 22:21:22 +0200 Subject: [PATCH 03/36] Rem performance log entries in normal logfile, console. --- src/main/resources/logback-spring.xml | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/main/resources/logback-spring.xml b/src/main/resources/logback-spring.xml index fc8c1fe5..85500d67 100644 --- a/src/main/resources/logback-spring.xml +++ b/src/main/resources/logback-spring.xml @@ -60,9 +60,7 @@ - - From a31aa22936bb401a5fb8def11df587c3db7d330d Mon Sep 17 00:00:00 2001 From: soyabeen Date: Wed, 2 Oct 2019 23:34:11 +0200 Subject: [PATCH 04/36] Add admin group to tests #326 --- .../uzh/ifi/access/keycloak/KeycloakClientTest.java | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/src/test/java/ch/uzh/ifi/access/keycloak/KeycloakClientTest.java b/src/test/java/ch/uzh/ifi/access/keycloak/KeycloakClientTest.java index 8259203d..4db6a0de 100644 --- a/src/test/java/ch/uzh/ifi/access/keycloak/KeycloakClientTest.java +++ b/src/test/java/ch/uzh/ifi/access/keycloak/KeycloakClientTest.java @@ -104,15 +104,19 @@ public void enrollUsersInCourse() { course.setTitle("Informatics 1"); course.setStudents(List.of("alice@example.com", "bob@example.com")); course.setAssistants(List.of("ta@uzh.ch", "dr.prof@uzh.ch")); + course.setAdmins(List.of("admin@uzh.ch")); Group group = client.enrollUsersInCourse(course); List studentEmails = group.getStudents().stream().map(UserRepresentation::getEmail).collect(Collectors.toList()); List assistantsEmails = group.getAuthors().stream().map(UserRepresentation::getEmail).collect(Collectors.toList()); + List adminEmails = group.getAdmins().stream().map(UserRepresentation::getEmail).collect(Collectors.toList()); Assert.assertEquals(group.getName(), course.getId()); Assert.assertEquals(Set.copyOf(studentEmails), Set.copyOf(course.getStudents())); Assert.assertEquals(Set.copyOf(assistantsEmails), Set.copyOf(course.getAssistants())); + Assert.assertEquals(Set.copyOf(adminEmails), Set.copyOf(course.getAdmins())); + } @Test @@ -123,15 +127,18 @@ public void enrollUsersAlreadyEnrolledInAnotherCourse() { course.setTitle("Informatics 1"); course.setStudents(List.of("alice@example.com", "bob@example.com")); course.setAssistants(List.of(emailAddressStudentAndTa, "dr.prof@uzh.ch")); + course.setAdmins(List.of("admin@uzh.ch")); Group info1 = client.enrollUsersInCourse(course); List studentEmails = info1.getStudents().stream().map(UserRepresentation::getEmail).collect(Collectors.toList()); List assistantsEmails = info1.getAuthors().stream().map(UserRepresentation::getEmail).collect(Collectors.toList()); + List adminEmails = info1.getAdmins().stream().map(UserRepresentation::getEmail).collect(Collectors.toList()); Assert.assertEquals(info1.getName(), course.getId()); Assert.assertEquals(Set.copyOf(studentEmails), Set.copyOf(course.getStudents())); Assert.assertEquals(Set.copyOf(assistantsEmails), Set.copyOf(course.getAssistants())); + Assert.assertEquals(Set.copyOf(adminEmails), Set.copyOf(course.getAdmins())); // Enrolling them in a second course should not remove them from the first one @@ -139,25 +146,30 @@ public void enrollUsersAlreadyEnrolledInAnotherCourse() { course2.setTitle("DBS"); course2.setStudents(List.of("alice@example.com", "bob@example.com", emailAddressStudentAndTa)); course2.setAssistants(List.of("dr.prof@uzh.ch")); + course2.setAdmins(List.of("admin@uzh.ch")); Group dbs = client.enrollUsersInCourse(course2); studentEmails = dbs.getStudents().stream().map(UserRepresentation::getEmail).collect(Collectors.toList()); assistantsEmails = dbs.getAuthors().stream().map(UserRepresentation::getEmail).collect(Collectors.toList()); + adminEmails = dbs.getAdmins().stream().map(UserRepresentation::getEmail).collect(Collectors.toList()); Assert.assertEquals(course2.getId(), dbs.getName()); Assert.assertEquals(Set.copyOf(studentEmails), Set.copyOf(course2.getStudents())); Assert.assertEquals(Set.copyOf(assistantsEmails), Set.copyOf(course2.getAssistants())); + Assert.assertEquals(Set.copyOf(adminEmails), Set.copyOf(course2.getAdmins())); // Get all students Set info1Users = info1.getStudents().stream().collect(Collectors.toSet()); Set info1Authors = info1.getAuthors().stream().collect(Collectors.toSet()); Set dbsUsers = dbs.getStudents().stream().collect(Collectors.toSet()); Set dbsAuthors = dbs.getAuthors().stream().collect(Collectors.toSet()); + Set dbsAdmins = dbs.getAdmins().stream().collect(Collectors.toSet()); Set users = new HashSet<>(info1Users); users.addAll(info1Authors); users.addAll(dbsUsers); users.addAll(dbsAuthors); + users.addAll(dbsAdmins); // Get up-to-date version of users users = users.stream().map(user -> realmResource.users().get(user.getId()).toRepresentation()).collect(Collectors.toSet()); From 822a4fb532a3d63d293b6a7abd68fb99f45fd040 Mon Sep 17 00:00:00 2001 From: soyabeen Date: Thu, 3 Oct 2019 00:12:48 +0200 Subject: [PATCH 05/36] Add admin group to tests #326 --- .../ch/uzh/ifi/access/keycloak/Group.java | 26 +++++++++---------- .../ifi/access/keycloak/KeycloakClient.java | 2 +- .../access/keycloak/KeycloakClientTest.java | 10 +++---- 3 files changed, 19 insertions(+), 19 deletions(-) diff --git a/src/main/java/ch/uzh/ifi/access/keycloak/Group.java b/src/main/java/ch/uzh/ifi/access/keycloak/Group.java index cd371c26..0e9eb04b 100644 --- a/src/main/java/ch/uzh/ifi/access/keycloak/Group.java +++ b/src/main/java/ch/uzh/ifi/access/keycloak/Group.java @@ -9,23 +9,23 @@ public class Group { private static final String STUDENTS_GROUP_NAME = "students"; - private static final String AUTHORS_GROUP_NAME = "authors"; + private static final String ASSISTANTS_GROUP_NAME = "assistants"; private static final String ADMINS_GROUP_NAME = "admins"; private GroupRepresentation course; private GroupRepresentation students; - private GroupRepresentation authors; + private GroupRepresentation assistants; private GroupRepresentation admins; private GroupsResource resource; - public Group(GroupRepresentation course, GroupRepresentation students, GroupRepresentation authors, GroupRepresentation admins, GroupsResource resource) { + public Group(GroupRepresentation course, GroupRepresentation students, GroupRepresentation assistants, GroupRepresentation admins, GroupsResource resource) { this.course = course; this.students = students; - this.authors = authors; + this.assistants = assistants; this.admins = admins; this.resource = resource; } @@ -38,7 +38,7 @@ public String getStudentsGroupId() { return students.getId(); } - public String getAuthorsGroupId() { return authors.getId(); } + public String getAssistantsGroupId() { return assistants.getId(); } public String getAdminsGroupId() { return admins.getId(); } @@ -46,8 +46,8 @@ public Users getStudents() { return new Users(resource.group(students.getId()).members(), List.of()); } - public Users getAuthors() { - return new Users(resource.group(authors.getId()).members(), List.of()); + public Users getAssistants() { + return new Users(resource.group(assistants.getId()).members(), List.of()); } public Users getAdmins() { @@ -66,24 +66,24 @@ static Group create(final String courseId, String title, GroupsResource resource students.setName(String.format("%s - %s", title.replace(STUDENTS_GROUP_NAME, ""), STUDENTS_GROUP_NAME)); students.singleAttribute("Title", title); - GroupRepresentation authors = new GroupRepresentation(); - authors.setName(String.format("%s - %s", title.replace(AUTHORS_GROUP_NAME, ""), AUTHORS_GROUP_NAME)); - authors.singleAttribute("Title", title); + GroupRepresentation assistants = new GroupRepresentation(); + assistants.setName(String.format("%s - %s", title.replace(ASSISTANTS_GROUP_NAME, ""), ASSISTANTS_GROUP_NAME)); + assistants.singleAttribute("Title", title); GroupRepresentation admins = new GroupRepresentation(); admins.setName(String.format("%s - %s", title.replace(ADMINS_GROUP_NAME, ""), ADMINS_GROUP_NAME)); admins.singleAttribute("Title", title); String studentsGroupId = Utils.getCreatedId(course.subGroup(students)); - String authorsGroupId = Utils.getCreatedId(course.subGroup(authors)); + String assistantsGroupId = Utils.getCreatedId(course.subGroup(assistants)); String adminsGroupId = Utils.getCreatedId(course.subGroup(admins)); GroupRepresentation studentsGroup = resource.group(studentsGroupId).toRepresentation(); - GroupRepresentation authorsGroup = resource.group(authorsGroupId).toRepresentation(); + GroupRepresentation assistantsGroup = resource.group(assistantsGroupId).toRepresentation(); GroupRepresentation adminsGroup = resource.group(adminsGroupId).toRepresentation(); return new Group(course.toRepresentation(), studentsGroup, - authorsGroup, + assistantsGroup, adminsGroup, resource); } diff --git a/src/main/java/ch/uzh/ifi/access/keycloak/KeycloakClient.java b/src/main/java/ch/uzh/ifi/access/keycloak/KeycloakClient.java index d8c0eba4..d79a5bff 100644 --- a/src/main/java/ch/uzh/ifi/access/keycloak/KeycloakClient.java +++ b/src/main/java/ch/uzh/ifi/access/keycloak/KeycloakClient.java @@ -78,7 +78,7 @@ public Group enrollUsersInCourse(Course course) { UsersResource usersResource = realmResource.users(); students.enrollUsersInGroup(courseGroup.getStudentsGroupId(), usersResource); - assistants.enrollUsersInGroup(courseGroup.getAuthorsGroupId(), usersResource); + assistants.enrollUsersInGroup(courseGroup.getAssistantsGroupId(), usersResource); admins.enrollUsersInGroup(courseGroup.getAdminsGroupId(), usersResource); return courseGroup; diff --git a/src/test/java/ch/uzh/ifi/access/keycloak/KeycloakClientTest.java b/src/test/java/ch/uzh/ifi/access/keycloak/KeycloakClientTest.java index 4db6a0de..014f81a9 100644 --- a/src/test/java/ch/uzh/ifi/access/keycloak/KeycloakClientTest.java +++ b/src/test/java/ch/uzh/ifi/access/keycloak/KeycloakClientTest.java @@ -109,7 +109,7 @@ public void enrollUsersInCourse() { Group group = client.enrollUsersInCourse(course); List studentEmails = group.getStudents().stream().map(UserRepresentation::getEmail).collect(Collectors.toList()); - List assistantsEmails = group.getAuthors().stream().map(UserRepresentation::getEmail).collect(Collectors.toList()); + List assistantsEmails = group.getAssistants().stream().map(UserRepresentation::getEmail).collect(Collectors.toList()); List adminEmails = group.getAdmins().stream().map(UserRepresentation::getEmail).collect(Collectors.toList()); Assert.assertEquals(group.getName(), course.getId()); @@ -132,7 +132,7 @@ public void enrollUsersAlreadyEnrolledInAnotherCourse() { Group info1 = client.enrollUsersInCourse(course); List studentEmails = info1.getStudents().stream().map(UserRepresentation::getEmail).collect(Collectors.toList()); - List assistantsEmails = info1.getAuthors().stream().map(UserRepresentation::getEmail).collect(Collectors.toList()); + List assistantsEmails = info1.getAssistants().stream().map(UserRepresentation::getEmail).collect(Collectors.toList()); List adminEmails = info1.getAdmins().stream().map(UserRepresentation::getEmail).collect(Collectors.toList()); Assert.assertEquals(info1.getName(), course.getId()); @@ -150,7 +150,7 @@ public void enrollUsersAlreadyEnrolledInAnotherCourse() { Group dbs = client.enrollUsersInCourse(course2); studentEmails = dbs.getStudents().stream().map(UserRepresentation::getEmail).collect(Collectors.toList()); - assistantsEmails = dbs.getAuthors().stream().map(UserRepresentation::getEmail).collect(Collectors.toList()); + assistantsEmails = dbs.getAssistants().stream().map(UserRepresentation::getEmail).collect(Collectors.toList()); adminEmails = dbs.getAdmins().stream().map(UserRepresentation::getEmail).collect(Collectors.toList()); Assert.assertEquals(course2.getId(), dbs.getName()); @@ -160,9 +160,9 @@ public void enrollUsersAlreadyEnrolledInAnotherCourse() { // Get all students Set info1Users = info1.getStudents().stream().collect(Collectors.toSet()); - Set info1Authors = info1.getAuthors().stream().collect(Collectors.toSet()); + Set info1Authors = info1.getAssistants().stream().collect(Collectors.toSet()); Set dbsUsers = dbs.getStudents().stream().collect(Collectors.toSet()); - Set dbsAuthors = dbs.getAuthors().stream().collect(Collectors.toSet()); + Set dbsAuthors = dbs.getAssistants().stream().collect(Collectors.toSet()); Set dbsAdmins = dbs.getAdmins().stream().collect(Collectors.toSet()); Set users = new HashSet<>(info1Users); From db0af92e8456bc20bdeeb52d338a84a6944b4ace Mon Sep 17 00:00:00 2001 From: soyabeen Date: Tue, 8 Oct 2019 10:10:16 +0200 Subject: [PATCH 06/36] Adjust evaluate functions of GrantedCourseAccess obj #326 --- .../model/security/GrantedCourseAccess.java | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/src/main/java/ch/uzh/ifi/access/course/model/security/GrantedCourseAccess.java b/src/main/java/ch/uzh/ifi/access/course/model/security/GrantedCourseAccess.java index 3790d813..19552411 100644 --- a/src/main/java/ch/uzh/ifi/access/course/model/security/GrantedCourseAccess.java +++ b/src/main/java/ch/uzh/ifi/access/course/model/security/GrantedCourseAccess.java @@ -7,26 +7,33 @@ @Value public class GrantedCourseAccess implements Serializable { - private static final GrantedCourseAccess EMPTY = new GrantedCourseAccess("", false, false); + private static final GrantedCourseAccess EMPTY = new GrantedCourseAccess("", false, false, false); private final String course; private final boolean isStudent; - private final boolean isAuthor; + private final boolean isAssistant; - public GrantedCourseAccess(String courseKey, boolean isStudent, boolean isAuthor) { + private final boolean isAdmin; + + public GrantedCourseAccess(String courseKey, boolean isStudent, boolean isAssistant, boolean isAdmin) { this.course = courseKey; this.isStudent = isStudent; - this.isAuthor = isAuthor; + this.isAssistant = isAssistant; + this.isAdmin = isAdmin; } public boolean evaluateAccess(String courseId) { return this.course.equals(courseId); } + public boolean evaluateAssistantAccess(String courseId) { + return evaluateAccess(courseId) && (isAssistant || isAdmin); + } + public boolean evaluateAdminAccess(String courseId) { - return evaluateAccess(courseId) && isAuthor; + return evaluateAccess(courseId) && isAdmin; } public static GrantedCourseAccess empty() { From a86ac4e9d344c05f63cd133e9d789f2eeb8c4cb1 Mon Sep 17 00:00:00 2001 From: soyabeen Date: Tue, 8 Oct 2019 10:16:06 +0200 Subject: [PATCH 07/36] Change admint to assistant access #326 --- src/test/java/ch/uzh/ifi/access/TestObjectFactory.java | 8 ++++++-- .../student/controller/SubmissionControllerTest.java | 10 +++++----- 2 files changed, 11 insertions(+), 7 deletions(-) diff --git a/src/test/java/ch/uzh/ifi/access/TestObjectFactory.java b/src/test/java/ch/uzh/ifi/access/TestObjectFactory.java index d00f0619..0f868431 100644 --- a/src/test/java/ch/uzh/ifi/access/TestObjectFactory.java +++ b/src/test/java/ch/uzh/ifi/access/TestObjectFactory.java @@ -146,10 +146,14 @@ public static CourseAuthentication createCourseAuthentication(Set Date: Tue, 8 Oct 2019 10:31:30 +0200 Subject: [PATCH 08/36] Adjust CoursePermissionE. to assistant and admin access #326 --- .../config/CoursePermissionEvaluatorTest.java | 6 ++-- .../util/CoursePermissionEnforcerTest.java | 34 +++++++++++++++++-- 2 files changed, 35 insertions(+), 5 deletions(-) diff --git a/src/test/java/ch/uzh/ifi/access/course/config/CoursePermissionEvaluatorTest.java b/src/test/java/ch/uzh/ifi/access/course/config/CoursePermissionEvaluatorTest.java index ba06cbc5..119676ad 100644 --- a/src/test/java/ch/uzh/ifi/access/course/config/CoursePermissionEvaluatorTest.java +++ b/src/test/java/ch/uzh/ifi/access/course/config/CoursePermissionEvaluatorTest.java @@ -18,8 +18,8 @@ public void hasAccessToCourseStudent() { String someOtherCourseName = "Info2"; Course course = new Course(""); course.setTitle(courseName); - GrantedCourseAccess info1 = new GrantedCourseAccess(course.getId(), true, false); - GrantedCourseAccess info2 = new GrantedCourseAccess(someOtherCourseName, true, false); + GrantedCourseAccess info1 = new GrantedCourseAccess(course.getId(), true, false, false); + GrantedCourseAccess info2 = new GrantedCourseAccess(someOtherCourseName, true, false, false); CourseAuthentication courseAuthentication = TestObjectFactory.createCourseAuthentication(Set.of(info1, info2)); @@ -30,7 +30,7 @@ public void hasAccessToCourseStudent() { public void hasNoAccessToCourse() { String courseName = "Info1"; String someOtherCourseName = "Info2"; - GrantedCourseAccess info1 = new GrantedCourseAccess(someOtherCourseName, true, false); + GrantedCourseAccess info1 = new GrantedCourseAccess(someOtherCourseName, true, false, false); Course course = new Course(""); course.setTitle(courseName); diff --git a/src/test/java/ch/uzh/ifi/access/course/util/CoursePermissionEnforcerTest.java b/src/test/java/ch/uzh/ifi/access/course/util/CoursePermissionEnforcerTest.java index 0229c492..2408992b 100644 --- a/src/test/java/ch/uzh/ifi/access/course/util/CoursePermissionEnforcerTest.java +++ b/src/test/java/ch/uzh/ifi/access/course/util/CoursePermissionEnforcerTest.java @@ -43,6 +43,31 @@ public void shouldAccessPublishedAssignmentNormalStudent() { Assertions.assertThat(hasAccess).hasValue(dto); } + @Test + public void shouldAccessNotPublishedAssignmentAssistant() { + Assignment assignment = notYetPublishedAssignment(); + + AssignmentMetadataDTO dto = new AssignmentMetadataDTO(assignment); + GrantedCourseAccess access = assistantAccess(); + CourseAuthentication authentication = TestObjectFactory.createCourseAuthentication(Set.of(access)); + Optional hasAccess = enforcer.shouldAccessAssignment(dto, course.getId(), authentication); + + Assertions.assertThat(hasAccess).hasValue(dto); + } + + @Test + public void shouldAccessPublishedAssignmentAssistant() { + Assignment assignment = publishedAssignment(); + + AssignmentMetadataDTO dto = new AssignmentMetadataDTO(assignment); + GrantedCourseAccess access = assistantAccess(); + CourseAuthentication authentication = TestObjectFactory.createCourseAuthentication(Set.of(access)); + Optional hasAccess = enforcer.shouldAccessAssignment(dto, course.getId(), authentication); + + Assertions.assertThat(hasAccess).isNotEmpty(); + Assertions.assertThat(hasAccess).hasValue(dto); + } + @Test public void shouldAccessNotPublishedAssignmentAdmin() { Assignment assignment = notYetPublishedAssignment(); @@ -77,10 +102,15 @@ private Assignment notYetPublishedAssignment() { } private GrantedCourseAccess adminAccess() { - return new GrantedCourseAccess(course.getId(), false, true); + return new GrantedCourseAccess(course.getId(), false, false, true); } + private GrantedCourseAccess assistantAccess() { + return new GrantedCourseAccess(course.getId(), false, true, false); + } + + private GrantedCourseAccess studentAccess() { - return new GrantedCourseAccess(course.getId(), true, false); + return new GrantedCourseAccess(course.getId(), true, false, false); } } \ No newline at end of file From f7111bada431d90aeaed769743797e60c4873d15 Mon Sep 17 00:00:00 2001 From: soyabeen Date: Tue, 8 Oct 2019 10:32:52 +0200 Subject: [PATCH 09/36] Check assistant not admin anymore in CourseServiceTest #326 --- .../ifi/access/course/service/CourseServiceTest.java | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/test/java/ch/uzh/ifi/access/course/service/CourseServiceTest.java b/src/test/java/ch/uzh/ifi/access/course/service/CourseServiceTest.java index 969553ee..b5d2a317 100644 --- a/src/test/java/ch/uzh/ifi/access/course/service/CourseServiceTest.java +++ b/src/test/java/ch/uzh/ifi/access/course/service/CourseServiceTest.java @@ -130,7 +130,7 @@ public void getFileCheckingPrivilegesStudentHasAccessToSolutionsAfterDueDate() { } @Test - public void getFileCheckingPrivilegesAdminAccess() { + public void getFileCheckingPrivilegesAssistantAccess() { Course course = TestObjectFactory.createCourseWithOneAssignmentAndOneExercise("Course", "Assignment", "exercise question"); Assignment assignment = course.getAssignments().get(0); Exercise exercise = assignment.getExercises().get(0); @@ -146,14 +146,14 @@ public void getFileCheckingPrivilegesAdminAccess() { exercise.setPrivate_files(List.of(privateFile)); exercise.setSolution_files(List.of(solutionFile)); - CourseAuthentication admin = TestObjectFactory.createCourseAuthentication(Set.of(TestObjectFactory.createAdminAccess(course.getId()))); + CourseAuthentication assistant = TestObjectFactory.createCourseAuthentication(Set.of(TestObjectFactory.createAssistantAccess(course.getId()))); CourseService courseService = new CourseService(dao, null); - Optional shouldGetVFile1 = courseService.getFileCheckingPrivileges(exercise, vFile1.getId(), admin); - Optional shouldGetVFile2 = courseService.getFileCheckingPrivileges(exercise, vFile2.getId(), admin); - Optional shouldGetPrivateFile = courseService.getFileCheckingPrivileges(exercise, privateFile.getId(), admin); - Optional shouldGetSolutionFile = courseService.getFileCheckingPrivileges(exercise, solutionFile.getId(), admin); + Optional shouldGetVFile1 = courseService.getFileCheckingPrivileges(exercise, vFile1.getId(), assistant); + Optional shouldGetVFile2 = courseService.getFileCheckingPrivileges(exercise, vFile2.getId(), assistant); + Optional shouldGetPrivateFile = courseService.getFileCheckingPrivileges(exercise, privateFile.getId(), assistant); + Optional shouldGetSolutionFile = courseService.getFileCheckingPrivileges(exercise, solutionFile.getId(), assistant); assertThat(shouldGetVFile1.isPresent()).isTrue(); assertThat(shouldGetVFile2.isPresent()).isTrue(); From c74a581000d5e4933fc44b69681318c79ca8f695 Mon Sep 17 00:00:00 2001 From: soyabeen Date: Tue, 8 Oct 2019 10:43:32 +0200 Subject: [PATCH 10/36] Parse new admin permission from token #326 --- .../config/JwtAccessTokenCustomizer.java | 5 ++-- .../config/JwtAccessTokenCustomizerTest.java | 24 +++++++++++++++---- 2 files changed, 22 insertions(+), 7 deletions(-) diff --git a/src/main/java/ch/uzh/ifi/access/config/JwtAccessTokenCustomizer.java b/src/main/java/ch/uzh/ifi/access/config/JwtAccessTokenCustomizer.java index 154f3ae6..b5cf7c08 100644 --- a/src/main/java/ch/uzh/ifi/access/config/JwtAccessTokenCustomizer.java +++ b/src/main/java/ch/uzh/ifi/access/config/JwtAccessTokenCustomizer.java @@ -160,8 +160,9 @@ GrantedCourseAccess parseCourseAccess(String courseElement) { } String courseKey = group.get(0); boolean isStudent = group.get(1).toLowerCase().contains("students"); - boolean isAuthor = group.get(1).toLowerCase().contains("authors"); + boolean isAssistant = group.get(1).toLowerCase().contains("assistants"); + boolean isAdmin = group.get(1).toLowerCase().contains("admins"); - return new GrantedCourseAccess(courseKey, isStudent, isAuthor); + return new GrantedCourseAccess(courseKey, isStudent, isAssistant, isAdmin); } } \ No newline at end of file diff --git a/src/test/java/ch/uzh/ifi/access/config/JwtAccessTokenCustomizerTest.java b/src/test/java/ch/uzh/ifi/access/config/JwtAccessTokenCustomizerTest.java index 18c6f6d4..bed4896c 100644 --- a/src/test/java/ch/uzh/ifi/access/config/JwtAccessTokenCustomizerTest.java +++ b/src/test/java/ch/uzh/ifi/access/config/JwtAccessTokenCustomizerTest.java @@ -77,7 +77,7 @@ public void extractAuthentication() throws IOException { Assert.assertEquals(authentication.getUserId(), "e095b656-f219-4e13-bc39-c6329f725cc1"); Assert.assertEquals(authentication.getName(), "carl-friedlich.abel@uzh.ch"); - Assert.assertEquals(authentication.getCourseAccesses(), Set.of(new GrantedCourseAccess("b75be786-f1c1-32d3-99fc-8af4ff155ade", true, false))); + Assert.assertEquals(authentication.getCourseAccesses(), Set.of(new GrantedCourseAccess("b75be786-f1c1-32d3-99fc-8af4ff155ade", true, false, false))); } @Test @@ -99,18 +99,32 @@ public void parseCourseAccessStudent() { Assert.assertEquals("b75be786-f1c1-32d3-99fc-8af4ff155ade", grantedCourseAccess.getCourse()); Assert.assertTrue(grantedCourseAccess.isStudent()); - Assert.assertFalse(grantedCourseAccess.isAuthor()); + Assert.assertFalse(grantedCourseAccess.isAssistant()); + Assert.assertFalse(grantedCourseAccess.isAdmin()); } @Test - public void parseCourseAccessAuthor() { + public void parseCourseAccessAssistant() { JwtAccessTokenCustomizer tokenCustomizer = new JwtAccessTokenCustomizer(null); - GrantedCourseAccess grantedCourseAccess = tokenCustomizer.parseCourseAccess("/b75be786-f1c1-32d3-99fc-8af4ff155ade/Informatics 1 - authors"); + GrantedCourseAccess grantedCourseAccess = tokenCustomizer.parseCourseAccess("/b75be786-f1c1-32d3-99fc-8af4ff155ade/Informatics 1 - assistants"); Assert.assertEquals("b75be786-f1c1-32d3-99fc-8af4ff155ade", grantedCourseAccess.getCourse()); Assert.assertFalse(grantedCourseAccess.isStudent()); - Assert.assertTrue(grantedCourseAccess.isAuthor()); + Assert.assertTrue(grantedCourseAccess.isAssistant()); + Assert.assertFalse(grantedCourseAccess.isAdmin()); + } + + @Test + public void parseCourseAccessAdmin() { + JwtAccessTokenCustomizer tokenCustomizer = new JwtAccessTokenCustomizer(null); + + GrantedCourseAccess grantedCourseAccess = tokenCustomizer.parseCourseAccess("/b75be786-f1c1-32d3-99fc-8af4ff155ade/Informatics 1 - admins"); + + Assert.assertEquals("b75be786-f1c1-32d3-99fc-8af4ff155ade", grantedCourseAccess.getCourse()); + Assert.assertFalse(grantedCourseAccess.isStudent()); + Assert.assertFalse(grantedCourseAccess.isAssistant()); + Assert.assertTrue(grantedCourseAccess.isAdmin()); } @Test(expected = IllegalArgumentException.class) From 242f0242d3622e3e258affa14968429a19cfca48 Mon Sep 17 00:00:00 2001 From: soyabeen Date: Tue, 8 Oct 2019 10:55:18 +0200 Subject: [PATCH 11/36] Check for assistant not admin in KeycloakClientTest #326 --- .../java/ch/uzh/ifi/access/keycloak/KeycloakClientTest.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/test/java/ch/uzh/ifi/access/keycloak/KeycloakClientTest.java b/src/test/java/ch/uzh/ifi/access/keycloak/KeycloakClientTest.java index 014f81a9..18d4ac58 100644 --- a/src/test/java/ch/uzh/ifi/access/keycloak/KeycloakClientTest.java +++ b/src/test/java/ch/uzh/ifi/access/keycloak/KeycloakClientTest.java @@ -179,15 +179,15 @@ public void enrollUsersAlreadyEnrolledInAnotherCourse() { Assert.assertEquals(groups.size(), 2); } - // ta-student should be both student and author depending on the course + // ta-student should be both student and assistant depending on the course UserRepresentation taStudent = users.stream().filter(u -> u.getEmail().equals(emailAddressStudentAndTa)).findFirst().orElseThrow(); List groups = realmResource.users().get(taStudent.getId()).groups(); for (GroupRepresentation group : groups) { if (group.getPath().contains(course.getId())) { Assertions .assertThat(group.getName()) - .withFailMessage(String.format("ta-student should be an 'author' of course '%s'", course.getId())) - .contains("authors"); + .withFailMessage(String.format("ta-student should be an 'assistant' of course '%s'", course.getId())) + .contains("assistants"); } else { Assertions .assertThat(group.getName()) From 77fd72ed6704e342ff6f4cd518579f088dbf93a5 Mon Sep 17 00:00:00 2001 From: soyabeen Date: Tue, 8 Oct 2019 10:55:59 +0200 Subject: [PATCH 12/36] DevSecurityConfigure uses new admin instead assistant #326 --- .../java/ch/uzh/ifi/access/config/DevSecurityConfigurer.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/ch/uzh/ifi/access/config/DevSecurityConfigurer.java b/src/main/java/ch/uzh/ifi/access/config/DevSecurityConfigurer.java index a326948d..2d742fed 100644 --- a/src/main/java/ch/uzh/ifi/access/config/DevSecurityConfigurer.java +++ b/src/main/java/ch/uzh/ifi/access/config/DevSecurityConfigurer.java @@ -64,7 +64,7 @@ public Authentication authentication(String courseId, String admin) { Authentication auth = new UsernamePasswordAuthenticationToken("testUser", "---", List.of(new SimpleGrantedAuthority("USER"))); boolean isAdmin = Boolean.parseBoolean(admin); boolean isStudent = !isAdmin; - GrantedCourseAccess access = new GrantedCourseAccess(Optional.ofNullable(courseId).orElse(""), isStudent, isAdmin); + GrantedCourseAccess access = new GrantedCourseAccess(Optional.ofNullable(courseId).orElse(""), isStudent, false, isAdmin); return new CourseAuthentication(request, auth, Set.of(access), "") { @Override public boolean hasAccess(String courseId) { From de96ae5086b4f0d80a18c19e3bd7a7215443e0a6 Mon Sep 17 00:00:00 2001 From: soyabeen Date: Tue, 8 Oct 2019 12:45:46 +0200 Subject: [PATCH 13/36] Use privileged access for assignment and solution visibility #326 --- .../java/ch/uzh/ifi/access/course/CoursePermissionsAdvice.java | 2 +- .../ch/uzh/ifi/access/course/controller/ExerciseController.java | 2 +- .../java/ch/uzh/ifi/access/course/service/CourseService.java | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/main/java/ch/uzh/ifi/access/course/CoursePermissionsAdvice.java b/src/main/java/ch/uzh/ifi/access/course/CoursePermissionsAdvice.java index 0d138291..0fa45c66 100644 --- a/src/main/java/ch/uzh/ifi/access/course/CoursePermissionsAdvice.java +++ b/src/main/java/ch/uzh/ifi/access/course/CoursePermissionsAdvice.java @@ -48,7 +48,7 @@ private void filterAssignmentsByStartDate(Collection courses, .getAssignments() .removeIf(assignment -> { // A course admin of a course has unrestricted access to said course - if (authentication.hasAdminAccess(course.getId())) { + if (authentication.hasPrivilegedAccess(course.getId())) { return false; } return !assignment.isPublished(); diff --git a/src/main/java/ch/uzh/ifi/access/course/controller/ExerciseController.java b/src/main/java/ch/uzh/ifi/access/course/controller/ExerciseController.java index 520e9b85..28cc9763 100644 --- a/src/main/java/ch/uzh/ifi/access/course/controller/ExerciseController.java +++ b/src/main/java/ch/uzh/ifi/access/course/controller/ExerciseController.java @@ -103,6 +103,6 @@ public ResponseEntity searchForFile( } private boolean hasAccessToExerciseSolutions(Exercise exercise, CourseAuthentication authentication) { - return exercise.isPastDueDate() || authentication.hasAdminAccess(exercise.getCourseId()); + return exercise.isPastDueDate() || authentication.hasPrivilegedAccess(exercise.getCourseId()); } } diff --git a/src/main/java/ch/uzh/ifi/access/course/service/CourseService.java b/src/main/java/ch/uzh/ifi/access/course/service/CourseService.java index 5e7807a5..d49cfc08 100644 --- a/src/main/java/ch/uzh/ifi/access/course/service/CourseService.java +++ b/src/main/java/ch/uzh/ifi/access/course/service/CourseService.java @@ -69,6 +69,6 @@ public Optional getExerciseMaxSubmissions(String exerciseId) { } private boolean hasAccessToExerciseSolutions(Exercise exercise, CourseAuthentication authentication) { - return exercise.isPastDueDate() || authentication.hasAdminAccess(exercise.getCourseId()); + return exercise.isPastDueDate() || authentication.hasPrivilegedAccess(exercise.getCourseId()); } } \ No newline at end of file From b9306ee79fa0f550afb2d906d3157e0b368e04e3 Mon Sep 17 00:00:00 2001 From: soyabeen Date: Wed, 9 Oct 2019 23:57:32 +0200 Subject: [PATCH 14/36] Use privileged course access #326 --- .../ch/uzh/ifi/access/course/config/CourseAuthentication.java | 4 ++++ .../ifi/access/course/config/CoursePermissionEvaluator.java | 2 +- .../uzh/ifi/access/course/util/CoursePermissionEnforcer.java | 2 +- 3 files changed, 6 insertions(+), 2 deletions(-) diff --git a/src/main/java/ch/uzh/ifi/access/course/config/CourseAuthentication.java b/src/main/java/ch/uzh/ifi/access/course/config/CourseAuthentication.java index 950f772b..8207dbc7 100644 --- a/src/main/java/ch/uzh/ifi/access/course/config/CourseAuthentication.java +++ b/src/main/java/ch/uzh/ifi/access/course/config/CourseAuthentication.java @@ -34,6 +34,10 @@ public boolean hasAccess(String courseId) { return courseAccesses.stream().anyMatch(access -> access.evaluateAccess(courseId)); } + public boolean hasPrivilegedAccess(String courseId) { + return courseAccesses.stream().anyMatch(access -> access.evaluateAssistantAccess(courseId)); + } + public boolean hasAdminAccess(String courseId) { return courseAccesses.stream().anyMatch(access -> access.evaluateAdminAccess(courseId)); } diff --git a/src/main/java/ch/uzh/ifi/access/course/config/CoursePermissionEvaluator.java b/src/main/java/ch/uzh/ifi/access/course/config/CoursePermissionEvaluator.java index b67fab9a..4ee2bb27 100644 --- a/src/main/java/ch/uzh/ifi/access/course/config/CoursePermissionEvaluator.java +++ b/src/main/java/ch/uzh/ifi/access/course/config/CoursePermissionEvaluator.java @@ -56,6 +56,6 @@ public boolean hasAdminAccessToCourse(CourseAuthentication authentication, Cours public boolean hasAccessToExercise(CourseAuthentication authentication, Exercise exercise) { Assignment assignment = exercise.getAssignment(); String courseId = exercise.getCourseId(); - return authentication.hasAccess(courseId) && (assignment.isPublished() || authentication.hasAdminAccess(courseId)); + return authentication.hasAccess(courseId) && (assignment.isPublished() || authentication.hasPrivilegedAccess(courseId)); } } \ No newline at end of file diff --git a/src/main/java/ch/uzh/ifi/access/course/util/CoursePermissionEnforcer.java b/src/main/java/ch/uzh/ifi/access/course/util/CoursePermissionEnforcer.java index faf06207..b23099e1 100644 --- a/src/main/java/ch/uzh/ifi/access/course/util/CoursePermissionEnforcer.java +++ b/src/main/java/ch/uzh/ifi/access/course/util/CoursePermissionEnforcer.java @@ -10,7 +10,7 @@ public class CoursePermissionEnforcer { public Optional shouldAccessAssignment(T assignment, String courseId, CourseAuthentication authentication) { - if (assignment.isPublished() || authentication.hasAdminAccess(courseId)) { + if (assignment.isPublished() || authentication.hasPrivilegedAccess(courseId)) { return Optional.of(assignment); } return Optional.empty(); From 359709b544a7240c7d253bb34311bab72264b6bf Mon Sep 17 00:00:00 2001 From: Alexander Hofmann Date: Wed, 23 Oct 2019 22:51:16 +0200 Subject: [PATCH 15/36] Bring dev and prod configs closer again to avoid problems --- src/main/java/ch/uzh/ifi/access/config/SecurityProperties.java | 3 +-- src/main/resources/application.properties | 2 +- src/test/resources/application-testing.properties | 2 +- 3 files changed, 3 insertions(+), 4 deletions(-) diff --git a/src/main/java/ch/uzh/ifi/access/config/SecurityProperties.java b/src/main/java/ch/uzh/ifi/access/config/SecurityProperties.java index eafce9b7..b40ed3c5 100644 --- a/src/main/java/ch/uzh/ifi/access/config/SecurityProperties.java +++ b/src/main/java/ch/uzh/ifi/access/config/SecurityProperties.java @@ -1,7 +1,6 @@ package ch.uzh.ifi.access.config; import lombok.Data; -import org.springframework.beans.factory.annotation.Value; import org.springframework.boot.context.properties.ConfigurationProperties; import org.springframework.context.annotation.Configuration; import org.springframework.stereotype.Component; @@ -33,7 +32,7 @@ public class SecurityProperties { private String keycloakApiPassword = "admin"; - private String realm = "dev"; + private String realm = "access"; private String frontendClientId= "access-frontend"; diff --git a/src/main/resources/application.properties b/src/main/resources/application.properties index 83badf24..68d8a1af 100644 --- a/src/main/resources/application.properties +++ b/src/main/resources/application.properties @@ -10,7 +10,7 @@ rest.security.cors.max-age=3600 rest.security.keycloak-api-admin=admin rest.security.keycloak-api-password=admin rest.security.auth-server=http://localhost:9999/auth -rest.security.realm=dev +rest.security.realm=access rest.security.frontend-client-id=access-frontend rest.security.redirect-uri-after-actions=http://localhost:3000 rest.security.issuer-uri=${rest.security.auth-server}/realms/${rest.security.realm} diff --git a/src/test/resources/application-testing.properties b/src/test/resources/application-testing.properties index 0029d206..ae1e9eea 100644 --- a/src/test/resources/application-testing.properties +++ b/src/test/resources/application-testing.properties @@ -8,7 +8,7 @@ rest.security.cors.allowed-methods=GET,POST,PUT,PATCH,DELETE,OPTIONS rest.security.cors.max-age=3600 # OpenID rest.security.auth-server=http://localhost:9999/auth -rest.security.issuer-uri=${rest.security.auth-server}/realms/dev +rest.security.issuer-uri=${rest.security.auth-server}/realms/access security.oauth2.resource.id=course-service security.oauth2.resource.token-info-uri=${rest.security.issuer-uri}/protocol/openid-connect/token/introspect security.oauth2.resource.user-info-uri=${rest.security.issuer-uri}/protocol/openid-connect/userinfo From ff84dc69f1f3e3058fc4d719e5323a6ca6ba3de8 Mon Sep 17 00:00:00 2001 From: Haeri Date: Thu, 31 Oct 2019 03:46:17 +0100 Subject: [PATCH 16/36] switched from fileIndex to fileId --- .../uzh/ifi/access/course/util/RepoCacher.java | 1 + .../evaluation/runner/SubmissionCodeRunner.java | 4 ++-- .../access/student/model/CodeSubmission.java | 17 +++++++++++++---- 3 files changed, 16 insertions(+), 6 deletions(-) diff --git a/src/main/java/ch/uzh/ifi/access/course/util/RepoCacher.java b/src/main/java/ch/uzh/ifi/access/course/util/RepoCacher.java index d3a25c6a..413fd6b5 100644 --- a/src/main/java/ch/uzh/ifi/access/course/util/RepoCacher.java +++ b/src/main/java/ch/uzh/ifi/access/course/util/RepoCacher.java @@ -124,6 +124,7 @@ private void cacheRepo(File file, Object context) { a.addExercise(exercise); next_context = exercise; } else if (file.getName().startsWith(PUBLIC_FOLDER_NAME)) { + // TODO investigate if it would make more sense to leave the root folder in tha path listFiles(file, ((Exercise) context).getPublic_files(), file.getPath()); } else if (file.getName().startsWith(PRIVATE_FOLDER_NAME)) { listFiles(file, ((Exercise) context).getPrivate_files(), file.getPath()); diff --git a/src/main/java/ch/uzh/ifi/access/student/evaluation/runner/SubmissionCodeRunner.java b/src/main/java/ch/uzh/ifi/access/student/evaluation/runner/SubmissionCodeRunner.java index 2c822169..e1490732 100644 --- a/src/main/java/ch/uzh/ifi/access/student/evaluation/runner/SubmissionCodeRunner.java +++ b/src/main/java/ch/uzh/ifi/access/student/evaluation/runner/SubmissionCodeRunner.java @@ -67,7 +67,7 @@ private ExecResult executeSmokeTest(Path workPath, CodeSubmission submission, Co hierarchySerializer.persistFilesIntoFolder(String.format("%s/%s", workPath.toString(), PUBLIC_FOLDER), submission.getPublicFiles()); Files.createFile(Paths.get(workPath.toAbsolutePath().toString(), INIT_FILE)); - VirtualFile selectedFileForRun = submission.getPublicFile(submission.getSelectedFile()); + VirtualFile selectedFileForRun = submission.getPublicFile(submission.getSelectedFileId()); String executeScriptCommand = buildExecScriptCommand(selectedFileForRun); String testCommand = buildExecTestSuiteCommand(PUBLIC_FOLDER); @@ -86,7 +86,7 @@ private ExecResult executeSubmission(Path workPath, CodeSubmission submission, E Files.createFile(Paths.get(workPath.toAbsolutePath().toString(), INIT_FILE)); - VirtualFile selectedFileForRun = submission.getPublicFile(submission.getSelectedFile()); + VirtualFile selectedFileForRun = submission.getPublicFile(submission.getSelectedFileId()); String executeScriptCommand = buildExecScriptCommand(selectedFileForRun); String testCommand = buildExecTestSuiteCommand(PRIVATE_FOLDER); String setupScriptCommand = exercise.hasGradingSetupScript() ? buildSetupScriptCommand(exercise.getGradingSetup()) : ""; diff --git a/src/main/java/ch/uzh/ifi/access/student/model/CodeSubmission.java b/src/main/java/ch/uzh/ifi/access/student/model/CodeSubmission.java index 62a8d487..0f43febb 100644 --- a/src/main/java/ch/uzh/ifi/access/student/model/CodeSubmission.java +++ b/src/main/java/ch/uzh/ifi/access/student/model/CodeSubmission.java @@ -18,15 +18,15 @@ public class CodeSubmission extends StudentSubmission { private List publicFiles; - private int selectedFile; + private String selectedFileId; private ExecResult console; @Builder - public CodeSubmission(String id, int version, String userId, String commitId, String exerciseId, boolean isGraded, Instant timestamp, boolean isInvalid, boolean isTriggeredReSubmission, List publicFiles, int selectedFile, ExecResult console) { + public CodeSubmission(String id, int version, String userId, String commitId, String exerciseId, boolean isGraded, Instant timestamp, boolean isInvalid, boolean isTriggeredReSubmission, List publicFiles, String selectedFileId, ExecResult console) { super(id, version, userId, commitId, exerciseId, isGraded, timestamp, null, isInvalid, isTriggeredReSubmission); this.publicFiles = publicFiles; - this.selectedFile = selectedFile; + this.selectedFileId = selectedFileId; this.console = console; } @@ -37,6 +37,15 @@ public VirtualFile getPublicFile(int index) { throw new IllegalArgumentException(String.format("Cannot access index %d of public files (size %d)", index, publicFiles.size())); } + public VirtualFile getPublicFile(String id) { + if (id.equals("-1")) { + return publicFiles.get(0); + }else{ + return publicFiles.stream().filter(file -> file.getId().equals(id)).findFirst() + .orElseThrow(() -> new IllegalArgumentException(String.format("Cannot find file with id %s in public folder", id))); + } + } + @Override public StudentSubmission stripSubmissionForReEvaluation() { CodeSubmission stripped = new CodeSubmission(); @@ -45,7 +54,7 @@ public StudentSubmission stripSubmissionForReEvaluation() { stripped.setExerciseId(this.getExerciseId()); stripped.setGraded(this.isGraded()); stripped.setPublicFiles(this.publicFiles); - stripped.setSelectedFile(this.getSelectedFile()); + stripped.setSelectedFileId(this.getSelectedFileId()); return stripped; } From 3d9fdf7f702d7d2ea45c59975ce7fb5640d9345e Mon Sep 17 00:00:00 2001 From: Haeri Date: Fri, 1 Nov 2019 01:46:03 +0100 Subject: [PATCH 17/36] Big Ufff - Tried to fix VirtualFiles generating non persistent ids - Trying to fix tests --- src/main/java/ch/uzh/ifi/access/course/model/VirtualFile.java | 2 +- .../student/evaluation/runner/SubmissionCodeRunnerTest.java | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/main/java/ch/uzh/ifi/access/course/model/VirtualFile.java b/src/main/java/ch/uzh/ifi/access/course/model/VirtualFile.java index 4a9fe14b..f557d860 100644 --- a/src/main/java/ch/uzh/ifi/access/course/model/VirtualFile.java +++ b/src/main/java/ch/uzh/ifi/access/course/model/VirtualFile.java @@ -43,7 +43,7 @@ public VirtualFile() { } public VirtualFile(String fullPath, String virtualPath) { - this.id = new Utils().getID(); + this.id = new Utils().getID(fullPath); this.file = new File(fullPath); this.path = virtualPath; diff --git a/src/test/java/ch/uzh/ifi/access/student/evaluation/runner/SubmissionCodeRunnerTest.java b/src/test/java/ch/uzh/ifi/access/student/evaluation/runner/SubmissionCodeRunnerTest.java index aa5a6809..16084109 100644 --- a/src/test/java/ch/uzh/ifi/access/student/evaluation/runner/SubmissionCodeRunnerTest.java +++ b/src/test/java/ch/uzh/ifi/access/student/evaluation/runner/SubmissionCodeRunnerTest.java @@ -51,7 +51,7 @@ public void testSubmission() throws DockerCertificateException, InterruptedExcep .id("s1") .exerciseId(ex.getId()) .publicFiles(Arrays.asList(init, src)) - .selectedFile(1) + .selectedFileId(src.getId()) .isGraded(true) .build(); @@ -73,7 +73,7 @@ public void testSmoketest() throws DockerCertificateException, InterruptedExcept .id("s1") .exerciseId(ex.getId()) .publicFiles(Arrays.asList(init, src)) - .selectedFile(1) + .selectedFileId(src.getId()) .isGraded(false) .build(); From 79fee7e65b9ccb190146078dc03cc0e3b398416e Mon Sep 17 00:00:00 2001 From: Haeri Date: Fri, 1 Nov 2019 17:42:18 +0100 Subject: [PATCH 18/36] cause circle CI was not happy --- .../java/ch/uzh/ifi/access/student/model/CodeSubmission.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/ch/uzh/ifi/access/student/model/CodeSubmission.java b/src/main/java/ch/uzh/ifi/access/student/model/CodeSubmission.java index 0f43febb..4b8b29c6 100644 --- a/src/main/java/ch/uzh/ifi/access/student/model/CodeSubmission.java +++ b/src/main/java/ch/uzh/ifi/access/student/model/CodeSubmission.java @@ -38,7 +38,7 @@ public VirtualFile getPublicFile(int index) { } public VirtualFile getPublicFile(String id) { - if (id.equals("-1")) { + if (("-1").equals(id)) { return publicFiles.get(0); }else{ return publicFiles.stream().filter(file -> file.getId().equals(id)).findFirst() From bc3fffa0db312197edf982a1f52092b8a6a8597d Mon Sep 17 00:00:00 2001 From: Alexander Hofmann Date: Sat, 2 Nov 2019 12:08:19 +0100 Subject: [PATCH 19/36] Better encapsulate getSelectedFile inside code submission --- .../evaluation/runner/SubmissionCodeRunner.java | 2 +- .../ifi/access/student/model/CodeSubmission.java | 13 ++++++------- 2 files changed, 7 insertions(+), 8 deletions(-) diff --git a/src/main/java/ch/uzh/ifi/access/student/evaluation/runner/SubmissionCodeRunner.java b/src/main/java/ch/uzh/ifi/access/student/evaluation/runner/SubmissionCodeRunner.java index e1490732..5f72a135 100644 --- a/src/main/java/ch/uzh/ifi/access/student/evaluation/runner/SubmissionCodeRunner.java +++ b/src/main/java/ch/uzh/ifi/access/student/evaluation/runner/SubmissionCodeRunner.java @@ -67,7 +67,7 @@ private ExecResult executeSmokeTest(Path workPath, CodeSubmission submission, Co hierarchySerializer.persistFilesIntoFolder(String.format("%s/%s", workPath.toString(), PUBLIC_FOLDER), submission.getPublicFiles()); Files.createFile(Paths.get(workPath.toAbsolutePath().toString(), INIT_FILE)); - VirtualFile selectedFileForRun = submission.getPublicFile(submission.getSelectedFileId()); + VirtualFile selectedFileForRun = submission.getSelectedFile(); String executeScriptCommand = buildExecScriptCommand(selectedFileForRun); String testCommand = buildExecTestSuiteCommand(PUBLIC_FOLDER); diff --git a/src/main/java/ch/uzh/ifi/access/student/model/CodeSubmission.java b/src/main/java/ch/uzh/ifi/access/student/model/CodeSubmission.java index 4b8b29c6..ac5a5845 100644 --- a/src/main/java/ch/uzh/ifi/access/student/model/CodeSubmission.java +++ b/src/main/java/ch/uzh/ifi/access/student/model/CodeSubmission.java @@ -1,6 +1,7 @@ package ch.uzh.ifi.access.student.model; import ch.uzh.ifi.access.course.model.VirtualFile; +import com.fasterxml.jackson.annotation.JsonIgnore; import lombok.*; import org.springframework.data.annotation.TypeAlias; import org.springframework.data.mongodb.core.mapping.Document; @@ -30,17 +31,15 @@ public CodeSubmission(String id, int version, String userId, String commitId, St this.console = console; } - public VirtualFile getPublicFile(int index) { - if (index < publicFiles.size()) { - return publicFiles.get(index); - } - throw new IllegalArgumentException(String.format("Cannot access index %d of public files (size %d)", index, publicFiles.size())); + @JsonIgnore + public VirtualFile getSelectedFile() { + return getPublicFile(selectedFileId); } - public VirtualFile getPublicFile(String id) { + private VirtualFile getPublicFile(String id) { if (("-1").equals(id)) { return publicFiles.get(0); - }else{ + } else { return publicFiles.stream().filter(file -> file.getId().equals(id)).findFirst() .orElseThrow(() -> new IllegalArgumentException(String.format("Cannot find file with id %s in public folder", id))); } From e5abb48576870361e1fad09e72904e9053c43d35 Mon Sep 17 00:00:00 2001 From: Alexander Hofmann Date: Sat, 2 Nov 2019 12:08:56 +0100 Subject: [PATCH 20/36] Now that we do not show any log output on submissions, we might as well not execute the script but only the tests on submissions --- .../student/evaluation/runner/SubmissionCodeRunner.java | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/main/java/ch/uzh/ifi/access/student/evaluation/runner/SubmissionCodeRunner.java b/src/main/java/ch/uzh/ifi/access/student/evaluation/runner/SubmissionCodeRunner.java index 5f72a135..76720734 100644 --- a/src/main/java/ch/uzh/ifi/access/student/evaluation/runner/SubmissionCodeRunner.java +++ b/src/main/java/ch/uzh/ifi/access/student/evaluation/runner/SubmissionCodeRunner.java @@ -86,12 +86,10 @@ private ExecResult executeSubmission(Path workPath, CodeSubmission submission, E Files.createFile(Paths.get(workPath.toAbsolutePath().toString(), INIT_FILE)); - VirtualFile selectedFileForRun = submission.getPublicFile(submission.getSelectedFileId()); - String executeScriptCommand = buildExecScriptCommand(selectedFileForRun); String testCommand = buildExecTestSuiteCommand(PRIVATE_FOLDER); String setupScriptCommand = exercise.hasGradingSetupScript() ? buildSetupScriptCommand(exercise.getGradingSetup()) : ""; - List commands = List.of(setupScriptCommand, executeScriptCommand, DELIMITER_CMD, testCommand) + List commands = List.of(setupScriptCommand, DELIMITER_CMD, testCommand) .stream() .filter(cmd -> !StringUtils.isEmpty(cmd)) .collect(Collectors.toList()); From ea656ce0d39e2b93b3fa7b1df4ef9e734a0af187 Mon Sep 17 00:00:00 2001 From: Alexander Hofmann Date: Sat, 2 Nov 2019 16:14:23 +0100 Subject: [PATCH 21/36] Use custom access image Add scheduled tasks to check for: - image updates (every hour) - is worker still online (every 5 mins) --- .../uzh/ifi/access/coderunner/CodeRunner.java | 25 ++++++++++-------- .../access/coderunner/PythonImageConfig.java | 2 +- .../student/config/SchedulingConfig.java | 26 ++++++++++++++++--- 3 files changed, 38 insertions(+), 15 deletions(-) diff --git a/src/main/java/ch/uzh/ifi/access/coderunner/CodeRunner.java b/src/main/java/ch/uzh/ifi/access/coderunner/CodeRunner.java index 304d061e..ab208217 100644 --- a/src/main/java/ch/uzh/ifi/access/coderunner/CodeRunner.java +++ b/src/main/java/ch/uzh/ifi/access/coderunner/CodeRunner.java @@ -6,7 +6,10 @@ import com.spotify.docker.client.DockerClient; import com.spotify.docker.client.exceptions.DockerCertificateException; import com.spotify.docker.client.exceptions.DockerException; -import com.spotify.docker.client.messages.*; +import com.spotify.docker.client.messages.ContainerConfig; +import com.spotify.docker.client.messages.ContainerCreation; +import com.spotify.docker.client.messages.ContainerState; +import com.spotify.docker.client.messages.Info; import org.slf4j.Logger; import org.slf4j.LoggerFactory; import org.springframework.stereotype.Service; @@ -15,7 +18,6 @@ import java.nio.file.Files; import java.nio.file.Path; import java.nio.file.Paths; -import java.util.List; import java.util.StringJoiner; import java.util.concurrent.*; import java.util.stream.Stream; @@ -34,11 +36,16 @@ public class CodeRunner { public CodeRunner() throws DockerCertificateException { docker = DefaultDockerClient.fromEnv().build(); - logDebugInfo(); - pullImageIfNotPresent(); + logDockerInfo(); + pullImage(); } - private void logDebugInfo() { + public void check() { + logDockerInfo(); + pullImage(); + } + + public void logDockerInfo() { try { Info info = docker.info(); logger.debug("Connected to docker daemon @ " + docker.getHost()); @@ -48,13 +55,9 @@ private void logDebugInfo() { } } - private void pullImageIfNotPresent() { + private void pullImage() { try { - List images = docker.listImages(DockerClient.ListImagesParam.byName(PythonImageConfig.PYTHON_DOCKER_IMAGE)); - if (images.isEmpty()) { - logger.debug(String.format("No suitable python image found (searched for %s), pulling new image from registry", PythonImageConfig.PYTHON_DOCKER_IMAGE)); - docker.pull(PythonImageConfig.PYTHON_DOCKER_IMAGE); - } + docker.pull(PythonImageConfig.PYTHON_DOCKER_IMAGE); } catch (DockerException | InterruptedException e) { logger.warn("Failed to pull python docker image", e); } diff --git a/src/main/java/ch/uzh/ifi/access/coderunner/PythonImageConfig.java b/src/main/java/ch/uzh/ifi/access/coderunner/PythonImageConfig.java index ea1d6dee..1f5f8ad7 100644 --- a/src/main/java/ch/uzh/ifi/access/coderunner/PythonImageConfig.java +++ b/src/main/java/ch/uzh/ifi/access/coderunner/PythonImageConfig.java @@ -12,7 +12,7 @@ @NoArgsConstructor public class PythonImageConfig { - public static final String PYTHON_DOCKER_IMAGE = "python:3.7-alpine"; + public static final String PYTHON_DOCKER_IMAGE = "hoal/access-python:3.7"; public static final String STUDENT_CODE_FOLDER = "/usr/src/"; diff --git a/src/main/java/ch/uzh/ifi/access/student/config/SchedulingConfig.java b/src/main/java/ch/uzh/ifi/access/student/config/SchedulingConfig.java index 33cee7bb..b8d26c7c 100644 --- a/src/main/java/ch/uzh/ifi/access/student/config/SchedulingConfig.java +++ b/src/main/java/ch/uzh/ifi/access/student/config/SchedulingConfig.java @@ -1,5 +1,6 @@ package ch.uzh.ifi.access.student.config; +import ch.uzh.ifi.access.coderunner.CodeRunner; import ch.uzh.ifi.access.student.evaluation.process.EvalMachineRepoService; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -16,12 +17,19 @@ public class SchedulingConfig { private static Logger logger = LoggerFactory.getLogger(SchedulingConfig.class); - private static final long FIXED_DELAY_IN_MINUTES = 5; + private static final long FIXED_DELAY_IN_MINUTES = 1; - private EvalMachineRepoService machineRepository; + private static final long DOCKER_WATCHDOG_DELAY_IN_MINUTES = 5; - public SchedulingConfig(EvalMachineRepoService machineRepository) { + private static final long DOCKER_IMAGE_UPDATER_DELAY_IN_MINUTES = 60; + + private final EvalMachineRepoService machineRepository; + + private final CodeRunner codeRunner; + + public SchedulingConfig(EvalMachineRepoService machineRepository, CodeRunner codeRunner) { this.machineRepository = machineRepository; + this.codeRunner = codeRunner; } @Scheduled(fixedDelay = FIXED_DELAY_IN_MINUTES * 60000) @@ -31,4 +39,16 @@ public void cleanUpRepo() { machineRepository.removeMachinesOlderThan(threshold); logger.info("Completed cleanup. Repository size {}", machineRepository.count()); } + + @Scheduled(fixedDelay = DOCKER_WATCHDOG_DELAY_IN_MINUTES * 60000) + public void dockerHealthCheck() { + logger.info("Docker worker health check"); + codeRunner.logDockerInfo(); + } + + @Scheduled(fixedDelay = DOCKER_IMAGE_UPDATER_DELAY_IN_MINUTES * 60000) + public void dockerImageUpdater() { + logger.info("Docker worker health and image update check"); + codeRunner.check(); + } } From 636274681704dc0ec57ed9db32407b2b966fdb6f Mon Sep 17 00:00:00 2001 From: Alexander Hofmann Date: Sat, 2 Nov 2019 16:38:48 +0100 Subject: [PATCH 22/36] Add api to fetch list of students for a course --- .../controller/AssistantController.java | 15 +++++++++++++-- .../controller/SubmissionController.java | 19 +++++++++++++++++++ .../service/AdminSubmissionService.java | 4 ++++ 3 files changed, 36 insertions(+), 2 deletions(-) diff --git a/src/main/java/ch/uzh/ifi/access/student/controller/AssistantController.java b/src/main/java/ch/uzh/ifi/access/student/controller/AssistantController.java index f64bc55d..d16ea35c 100644 --- a/src/main/java/ch/uzh/ifi/access/student/controller/AssistantController.java +++ b/src/main/java/ch/uzh/ifi/access/student/controller/AssistantController.java @@ -6,6 +6,7 @@ import ch.uzh.ifi.access.course.service.CourseService; import ch.uzh.ifi.access.student.reporting.AssignmentReport; import ch.uzh.ifi.access.student.service.AdminSubmissionService; +import ch.uzh.ifi.access.student.service.UserService; import org.springframework.http.ResponseEntity; import org.springframework.web.bind.annotation.GetMapping; import org.springframework.web.bind.annotation.PathVariable; @@ -48,8 +49,18 @@ public ResponseEntity reevaluateInvalidateSubmissionsForAssignment(@PathVaria Assignment assignment = course.getAssignmentById(assignmentId) .orElseThrow(() -> new ResourceNotFoundException("No assignment found")); - adminSubmissionService.reevaluateAssignmentsInvalidSubmissions(course, assignment); - return ResponseEntity.ok("ok"); + adminSubmissionService.reevaluateAssignmentsInvalidSubmissions(course, assignment); + return ResponseEntity.ok("ok"); } + @GetMapping("/courses/{courseId}/participants") + public ResponseEntity getCourseParticipants(@PathVariable String courseId) { + Course course = courseService + .getCourseById(courseId) + .orElseThrow(() -> new ResourceNotFoundException("No course found")); + + UserService.UserQueryResult courseStudents = adminSubmissionService.getCourseStudents(course); + + return ResponseEntity.ok(courseStudents); + } } diff --git a/src/main/java/ch/uzh/ifi/access/student/controller/SubmissionController.java b/src/main/java/ch/uzh/ifi/access/student/controller/SubmissionController.java index ad56ecc4..a4894b88 100644 --- a/src/main/java/ch/uzh/ifi/access/student/controller/SubmissionController.java +++ b/src/main/java/ch/uzh/ifi/access/student/controller/SubmissionController.java @@ -62,6 +62,25 @@ public ResponseEntity getSubmissionById(@PathVariable String return ResponseEntity.ok(submission); } + // TODO how to get submissionId if I only have userId? + @GetMapping("/{submissionId}") + public ResponseEntity getSubmissionByIdByUser(@PathVariable String submissionId, @ApiIgnore CourseAuthentication authentication) { + StudentSubmission submission = studentSubmissionService.findById(submissionId).orElse(null); + if (submission == null) { + return ResponseEntity.badRequest().build(); + } + + // Check for permissions to access submissions + Exercise exercise = courseService.getExerciseById(submission.getExerciseId()).orElseThrow(() -> new IllegalArgumentException("Did not find exercise for submission " + submissionId)); + String courseId = exercise.getCourseId(); + + if (!authentication.hasAdminAccess(courseId)) { + return ResponseEntity.badRequest().build(); + } + + return ResponseEntity.ok(submission); + } + @GetMapping("/exercises/{exerciseId}") public ResponseEntity getSubmissionByExercise(@PathVariable String exerciseId, @ApiIgnore CourseAuthentication authentication) { Assert.notNull(authentication, "No authentication object found for user"); diff --git a/src/main/java/ch/uzh/ifi/access/student/service/AdminSubmissionService.java b/src/main/java/ch/uzh/ifi/access/student/service/AdminSubmissionService.java index a93a74d7..4615da79 100644 --- a/src/main/java/ch/uzh/ifi/access/student/service/AdminSubmissionService.java +++ b/src/main/java/ch/uzh/ifi/access/student/service/AdminSubmissionService.java @@ -92,6 +92,10 @@ protected void triggerReEvaluation(List submissions) { logger.debug(String.format("Fire re-evaluation for processId %s with new submission %s)", reSubmitted.getId(), processId)); evaluationService.fireEvalProcessExecutionAsync(processId); } + } + @PreAuthorize("@coursePermissionEvaluator.hasAdminAccessToCourse(authentication, #course)") + public UserService.UserQueryResult getCourseStudents(Course course) { + return userService.getCourseStudents(course); } } From 122dab6ad80ed08de4401a6efe69bb4b9757e1ac Mon Sep 17 00:00:00 2001 From: Alexander Hofmann Date: Tue, 5 Nov 2019 00:11:09 +0100 Subject: [PATCH 23/36] Add method to create an impersonation auth object --- .../course/config/CourseAuthentication.java | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/src/main/java/ch/uzh/ifi/access/course/config/CourseAuthentication.java b/src/main/java/ch/uzh/ifi/access/course/config/CourseAuthentication.java index 950f772b..38675ea5 100644 --- a/src/main/java/ch/uzh/ifi/access/course/config/CourseAuthentication.java +++ b/src/main/java/ch/uzh/ifi/access/course/config/CourseAuthentication.java @@ -2,6 +2,7 @@ import ch.uzh.ifi.access.course.model.security.GrantedCourseAccess; import org.springframework.security.core.Authentication; +import org.springframework.security.core.context.SecurityContextHolder; import org.springframework.security.oauth2.provider.OAuth2Authentication; import org.springframework.security.oauth2.provider.OAuth2Request; @@ -41,4 +42,22 @@ public boolean hasAdminAccess(String courseId) { public String getUserId() { return userId; } + + /** + * Creates a new authentication object for the given user and sets it to the security context. + * If the user is not allowed to impersonate a user, does nothing and returns null. + * + * @param userId the user to impersonate + * @param courseId the course for which the calling user has admin access + * @return impersonated authentication object if admin, null otherwise. + */ + public CourseAuthentication impersonateUser(String userId, String courseId) { + if (hasAdminAccess(courseId)) { + CourseAuthentication impersonatedAuth = new CourseAuthentication(getOAuth2Request(), this, courseAccesses, userId); + SecurityContextHolder.getContext().setAuthentication(impersonatedAuth); + return impersonatedAuth; + } + + return null; + } } \ No newline at end of file From e656a56f4daa8f99cf5c709464efbfe6cf467254 Mon Sep 17 00:00:00 2001 From: Alexander Hofmann Date: Tue, 5 Nov 2019 00:11:29 +0100 Subject: [PATCH 24/36] Add method to fetch course by exercise id --- .../java/ch/uzh/ifi/access/course/service/CourseService.java | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/main/java/ch/uzh/ifi/access/course/service/CourseService.java b/src/main/java/ch/uzh/ifi/access/course/service/CourseService.java index a7e7d302..acb236cf 100644 --- a/src/main/java/ch/uzh/ifi/access/course/service/CourseService.java +++ b/src/main/java/ch/uzh/ifi/access/course/service/CourseService.java @@ -70,6 +70,10 @@ public Optional getExerciseMaxSubmissions(String exerciseId) { return courseDao.selectExerciseById(exerciseId).map(Exercise::getMaxSubmits); } + public Optional getCourseByExerciseId(String exerciseId) { + return getExerciseById(exerciseId).flatMap(ex -> getCourseById(ex.getCourseId())); + } + private boolean hasAccessToExerciseSolutions(Exercise exercise, CourseAuthentication authentication) { return exercise.isPastDueDate() || authentication.hasAdminAccess(exercise.getCourseId()); } From d2e1fd946c3b89f47dafbe7a02d62f9df920932d Mon Sep 17 00:00:00 2001 From: Alexander Hofmann Date: Tue, 5 Nov 2019 00:12:03 +0100 Subject: [PATCH 25/36] Add impersonation version of fetching submission, history and last submission for user --- .../controller/SubmissionController.java | 68 +++++++++++++------ 1 file changed, 49 insertions(+), 19 deletions(-) diff --git a/src/main/java/ch/uzh/ifi/access/student/controller/SubmissionController.java b/src/main/java/ch/uzh/ifi/access/student/controller/SubmissionController.java index a4894b88..926f04ef 100644 --- a/src/main/java/ch/uzh/ifi/access/student/controller/SubmissionController.java +++ b/src/main/java/ch/uzh/ifi/access/student/controller/SubmissionController.java @@ -4,6 +4,7 @@ import ch.uzh.ifi.access.course.config.CourseAuthentication; import ch.uzh.ifi.access.course.config.CoursePermissionEvaluator; import ch.uzh.ifi.access.course.controller.ResourceNotFoundException; +import ch.uzh.ifi.access.course.model.Course; import ch.uzh.ifi.access.course.model.Exercise; import ch.uzh.ifi.access.course.service.CourseService; import ch.uzh.ifi.access.student.dto.StudentAnswerDTO; @@ -51,30 +52,27 @@ public SubmissionController(StudentSubmissionService studentSubmissionService, C this.gracefulShutdown = gracefulShutdown; } - @GetMapping("/{submissionId}") - public ResponseEntity getSubmissionById(@PathVariable String submissionId, @ApiIgnore CourseAuthentication authentication) { - StudentSubmission submission = studentSubmissionService.findById(submissionId).orElse(null); - - if (submission == null || !submission.userIdMatches(authentication.getUserId())) { - return ResponseEntity.badRequest().build(); - } - - return ResponseEntity.ok(submission); + @GetMapping("/{submissionId}/users/{userId}") + public ResponseEntity getSubmissionById(@PathVariable String submissionId, @PathVariable String userId, @ApiIgnore CourseAuthentication authentication) { + Optional subOpt = studentSubmissionService.findById(submissionId); + + return subOpt + .flatMap(submission -> courseService.getCourseByExerciseId(submission.getExerciseId())) + .flatMap(course -> { + if (authentication.hasAdminAccess(course.getId())) { + return subOpt; + } + return Optional.empty(); + }) + .map(ResponseEntity::ok) + .orElse(ResponseEntity.badRequest().build()); } - // TODO how to get submissionId if I only have userId? @GetMapping("/{submissionId}") - public ResponseEntity getSubmissionByIdByUser(@PathVariable String submissionId, @ApiIgnore CourseAuthentication authentication) { + public ResponseEntity getSubmissionById(@PathVariable String submissionId, @ApiIgnore CourseAuthentication authentication) { StudentSubmission submission = studentSubmissionService.findById(submissionId).orElse(null); - if (submission == null) { - return ResponseEntity.badRequest().build(); - } - // Check for permissions to access submissions - Exercise exercise = courseService.getExerciseById(submission.getExerciseId()).orElseThrow(() -> new IllegalArgumentException("Did not find exercise for submission " + submissionId)); - String courseId = exercise.getCourseId(); - - if (!authentication.hasAdminAccess(courseId)) { + if (submission == null || !submission.userIdMatches(authentication.getUserId())) { return ResponseEntity.badRequest().build(); } @@ -97,6 +95,22 @@ public ResponseEntity getSubmissionByExercise(@PathVariable S .orElse(ResponseEntity.noContent().build()); } + @GetMapping("/exercises/{exerciseId}/users/{userId}") + public ResponseEntity getSubmissionByExerciseAsAdmin(@PathVariable String exerciseId, @PathVariable String userId, @ApiIgnore CourseAuthentication authentication) { + Assert.notNull(authentication, "No authentication object found for user"); + + Course course = courseService.getCourseByExerciseId(exerciseId).orElseThrow(IllegalArgumentException::new); + + CourseAuthentication impersonation = authentication.impersonateUser(userId, course.getId()); + if (impersonation != null) { + return getSubmissionByExercise(exerciseId, impersonation); + } + + logger.warn("User {} called a restricted function for which it does not have enough rights!", authentication.getUserId()); + + return ResponseEntity.status(403).build(); + } + @PostMapping("/exercises/{exerciseId}") public ResponseEntity submit(@PathVariable String exerciseId, @RequestBody StudentAnswerDTO submissionDTO, @ApiIgnore CourseAuthentication authentication) { Assert.notNull(authentication, "No authentication object found for user"); @@ -146,6 +160,22 @@ public Map getEvalProcessState(@PathVariable String processId, @ return processService.getEvalProcessState(processId); } + @GetMapping("/exercises/{exerciseId}/users/{userId}/history") + public ResponseEntity getAllSubmissionsForExercise(@PathVariable String exerciseId, @PathVariable String userId, @ApiIgnore CourseAuthentication authentication) { + Assert.notNull(authentication, "No authentication object found for user"); + + Course course = courseService.getCourseByExerciseId(exerciseId).orElseThrow(IllegalArgumentException::new); + + CourseAuthentication impersonation = authentication.impersonateUser(userId, course.getId()); + if (impersonation != null) { + return ResponseEntity.ok(getAllSubmissionsForExercise(exerciseId, impersonation)); + } + + logger.warn("User {} called a restricted function for which it does not have enough rights!", authentication.getUserId()); + + return ResponseEntity.status(403).build(); + } + @GetMapping("/exercises/{exerciseId}/history") public SubmissionHistoryDTO getAllSubmissionsForExercise(@PathVariable String exerciseId, @ApiIgnore CourseAuthentication authentication) { Assert.notNull(authentication, "No authentication object found for user"); From 09eae9e489544cf451ab39c9189fb5caa564b05f Mon Sep 17 00:00:00 2001 From: soyabeen Date: Wed, 6 Nov 2019 01:20:54 +0100 Subject: [PATCH 26/36] include exercise resource folder in code exec. --- .../student/evaluation/runner/SubmissionCodeRunner.java | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/main/java/ch/uzh/ifi/access/student/evaluation/runner/SubmissionCodeRunner.java b/src/main/java/ch/uzh/ifi/access/student/evaluation/runner/SubmissionCodeRunner.java index 76720734..2c9b5e0a 100644 --- a/src/main/java/ch/uzh/ifi/access/student/evaluation/runner/SubmissionCodeRunner.java +++ b/src/main/java/ch/uzh/ifi/access/student/evaluation/runner/SubmissionCodeRunner.java @@ -32,6 +32,8 @@ public class SubmissionCodeRunner { private static final String PUBLIC_FOLDER = "public"; + private static final String RESOURCE_FOLDER = "resource"; + private static final String PRIVATE_FOLDER = "private"; private static final String INIT_FILE = "__init__.py"; @@ -56,15 +58,16 @@ public ExecResult execSubmissionForExercise(CodeSubmission submission, Exercise logger.debug(path.toAbsolutePath().normalize().toString()); CodeExecutionLimits executionLimits = exercise.getExecutionLimits(); - ExecResult res = submission.isGraded() ? executeSubmission(path, submission, exercise, executionLimits) : executeSmokeTest(path, submission, executionLimits); + ExecResult res = submission.isGraded() ? executeSubmission(path, submission, exercise, executionLimits) : executeSmokeTest(path, submission, exercise, executionLimits); hierarchySerializer.removeDirectory(path); return res; } - private ExecResult executeSmokeTest(Path workPath, CodeSubmission submission, CodeExecutionLimits executionLimits) throws IOException, DockerException, InterruptedException { + private ExecResult executeSmokeTest(Path workPath, CodeSubmission submission, Exercise exercise, CodeExecutionLimits executionLimits) throws IOException, DockerException, InterruptedException { hierarchySerializer.persistFilesIntoFolder(String.format("%s/%s", workPath.toString(), PUBLIC_FOLDER), submission.getPublicFiles()); + hierarchySerializer.persistFilesIntoFolder(String.format("%s/%s", workPath.toString(), RESOURCE_FOLDER), exercise.getResource_files()); Files.createFile(Paths.get(workPath.toAbsolutePath().toString(), INIT_FILE)); VirtualFile selectedFileForRun = submission.getSelectedFile(); @@ -82,6 +85,7 @@ private ExecResult executeSmokeTest(Path workPath, CodeSubmission submission, Co private ExecResult executeSubmission(Path workPath, CodeSubmission submission, Exercise exercise, CodeExecutionLimits executionLimits) throws IOException, DockerException, InterruptedException { hierarchySerializer.persistFilesIntoFolder(String.format("%s/%s", workPath.toString(), PUBLIC_FOLDER), submission.getPublicFiles()); + hierarchySerializer.persistFilesIntoFolder(String.format("%s/%s", workPath.toString(), RESOURCE_FOLDER), exercise.getResource_files()); hierarchySerializer.persistFilesIntoFolder(String.format("%s/%s", workPath.toString(), PRIVATE_FOLDER), exercise.getPrivate_files()); Files.createFile(Paths.get(workPath.toAbsolutePath().toString(), INIT_FILE)); From 587d6eb9638beea5bc4b9fa8babc744afdfb7a20 Mon Sep 17 00:00:00 2001 From: Alexander Hofmann Date: Wed, 6 Nov 2019 13:38:35 +0100 Subject: [PATCH 27/36] Make maxScore double instead of int --- .../access/course/dto/ExerciseMetadataDTO.java | 2 +- .../uzh/ifi/access/course/model/Assignment.java | 4 ++-- .../ifi/access/course/model/ExerciseConfig.java | 2 +- .../evaluator/MultipleChoiceEvaluator.java | 10 ++++------ .../student/model/SubmissionEvaluation.java | 16 +++++----------- 5 files changed, 13 insertions(+), 21 deletions(-) diff --git a/src/main/java/ch/uzh/ifi/access/course/dto/ExerciseMetadataDTO.java b/src/main/java/ch/uzh/ifi/access/course/dto/ExerciseMetadataDTO.java index 7089ffc6..5e2bd861 100644 --- a/src/main/java/ch/uzh/ifi/access/course/dto/ExerciseMetadataDTO.java +++ b/src/main/java/ch/uzh/ifi/access/course/dto/ExerciseMetadataDTO.java @@ -14,7 +14,7 @@ public class ExerciseMetadataDTO { private ExerciseType type; private String language; private Boolean isGraded; - private int maxScore; + private double maxScore; public ExerciseMetadataDTO(Exercise exercise) { this.id = exercise.getId(); diff --git a/src/main/java/ch/uzh/ifi/access/course/model/Assignment.java b/src/main/java/ch/uzh/ifi/access/course/model/Assignment.java index 098b8474..5da1095a 100644 --- a/src/main/java/ch/uzh/ifi/access/course/model/Assignment.java +++ b/src/main/java/ch/uzh/ifi/access/course/model/Assignment.java @@ -69,8 +69,8 @@ public Optional findExerciseById(String id) { return exercises.stream().filter(e -> e.getId().equals(id)).findFirst(); } - public int getMaxScore() { - return exercises.stream().mapToInt(e -> e.getMaxScore()).sum(); + public double getMaxScore() { + return exercises.stream().mapToDouble(ExerciseConfig::getMaxScore).sum(); } diff --git a/src/main/java/ch/uzh/ifi/access/course/model/ExerciseConfig.java b/src/main/java/ch/uzh/ifi/access/course/model/ExerciseConfig.java index bd6babc9..4511c8a6 100644 --- a/src/main/java/ch/uzh/ifi/access/course/model/ExerciseConfig.java +++ b/src/main/java/ch/uzh/ifi/access/course/model/ExerciseConfig.java @@ -15,7 +15,7 @@ public class ExerciseConfig implements Serializable { protected ExerciseType type; protected String language; protected Boolean isGraded; - protected int maxScore; + protected double maxScore; protected int maxSubmits; protected String gradingSetup; diff --git a/src/main/java/ch/uzh/ifi/access/student/evaluation/evaluator/MultipleChoiceEvaluator.java b/src/main/java/ch/uzh/ifi/access/student/evaluation/evaluator/MultipleChoiceEvaluator.java index 43422d8b..8e88f2ee 100644 --- a/src/main/java/ch/uzh/ifi/access/student/evaluation/evaluator/MultipleChoiceEvaluator.java +++ b/src/main/java/ch/uzh/ifi/access/student/evaluation/evaluator/MultipleChoiceEvaluator.java @@ -9,9 +9,7 @@ import java.time.Instant; import java.util.Collection; -import java.util.Collections; -import java.util.Set; -import java.util.stream.Collectors; +import java.util.HashSet; public class MultipleChoiceEvaluator implements StudentSubmissionEvaluator { @@ -27,7 +25,7 @@ public SubmissionEvaluation evaluate(StudentSubmission submission, Exercise exer var wrongAnswers = getNrWrongAnswers(answer, solution); var calc = correctAnswers - wrongAnswers; - var points = calc < 0 ? 0 : calc; + var points = Math.max(calc, 0); var hints = points < exercise.getMaxScore() ? exercise.getHints() : null; @@ -40,13 +38,13 @@ public SubmissionEvaluation evaluate(StudentSubmission submission, Exercise exer } private int getNrCorrectAnswers(final Collection answers, final Collection solutions) { - var ans = answers.stream().collect(Collectors.toSet()); + var ans = new HashSet<>(answers); ans.retainAll(solutions); return ans.size(); } private int getNrWrongAnswers(final Collection answers, final Collection solutions) { - var ans = answers.stream().collect(Collectors.toSet()); + var ans = new HashSet<>(answers); ans.removeAll(solutions); return ans.size(); } diff --git a/src/main/java/ch/uzh/ifi/access/student/model/SubmissionEvaluation.java b/src/main/java/ch/uzh/ifi/access/student/model/SubmissionEvaluation.java index 11d86401..00480415 100644 --- a/src/main/java/ch/uzh/ifi/access/student/model/SubmissionEvaluation.java +++ b/src/main/java/ch/uzh/ifi/access/student/model/SubmissionEvaluation.java @@ -1,18 +1,12 @@ package ch.uzh.ifi.access.student.model; +import com.fasterxml.jackson.annotation.JsonProperty; +import lombok.*; + import java.time.Instant; -import java.util.Arrays; import java.util.Collections; import java.util.List; -import com.fasterxml.jackson.annotation.JsonProperty; - -import lombok.AllArgsConstructor; -import lombok.Builder; -import lombok.Data; -import lombok.NoArgsConstructor; -import lombok.Value; - @SuppressWarnings("unused") @Value @Data @@ -24,7 +18,7 @@ public class SubmissionEvaluation { private Points points; - private int maxScore; + private double maxScore; private Instant timestamp; @@ -43,7 +37,7 @@ public double getScore() { } public List getHints() { - return hints != null && hints.size() > 1 ? Arrays.asList(hints.get(0)) : hints; + return hints != null && hints.size() > 1 ? List.of(hints.get(0)) : hints; } @Data From 0fe3f4b02c5928155f511b7466dcd73d529f081d Mon Sep 17 00:00:00 2001 From: soyabeen Date: Wed, 6 Nov 2019 14:10:05 +0100 Subject: [PATCH 28/36] Introduce rounding strategies to exercise #384. --- .../uzh/ifi/access/course/model/Exercise.java | 6 ++++-- .../access/course/model/ExerciseConfig.java | 4 +++- .../uzh/ifi/access/course/model/Rounding.java | 20 +++++++++++++++++++ 3 files changed, 27 insertions(+), 3 deletions(-) create mode 100644 src/main/java/ch/uzh/ifi/access/course/model/Rounding.java diff --git a/src/main/java/ch/uzh/ifi/access/course/model/Exercise.java b/src/main/java/ch/uzh/ifi/access/course/model/Exercise.java index 5b805ff2..a0585ca4 100644 --- a/src/main/java/ch/uzh/ifi/access/course/model/Exercise.java +++ b/src/main/java/ch/uzh/ifi/access/course/model/Exercise.java @@ -28,6 +28,7 @@ public class Exercise extends ExerciseConfig implements Indexed, HasBr @ToString.Exclude private String question; + @JsonIgnore @ToString.Exclude private List private_files; @@ -51,8 +52,8 @@ public Exercise(String name) { } @Builder - private Exercise(ExerciseType type, String language, Boolean isGraded, int maxScore, int maxSubmits, String gradingSetup, List options, List solutions, List hints, String id, int index, String gitHash, Assignment assignment, String question, List private_files, List solution_files, List resource_files, List public_files, CodeExecutionLimits executionLimits, String title, String longTitle) { - super(title, longTitle, type, language, isGraded, maxScore, maxSubmits, gradingSetup, options, solutions, hints, executionLimits); + private Exercise(ExerciseType type, String language, Boolean isGraded, int maxScore, int maxSubmits, String gradingSetup, List options, List solutions, List hints, String id, int index, String gitHash, Assignment assignment, String question, List private_files, List solution_files, List resource_files, List public_files, CodeExecutionLimits executionLimits, String title, String longTitle, Rounding rounding) { + super(title, longTitle, type, language, isGraded, maxScore, maxSubmits, gradingSetup, rounding, options, solutions, hints, executionLimits); this.id = id; this.index = index; this.gitHash = gitHash; @@ -82,6 +83,7 @@ public void set(ExerciseConfig other) { this.solutions = other.getSolutions(); this.hints = other.getHints(); this.executionLimits = other.getExecutionLimits(); + this.rounding = other.getRounding(); } /** diff --git a/src/main/java/ch/uzh/ifi/access/course/model/ExerciseConfig.java b/src/main/java/ch/uzh/ifi/access/course/model/ExerciseConfig.java index 4511c8a6..321e02d2 100644 --- a/src/main/java/ch/uzh/ifi/access/course/model/ExerciseConfig.java +++ b/src/main/java/ch/uzh/ifi/access/course/model/ExerciseConfig.java @@ -18,6 +18,7 @@ public class ExerciseConfig implements Serializable { protected double maxScore; protected int maxSubmits; protected String gradingSetup; + protected Rounding rounding; protected List options; protected List solutions; @@ -25,11 +26,12 @@ public class ExerciseConfig implements Serializable { protected CodeExecutionLimits executionLimits; - public ExerciseConfig() { this.isGraded = true; this.maxSubmits = 1; this.maxScore = 1; this.executionLimits = CodeExecutionLimits.DEFAULTS; + this.rounding = Rounding.DEFAULT; } + } diff --git a/src/main/java/ch/uzh/ifi/access/course/model/Rounding.java b/src/main/java/ch/uzh/ifi/access/course/model/Rounding.java new file mode 100644 index 00000000..5866ab83 --- /dev/null +++ b/src/main/java/ch/uzh/ifi/access/course/model/Rounding.java @@ -0,0 +1,20 @@ +package ch.uzh.ifi.access.course.model; + +public class Rounding { + + public static final Rounding DEFAULT = new Rounding(Rounding.Strategy.QUARTER_UP, 2); + + private Strategy strategy; + private int precision; + + public Rounding() { } + + public Rounding(Strategy strategy, int precision) { + this.strategy = strategy; + this.precision = precision; + } + + public enum Strategy {UP, DOWN, HALP_UP, HALF_DOWN, QUARTER_UP, QUARTER_DOWN} + + +} From 3d16f24622998442d4b690a2512a5e555413b641 Mon Sep 17 00:00:00 2001 From: soyabeen Date: Wed, 6 Nov 2019 14:19:49 +0100 Subject: [PATCH 29/36] Introduce rounding to submission evaluation #384. --- .../evaluation/evaluator/CodeEvaluator.java | 327 +++++++++--------- .../evaluator/MultipleChoiceEvaluator.java | 1 + .../evaluator/SingleChoiceEvaluator.java | 1 + .../evaluation/evaluator/TextEvaluator.java | 1 + .../student/model/SubmissionEvaluation.java | 5 +- 5 files changed, 171 insertions(+), 164 deletions(-) diff --git a/src/main/java/ch/uzh/ifi/access/student/evaluation/evaluator/CodeEvaluator.java b/src/main/java/ch/uzh/ifi/access/student/evaluation/evaluator/CodeEvaluator.java index 73413877..9cb7d891 100644 --- a/src/main/java/ch/uzh/ifi/access/student/evaluation/evaluator/CodeEvaluator.java +++ b/src/main/java/ch/uzh/ifi/access/student/evaluation/evaluator/CodeEvaluator.java @@ -1,175 +1,176 @@ package ch.uzh.ifi.access.student.evaluation.evaluator; -import java.util.ArrayList; -import java.util.Arrays; -import java.util.List; -import java.util.regex.Matcher; -import java.util.regex.Pattern; - -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; -import org.springframework.util.Assert; -import org.springframework.util.StringUtils; - import ch.uzh.ifi.access.course.model.Exercise; import ch.uzh.ifi.access.course.model.ExerciseType; import ch.uzh.ifi.access.student.model.CodeSubmission; import ch.uzh.ifi.access.student.model.StudentSubmission; import ch.uzh.ifi.access.student.model.SubmissionEvaluation; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; +import org.springframework.util.Assert; +import org.springframework.util.StringUtils; + +import java.util.ArrayList; +import java.util.Arrays; +import java.util.List; +import java.util.regex.Matcher; +import java.util.regex.Pattern; public class CodeEvaluator implements StudentSubmissionEvaluator { - private static final Logger logger = LoggerFactory.getLogger(CodeEvaluator.class); - - private static final String HINT_ANNOTATION = "@@"; - private static final String HINT_PATTERN = "^Assertion.*?:.*?(" + HINT_ANNOTATION + ".*?" + HINT_ANNOTATION + ")$"; - private static final String LAST_CRASH_PATTERN = "^(.*?Error):.*?"; - - private static final String PYTHON_ASSERTION_ERROR = "AssertionError"; - static final String TEST_FAILED_WITHOUT_HINTS = "Test failed without solution hints"; - - private final String runNTestPattern = "^Ran (\\d++) test.*"; - private final String nokNTestPattern = "^FAILED \\p{Punct}(failures|errors)=(\\d++)\\p{Punct}.*"; - - private Pattern hintPattern; - private Pattern crashPattern; - - private Pattern failedTestPattern; - - public CodeEvaluator() { - this.hintPattern = Pattern.compile(HINT_PATTERN, Pattern.MULTILINE | Pattern.DOTALL); - this.crashPattern = Pattern.compile(LAST_CRASH_PATTERN, Pattern.MULTILINE); - this.failedTestPattern = Pattern.compile(nokNTestPattern, Pattern.MULTILINE); - } - - @Override - public SubmissionEvaluation evaluate(StudentSubmission submission, Exercise exercise) { - validate(submission, exercise); - CodeSubmission codeSub = (CodeSubmission) submission; - - String log = codeSub.getConsole().getEvalLog(); - - SubmissionEvaluation.Points testResults = parseScoreFromLog(log); - List hints = testResults.isEverythingCorrect() ? new ArrayList() : parseHintsFromLog(log); - - return SubmissionEvaluation.builder().points(testResults).maxScore(exercise.getMaxScore()).hints(hints).build(); - } - - public List parseHintsFromLog(String evalLog) { - List hints = new ArrayList<>(); - - Matcher matcher = hintPattern.matcher(evalLog); - while (matcher.find()) { - String possibleHint = matcher.group(1); - if (!StringUtils.isEmpty(possibleHint) && possibleHint.contains(HINT_ANNOTATION)) { - hints.add(possibleHint.replace(HINT_ANNOTATION, "")); - } - } - - boolean hasFailedTests = failedTestPattern.matcher(evalLog).find(); - if (hints.isEmpty() && hasFailedTests) { - matcher = crashPattern.matcher(evalLog); - - String lastCrash = null; - while (matcher.find()) { - String error = matcher.group(1); - - if (!error.equals(PYTHON_ASSERTION_ERROR)) { - lastCrash = error; - } - } - if (lastCrash != null) { - hints.add("Error during execution: " + lastCrash); - } - - if (hints.isEmpty()) { - hints.add(TEST_FAILED_WITHOUT_HINTS); - } - } - - if (hints.isEmpty()) { - String[] lines = evalLog.split("\n"); - if (lines.length > 0) { - String lastLine = lines[lines.length - 1]; - int idxColon = lastLine.indexOf(':'); - if (idxColon != -1) { - String everythingBeforeColon = lastLine.substring(0, idxColon).trim(); - hints.add("Error during import: " + everythingBeforeColon); - } - } - } - - if (hints.isEmpty()) { - hints.add("No hint could be provided. This is likely caused by a crash during the execution."); - } - - return hints; - } - - private SubmissionEvaluation.Points parseScoreFromLog(String log) { - int points = 0; - int nrOfTest = -1; - - if (log != null && !log.trim().isEmpty()) { - List lines = Arrays.asList(log.split("\n")); - if (lines.size() >= 3) { - String resultLine = lines.get(lines.size() - 1); - - nrOfTest = extractNrOfTests(lines.get(lines.size() - 3)); - - if (resultLine.startsWith("OK")) { - points = nrOfTest; - } else if (resultLine.startsWith("FAILED")) { - points = nrOfTest - extractNrOfNOKTests(resultLine); - } - } else { - points = 0; - nrOfTest = 1; - logger.info("Log is too short, likely not a valid test output."); - } - } else { - logger.info("No console log to evaluate."); - } - - return new SubmissionEvaluation.Points(points, nrOfTest); - } - - private int extractNrOfTests(String line) { - int nrTests = 0; - Pattern p = Pattern.compile(runNTestPattern); - Matcher m = p.matcher(line); - if (m.find()) { - // group0 = line - // group1 = nr of tests - String group1 = m.group(1); - nrTests = Integer.parseInt(group1); - } - logger.debug(String.format("Exracted nr of test (%s) from line: %s", nrTests, line)); - return nrTests; - } - - private int extractNrOfNOKTests(String line) { - int nrTests = 0; - Matcher m = failedTestPattern.matcher(line); - if (m.find()) { - // group0 = line - // group1 = failures / errors - // group2 = nr of tests - String nrOfTests = m.group(2); - nrTests = Integer.parseInt(nrOfTests); - } - logger.debug(String.format("Exracted nr of NOK tests (%s) from line: %s", nrTests, line)); - return nrTests; - } - - private void validate(StudentSubmission submission, Exercise exercise) throws IllegalArgumentException { - Assert.notNull(submission, "Submission object for evaluation cannot be null."); - Assert.isInstanceOf(CodeSubmission.class, submission); - - Assert.notNull(exercise, "Exercise object for evaluation cannot be null."); - Assert.isTrue(exercise.getType().isCodeType(), - String.format("Exercise object for evaluation must be of type %s or %s", ExerciseType.code, - ExerciseType.codeSnippet)); - } + static final String TEST_FAILED_WITHOUT_HINTS = "Test failed without solution hints"; + private static final Logger logger = LoggerFactory.getLogger(CodeEvaluator.class); + private static final String HINT_ANNOTATION = "@@"; + private static final String HINT_PATTERN = "^Assertion.*?:.*?(" + HINT_ANNOTATION + ".*?" + HINT_ANNOTATION + ")$"; + private static final String LAST_CRASH_PATTERN = "^(.*?Error):.*?"; + private static final String PYTHON_ASSERTION_ERROR = "AssertionError"; + private final String runNTestPattern = "^Ran (\\d++) test.*"; + private final String nokNTestPattern = "^FAILED \\p{Punct}(failures|errors)=(\\d++)\\p{Punct}.*"; + + private Pattern hintPattern; + private Pattern crashPattern; + + private Pattern failedTestPattern; + + public CodeEvaluator() { + this.hintPattern = Pattern.compile(HINT_PATTERN, Pattern.MULTILINE | Pattern.DOTALL); + this.crashPattern = Pattern.compile(LAST_CRASH_PATTERN, Pattern.MULTILINE); + this.failedTestPattern = Pattern.compile(nokNTestPattern, Pattern.MULTILINE); + } + + @Override + public SubmissionEvaluation evaluate(StudentSubmission submission, Exercise exercise) { + validate(submission, exercise); + CodeSubmission codeSub = (CodeSubmission) submission; + + String log = codeSub.getConsole().getEvalLog(); + + SubmissionEvaluation.Points testResults = parseScoreFromLog(log); + List hints = testResults.isEverythingCorrect() ? new ArrayList() : parseHintsFromLog(log); + + return SubmissionEvaluation.builder() + .rounding(exercise.getRounding()) + .points(testResults) + .maxScore(exercise.getMaxScore()) + .hints(hints) + .build(); + } + + public List parseHintsFromLog(String evalLog) { + List hints = new ArrayList<>(); + + Matcher matcher = hintPattern.matcher(evalLog); + while (matcher.find()) { + String possibleHint = matcher.group(1); + if (!StringUtils.isEmpty(possibleHint) && possibleHint.contains(HINT_ANNOTATION)) { + hints.add(possibleHint.replace(HINT_ANNOTATION, "")); + } + } + + boolean hasFailedTests = failedTestPattern.matcher(evalLog).find(); + if (hints.isEmpty() && hasFailedTests) { + matcher = crashPattern.matcher(evalLog); + + String lastCrash = null; + while (matcher.find()) { + String error = matcher.group(1); + + if (!error.equals(PYTHON_ASSERTION_ERROR)) { + lastCrash = error; + } + } + if (lastCrash != null) { + hints.add("Error during execution: " + lastCrash); + } + + if (hints.isEmpty()) { + hints.add(TEST_FAILED_WITHOUT_HINTS); + } + } + + if (hints.isEmpty()) { + String[] lines = evalLog.split("\n"); + if (lines.length > 0) { + String lastLine = lines[lines.length - 1]; + int idxColon = lastLine.indexOf(':'); + if (idxColon != -1) { + String everythingBeforeColon = lastLine.substring(0, idxColon).trim(); + hints.add("Error during import: " + everythingBeforeColon); + } + } + } + + if (hints.isEmpty()) { + hints.add("No hint could be provided. This is likely caused by a crash during the execution."); + } + + return hints; + } + + private SubmissionEvaluation.Points parseScoreFromLog(String log) { + int points = 0; + int nrOfTest = -1; + + if (log != null && !log.trim().isEmpty()) { + List lines = Arrays.asList(log.split("\n")); + if (lines.size() >= 3) { + String resultLine = lines.get(lines.size() - 1); + + nrOfTest = extractNrOfTests(lines.get(lines.size() - 3)); + + if (resultLine.startsWith("OK")) { + points = nrOfTest; + } else if (resultLine.startsWith("FAILED")) { + points = nrOfTest - extractNrOfNOKTests(resultLine); + } + } else { + points = 0; + nrOfTest = 1; + logger.info("Log is too short, likely not a valid test output."); + } + } else { + logger.info("No console log to evaluate."); + } + + return new SubmissionEvaluation.Points(points, nrOfTest); + } + + private int extractNrOfTests(String line) { + int nrTests = 0; + Pattern p = Pattern.compile(runNTestPattern); + Matcher m = p.matcher(line); + if (m.find()) { + // group0 = line + // group1 = nr of tests + String group1 = m.group(1); + nrTests = Integer.parseInt(group1); + } + logger.debug(String.format("Exracted nr of test (%s) from line: %s", nrTests, line)); + return nrTests; + } + + private int extractNrOfNOKTests(String line) { + int nrTests = 0; + Matcher m = failedTestPattern.matcher(line); + if (m.find()) { + // group0 = line + // group1 = failures / errors + // group2 = nr of tests + String nrOfTests = m.group(2); + nrTests = Integer.parseInt(nrOfTests); + } + logger.debug(String.format("Exracted nr of NOK tests (%s) from line: %s", nrTests, line)); + return nrTests; + } + + private void validate(StudentSubmission submission, Exercise exercise) throws IllegalArgumentException { + Assert.notNull(submission, "Submission object for evaluation cannot be null."); + Assert.isInstanceOf(CodeSubmission.class, submission); + + Assert.notNull(exercise, "Exercise object for evaluation cannot be null."); + Assert.isTrue(exercise.getType().isCodeType(), + String.format("Exercise object for evaluation must be of type %s or %s", ExerciseType.code, + ExerciseType.codeSnippet)); + } } diff --git a/src/main/java/ch/uzh/ifi/access/student/evaluation/evaluator/MultipleChoiceEvaluator.java b/src/main/java/ch/uzh/ifi/access/student/evaluation/evaluator/MultipleChoiceEvaluator.java index 8e88f2ee..fea77a04 100644 --- a/src/main/java/ch/uzh/ifi/access/student/evaluation/evaluator/MultipleChoiceEvaluator.java +++ b/src/main/java/ch/uzh/ifi/access/student/evaluation/evaluator/MultipleChoiceEvaluator.java @@ -30,6 +30,7 @@ public SubmissionEvaluation evaluate(StudentSubmission submission, Exercise exer var hints = points < exercise.getMaxScore() ? exercise.getHints() : null; return SubmissionEvaluation.builder() + .rounding(exercise.getRounding()) .points(new SubmissionEvaluation.Points(points, solution.size())) .maxScore(exercise.getMaxScore()) .timestamp(Instant.now()) diff --git a/src/main/java/ch/uzh/ifi/access/student/evaluation/evaluator/SingleChoiceEvaluator.java b/src/main/java/ch/uzh/ifi/access/student/evaluation/evaluator/SingleChoiceEvaluator.java index 936fd85d..7b402d81 100644 --- a/src/main/java/ch/uzh/ifi/access/student/evaluation/evaluator/SingleChoiceEvaluator.java +++ b/src/main/java/ch/uzh/ifi/access/student/evaluation/evaluator/SingleChoiceEvaluator.java @@ -24,6 +24,7 @@ public SubmissionEvaluation evaluate(StudentSubmission submission, Exercise exer var hints = point != 1 ? exercise.getHints() : null; return SubmissionEvaluation.builder() + .rounding(exercise.getRounding()) .points(new SubmissionEvaluation.Points(point, 1)) .maxScore(exercise.getMaxScore()) .timestamp(Instant.now()) diff --git a/src/main/java/ch/uzh/ifi/access/student/evaluation/evaluator/TextEvaluator.java b/src/main/java/ch/uzh/ifi/access/student/evaluation/evaluator/TextEvaluator.java index a2d755bb..93fd2465 100644 --- a/src/main/java/ch/uzh/ifi/access/student/evaluation/evaluator/TextEvaluator.java +++ b/src/main/java/ch/uzh/ifi/access/student/evaluation/evaluator/TextEvaluator.java @@ -30,6 +30,7 @@ public SubmissionEvaluation evaluate(StudentSubmission submission, Exercise exer } return SubmissionEvaluation.builder() + .rounding(exercise.getRounding()) .points(new SubmissionEvaluation.Points(0, 1)) .maxScore(exercise.getMaxScore()) .timestamp(Instant.now()) diff --git a/src/main/java/ch/uzh/ifi/access/student/model/SubmissionEvaluation.java b/src/main/java/ch/uzh/ifi/access/student/model/SubmissionEvaluation.java index 00480415..80f877dd 100644 --- a/src/main/java/ch/uzh/ifi/access/student/model/SubmissionEvaluation.java +++ b/src/main/java/ch/uzh/ifi/access/student/model/SubmissionEvaluation.java @@ -1,5 +1,6 @@ package ch.uzh.ifi.access.student.model; +import ch.uzh.ifi.access.course.model.Rounding; import com.fasterxml.jackson.annotation.JsonProperty; import lombok.*; @@ -14,7 +15,7 @@ public class SubmissionEvaluation { public static SubmissionEvaluation NO_SUBMISSION = new SubmissionEvaluation(new Points(0, 0), 0, Instant.MIN, - Collections.emptyList()); + Collections.emptyList(), Rounding.DEFAULT); private Points points; @@ -24,6 +25,8 @@ public class SubmissionEvaluation { private List hints; + private Rounding rounding; + @JsonProperty public boolean hasSubmitted() { return !NO_SUBMISSION.equals(this); From f72915110fafaa81d42ebe427aa3adba9f130557 Mon Sep 17 00:00:00 2001 From: soyabeen Date: Wed, 6 Nov 2019 14:23:48 +0100 Subject: [PATCH 30/36] MaxScore is a double #384. --- .../java/ch/uzh/ifi/access/student/dto/AssignmentResults.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/ch/uzh/ifi/access/student/dto/AssignmentResults.java b/src/main/java/ch/uzh/ifi/access/student/dto/AssignmentResults.java index 8dd30c5f..c575b408 100644 --- a/src/main/java/ch/uzh/ifi/access/student/dto/AssignmentResults.java +++ b/src/main/java/ch/uzh/ifi/access/student/dto/AssignmentResults.java @@ -16,7 +16,7 @@ public class AssignmentResults { private String userId; private String assignmentId; - private int maxScore; + private double maxScore; private List gradedSubmissions; public double getStudentScore() { From 2135fe9506e1f930a08ce4345029b0bf01c5ca66 Mon Sep 17 00:00:00 2001 From: soyabeen Date: Wed, 6 Nov 2019 14:34:15 +0100 Subject: [PATCH 31/36] Call new rounding in evaluation #384. --- .../ch/uzh/ifi/access/course/model/Rounding.java | 15 ++++++++++++++- .../student/model/SubmissionEvaluation.java | 2 +- 2 files changed, 15 insertions(+), 2 deletions(-) diff --git a/src/main/java/ch/uzh/ifi/access/course/model/Rounding.java b/src/main/java/ch/uzh/ifi/access/course/model/Rounding.java index 5866ab83..f5a86ecc 100644 --- a/src/main/java/ch/uzh/ifi/access/course/model/Rounding.java +++ b/src/main/java/ch/uzh/ifi/access/course/model/Rounding.java @@ -1,5 +1,8 @@ package ch.uzh.ifi.access.course.model; +import lombok.Data; + +@Data public class Rounding { public static final Rounding DEFAULT = new Rounding(Rounding.Strategy.QUARTER_UP, 2); @@ -14,7 +17,17 @@ public Rounding(Strategy strategy, int precision) { this.precision = precision; } - public enum Strategy {UP, DOWN, HALP_UP, HALF_DOWN, QUARTER_UP, QUARTER_DOWN} + public double round(double value){ + return strategy.round(value, precision); + } + + public enum Strategy { + UP, DOWN, HALP_UP, HALF_DOWN, QUARTER_UP, QUARTER_DOWN; + double round(double unroundedValue, int precision) { + return 2.0d; + } + + } } diff --git a/src/main/java/ch/uzh/ifi/access/student/model/SubmissionEvaluation.java b/src/main/java/ch/uzh/ifi/access/student/model/SubmissionEvaluation.java index 80f877dd..f61b26fe 100644 --- a/src/main/java/ch/uzh/ifi/access/student/model/SubmissionEvaluation.java +++ b/src/main/java/ch/uzh/ifi/access/student/model/SubmissionEvaluation.java @@ -36,7 +36,7 @@ public double getScore() { if (points.getMax() == 0) { return 0.0; } - return Math.round((points.getCorrect() / (double) points.getMax() * maxScore) * 4) / 4d; + return rounding.round((points.getCorrect() / (double) points.getMax() * maxScore)); } public List getHints() { From 388f41520510ebbfbfe483f3be6a80de18e503c9 Mon Sep 17 00:00:00 2001 From: Alexander Hofmann Date: Wed, 6 Nov 2019 15:31:44 +0100 Subject: [PATCH 32/36] Add tests and fix tests --- .../uzh/ifi/access/course/model/Rounding.java | 54 ++++++++++-- .../student/model/SubmissionEvaluation.java | 62 +++++++------- .../ifi/access/course/model/RoundingTest.java | 85 +++++++++++++++++++ .../evaluator/CodeEvaluatorTest.java | 19 ++--- .../service/AdminSubmissionServiceTest.java | 2 +- 5 files changed, 174 insertions(+), 48 deletions(-) create mode 100644 src/test/java/ch/uzh/ifi/access/course/model/RoundingTest.java diff --git a/src/main/java/ch/uzh/ifi/access/course/model/Rounding.java b/src/main/java/ch/uzh/ifi/access/course/model/Rounding.java index f5a86ecc..ee1763f0 100644 --- a/src/main/java/ch/uzh/ifi/access/course/model/Rounding.java +++ b/src/main/java/ch/uzh/ifi/access/course/model/Rounding.java @@ -2,32 +2,72 @@ import lombok.Data; +import java.io.Serializable; +import java.math.BigDecimal; +import java.math.RoundingMode; +import java.util.function.BiFunction; + @Data -public class Rounding { +public class Rounding implements Serializable { - public static final Rounding DEFAULT = new Rounding(Rounding.Strategy.QUARTER_UP, 2); + public static final Rounding DEFAULT = new Rounding(Strategy.QUARTER_UP, 0); private Strategy strategy; private int precision; - public Rounding() { } + public Rounding() { + } public Rounding(Strategy strategy, int precision) { this.strategy = strategy; this.precision = precision; } - public double round(double value){ + public double round(double value) { return strategy.round(value, precision); } public enum Strategy { - UP, DOWN, HALP_UP, HALF_DOWN, QUARTER_UP, QUARTER_DOWN; + UP(Strategy::up), + DOWN(Strategy::down), + HALP_UP(Strategy::halfUp), + HALF_DOWN(Strategy::halfDown), + QUARTER_UP(Strategy::quarterUp), + QUARTER_DOWN(Strategy::quarterDown); + + private BiFunction algorithm; + + Strategy(BiFunction algorithm) { + this.algorithm = algorithm; + } + + double round(double unroundedValue, int precision) { + return algorithm.apply(unroundedValue, precision); + } + + static double up(Double unroundedValue, Integer precision) { + return new BigDecimal(unroundedValue).setScale(precision, RoundingMode.HALF_UP).doubleValue(); + } + + static double down(Double unroundedValue, Integer precision) { + return new BigDecimal(unroundedValue).setScale(precision, RoundingMode.HALF_DOWN).doubleValue(); + } + + static double halfUp(double unroundedValue, Integer precision) { + return (double) Math.round(unroundedValue * 2) / 2; + } + + static double halfDown(double unroundedValue, Integer precision) { + return Math.floor(unroundedValue * 2) / 2; + } - double round(double unroundedValue, int precision) { - return 2.0d; + static double quarterUp(double unroundedValue, Integer precision) { + return (double) Math.round(unroundedValue * 4) / 4; } + static double quarterDown(double unroundedValue, Integer precision) { + return Math.floor(unroundedValue * 4) / 4; + } } } diff --git a/src/main/java/ch/uzh/ifi/access/student/model/SubmissionEvaluation.java b/src/main/java/ch/uzh/ifi/access/student/model/SubmissionEvaluation.java index f61b26fe..26e53133 100644 --- a/src/main/java/ch/uzh/ifi/access/student/model/SubmissionEvaluation.java +++ b/src/main/java/ch/uzh/ifi/access/student/model/SubmissionEvaluation.java @@ -14,44 +14,46 @@ @Builder public class SubmissionEvaluation { - public static SubmissionEvaluation NO_SUBMISSION = new SubmissionEvaluation(new Points(0, 0), 0, Instant.MIN, - Collections.emptyList(), Rounding.DEFAULT); + public static SubmissionEvaluation NO_SUBMISSION = new SubmissionEvaluation(new Points(0, 0), 0, Instant.MIN, + Collections.emptyList(), Rounding.DEFAULT); - private Points points; + private Points points; - private double maxScore; + private double maxScore; - private Instant timestamp; + private Instant timestamp; - private List hints; + private List hints; - private Rounding rounding; + private Rounding rounding; - @JsonProperty - public boolean hasSubmitted() { - return !NO_SUBMISSION.equals(this); - } + @JsonProperty + public boolean hasSubmitted() { + return !NO_SUBMISSION.equals(this); + } - public double getScore() { - if (points.getMax() == 0) { - return 0.0; - } - return rounding.round((points.getCorrect() / (double) points.getMax() * maxScore)); - } + public double getScore() { + if (points.getMax() == 0) { + return 0.0; + } else if (rounding == null) { + return Rounding.DEFAULT.round((points.getCorrect() / (double) points.getMax() * maxScore)); + } + return rounding.round((points.getCorrect() / (double) points.getMax() * maxScore)); + } - public List getHints() { - return hints != null && hints.size() > 1 ? List.of(hints.get(0)) : hints; - } + public List getHints() { + return hints != null && hints.size() > 1 ? List.of(hints.get(0)) : hints; + } - @Data - @AllArgsConstructor - @NoArgsConstructor - public static class Points { - private int correct; - private int max; + @Data + @AllArgsConstructor + @NoArgsConstructor + public static class Points { + private int correct; + private int max; - public boolean isEverythingCorrect() { - return correct == max; - } - } + public boolean isEverythingCorrect() { + return correct == max; + } + } } diff --git a/src/test/java/ch/uzh/ifi/access/course/model/RoundingTest.java b/src/test/java/ch/uzh/ifi/access/course/model/RoundingTest.java new file mode 100644 index 00000000..48385cfa --- /dev/null +++ b/src/test/java/ch/uzh/ifi/access/course/model/RoundingTest.java @@ -0,0 +1,85 @@ +package ch.uzh.ifi.access.course.model; + +import org.assertj.core.api.Assertions; +import org.junit.Test; + +public class RoundingTest { + + @Test + public void defaultRounding() { + double val = 1.375412; + Assertions.assertThat(Rounding.DEFAULT.round(val)).isEqualTo(1.5); + + val = 1.305412; + Assertions.assertThat(Rounding.DEFAULT.round(val)).isEqualTo(1.25); + } + + @Test + public void halfUp() { + double val = 1.375412; + Assertions.assertThat(new Rounding(Rounding.Strategy.HALP_UP, 2).round(val)).isEqualTo(1.5); + + val = 1.245412; + Assertions.assertThat(new Rounding(Rounding.Strategy.HALP_UP, 2).round(val)).isEqualTo(1.); + } + + @Test + public void halfDown() { + double val = 1.375412; + Assertions.assertThat(new Rounding(Rounding.Strategy.HALF_DOWN, 2).round(val)).isEqualTo(1.); + + val = 1.245412; + Assertions.assertThat(new Rounding(Rounding.Strategy.HALF_DOWN, 2).round(val)).isEqualTo(1.); + } + + @Test + public void quarterUp() { + double val = 1.375412; + Assertions.assertThat(new Rounding(Rounding.Strategy.QUARTER_UP, 2).round(val)).isEqualTo(1.5); + + val = 1.245412; + Assertions.assertThat(new Rounding(Rounding.Strategy.QUARTER_UP, 2).round(val)).isEqualTo(1.25); + } + + @Test + public void quarterDown() { + double val = 1.375412; + Assertions.assertThat(new Rounding(Rounding.Strategy.QUARTER_DOWN, 2).round(val)).isEqualTo(1.25); + + val = 1.245412; + Assertions.assertThat(new Rounding(Rounding.Strategy.QUARTER_DOWN, 2).round(val)).isEqualTo(1.); + } + + @Test + public void up() { + double val = 1.375; + Assertions.assertThat(new Rounding(Rounding.Strategy.UP, 2).round(val)).isEqualTo(1.38); + + val = 1.3751; + Assertions.assertThat(new Rounding(Rounding.Strategy.UP, 2).round(val)).isEqualTo(1.38); + + val = 1.244; + Assertions.assertThat(new Rounding(Rounding.Strategy.UP, 2).round(val)).isEqualTo(1.24); + } + + @Test + public void down() { + double val = 1.375; + Assertions.assertThat(new Rounding(Rounding.Strategy.DOWN, 2).round(val)).isEqualTo(1.37); + + val = 1.3751; + Assertions.assertThat(new Rounding(Rounding.Strategy.DOWN, 2).round(val)).isEqualTo(1.38); + + val = 1.244; + Assertions.assertThat(new Rounding(Rounding.Strategy.DOWN, 2).round(val)).isEqualTo(1.24); + } + + @Test + public void higherPrecision() { + double val = 1.37517; + Assertions.assertThat(new Rounding(Rounding.Strategy.UP, 4).round(val)).isEqualTo(1.3752); + + val = 1.3751; + Assertions.assertThat(new Rounding(Rounding.Strategy.UP, 4).round(val)).isEqualTo(val); + } +} \ No newline at end of file diff --git a/src/test/java/ch/uzh/ifi/access/student/evaluation/evaluator/CodeEvaluatorTest.java b/src/test/java/ch/uzh/ifi/access/student/evaluation/evaluator/CodeEvaluatorTest.java index 5725a4ff..d3f787b8 100644 --- a/src/test/java/ch/uzh/ifi/access/student/evaluation/evaluator/CodeEvaluatorTest.java +++ b/src/test/java/ch/uzh/ifi/access/student/evaluation/evaluator/CodeEvaluatorTest.java @@ -1,19 +1,18 @@ package ch.uzh.ifi.access.student.evaluation.evaluator; -import static ch.uzh.ifi.access.student.evaluation.evaluator.CodeEvaluator.TEST_FAILED_WITHOUT_HINTS; -import static com.spotify.docker.client.shaded.com.google.common.collect.Lists.newArrayList; -import static org.junit.Assert.assertEquals; - -import java.util.List; - -import org.junit.Before; -import org.junit.Test; - import ch.uzh.ifi.access.course.model.Exercise; import ch.uzh.ifi.access.course.model.ExerciseType; import ch.uzh.ifi.access.student.model.CodeSubmission; import ch.uzh.ifi.access.student.model.ExecResult; import ch.uzh.ifi.access.student.model.SubmissionEvaluation; +import org.junit.Before; +import org.junit.Test; + +import java.util.List; + +import static ch.uzh.ifi.access.student.evaluation.evaluator.CodeEvaluator.TEST_FAILED_WITHOUT_HINTS; +import static com.spotify.docker.client.shaded.com.google.common.collect.Lists.newArrayList; +import static org.junit.Assert.assertEquals; public class CodeEvaluatorTest { @@ -138,7 +137,7 @@ public void outOfMemoryHasEmptyEvalLog() { SubmissionEvaluation grade = evaluate(""); assertEquals(0, grade.getPoints().getCorrect()); - assertEquals(exercise.getMaxScore(), grade.getMaxScore()); + assertEquals(exercise.getMaxScore(), grade.getMaxScore(), 0.1); } @Test diff --git a/src/test/java/ch/uzh/ifi/access/student/service/AdminSubmissionServiceTest.java b/src/test/java/ch/uzh/ifi/access/student/service/AdminSubmissionServiceTest.java index a19eb3bd..261e365e 100644 --- a/src/test/java/ch/uzh/ifi/access/student/service/AdminSubmissionServiceTest.java +++ b/src/test/java/ch/uzh/ifi/access/student/service/AdminSubmissionServiceTest.java @@ -148,7 +148,7 @@ private void setUserForSubmission(List submissions, String us submissions.forEach(submission -> submission.setUserId(userId)); } - private void setResultForSubmission(StudentSubmission submission, SubmissionEvaluation.Points points, int maxScore ) { + private void setResultForSubmission(StudentSubmission submission, SubmissionEvaluation.Points points, double maxScore ) { submission.setResult( SubmissionEvaluation.builder().points(points).maxScore( maxScore).timestamp(Instant.now()).build()); } } \ No newline at end of file From 738678db0ed854fa75f26f8769d2905a1436f585 Mon Sep 17 00:00:00 2001 From: Alexander Hofmann Date: Wed, 6 Nov 2019 15:42:45 +0100 Subject: [PATCH 33/36] Fix typo --- src/main/java/ch/uzh/ifi/access/course/model/Rounding.java | 2 +- .../java/ch/uzh/ifi/access/course/model/RoundingTest.java | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/main/java/ch/uzh/ifi/access/course/model/Rounding.java b/src/main/java/ch/uzh/ifi/access/course/model/Rounding.java index ee1763f0..adbfad62 100644 --- a/src/main/java/ch/uzh/ifi/access/course/model/Rounding.java +++ b/src/main/java/ch/uzh/ifi/access/course/model/Rounding.java @@ -30,7 +30,7 @@ public double round(double value) { public enum Strategy { UP(Strategy::up), DOWN(Strategy::down), - HALP_UP(Strategy::halfUp), + HALF_UP(Strategy::halfUp), HALF_DOWN(Strategy::halfDown), QUARTER_UP(Strategy::quarterUp), QUARTER_DOWN(Strategy::quarterDown); diff --git a/src/test/java/ch/uzh/ifi/access/course/model/RoundingTest.java b/src/test/java/ch/uzh/ifi/access/course/model/RoundingTest.java index 48385cfa..2a93a50d 100644 --- a/src/test/java/ch/uzh/ifi/access/course/model/RoundingTest.java +++ b/src/test/java/ch/uzh/ifi/access/course/model/RoundingTest.java @@ -17,10 +17,10 @@ public void defaultRounding() { @Test public void halfUp() { double val = 1.375412; - Assertions.assertThat(new Rounding(Rounding.Strategy.HALP_UP, 2).round(val)).isEqualTo(1.5); + Assertions.assertThat(new Rounding(Rounding.Strategy.HALF_UP, 2).round(val)).isEqualTo(1.5); val = 1.245412; - Assertions.assertThat(new Rounding(Rounding.Strategy.HALP_UP, 2).round(val)).isEqualTo(1.); + Assertions.assertThat(new Rounding(Rounding.Strategy.HALF_UP, 2).round(val)).isEqualTo(1.); } @Test From 4acbadddc9666b197b1de5c14b176972fdf04ea7 Mon Sep 17 00:00:00 2001 From: soyabeen Date: Wed, 6 Nov 2019 19:21:12 +0100 Subject: [PATCH 34/36] Use new rounding draft #384. --- .../uzh/ifi/access/course/model/Rounding.java | 62 +++++++------- .../ifi/access/course/model/RoundingTest.java | 82 ++++++++++--------- 2 files changed, 76 insertions(+), 68 deletions(-) diff --git a/src/main/java/ch/uzh/ifi/access/course/model/Rounding.java b/src/main/java/ch/uzh/ifi/access/course/model/Rounding.java index adbfad62..8c784f86 100644 --- a/src/main/java/ch/uzh/ifi/access/course/model/Rounding.java +++ b/src/main/java/ch/uzh/ifi/access/course/model/Rounding.java @@ -10,64 +10,68 @@ @Data public class Rounding implements Serializable { - public static final Rounding DEFAULT = new Rounding(Strategy.QUARTER_UP, 0); + public static final Rounding DEFAULT = new Rounding(Strategy.ROUND, 4); private Strategy strategy; + private int steps; private int precision; public Rounding() { } - public Rounding(Strategy strategy, int precision) { + public Rounding(Strategy strategy, int steps) { this.strategy = strategy; - this.precision = precision; + this.steps = steps; + this.precision = calcPrecision(steps); } public double round(double value) { - return strategy.round(value, precision); + return strategy.round(value, steps, precision); + } + + public int calcPrecision(Integer steps) { + if(steps == 1 ) { + return 0; + } + + String strinRep = Double.toString( 1/steps); + return strinRep.length() - strinRep.indexOf('.') - 1; } public enum Strategy { - UP(Strategy::up), - DOWN(Strategy::down), - HALF_UP(Strategy::halfUp), - HALF_DOWN(Strategy::halfDown), - QUARTER_UP(Strategy::quarterUp), - QUARTER_DOWN(Strategy::quarterDown); + CEILING(Strategy::ceil), + FLOOR(Strategy::floor), + ROUND(Strategy::roundUp); - private BiFunction algorithm; + private TriFunction algorithm; - Strategy(BiFunction algorithm) { + Strategy(TriFunction algorithm) { this.algorithm = algorithm; } - double round(double unroundedValue, int precision) { - return algorithm.apply(unroundedValue, precision); + double round(double unroundedValue, int steps, int precision) { + return algorithm.apply(unroundedValue, steps, precision); } - static double up(Double unroundedValue, Integer precision) { - return new BigDecimal(unroundedValue).setScale(precision, RoundingMode.HALF_UP).doubleValue(); - } + static double ceil(Double unroundedValue, Integer steps, Integer precision) { + // return new BigDecimal(unroundedValue*steps).setScale(precision, RoundingMode.CEILING).doubleValue()/steps; + return Math.ceil( unroundedValue*steps)/steps; - static double down(Double unroundedValue, Integer precision) { - return new BigDecimal(unroundedValue).setScale(precision, RoundingMode.HALF_DOWN).doubleValue(); } - static double halfUp(double unroundedValue, Integer precision) { - return (double) Math.round(unroundedValue * 2) / 2; + static double floor(Double unroundedValue, Integer steps, Integer precision) { + return Math.floor(unroundedValue * steps)/steps; } - static double halfDown(double unroundedValue, Integer precision) { - return Math.floor(unroundedValue * 2) / 2; + static double roundUp(double unroundedValue, Integer steps, Integer precision) { + return (double) Math.round(unroundedValue * steps) / steps; } - static double quarterUp(double unroundedValue, Integer precision) { - return (double) Math.round(unroundedValue * 4) / 4; - } + } - static double quarterDown(double unroundedValue, Integer precision) { - return Math.floor(unroundedValue * 4) / 4; - } + @FunctionalInterface + public interface TriFunction { + R apply(F f, S s, T t); } } diff --git a/src/test/java/ch/uzh/ifi/access/course/model/RoundingTest.java b/src/test/java/ch/uzh/ifi/access/course/model/RoundingTest.java index 2a93a50d..58aae16c 100644 --- a/src/test/java/ch/uzh/ifi/access/course/model/RoundingTest.java +++ b/src/test/java/ch/uzh/ifi/access/course/model/RoundingTest.java @@ -15,71 +15,75 @@ public void defaultRounding() { } @Test - public void halfUp() { - double val = 1.375412; - Assertions.assertThat(new Rounding(Rounding.Strategy.HALF_UP, 2).round(val)).isEqualTo(1.5); + public void roundUpStep2() { + double val = 1.38412; + Assertions.assertThat(new Rounding(Rounding.Strategy.ROUND, 2).round(val)).isEqualTo(1.5); - val = 1.245412; - Assertions.assertThat(new Rounding(Rounding.Strategy.HALF_UP, 2).round(val)).isEqualTo(1.); + val = 1.235412; + Assertions.assertThat(new Rounding(Rounding.Strategy.ROUND, 2).round(val)).isEqualTo(1); } @Test - public void halfDown() { - double val = 1.375412; - Assertions.assertThat(new Rounding(Rounding.Strategy.HALF_DOWN, 2).round(val)).isEqualTo(1.); + public void roundUpStep3() { + double val = 1.38412; + Assertions.assertThat(new Rounding(Rounding.Strategy.ROUND, 3).round(val)).isEqualTo(1.0 + 1d/3d); - val = 1.245412; - Assertions.assertThat(new Rounding(Rounding.Strategy.HALF_DOWN, 2).round(val)).isEqualTo(1.); + val = 1.125412; + Assertions.assertThat(new Rounding(Rounding.Strategy.ROUND, 3).round(val)).isEqualTo(1); } @Test - public void quarterUp() { - double val = 1.375412; - Assertions.assertThat(new Rounding(Rounding.Strategy.QUARTER_UP, 2).round(val)).isEqualTo(1.5); + public void roundUpStep4() { + double val = 1.38412; + Assertions.assertThat(new Rounding(Rounding.Strategy.ROUND, 4).round(val)).isEqualTo(1.5); - val = 1.245412; - Assertions.assertThat(new Rounding(Rounding.Strategy.QUARTER_UP, 2).round(val)).isEqualTo(1.25); + val = 1.126412; + Assertions.assertThat(new Rounding(Rounding.Strategy.ROUND, 4).round(val)).isEqualTo(1.25); + + val = 1.01412; + Assertions.assertThat(new Rounding(Rounding.Strategy.ROUND, 4).round(val)).isEqualTo(1); } @Test - public void quarterDown() { - double val = 1.375412; - Assertions.assertThat(new Rounding(Rounding.Strategy.QUARTER_DOWN, 2).round(val)).isEqualTo(1.25); + public void roundUpStep8() { + double val = 1.46412; + Assertions.assertThat(new Rounding(Rounding.Strategy.ROUND, 8).round(val)).isEqualTo(1.5); - val = 1.245412; - Assertions.assertThat(new Rounding(Rounding.Strategy.QUARTER_DOWN, 2).round(val)).isEqualTo(1.); + val = 1.125412; + Assertions.assertThat(new Rounding(Rounding.Strategy.ROUND, 8).round(val)).isEqualTo(1.125); + + val = 1.01412; + Assertions.assertThat(new Rounding(Rounding.Strategy.ROUND, 8).round(val)).isEqualTo(1); } @Test - public void up() { - double val = 1.375; - Assertions.assertThat(new Rounding(Rounding.Strategy.UP, 2).round(val)).isEqualTo(1.38); + public void roundUpStep10() { + double val = 1.46412; + Assertions.assertThat(new Rounding(Rounding.Strategy.ROUND, 10).round(val)).isEqualTo(1.5); - val = 1.3751; - Assertions.assertThat(new Rounding(Rounding.Strategy.UP, 2).round(val)).isEqualTo(1.38); + val = 1.125412; + Assertions.assertThat(new Rounding(Rounding.Strategy.ROUND, 10).round(val)).isEqualTo(1.1); - val = 1.244; - Assertions.assertThat(new Rounding(Rounding.Strategy.UP, 2).round(val)).isEqualTo(1.24); + val = 1.01412; + Assertions.assertThat(new Rounding(Rounding.Strategy.ROUND, 10).round(val)).isEqualTo(1); } @Test - public void down() { + public void up() { double val = 1.375; - Assertions.assertThat(new Rounding(Rounding.Strategy.DOWN, 2).round(val)).isEqualTo(1.37); + Assertions.assertThat(new Rounding(Rounding.Strategy.CEILING, 4).round(val)).isEqualTo(1.5); - val = 1.3751; - Assertions.assertThat(new Rounding(Rounding.Strategy.DOWN, 2).round(val)).isEqualTo(1.38); - - val = 1.244; - Assertions.assertThat(new Rounding(Rounding.Strategy.DOWN, 2).round(val)).isEqualTo(1.24); + val = 1.044; + Assertions.assertThat(new Rounding(Rounding.Strategy.CEILING, 2).round(val)).isEqualTo(1.5); } @Test - public void higherPrecision() { - double val = 1.37517; - Assertions.assertThat(new Rounding(Rounding.Strategy.UP, 4).round(val)).isEqualTo(1.3752); + public void down() { + double val = 1.375; + Assertions.assertThat(new Rounding(Rounding.Strategy.FLOOR, 4).round(val)).isEqualTo(1.25); - val = 1.3751; - Assertions.assertThat(new Rounding(Rounding.Strategy.UP, 4).round(val)).isEqualTo(val); + val = 1.244; + Assertions.assertThat(new Rounding(Rounding.Strategy.FLOOR, 2).round(val)).isEqualTo(1); } + } \ No newline at end of file From ac279270dc862515d5a293714d80092dfb54ba63 Mon Sep 17 00:00:00 2001 From: soyabeen Date: Wed, 6 Nov 2019 19:23:07 +0100 Subject: [PATCH 35/36] Beautify #384. --- .../uzh/ifi/access/course/model/Rounding.java | 37 +++++-------------- 1 file changed, 9 insertions(+), 28 deletions(-) diff --git a/src/main/java/ch/uzh/ifi/access/course/model/Rounding.java b/src/main/java/ch/uzh/ifi/access/course/model/Rounding.java index 8c784f86..c6c20a61 100644 --- a/src/main/java/ch/uzh/ifi/access/course/model/Rounding.java +++ b/src/main/java/ch/uzh/ifi/access/course/model/Rounding.java @@ -3,8 +3,6 @@ import lombok.Data; import java.io.Serializable; -import java.math.BigDecimal; -import java.math.RoundingMode; import java.util.function.BiFunction; @Data @@ -14,7 +12,6 @@ public class Rounding implements Serializable { private Strategy strategy; private int steps; - private int precision; public Rounding() { } @@ -22,56 +19,40 @@ public Rounding() { public Rounding(Strategy strategy, int steps) { this.strategy = strategy; this.steps = steps; - this.precision = calcPrecision(steps); } public double round(double value) { - return strategy.round(value, steps, precision); - } - - public int calcPrecision(Integer steps) { - if(steps == 1 ) { - return 0; - } - - String strinRep = Double.toString( 1/steps); - return strinRep.length() - strinRep.indexOf('.') - 1; + return strategy.round(value, steps); } public enum Strategy { + CEILING(Strategy::ceil), FLOOR(Strategy::floor), ROUND(Strategy::roundUp); - private TriFunction algorithm; + private BiFunction algorithm; - Strategy(TriFunction algorithm) { + Strategy(BiFunction algorithm) { this.algorithm = algorithm; } - double round(double unroundedValue, int steps, int precision) { - return algorithm.apply(unroundedValue, steps, precision); + double round(double unroundedValue, int steps) { + return algorithm.apply(unroundedValue, steps); } - static double ceil(Double unroundedValue, Integer steps, Integer precision) { - // return new BigDecimal(unroundedValue*steps).setScale(precision, RoundingMode.CEILING).doubleValue()/steps; + static double ceil(Double unroundedValue, Integer steps) { return Math.ceil( unroundedValue*steps)/steps; - } - static double floor(Double unroundedValue, Integer steps, Integer precision) { + static double floor(Double unroundedValue, Integer steps) { return Math.floor(unroundedValue * steps)/steps; } - static double roundUp(double unroundedValue, Integer steps, Integer precision) { + static double roundUp(double unroundedValue, Integer steps) { return (double) Math.round(unroundedValue * steps) / steps; } } - @FunctionalInterface - public interface TriFunction { - R apply(F f, S s, T t); - } - } From c88d06b499428bf837c117733da6aaa971f3762e Mon Sep 17 00:00:00 2001 From: Haeri Date: Thu, 7 Nov 2019 17:58:48 +0100 Subject: [PATCH 36/36] added Semester property --- .../java/ch/uzh/ifi/access/course/dto/CourseMetadataDTO.java | 2 ++ src/main/java/ch/uzh/ifi/access/course/model/Course.java | 5 +++-- .../java/ch/uzh/ifi/access/course/model/CourseConfig.java | 2 ++ 3 files changed, 7 insertions(+), 2 deletions(-) diff --git a/src/main/java/ch/uzh/ifi/access/course/dto/CourseMetadataDTO.java b/src/main/java/ch/uzh/ifi/access/course/dto/CourseMetadataDTO.java index 9f2d5a1c..841db678 100644 --- a/src/main/java/ch/uzh/ifi/access/course/dto/CourseMetadataDTO.java +++ b/src/main/java/ch/uzh/ifi/access/course/dto/CourseMetadataDTO.java @@ -17,6 +17,7 @@ public class CourseMetadataDTO { private String title; private String description; private String owner; + private String semester; private String gitHash; private ZonedDateTime startDate; private ZonedDateTime endDate; @@ -29,6 +30,7 @@ public CourseMetadataDTO(Course course) { this.title = course.getTitle(); this.description = course.getDescription(); this.owner = course.getOwner(); + this.semester = course.getSemester(); this.gitHash = course.getGitHash(); this.startDate = course.getStartDate(); this.endDate = course.getEndDate(); diff --git a/src/main/java/ch/uzh/ifi/access/course/model/Course.java b/src/main/java/ch/uzh/ifi/access/course/model/Course.java index 9f99dc6f..434e6895 100644 --- a/src/main/java/ch/uzh/ifi/access/course/model/Course.java +++ b/src/main/java/ch/uzh/ifi/access/course/model/Course.java @@ -35,8 +35,8 @@ public Course(String name) { } @Builder - public Course(String title, String description, String owner, ZonedDateTime startDate, ZonedDateTime endDate, List admins, List assistants, List students, String id, String gitHash, String gitURL, String directory, List assignments) { - super(title, description, owner, startDate, endDate, admins, assistants, students); + public Course(String title, String description, String owner, String semester, ZonedDateTime startDate, ZonedDateTime endDate, List admins, List assistants, List students, String id, String gitHash, String gitURL, String directory, List assignments) { + super(title, description, owner, semester, startDate, endDate, admins, assistants, students); this.id = id; this.gitHash = gitHash; this.gitURL = gitURL; @@ -49,6 +49,7 @@ public void set(CourseConfig other) { this.title = other.getTitle(); this.description = other.getDescription(); this.owner = other.getOwner(); + this.semester = other.getSemester(); this.startDate = other.getStartDate(); this.endDate = other.getEndDate(); this.admins = other.getAdmins(); diff --git a/src/main/java/ch/uzh/ifi/access/course/model/CourseConfig.java b/src/main/java/ch/uzh/ifi/access/course/model/CourseConfig.java index b7af50ee..7cbe7cc0 100644 --- a/src/main/java/ch/uzh/ifi/access/course/model/CourseConfig.java +++ b/src/main/java/ch/uzh/ifi/access/course/model/CourseConfig.java @@ -14,6 +14,7 @@ public class CourseConfig implements Serializable { protected String title; protected String description; protected String owner; + protected String semester; protected ZonedDateTime startDate; protected ZonedDateTime endDate; @@ -24,6 +25,7 @@ public class CourseConfig implements Serializable { public CourseConfig() { this.description = ""; this.owner = ""; + this.semester = ""; } } \ No newline at end of file