Implemented User Route MIddleware Tests for Issue 1527 - #1594
Implemented User Route MIddleware Tests for Issue 1527#1594Andrew-Bonner wants to merge 2 commits into
Conversation
Andrew-Bonner
changed the title
huss
left a comment
huss
left a comment
There was a problem hiding this comment.
@Andrew-Bonner Thank you for doing this work. I have made a number of comments to consider. I still need to verify it is testing everything correctly but I'm going to wait to do that once more commenting is in place and any changes are made. Please let me know if anything is not clear, you have thoughts or you would like help.
| .query(sharedQuery); | ||
| const res = await req.send(sharedBody); | ||
| //status code 400, 202, or 200 is considered a pass an a route was accessed | ||
| expect(res.status).to.be.oneOf([400, 202, 200]); |
There was a problem hiding this comment.
src/server/util/readingsUtils.js now has HTTP_CODE so the named value should be used. Note the location is likely to move per PR #1581 when it is merged.
I also see that the allowed responses vary by test. Maybe comments about why and why each test has what it does. Any chance they could be centralized if overlap?
| }); | ||
| }); | ||
| }); | ||
| //test export uaer against user route (just login - this should pass for every user that is given a role) |
| .set('token', token) | ||
| .query(sharedQuery); | ||
| const res = await req.send(sharedBody); | ||
| //user should be forbiden access with either a 401 or 403 |
There was a problem hiding this comment.
forbiden spelling. You should spell check everything as I'm not going to mark any more (if they exist).
| }); | ||
|
|
||
| //ADMIN USER TESTS | ||
| mocha.describe('ADMIN USER', () => { |
There was a problem hiding this comment.
This comment applies to all the following code. It seems you test for admin allowed here and then each not allowed user later. I would think the code would be much simpler if it tested them all together for the appropriate response. It is done that way in other places in OED (as I think we discussed) so I want to get your input on this. The same idea applies for all users.
Note there is a function within OED to tell you if a given role has the rights to another role.
| //test an export user against admin routes | ||
| mocha.describe('Admin Auth GET + POST Routes', () => { | ||
| routeData.admin.GET.forEach((route) => { | ||
| mocha.it(`GET ${route} - shouldnt allow export`, async () => { |
There was a problem hiding this comment.
shouldnt is not correct in a number of places.
|
@huss I am about to make this PR a draft and continue working on it. I was running the test and I ran into an issue where it is not recognizing the token that is defined in each test. This is a bit puzzling considering that it was ran less than a week ago and it was working. Nonetheless, until I figure out what the issue is, I am not going to make any alterations in response to any of the comments you have made thus far. |
@Andrew-Bonner Thank you for the update. OED would very much like to get this work included into the project to guarantee all is secure via testing. Also, other work is adding routes that are going to need to be added which cannot easily be done until this is merged. It is fine to take some time to get this to a good place but it would be nice if it can be finalized in the coming weeks. Please let me know if you have any thoughts, concerns or need any help. |
@Andrew-Bonner Thank you for your work on this. As the previous comment from 4/22/26 indicated, OED wanted this work finalized. Since it has not yet happened, I'm going to get someone else to complete this work unless you comment in the next few days that you intend to finish it soon. |
2 participants
Description
This change adds tests that verify all user roles against the various authMiddleware functions for each route to ensure proper access control is enforced where required. The tests also include a check that scans all routes in OED to confirm they are covered. Additionally, the tests flag any routes that are either missing from the test suite or included in the tests but no longer exist in OED.
Fixes #1527
(In general, OED likes to have at least one issue associated with each pull request. Replace [issue] with the OED GitHub issue number. In the preview you will see an issue description if you hover over that number. You can create one yourself before doing this pull request. This is where details are normally given on what is being addressed. Note you should not use the word "Fixes" if it does not completely address the issue since the issue would automatically be closed on merging the pull request. In that case use "Partly Addresses #[issue].)
Type of change
(Check the ones that apply by placing an "x" instead of the space in the [ ] so it becomes [x])
Checklist
(Note what you have done by placing an "x" instead of the space in the [ ] so it becomes [x]. It is hoped you do all of them.)
Limitations
(Describe any issues that remain or work that should still be done.)
Any routes that are flagged to be missing must be updated in the routes.json file.