Skip to content

Commit

Permalink
[HWORKS-956] Sub-resources should not do work outside of the endpoint. (
Browse files Browse the repository at this point in the history
  • Loading branch information
ErmiasG committed May 16, 2024
1 parent f71dedb commit 96dbc7b
Show file tree
Hide file tree
Showing 100 changed files with 3,567 additions and 2,127 deletions.
146 changes: 146 additions & 0 deletions hopsworks-UT/src/test/java/io/hops/hopsworks/TestSubResources.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,146 @@
/*
* This file is part of Hopsworks
* Copyright (C) 2024, Hopsworks AB. All rights reserved
*
* Hopsworks is free software: you can redistribute it and/or modify it under the terms of
* the GNU Affero General Public License as published by the Free Software Foundation,
* either version 3 of the License, or (at your option) any later version.
*
* Hopsworks is distributed in the hope that it will be useful, but WITHOUT ANY WARRANTY;
* without even the implied warranty of MERCHANTABILITY or FITNESS FOR A PARTICULAR
* PURPOSE. See the GNU Affero General Public License for more details.
*
* You should have received a copy of the GNU Affero General Public License along with this program.
* If not, see <https://www.gnu.org/licenses/>.
*/
package io.hops.hopsworks;

import io.hops.hopsworks.common.dataset.util.DatasetPath;
import org.junit.Assert;
import org.junit.Before;
import org.junit.Test;
import org.reflections.Reflections;
import org.reflections.scanners.SubTypesScanner;
import org.reflections.scanners.TypeAnnotationsScanner;

import javax.enterprise.context.RequestScoped;
import javax.persistence.Entity;
import javax.ws.rs.Path;
import java.lang.reflect.Field;
import java.lang.reflect.Modifier;
import java.util.HashSet;
import java.util.Set;

import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertTrue;

/**
* This test will ensure that sub resources do not contain entity class properties.
* 1. Sub resources are request scoped and so should not cache anything.
* 2. If an entity class is passed to a sub resource that means it needs to be fetched from database before the sub
* resource is called. This will bypass any filters that are set to intercept methods on the sub resource.
* <p>
* So if we have a project entity in a sub resource, and we pass it from a parent resource the project entity will be
* fetched from database before the user, that is making the request, is authenticated and verified if he/she has access
* to the project.
*/
public class TestSubResources {
Set<Class<?>> subResourceClasses = new HashSet<>();
Set<Class<?>> resourceClasses = new HashSet<>();
Set<Class<?>> entityClasses = new HashSet<>();

@Before
public void beforeTest() {
Reflections reflections = new Reflections("io.hops.hopsworks", new SubTypesScanner(false),
new TypeAnnotationsScanner());
for (Class<?> c : reflections.getTypesAnnotatedWith(Path.class, true)) {
if (c.getName().startsWith("io.hops.hopsworks.api")) {
resourceClasses.add(c);
}
}
for (Class<?> c : reflections.getTypesAnnotatedWith(RequestScoped.class, true)) {
if (c.getName().startsWith("io.hops.hopsworks.api")) {
subResourceClasses.add(c);
}
}

for (Class<?> c : reflections.getTypesAnnotatedWith(Entity.class, true)) {
if (c.getName().startsWith("io.hops.hopsworks.persistence.entity")) {
entityClasses.add(c);
}
}

}

@Test
public void testIfClassesAreFound() {
System.out.println("SubResources: " + subResourceClasses.size());
assertFalse("Sub Resource classes should not be empty.", subResourceClasses.isEmpty());
System.out.println("Entity classes: " + entityClasses.size());
assertFalse("Entity classes should not be empty.", entityClasses.isEmpty());
}

@Test
public void testSubResourceAreFinal() {
for (Class<?> c : resourceClasses) {
if (!Modifier.isFinal(c.getModifiers())) {
System.out.println("Api Resources should be final. Inheriting a resource class will force the swagger " +
"documentation to be too general. Offending classes " + c.getName());
}
// assertTrue("Api Resources should be final. Inheriting a resource class will force the swagger " +
// "documentation to be too general. Offending classes " + c.getName(),
// Modifier.isFinal(c.getModifiers()));
}
for (Class<?> c : subResourceClasses) {
if (!Modifier.isFinal(c.getModifiers())) {
System.out.println("Api Resources should be final. Inheriting a resource class will force the swagger " +
"documentation to be too general. Offending classes " + c.getName());
}
// assertTrue("Api Resources should be final. Inheriting a resource class will force the swagger " +
// "documentation to be too general. Offending classes " + c.getName(),
// Modifier.isFinal(c.getModifiers()));
}
}

@Test
public void testEntityClassesInFields() {
for (Class<?> c : subResourceClasses) {
Set<Class<?>> entityClassesInFields = containsEntity(c);
if (!entityClassesInFields.isEmpty()) {
System.out.println("Class " + c.getName() + " contains entity class fields. ");
System.out.println(entityClassesInFields);
}
assertTrue("SubResources should not contain entity classes fields that query the database.",
entityClassesInFields.isEmpty());
}
}

@Test
public void testDatasetPathClassesInFields() {
for (Class<?> c : subResourceClasses) {
if (containsClass(c, DatasetPath.class)) {
System.out.println("Class " + c.getName() + " contains DatasetPath in fields. ");
Assert.fail("SubResources should not contain any Class that queries the database. Found DatasetPath");
}
}
}

private Set<Class<?>> containsEntity(Class<?> c) {
Set<Class<?>> entityClassesInFields = new HashSet<>();
for (Field field : c.getDeclaredFields()) {
if (entityClasses.contains(field.getType())) {
entityClassesInFields.add(field.getType());
}
}
return entityClassesInFields;
}

private boolean containsClass(Class<?> c, Class<?> c1) {
for (Field field : c.getDeclaredFields()) {
if (field.getType().equals(c1)) {
return true;
}
}
return false;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -17,14 +17,14 @@

import io.hops.hopsworks.api.filter.AllowedProjectRoles;
import io.hops.hopsworks.api.filter.Audience;
import io.hops.hopsworks.api.project.ProjectSubResource;
import io.hops.hopsworks.api.util.Pagination;
import io.hops.hopsworks.common.api.ResourceRequest;
import io.hops.hopsworks.common.dao.project.ProjectFacade;
import io.hops.hopsworks.common.project.ProjectController;
import io.hops.hopsworks.exceptions.ActivitiesException;
import io.hops.hopsworks.exceptions.ProjectException;
import io.hops.hopsworks.jwt.annotation.JWTRequired;
import io.hops.hopsworks.persistence.entity.project.Project;
import io.hops.hopsworks.restutils.RESTCodes;
import io.swagger.annotations.Api;
import io.swagger.annotations.ApiOperation;

Expand All @@ -42,53 +42,25 @@
import javax.ws.rs.core.Response;
import javax.ws.rs.core.SecurityContext;
import javax.ws.rs.core.UriInfo;
import java.util.logging.Level;
import java.util.logging.Logger;

@Api(value = "Project Activities Resource")
@RequestScoped
@TransactionAttribute(TransactionAttributeType.NEVER)
public class ProjectActivitiesResource {
public class ProjectActivitiesResource extends ProjectSubResource {

private static final Logger LOGGER = Logger.getLogger(ProjectActivitiesResource.class.getName());
@EJB
private ActivitiesBuilder activitiesBuilder;
@EJB
private ProjectFacade projectFacade;

private Integer projectId;
private String projectName;
private ProjectController projectController;

public ProjectActivitiesResource() {
}

public void setProjectId(Integer projectId) {
this.projectId = projectId;
}

public void setProjectName(String projectName) {
this.projectName = projectName;
}

private Project getProjectById() throws ProjectException {
Project project = projectFacade.find(this.projectId);
if (project == null) {
throw new ProjectException(RESTCodes.ProjectErrorCode.PROJECT_NOT_FOUND, Level.FINE, "projectId: " + projectId);
}
return project;
}

private Project getProjectByName() throws ProjectException {
Project project = projectFacade.findByName(this.projectName);
if (project == null) {
throw new ProjectException(RESTCodes.ProjectErrorCode.PROJECT_NOT_FOUND, Level.FINE, "projectName: " +
projectName);
}
return project;
}

private Project getProject() throws ProjectException {
return this.projectId != null ? getProjectById() : getProjectByName();
@Override
protected ProjectController getProjectController() {
return projectController;
}

@GET
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,12 +19,13 @@
import io.hops.hopsworks.api.filter.AllowedProjectRoles;
import io.hops.hopsworks.api.filter.Audience;
import io.hops.hopsworks.api.jwt.JWTHelper;
import io.hops.hopsworks.api.project.ProjectSubResource;
import io.hops.hopsworks.common.airflow.AirflowController;
import io.hops.hopsworks.common.airflow.AirflowDagDTO;
import io.hops.hopsworks.common.dao.project.ProjectFacade;
import io.hops.hopsworks.common.project.ProjectController;
import io.hops.hopsworks.exceptions.AirflowException;
import io.hops.hopsworks.exceptions.ProjectException;
import io.hops.hopsworks.jwt.annotation.JWTRequired;
import io.hops.hopsworks.persistence.entity.project.Project;
import io.hops.hopsworks.persistence.entity.user.Users;
import io.swagger.annotations.Api;
import io.swagger.annotations.ApiOperation;
Expand All @@ -45,31 +46,20 @@
@RequestScoped
@TransactionAttribute(TransactionAttributeType.NEVER)
@Api(value = "Airflow related endpoints")
public class AirflowService {
public class AirflowService extends ProjectSubResource {
@EJB
private ProjectFacade projectFacade;
private ProjectController projectController;
@EJB
private JWTHelper jwtHelper;
@EJB
private AirflowController airflowController;
private Integer projectId;
// No @EJB annotation for Project, it's injected explicitly in ProjectService.
private Project project;


// Audience for Airflow JWTs
private static final String[] JWT_AUDIENCE = new String[]{Audience.API};

public AirflowService() {
}

public void setProjectId(Integer projectId) {
this.projectId = projectId;
this.project = this.projectFacade.find(projectId);
}

public Integer getProjectId() {
return projectId;
@Override
protected ProjectController getProjectController() {
return projectController;
}

@POST
Expand All @@ -80,9 +70,9 @@ public Integer getProjectId() {
@ApiOperation(value = "Generate an Airflow Python DAG file from a DAG definition")
public Response composeDAG(AirflowDagDTO dagDefinition,
@Context HttpServletRequest req,
@Context SecurityContext sc) throws AirflowException {
@Context SecurityContext sc) throws AirflowException, ProjectException {
Users user = jwtHelper.getUserPrincipal(sc);
airflowController.composeDAG(project, user, dagDefinition);
airflowController.composeDAG(getProject(), user, dagDefinition);
return Response.ok().build();
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -27,13 +27,13 @@
import io.hops.hopsworks.api.alert.silence.SilenceResource;
import io.hops.hopsworks.api.filter.AllowedProjectRoles;
import io.hops.hopsworks.api.filter.Audience;
import io.hops.hopsworks.api.project.ProjectSubResource;
import io.hops.hopsworks.api.util.Pagination;
import io.hops.hopsworks.common.api.ResourceRequest;
import io.hops.hopsworks.common.project.ProjectController;
import io.hops.hopsworks.exceptions.AlertException;
import io.hops.hopsworks.exceptions.ProjectException;
import io.hops.hopsworks.jwt.annotation.JWTRequired;
import io.hops.hopsworks.persistence.entity.project.Project;
import io.hops.hopsworks.restutils.RESTCodes;
import io.swagger.annotations.Api;
import io.swagger.annotations.ApiOperation;
Expand Down Expand Up @@ -64,7 +64,7 @@
@RequestScoped
@Api(value = "Alert Resource")
@TransactionAttribute(TransactionAttributeType.NEVER)
public class AlertResource {
public class AlertResource extends ProjectSubResource {

private static final Logger LOGGER = Logger.getLogger(AlertResource.class.getName());

Expand All @@ -81,24 +81,9 @@ public class AlertResource {
@Inject
private ReceiverResource receiverResource;

private Integer projectId;
private String projectName;

public void setProjectId(Integer projectId) {
this.projectId = projectId;
}

public void setProjectName(String projectName) {
this.projectName = projectName;
}

private Project getProject() throws ProjectException {
if (this.projectId != null) {
return projectController.findProjectById(this.projectId);
} else if (this.projectName != null) {
return projectController.findProjectByName(this.projectName);
}
throw new ProjectException(RESTCodes.ProjectErrorCode.PROJECT_NOT_FOUND, Level.FINE);
@Override
protected ProjectController getProjectController() {
return projectController;
}

@GET
Expand Down Expand Up @@ -169,19 +154,19 @@ public Response createAlerts(PostableAlertDTOs alerts, @Context UriInfo uriInfo,

@Path("silences")
public SilenceResource silence() {
this.silenceResource.setProjectId(this.projectId);
this.silenceResource.setProjectId(getProjectId());
return silenceResource;
}

@Path("receivers")
public ReceiverResource receiver() {
this.receiverResource.setProjectId(this.projectId);
this.receiverResource.setProjectId(getProjectId());
return receiverResource;
}

@Path("routes")
public RouteResource route() {
this.routeResource.setProjectId(this.projectId);
this.routeResource.setProjectId(getProjectId());
return routeResource;
}
}
Loading

0 comments on commit 96dbc7b

Please sign in to comment.