[api] Update endpoint for getting project cost surface min and max values [MRXN23-320] #1526
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
yulia-bel
changed the title
There was a problem hiding this comment.
thanks @yulia-bel!
overall all works as expected, but see my note about the ACL check you chose - I think it will prevent users with only project viewer role from getting the range.
plus a bonus rant about not trusting user input, even if your code ends up dealing with this correctly 😄
| > | ||
| > { | ||
| if ( | ||
| !(await this.projectAclService.canEditCostSurfaceInProject( |
There was a problem hiding this comment.
why canEditCostSurfaceInProject()? this would prevent project viewers from getting cost surface range.
I'd go easy with something like this:
| !(await this.projectAclService.canEditCostSurfaceInProject( | |
| !(await this.projectAclService.canViewProjectt( |
Or, if we want to be more specific (but I don't see much value in this specifically: users who can "view" projects should be able to view every aspect of the project, at least in terms of functionality implemented so far in MaPP), create a method such as canViewCostSurfacesInProject().
There was a problem hiding this comment.
well, actually.
The trouble with the kind of logic we have here for ACLs (I've been yelling at clouds about this, just not with you... yet 😅) is that in practice, all we need to get in the endpoint URL is a scenarioId, but we include a projectId for reasons (URL design blah blah), but then, I'm not really thrilled about the way we use these.
If a malicious user somehow knows the UUID of a cost surface (or any other thing, such as a feature, the overall logic is the same) in a project they don't have access to, and request a cost surface (range, in this case) or tile, or feature, or whatever, supplying the UUID of the thing they want to see, and the projectId of any project they have access to... in your implementation here they won't have any luck because the findOne() below looks for a specific cost surface in a specific project, so they will receive a 404. But my point is, this is slightly brittle. If someone comes in months from now and makes some changes without full context, for example decide to simplify the where for whatever reason and only search for the costSurfaceId because hey, that's unique anyway, then the malicious user above will win (tm).
So I'm happy to keep this as is, because it works and it's a low-risk scenario in any case, but in practice my recommendation for these kinds of checks would be to outright ignore the projectId supplied by the user, and instead look up the projectId from the supplied scenarioId, and then check if the user can view the project.
I would not even extract the projectId from the request, nor pass it on from the controller handler to CostSurfaceService.getCostSurfaceRange() - just scenarioId and userId. Just keep the projectId in the URL to make it look kind of logical-looking but then ignore it. I'd even go as far as removing the /projects/:projectId part of the URL, but we do have this kind of parent/child URL design in several places, so I'd just leave this as is.
| projectId, | ||
| )) | ||
| ) { | ||
| return left(projectNotEditable); |
There was a problem hiding this comment.
likewise, this should be a different kind of left, such as projectNotVisible from projects.service.ts
| @@ -131,23 +131,35 @@ export class ProjectCostSurfaceController { | |||
|
|
|||
| @ImplementsAcl() | |||
| @UseGuards(JwtAuthGuard) | |||
| @ApiParam({ | |||
| name: 'costSurfaceId', | |||
| description: 'The id of the Cost Surface to be updated', | |||
There was a problem hiding this comment.
| description: 'The id of the Cost Surface to be updated', | |
| description: 'The id of the Cost Surface for which to retrieve [min,max] cost range', |
yulia-bel
force-pushed
the
feature/api/MRXN23-320-cost-surface-min-max-endpoint
branch
from
d5193fe to
d3e205c
Compare
hotzevzl
deleted the
feature/api/MRXN23-320-cost-surface-min-max-endpoint
branch
Endpoint for getting project cost surface min and max values
Overview
The endpoint has already been part of ProjectCostSurfaceController and was using ScenariosService to get min and max values from
cost_surface_pu_data. This PR implements Cost Surface service to retrieve min and max data from(api)cost_surfaces tableand adds test.Designs
Link to the related design prototypes (if applicable)
Testing instructions
Please explain how to test the PR: ID of a dataset, steps to reach the feature, etc.
Feature relevant tickets
Link to the related task manager tickets
Checklist before submitting
develop.deploying to staging/production, please add brief testing instructions
to the deploy checklist (
docs/deployment-checklist.md)