From 69a9b653158849e89708a239db4a7bb7b426926c Mon Sep 17 00:00:00 2001 From: Robin Beer Date: Fri, 13 Oct 2023 11:08:58 +0100 Subject: [PATCH 1/3] refactor(Added auth checks): Auth checks and added notes and org id to feed source summary --- .../controllers/api/FeedSourceController.java | 40 +++++--- .../manager/models/FeedSourceSummary.java | 47 +++++++-- .../datatools/manager/models/Label.java | 13 ++- .../datatools/manager/models/Note.java | 15 +++ .../datatools/manager/models/Project.java | 2 +- .../api/FeedSourceControllerTest.java | 97 ++++++++++++++++--- 6 files changed, 177 insertions(+), 37 deletions(-) diff --git a/src/main/java/com/conveyal/datatools/manager/controllers/api/FeedSourceController.java b/src/main/java/com/conveyal/datatools/manager/controllers/api/FeedSourceController.java index badedda76..080c79915 100644 --- a/src/main/java/com/conveyal/datatools/manager/controllers/api/FeedSourceController.java +++ b/src/main/java/com/conveyal/datatools/manager/controllers/api/FeedSourceController.java @@ -45,6 +45,7 @@ import static com.conveyal.datatools.common.utils.SparkUtils.getPOJOFromRequestBody; import static com.conveyal.datatools.common.utils.SparkUtils.logMessageAndHalt; import static com.conveyal.datatools.manager.models.ExternalFeedSourceProperty.constructId; +import static com.conveyal.datatools.manager.models.FeedSourceSummary.cleanFeedSourceSummaryForNonAdmins; import static com.conveyal.datatools.manager.models.transform.NormalizeFieldTransformation.getInvalidSubstitutionMessage; import static com.conveyal.datatools.manager.models.transform.NormalizeFieldTransformation.getInvalidSubstitutionPatterns; import static com.mongodb.client.model.Filters.in; @@ -103,6 +104,31 @@ private static Collection getProjectFeedSources(Request req, Respons return feedSourcesToReturn; } + private static Collection getAllFeedSourceSummaries(Request req, Response res) { + Collection feedSourcesToReturn = new ArrayList<>(); + Auth0UserProfile user = req.attribute("user"); + String projectId = req.queryParams("projectId"); + + Project project = Persistence.projects.getById(projectId); + + if (project == null) { + logMessageAndHalt(req, 400, "Must provide valid projectId value."); + } else { + boolean isAdmin = user.canAdministerProject(project); + Collection feedSourceSummaries = project.retrieveFeedSourceSummaries(); + for (FeedSourceSummary feedSourceSummary : feedSourceSummaries) { + // If user can view or manage feed, add to list of feeds to return. NOTE: By default most users with access + // to a project should be able to view all feed sources. Custom privileges would need to be provided to + // override this behavior. + if (user.canManageOrViewFeed(project.organizationId, feedSourceSummary.projectId, feedSourceSummary.id)) { + // Remove labels user can't view, then add to list of feeds to return. + feedSourcesToReturn.add(cleanFeedSourceSummaryForNonAdmins(feedSourceSummary, isAdmin)); + } + } + } + return feedSourcesToReturn; + } + /** * HTTP endpoint to create a new feed source. */ @@ -397,20 +423,6 @@ protected static FeedSource cleanFeedSourceForNonAdmins(FeedSource feedSource, b return feedSource; } - private static Collection getAllFeedSourceSummaries(Request req, Response res) { - Auth0UserProfile userProfile = req.attribute("user"); - String projectId = req.queryParams("projectId"); - Project project = Persistence.projects.getById(projectId); - if (project == null) { - logMessageAndHalt(req, 400, "Must provide valid projectId value."); - } - if (!userProfile.canAdministerProject(project)) { - logMessageAndHalt(req, 401, "User not authorized to view project feed sources."); - } - return project.retrieveFeedSourceSummaries(); - } - - // FIXME: use generic API controller and return JSON documents via BSON/Mongo public static void register (String apiPrefix) { get(apiPrefix + "secure/feedsource/:id", FeedSourceController::getFeedSource, json::write); diff --git a/src/main/java/com/conveyal/datatools/manager/models/FeedSourceSummary.java b/src/main/java/com/conveyal/datatools/manager/models/FeedSourceSummary.java index 2c5d6197f..6116bac3f 100644 --- a/src/main/java/com/conveyal/datatools/manager/models/FeedSourceSummary.java +++ b/src/main/java/com/conveyal/datatools/manager/models/FeedSourceSummary.java @@ -2,6 +2,7 @@ import com.conveyal.datatools.editor.utils.JacksonSerializers; import com.conveyal.datatools.manager.persistence.Persistence; +import com.conveyal.datatools.manager.utils.PersistenceUtils; import com.conveyal.gtfs.validator.ValidationResult; import com.fasterxml.jackson.databind.annotation.JsonDeserialize; import com.fasterxml.jackson.databind.annotation.JsonSerialize; @@ -20,6 +21,7 @@ import java.util.HashMap; import java.util.List; import java.util.Map; +import java.util.stream.Collectors; import static com.mongodb.client.model.Aggregates.group; import static com.mongodb.client.model.Aggregates.limit; @@ -64,11 +66,16 @@ public class FeedSourceSummary { public String url; + public List noteIds = new ArrayList<>(); + + public String organizationId; + public FeedSourceSummary() { } - public FeedSourceSummary(String projectId, Document feedSourceDocument) { + public FeedSourceSummary(String projectId, String organizationId, Document feedSourceDocument) { this.projectId = projectId; + this.organizationId = organizationId; this.id = feedSourceDocument.getString("_id"); this.name = feedSourceDocument.getString("name"); this.deployable = feedSourceDocument.getBoolean("deployable"); @@ -77,11 +84,35 @@ public FeedSourceSummary(String projectId, Document feedSourceDocument) { if (documentLabelIds != null) { this.labelIds = documentLabelIds; } + List documentNoteIds = feedSourceDocument.getList("noteIds", String.class); + if (documentNoteIds != null) { + this.noteIds = documentNoteIds; + } // Convert to local date type for consistency. this.lastUpdated = getLocalDateFromDate(feedSourceDocument.getDate("lastUpdated")); this.url = feedSourceDocument.getString("url"); } + /** + * Removes labels and notes from a feed that a user is not allowed to view. Returns cleaned feed source. + * @param feedSourceSummary The feed source to clean + * @param isAdmin Is the user an admin? Changes what is returned. + * @return A feed source containing only labels/notes the user is allowed to see + */ + public static FeedSourceSummary cleanFeedSourceSummaryForNonAdmins(FeedSourceSummary feedSourceSummary, boolean isAdmin) { + // Admin can view all feed labels, but a non-admin should only see those with adminOnly=false + feedSourceSummary.labelIds = Persistence.labels + .getFiltered(PersistenceUtils.applyAdminFilter(in("_id", feedSourceSummary.labelIds), isAdmin)).stream() + .map(label -> label.id) + .collect(Collectors.toList()); + feedSourceSummary.noteIds = Persistence.notes + .getFiltered(PersistenceUtils.applyAdminFilter(in("_id", feedSourceSummary.noteIds), isAdmin)).stream() + .map(note -> note.id) + .collect(Collectors.toList()); + return feedSourceSummary; + } + + /** * Set the appropriate feed version. For consistency, if no error count is available set the related number of * issues to null. @@ -104,7 +135,7 @@ public void setFeedVersion(FeedVersionSummary feedVersionSummary, boolean isDepl /** * Get all feed source summaries matching the project id. */ - public static List getFeedSourceSummaries(String projectId) { + public static List getFeedSourceSummaries(String projectId, String organizationId) { /* db.getCollection('FeedSource').aggregate([ { @@ -121,7 +152,8 @@ public static List getFeedSourceSummaries(String projectId) { "isPublic": 1, "lastUpdated": 1, "labelIds": 1, - "url": 1 + "url": 1, + "noteIds": 1 } }, { @@ -143,12 +175,13 @@ public static List getFeedSourceSummaries(String projectId) { "isPublic", "lastUpdated", "labelIds", - "url") + "url", + "noteIds") ) ), sort(Sorts.ascending("name")) ); - return extractFeedSourceSummaries(projectId, stages); + return extractFeedSourceSummaries(projectId, organizationId, stages); } /** @@ -423,10 +456,10 @@ public static Map getFeedVersionsFromPinnedDeploymen /** * Produce a list of all feed source summaries for a project. */ - private static List extractFeedSourceSummaries(String projectId, List stages) { + private static List extractFeedSourceSummaries(String projectId, String organizationId, List stages) { List feedSourceSummaries = new ArrayList<>(); for (Document feedSourceDocument : Persistence.getDocuments("FeedSource", stages)) { - feedSourceSummaries.add(new FeedSourceSummary(projectId, feedSourceDocument)); + feedSourceSummaries.add(new FeedSourceSummary(projectId, organizationId, feedSourceDocument)); } return feedSourceSummaries; } diff --git a/src/main/java/com/conveyal/datatools/manager/models/Label.java b/src/main/java/com/conveyal/datatools/manager/models/Label.java index 8bf239798..0afe2563e 100644 --- a/src/main/java/com/conveyal/datatools/manager/models/Label.java +++ b/src/main/java/com/conveyal/datatools/manager/models/Label.java @@ -39,15 +39,24 @@ public String organizationId () { public Auth0UserProfile user; /** - * Create a new label + * Create a new label with auto-gen id. */ public Label (String name, String description, String color, boolean adminOnly, String projectId) { + this(null, name, description, color, adminOnly, projectId); + } + + /** + * Create a new label with provided id. + */ + public Label (String id, String name, String description, String color, boolean adminOnly, String projectId) { super(); + if (id != null) { + this.id = id; + } this.name = name; this.description = description != null ? description : ""; this.color = color != null ? color : "#000000"; this.adminOnly = adminOnly; - this.projectId = projectId; } diff --git a/src/main/java/com/conveyal/datatools/manager/models/Note.java b/src/main/java/com/conveyal/datatools/manager/models/Note.java index 87b5f452f..438a784b5 100644 --- a/src/main/java/com/conveyal/datatools/manager/models/Note.java +++ b/src/main/java/com/conveyal/datatools/manager/models/Note.java @@ -32,6 +32,21 @@ public class Note extends Model implements Serializable { /** Whether the note should be visible to project admins only */ public boolean adminOnly; + /** + * Create a new note with provided id. + */ + public Note(String id, String body, boolean adminOnly) { + super(); + if (id != null) { + this.id = id; + } + this.body = body; + this.adminOnly = adminOnly; + } + + public Note() { + } + /** * The types of object that can have notes recorded on them. */ diff --git a/src/main/java/com/conveyal/datatools/manager/models/Project.java b/src/main/java/com/conveyal/datatools/manager/models/Project.java index 4ccad306f..eccb7d33a 100644 --- a/src/main/java/com/conveyal/datatools/manager/models/Project.java +++ b/src/main/java/com/conveyal/datatools/manager/models/Project.java @@ -173,7 +173,7 @@ public Collection retrieveDeploymentSummaries() { * Get all feed source summaries for this project. */ public Collection retrieveFeedSourceSummaries() { - List feedSourceSummaries = FeedSourceSummary.getFeedSourceSummaries(id); + List feedSourceSummaries = FeedSourceSummary.getFeedSourceSummaries(id, organizationId); Map latestFeedVersionForFeedSources = FeedSourceSummary.getLatestFeedVersionForFeedSources(id); Map deployedFeedVersions = FeedSourceSummary.getFeedVersionsFromPinnedDeployment(id); if (deployedFeedVersions.isEmpty()) { diff --git a/src/test/java/com/conveyal/datatools/manager/controllers/api/FeedSourceControllerTest.java b/src/test/java/com/conveyal/datatools/manager/controllers/api/FeedSourceControllerTest.java index a93f14630..a4c6d8d0a 100644 --- a/src/test/java/com/conveyal/datatools/manager/controllers/api/FeedSourceControllerTest.java +++ b/src/test/java/com/conveyal/datatools/manager/controllers/api/FeedSourceControllerTest.java @@ -11,6 +11,7 @@ import com.conveyal.datatools.manager.models.FeedVersion; import com.conveyal.datatools.manager.models.FetchFrequency; import com.conveyal.datatools.manager.models.Label; +import com.conveyal.datatools.manager.models.Note; import com.conveyal.datatools.manager.models.Project; import com.conveyal.datatools.manager.persistence.Persistence; import com.conveyal.datatools.manager.utils.HttpUtils; @@ -22,6 +23,7 @@ import org.junit.jupiter.api.Test; import java.io.IOException; +import java.net.MalformedURLException; import java.net.URL; import java.time.LocalDate; import java.time.Month; @@ -46,7 +48,10 @@ public class FeedSourceControllerTest extends DatatoolsTest { private static FeedSource feedSourceWithInvalidLabels = null; private static Label publicLabel = null; private static Label adminOnlyLabel = null; - + private static Label feedSourceWithLatestDeploymentAdminOnlyLabel = null; + private static Label feedSourceWithPinnedDeploymentAdminOnlyLabel = null; + private static Note feedSourceWithLatestDeploymentAdminOnlyNote = null; + private static Note feedSourceWithPinnedDeploymentAdminOnlyNote = null; private static Project projectWithLatestDeployment = null; private static FeedSource feedSourceWithLatestDeploymentFeedVersion = null; private static FeedVersion feedVersionFromLatestDeployment = null; @@ -78,22 +83,37 @@ public static void setUp() throws IOException { feedSourceWithLabels = createFeedSource("FeedSourceThree", new URL("http://www.feedsource.com"), projectToBeDeleted); feedSourceWithInvalidLabels = createFeedSource("FeedSourceFour", new URL("http://www.feedsource.com"), project); - adminOnlyLabel = createLabel("Admin Only Label"); + adminOnlyLabel = createLabel("Admin Only Label", projectToBeDeleted.id); adminOnlyLabel.adminOnly = true; - publicLabel = createLabel("Public Label"); + publicLabel = createLabel("Public Label", projectToBeDeleted.id); + + setUpFeedVersionFromLatestDeployment(); + setUpFeedVersionFromPinnedDeployment(); + + } - // Feed version from latest deployment. + /** + * Create all the required objects to test a feed version from the latest deployment. + */ + private static void setUpFeedVersionFromLatestDeployment() throws MalformedURLException { projectWithLatestDeployment = new Project(); projectWithLatestDeployment.id = "project-with-latest-deployment"; + projectWithLatestDeployment.organizationId = "project-with-latest-deployment-org-id"; Persistence.projects.create(projectWithLatestDeployment); + + feedSourceWithLatestDeploymentAdminOnlyLabel = createLabel("label-id-latest-deployment", "Admin Only Label", projectWithLatestDeployment.id); + feedSourceWithLatestDeploymentAdminOnlyNote = createNote("note-id-latest-deployment", "A test note"); + feedSourceWithLatestDeploymentFeedVersion = createFeedSource( "feed-source-with-latest-deployment-feed-version", "FeedSource", new URL("http://www.feedsource.com"), projectWithLatestDeployment, true, - List.of("labelOne", "labelTwo") + List.of(feedSourceWithLatestDeploymentAdminOnlyLabel.id), + List.of(feedSourceWithLatestDeploymentAdminOnlyNote.id) ); + LocalDate deployedSuperseded = LocalDate.of(2020, Month.MARCH, 12); LocalDate deployedEndDate = LocalDate.of(2021, Month.MARCH, 12); LocalDate deployedStartDate = LocalDate.of(2021, Month.MARCH, 1); @@ -115,18 +135,28 @@ public static void setUp() throws IOException { feedVersionFromLatestDeployment.id, deployedEndDate ); + } - // Feed version from pinned deployment. + /** + * Create all the required objects to test a feed version from a pinned deployment. + */ + private static void setUpFeedVersionFromPinnedDeployment() throws MalformedURLException { projectWithPinnedDeployment = new Project(); projectWithPinnedDeployment.id = "project-with-pinned-deployment"; + projectWithPinnedDeployment.organizationId = "project-with-pinned-deployment-org-id"; Persistence.projects.create(projectWithPinnedDeployment); + + feedSourceWithPinnedDeploymentAdminOnlyLabel = createLabel("label-id-pinned-deployment", "Admin Only Label", projectWithPinnedDeployment.id); + feedSourceWithPinnedDeploymentAdminOnlyNote = createNote("note-id-pinned-deployment", "A test note"); + feedSourceWithPinnedDeploymentFeedVersion = createFeedSource( "feed-source-with-pinned-deployment-feed-version", "FeedSourceWithPinnedFeedVersion", new URL("http://www.feedsource.com"), projectWithPinnedDeployment, true, - List.of("labelOne", "labelTwo") + List.of(feedSourceWithPinnedDeploymentAdminOnlyLabel.id), + List.of(feedSourceWithPinnedDeploymentAdminOnlyNote.id) ); feedVersionFromPinnedDeployment = createFeedVersion( "feed-version-from-pinned-deployment", @@ -137,7 +167,7 @@ public static void setUp() throws IOException { "deployment-pinned", projectWithPinnedDeployment, feedVersionFromPinnedDeployment.id, - deployedEndDate + LocalDate.of(2021, Month.MARCH, 12) ); projectWithPinnedDeployment.pinnedDeploymentId = deploymentPinned.id; Persistence.projects.replace(projectWithPinnedDeployment.id, projectWithPinnedDeployment); @@ -200,6 +230,18 @@ private static void tearDownDeployedFeedVersion() { if (deploymentSuperseded != null) { Persistence.deployments.removeById(deploymentSuperseded.id); } + if (feedSourceWithPinnedDeploymentAdminOnlyLabel != null) { + Persistence.labels.removeById(feedSourceWithPinnedDeploymentAdminOnlyLabel.id); + } + if (feedSourceWithLatestDeploymentAdminOnlyLabel != null) { + Persistence.labels.removeById(feedSourceWithLatestDeploymentAdminOnlyLabel.id); + } + if (feedSourceWithPinnedDeploymentAdminOnlyNote != null) { + Persistence.notes.removeById(feedSourceWithPinnedDeploymentAdminOnlyNote.id); + } + if (feedSourceWithLatestDeploymentAdminOnlyNote != null) { + Persistence.notes.removeById(feedSourceWithLatestDeploymentAdminOnlyNote.id); + } } /** @@ -345,6 +387,8 @@ void canRetrieveDeployedFeedVersionFromLatestDeployment() throws IOException { assertEquals(feedSourceWithLatestDeploymentFeedVersion.projectId, feedSourceSummaries.get(0).projectId); assertEquals(feedSourceWithLatestDeploymentFeedVersion.labelIds, feedSourceSummaries.get(0).labelIds); assertEquals(feedSourceWithLatestDeploymentFeedVersion.url.toString(), feedSourceSummaries.get(0).url); + assertEquals(feedSourceWithLatestDeploymentFeedVersion.noteIds, feedSourceSummaries.get(0).noteIds); + assertEquals(feedSourceWithLatestDeploymentFeedVersion.organizationId(), feedSourceSummaries.get(0).organizationId); assertEquals(feedVersionFromLatestDeployment.id, feedSourceSummaries.get(0).deployedFeedVersionId); assertEquals(feedVersionFromLatestDeployment.validationSummary().startDate, feedSourceSummaries.get(0).deployedFeedVersionStartDate); assertEquals(feedVersionFromLatestDeployment.validationSummary().endDate, feedSourceSummaries.get(0).deployedFeedVersionEndDate); @@ -377,6 +421,8 @@ void canRetrieveDeployedFeedVersionFromPinnedDeployment() throws IOException { assertEquals(feedSourceWithPinnedDeploymentFeedVersion.projectId, feedSourceSummaries.get(0).projectId); assertEquals(feedSourceWithPinnedDeploymentFeedVersion.labelIds, feedSourceSummaries.get(0).labelIds); assertEquals(feedSourceWithPinnedDeploymentFeedVersion.url.toString(), feedSourceSummaries.get(0).url); + assertEquals(feedSourceWithPinnedDeploymentFeedVersion.noteIds, feedSourceSummaries.get(0).noteIds); + assertEquals(feedSourceWithPinnedDeploymentFeedVersion.organizationId(), feedSourceSummaries.get(0).organizationId); assertEquals(feedVersionFromPinnedDeployment.id, feedSourceSummaries.get(0).deployedFeedVersionId); assertEquals(feedVersionFromPinnedDeployment.validationSummary().startDate, feedSourceSummaries.get(0).deployedFeedVersionStartDate); assertEquals(feedVersionFromPinnedDeployment.validationSummary().endDate, feedSourceSummaries.get(0).deployedFeedVersionEndDate); @@ -395,7 +441,7 @@ private static FeedSource createFeedSource(String name, URL url, Project project * Helper method to create feed source. */ private static FeedSource createFeedSource(String id, String name, URL url, Project project, boolean persist) { - return createFeedSource(id, name, url, project, persist, null); + return createFeedSource(id, name, url, project, persist, null, null); } private static FeedSource createFeedSource( String id, @@ -403,7 +449,8 @@ private static FeedSource createFeedSource( URL url, Project project, boolean persist, - List labels + List labels, + List notes ) { FeedSource feedSource = new FeedSource(); if (id != null) feedSource.id = id; @@ -415,6 +462,7 @@ private static FeedSource createFeedSource( feedSource.retrievalMethod = FeedRetrievalMethod.FETCHED_AUTOMATICALLY; feedSource.url = url; if (labels != null) feedSource.labelIds = labels; + if (notes != null) feedSource.noteIds = notes; if (persist) Persistence.feedSources.create(feedSource); return feedSource; } @@ -461,10 +509,33 @@ private static FeedVersion createFeedVersion(String id, String feedSourceId, Loc } /** - * Helper method to create label + * Helper method to create note. + */ + private static Note createNote(String id, String body) { + Note note = new Note(id, body, false); + Persistence.notes.create(note); + return note; + } + + /** + * Helper method to create label. If the id is provided save the label now if not deffer to test to save. + */ + private static Label createLabel(String id, String name, String projectId) { + Label label; + if (id != null) { + label = new Label(id, name, "A label used during testing", "#123", false, projectId); + Persistence.labels.create(label); + } else { + label = new Label(name, "A label used during testing", "#123", false, projectId); + } + return label; + } + + /** + * Helper method to create label. */ - private static Label createLabel(String name) { - return new Label(name, "A label used during testing", "#123", false, projectToBeDeleted.id); + private static Label createLabel(String name, String projectId) { + return createLabel(null, name, projectId); } /** From 6695674e23dda07cd1f3a9eb32e347338286b179 Mon Sep 17 00:00:00 2001 From: Robin Beer Date: Fri, 13 Oct 2023 12:47:13 +0100 Subject: [PATCH 2/3] refactor(Refactored label and note auth checks): --- .../controllers/api/FeedSourceController.java | 77 ++++++++++++++----- .../manager/models/FeedSourceSummary.java | 29 ++----- 2 files changed, 63 insertions(+), 43 deletions(-) diff --git a/src/main/java/com/conveyal/datatools/manager/controllers/api/FeedSourceController.java b/src/main/java/com/conveyal/datatools/manager/controllers/api/FeedSourceController.java index 080c79915..628d7815c 100644 --- a/src/main/java/com/conveyal/datatools/manager/controllers/api/FeedSourceController.java +++ b/src/main/java/com/conveyal/datatools/manager/controllers/api/FeedSourceController.java @@ -9,7 +9,6 @@ import com.conveyal.datatools.manager.extensions.ExternalFeedResource; import com.conveyal.datatools.manager.jobs.FetchSingleFeedJob; import com.conveyal.datatools.manager.jobs.NotifyUsersForSubscriptionJob; -import com.conveyal.datatools.manager.models.DeploymentSummary; import com.conveyal.datatools.manager.models.ExternalFeedSourceProperty; import com.conveyal.datatools.manager.models.FeedRetrievalMethod; import com.conveyal.datatools.manager.models.FeedSource; @@ -45,7 +44,6 @@ import static com.conveyal.datatools.common.utils.SparkUtils.getPOJOFromRequestBody; import static com.conveyal.datatools.common.utils.SparkUtils.logMessageAndHalt; import static com.conveyal.datatools.manager.models.ExternalFeedSourceProperty.constructId; -import static com.conveyal.datatools.manager.models.FeedSourceSummary.cleanFeedSourceSummaryForNonAdmins; import static com.conveyal.datatools.manager.models.transform.NormalizeFieldTransformation.getInvalidSubstitutionMessage; import static com.conveyal.datatools.manager.models.transform.NormalizeFieldTransformation.getInvalidSubstitutionPatterns; import static com.mongodb.client.model.Filters.in; @@ -88,7 +86,7 @@ private static Collection getProjectFeedSources(Request req, Respons boolean isAdmin = user.canAdministerProject(project); Collection projectFeedSources = project.retrieveProjectFeedSources(); - for (FeedSource source: projectFeedSources) { + for (FeedSource source : projectFeedSources) { String orgId = source.organizationId(); // If user can view or manage feed, add to list of feeds to return. NOTE: By default most users with access // to a project should be able to view all feed sources. Custom privileges would need to be provided to @@ -323,7 +321,7 @@ private static FeedSource deleteFeedSource(Request req, Response res) { /** * Re-fetch this feed from the feed source URL. */ - private static String fetch (Request req, Response res) { + private static String fetch(Request req, Response res) { FeedSource s = requestFeedSourceById(req, Actions.MANAGE); if (s.url == null) { logMessageAndHalt(req, HttpStatus.BAD_REQUEST_400, "Cannot fetch feed source with null URL."); @@ -341,7 +339,8 @@ private static String fetch (Request req, Response res) { /** * Helper function returns feed source if user has permission for specified action. - * @param req spark Request object from API request + * + * @param req spark Request object from API request * @param action action type (either "view" or Permission.MANAGE) * @return feedsource object for ID */ @@ -392,39 +391,79 @@ public static FeedSource checkFeedSourcePermissions(Request req, FeedSource feed return cleanFeedSourceForNonAdmins(feedSource, isProjectAdmin); } - /** Determines whether a change to a feed source is significant enough that it warrants sending a notification + /** + * Determines whether a change to a feed source is significant enough that it warrants sending a notification * * @param formerFeedSource A feed source object, without new changes * @param updatedFeedSource A feed source object, with new changes - * @return A boolean value indicating if the updated feed source is changed enough to warrant sending a notification. + * @return A boolean value indicating if the updated feed source is changed enough to warrant sending a notification. */ private static boolean shouldNotifyUsersOnFeedUpdated(FeedSource formerFeedSource, FeedSource updatedFeedSource) { - return - // If only labels have changed, don't send out an email - formerFeedSource.equalsExceptLabels(updatedFeedSource); + // If only labels have changed, don't send out an email. + return formerFeedSource.equalsExceptLabels(updatedFeedSource); } /** * Removes labels and notes from a feed that a user is not allowed to view. Returns cleaned feed source. - * @param feedSource The feed source to clean - * @param isAdmin Is the user an admin? Changes what is returned. - * @return A feed source containing only labels/notes the user is allowed to see + * + * @param feedSource The feed source to clean. + * @param isAdmin Is the user an admin? Changes what is returned. + * @return A feed source containing only labels/notes the user is allowed to see. */ protected static FeedSource cleanFeedSourceForNonAdmins(FeedSource feedSource, boolean isAdmin) { // Admin can view all feed labels, but a non-admin should only see those with adminOnly=false - feedSource.labelIds = Persistence.labels - .getFiltered(PersistenceUtils.applyAdminFilter(in("_id", feedSource.labelIds), isAdmin)).stream() + feedSource.labelIds = cleanFeedSourceLabelIdsForNonAdmins(feedSource.labelIds, isAdmin); + feedSource.noteIds = cleanFeedSourceNotesForNonAdmins(feedSource.noteIds, isAdmin); + return feedSource; + } + + /** + * Removes labels and notes from a feed that a user is not allowed to view. Returns cleaned feed source summary. + * + * @param feedSourceSummary The feed source to clean. + * @param isAdmin Is the user an admin? Changes what is returned. + * @return A feed source summary containing only labels/notes the user is allowed to see. + */ + protected static FeedSourceSummary cleanFeedSourceSummaryForNonAdmins(FeedSourceSummary feedSourceSummary, boolean isAdmin) { + // Admin can view all feed labels, but a non-admin should only see those with adminOnly=false + feedSourceSummary.labelIds = cleanFeedSourceLabelIdsForNonAdmins(feedSourceSummary.labelIds, isAdmin); + feedSourceSummary.noteIds = cleanFeedSourceNotesForNonAdmins(feedSourceSummary.noteIds, isAdmin); + return feedSourceSummary; + } + + /** + * Removes labels from a feed that a user is not allowed to view. Returns cleaned notes. + * + * @param labelIds The labels to clean. + * @param isAdmin Is the user an admin? Changes what is returned. + * @return Labels the user is allowed to see. + */ + protected static List cleanFeedSourceLabelIdsForNonAdmins(List labelIds, boolean isAdmin) { + // Admin can view all feed labels, but a non-admin should only see those with adminOnly=false. + return Persistence.labels + .getFiltered(PersistenceUtils.applyAdminFilter(in("_id", labelIds), isAdmin)).stream() .map(label -> label.id) .collect(Collectors.toList()); - feedSource.noteIds = Persistence.notes - .getFiltered(PersistenceUtils.applyAdminFilter(in("_id", feedSource.noteIds), isAdmin)).stream() + } + + /** + * Removes notes from a feed that a user is not allowed to view. Returns cleaned notes. + * + * @param noteIds The notes to clean. + * @param isAdmin Is the user an admin? Changes what is returned. + * @return Notes the user is allowed to see. + */ + protected static List cleanFeedSourceNotesForNonAdmins(List noteIds, boolean isAdmin) { + // Admin can view all feed notes, but a non-admin should only see those with adminOnly=false. + return Persistence.notes + .getFiltered(PersistenceUtils.applyAdminFilter(in("_id", noteIds), isAdmin)).stream() .map(note -> note.id) .collect(Collectors.toList()); - return feedSource; } + // FIXME: use generic API controller and return JSON documents via BSON/Mongo - public static void register (String apiPrefix) { + public static void register(String apiPrefix) { get(apiPrefix + "secure/feedsource/:id", FeedSourceController::getFeedSource, json::write); get(apiPrefix + "secure/feedsource", FeedSourceController::getProjectFeedSources, json::write); post(apiPrefix + "secure/feedsource", FeedSourceController::createFeedSource, json::write); diff --git a/src/main/java/com/conveyal/datatools/manager/models/FeedSourceSummary.java b/src/main/java/com/conveyal/datatools/manager/models/FeedSourceSummary.java index 6116bac3f..15f93e57b 100644 --- a/src/main/java/com/conveyal/datatools/manager/models/FeedSourceSummary.java +++ b/src/main/java/com/conveyal/datatools/manager/models/FeedSourceSummary.java @@ -2,7 +2,6 @@ import com.conveyal.datatools.editor.utils.JacksonSerializers; import com.conveyal.datatools.manager.persistence.Persistence; -import com.conveyal.datatools.manager.utils.PersistenceUtils; import com.conveyal.gtfs.validator.ValidationResult; import com.fasterxml.jackson.databind.annotation.JsonDeserialize; import com.fasterxml.jackson.databind.annotation.JsonSerialize; @@ -21,7 +20,6 @@ import java.util.HashMap; import java.util.List; import java.util.Map; -import java.util.stream.Collectors; import static com.mongodb.client.model.Aggregates.group; import static com.mongodb.client.model.Aggregates.limit; @@ -93,26 +91,6 @@ public FeedSourceSummary(String projectId, String organizationId, Document feedS this.url = feedSourceDocument.getString("url"); } - /** - * Removes labels and notes from a feed that a user is not allowed to view. Returns cleaned feed source. - * @param feedSourceSummary The feed source to clean - * @param isAdmin Is the user an admin? Changes what is returned. - * @return A feed source containing only labels/notes the user is allowed to see - */ - public static FeedSourceSummary cleanFeedSourceSummaryForNonAdmins(FeedSourceSummary feedSourceSummary, boolean isAdmin) { - // Admin can view all feed labels, but a non-admin should only see those with adminOnly=false - feedSourceSummary.labelIds = Persistence.labels - .getFiltered(PersistenceUtils.applyAdminFilter(in("_id", feedSourceSummary.labelIds), isAdmin)).stream() - .map(label -> label.id) - .collect(Collectors.toList()); - feedSourceSummary.noteIds = Persistence.notes - .getFiltered(PersistenceUtils.applyAdminFilter(in("_id", feedSourceSummary.noteIds), isAdmin)).stream() - .map(note -> note.id) - .collect(Collectors.toList()); - return feedSourceSummary; - } - - /** * Set the appropriate feed version. For consistency, if no error count is available set the related number of * issues to null. @@ -573,8 +551,11 @@ public static class LatestValidationResult { public Integer errorCount; - /** Required for JSON de/serializing. **/ - public LatestValidationResult() {} + /** + * Required for JSON de/serializing. + **/ + public LatestValidationResult() { + } LatestValidationResult(FeedVersionSummary feedVersionSummary) { this.feedVersionId = feedVersionSummary.id; From 3fc322f1674209faf3aaccc961c5b14e79c02942 Mon Sep 17 00:00:00 2001 From: Robin Beer Date: Tue, 24 Oct 2023 17:13:16 +0100 Subject: [PATCH 3/3] refactor(Addressed PR feedback): Cleaned constructors --- .../controllers/api/FeedSourceController.java | 6 ++++-- .../datatools/manager/models/Label.java | 18 ++++++++---------- .../datatools/manager/models/Note.java | 4 +--- .../api/FeedSourceControllerTest.java | 2 +- 4 files changed, 14 insertions(+), 16 deletions(-) diff --git a/src/main/java/com/conveyal/datatools/manager/controllers/api/FeedSourceController.java b/src/main/java/com/conveyal/datatools/manager/controllers/api/FeedSourceController.java index 628d7815c..361c74f6d 100644 --- a/src/main/java/com/conveyal/datatools/manager/controllers/api/FeedSourceController.java +++ b/src/main/java/com/conveyal/datatools/manager/controllers/api/FeedSourceController.java @@ -441,7 +441,8 @@ protected static FeedSourceSummary cleanFeedSourceSummaryForNonAdmins(FeedSource protected static List cleanFeedSourceLabelIdsForNonAdmins(List labelIds, boolean isAdmin) { // Admin can view all feed labels, but a non-admin should only see those with adminOnly=false. return Persistence.labels - .getFiltered(PersistenceUtils.applyAdminFilter(in("_id", labelIds), isAdmin)).stream() + .getFiltered(PersistenceUtils.applyAdminFilter(in("_id", labelIds), isAdmin)) + .stream() .map(label -> label.id) .collect(Collectors.toList()); } @@ -456,7 +457,8 @@ protected static List cleanFeedSourceLabelIdsForNonAdmins(List l protected static List cleanFeedSourceNotesForNonAdmins(List noteIds, boolean isAdmin) { // Admin can view all feed notes, but a non-admin should only see those with adminOnly=false. return Persistence.notes - .getFiltered(PersistenceUtils.applyAdminFilter(in("_id", noteIds), isAdmin)).stream() + .getFiltered(PersistenceUtils.applyAdminFilter(in("_id", noteIds), isAdmin)) + .stream() .map(note -> note.id) .collect(Collectors.toList()); } diff --git a/src/main/java/com/conveyal/datatools/manager/models/Label.java b/src/main/java/com/conveyal/datatools/manager/models/Label.java index 0afe2563e..46c265f54 100644 --- a/src/main/java/com/conveyal/datatools/manager/models/Label.java +++ b/src/main/java/com/conveyal/datatools/manager/models/Label.java @@ -42,17 +42,7 @@ public String organizationId () { * Create a new label with auto-gen id. */ public Label (String name, String description, String color, boolean adminOnly, String projectId) { - this(null, name, description, color, adminOnly, projectId); - } - - /** - * Create a new label with provided id. - */ - public Label (String id, String name, String description, String color, boolean adminOnly, String projectId) { super(); - if (id != null) { - this.id = id; - } this.name = name; this.description = description != null ? description : ""; this.color = color != null ? color : "#000000"; @@ -60,6 +50,14 @@ public Label (String id, String name, String description, String color, boolean this.projectId = projectId; } + /** + * Create a new label with provided id. + */ + public Label (String id, String name, String description, String color, boolean adminOnly, String projectId) { + this(name, description, color, adminOnly, projectId); + this.id = id; + } + /** * No-arg constructor to yield an uninitialized label, for dump/restore. * Should not be used in general code. diff --git a/src/main/java/com/conveyal/datatools/manager/models/Note.java b/src/main/java/com/conveyal/datatools/manager/models/Note.java index 438a784b5..d6a745dd6 100644 --- a/src/main/java/com/conveyal/datatools/manager/models/Note.java +++ b/src/main/java/com/conveyal/datatools/manager/models/Note.java @@ -37,9 +37,7 @@ public class Note extends Model implements Serializable { */ public Note(String id, String body, boolean adminOnly) { super(); - if (id != null) { - this.id = id; - } + this.id = id; this.body = body; this.adminOnly = adminOnly; } diff --git a/src/test/java/com/conveyal/datatools/manager/controllers/api/FeedSourceControllerTest.java b/src/test/java/com/conveyal/datatools/manager/controllers/api/FeedSourceControllerTest.java index a4c6d8d0a..1350fbe52 100644 --- a/src/test/java/com/conveyal/datatools/manager/controllers/api/FeedSourceControllerTest.java +++ b/src/test/java/com/conveyal/datatools/manager/controllers/api/FeedSourceControllerTest.java @@ -518,7 +518,7 @@ private static Note createNote(String id, String body) { } /** - * Helper method to create label. If the id is provided save the label now if not deffer to test to save. + * Helper method to create label. If the id is provided save the label now if not defer to test to save. */ private static Label createLabel(String id, String name, String projectId) { Label label;