Lf 4765 [draft] - for consideration

[Duncan-Brain]

Description

Addresses a comment in LF-4765

The main challenge with express is there seems to be no way to communicate between middlewares that the type on the request object have been narrowed. I feels like NextFunction type should do something like that but I dno.

This option casts the type in the route so that it is clear that it should be alongside checkScope() middleware.

Notes:

• checkJwt I don't think checks it, it just unpacks it -- something better could be done there to move the auth checks up.

Jira link:

Type of change

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

• Passes test case
• UI components visually reviewed on desktop view
• UI components visually reviewed on mobile view
• Other (please explain)

Checklist:

• I have commented my code, particularly in hard-to-understand areas
• My changes generate no new warnings
• I have added tests that prove my fix is effective or that my feature works
• New and existing unit tests pass locally with my changes
• The precommit and linting ran successfully
• I have added or updated language tags for text that's part of the UI
• I have ordered translation keys alphabetically (optional: run pnpm i18n to help with this)
• I have added the GNU General Public License to all new files

[Duncan-Brain]

Did not exist here prior to this PR.

[Duncan-Brain]

The main change 1...

[Duncan-Brain]

The main change 2...

[Duncan-Brain]

@kathyavini Did you end up seeing any value with merging these typescript formalities? Or can I close the PR?

[kathyavini]

@Duncan-Brain do you want to close it because you don't think it's the best approach anymore? I wasn't sure where that conversation in the other thread landed -- particularly if you were in agreement with Anto that the benefit was too marginal for the diff (then yes, please do close) or if you still think this is the more thorough/correct solution.

If the second, since the @ts-expect-error landed on being non-blocking for the other ticket, I don't think we would have to be in a rush to either close or evaluate this one, but it could be undrafted and put forward as a proposal.

[Duncan-Brain]

@kathyavini I do think it is the more thorough... but any help evaluating that is appreciated (obv no rush as you said)

I think the actual lines of code for the fix are lowish.. the types and getScope.ts migration being generally useful...

The main benefits as I see them are

• req.auth is more fully scoped about what is being put on the object at different points in the app
• Specifying the request type in the controller (and casting it in the route) should help us in the future avoid 'farm_id type should not be null errors' and correctly type controllers
• The typing assertion is in context, so while not perfect it is hopefully clear -- but that may be subjective.

Drawbacks

• routes file looks maybe messy to me
• if we ever use a validation package like zod this may change
• type assertion is hopefully clear with its context and jsdoc but maybe not for everyone
• TODO: Still need to check that getScope is never called by anything that skips auth checks (that one auth check added in there)

My understanding is express typings are not great so tradeoffs are probably necessary.