Enforce file size limits and user role checks on server

[Nespina24]

Description

The export graph meter data route in the client (src/client/app/redux/thunks/exportThunk.ts) enforces a file size limit that prevents downloads of meter data if the estimated file size is greater than the set limit and the user’s role is not EXPORT or ADMIN. This change adds the same file size and user role checks to the server route (src/server/routes/readings.js). Added unit tests to src/server/test/routes/readingsParamsTest.js.

Fixes #1675

Authors: @Nespina24 and @stockingstocker

@huss added: This also fixed an issue with the parameter checking of the meter_id in this route and used the standard return functions.

Type of change

• Note merging this changes the database configuration.
• This change requires a documentation update

Checklist

• I have followed the OED pull request ideas
• I have removed text in ( ) from the issue request
• You acknowledge that every person contributing to this work has signed the OED Contributing License Agreement and each author is listed in the Description section.

Limitations

No limitations.

[huss]

@Nespina24 & @stockingstocker Testing found this works as desired. I have made a few comments to consider. In addition, I am noting these items to see if you have thoughts or want to address (if not then an issue will be created):

• If the readings route you worked on returns an error then the client hangs up for a while waiting and does not give any message. I did not check but it may have existed before. I saw it since I modified your file size check to force it to fail.
• src/client/app/components/admin/PreferencesComponent.tsx has an issue that is stopping the admin from saving the default file size limits for export. It is because it sends a string and not a number when updated. The changes to route param checks now detects this. On line 42 I added the Number() as shown here:

setLocalAdminPref({ ...localAdminPref, [key]: Number(value) });

This seems to have fixed it. There are also other Number casts that can probably go away given this change but I did not check that.

Please let me know if anything is not clear or you have thought/questions.

[stockingstocker]

* If the readings route you worked on returns an error then the client hangs up for a while waiting and does not give any message. I did not check but it may have existed before. I saw it since I modified your file size check to force it to fail.

I don't think I've been able to reproduce this. Could you elaborate on how you modified the file size check to force it to fail?

I did think that maybe its a scenario where the client accepts the request but the server rejects which causes lineReadings = await response.unwrap() to throw an error so I added a try-catch with corresponding error notification around it, does that fix the issue? I didn't manage to find a good error message in src/client/app/translations/data.ts so I just wrote a unique one for now.

[huss]

I'm just noting I pushed a commit to clarify two comments since that was easy. Let me know if you have any thoughts.

[huss]

src/client/app/components/admin/PreferencesComponent.tsx has an issue that is stopping the admin from saving the default file size limits for export. It is because it sends a string and not a number when updated. The changes to route param checks now detects this. On line 42 I added the Number() as shown here:

setLocalAdminPref({ ...localAdminPref, [key]: Number(value) });

This seems to have fixed it. There are also other Number casts that can probably go away given this change but I did not check that.

I wanted to know if you want to do this as part of this PR or if it should be an issue (by you or me) for someone else. Okay either way.

[huss]

don't think I've been able to reproduce this. Could you elaborate on how you modified the file size check to force it to fail?

I went into the raw reading route (currently line 118) and changed it to const fileSize = 1000000; so it rejected the file size on check. This causes an error if you ask for a raw/meter readings export from the line graphic page on the UI.

I did think that maybe its a scenario where the client accepts the request but the server rejects which causes lineReadings = await response.unwrap() to throw an error so I added a try-catch with corresponding error notification around it, does that fix the issue? I didn't manage to find a good error message in src/client/app/translations/data.ts so I just wrote a unique one for now.

Yes, it now works by putting up an error msg rather than taking a while and showing nothing.

[huss]

@Nespina24 and @stockingstocker Thank you for the updates. Overall, they worked well. I have made a little over a half dozen comments to consider where about half are new and half are about previous items. I think all resolved items are done. To try to help me keep items needing my attention as future changes are made, I'm asking you not resolve any comments. You are welcome (encouraged) to put a thumbs up if you think you addressed it or put in a reply. Thanks. Please let me know if anything is not clear or you have thoughts/questions.

[stockingstocker]

* src/client/app/components/admin/PreferencesComponent.tsx has an issue that is stopping the admin from saving the default file size limits for export. It is because it sends a string and not a number when updated. The changes to route param checks now detects this. On line 42 I added the Number() as shown here:

setLocalAdminPref({ ...localAdminPref, [key]: Number(value) });

This seems to have fixed it. There are also other Number casts that can probably go away given this change but I did not check that.

With the Number cast on line 42, I was unable to change the default minimum and maximum reading date checks on the admin settings page. I believe this is because makeLocalChanges() is also used for non-numerical fields so casting value to Number breaks those fields. I opted to add Number casts for the fields defaultWarningFileSize and defaultFileSizeLimit instead and I was able to successfully save those preferences.

[huss]

I opted to add Number casts for the fields defaultWarningFileSize and defaultFileSizeLimit instead and I was able to successfully save those preferences.

Thanks for figuring that out. I looked at the code and tried it and it works well for me too.

[huss]

Noting that all comments before now are resolved and the code seems to work fine. Awaiting GitHub CI for final validation.

[huss]

@Nespina24 and @stockingstocker Thank you for the updates. Review and testing found everything is good. Congratulations on an accepted contribution to OED.