chore: Make this.user optional instead of nullable for non-auth endpoints#38861
chore: Make this.user optional instead of nullable for non-auth endpoints#38861
Conversation
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
|
Co-authored-by: ggazzo <5263975+ggazzo@users.noreply.github.com>
Co-authored-by: ggazzo <5263975+ggazzo@users.noreply.github.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #38861 +/- ##
===========================================
- Coverage 70.60% 70.55% -0.05%
===========================================
Files 3189 3189
Lines 112703 112703
Branches 20436 20400 -36
===========================================
- Hits 79573 79517 -56
- Misses 31069 31124 +55
- Partials 2061 2062 +1
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
- Replace direct assignment of `this.user` with a local variable `user` for clarity. - Update token assignment to ensure proper non-null assertion. - Adjust success response to utilize the new `user` variable for consistency.
135383b to
4cdc18e
Compare
- Refactor user authentication in APIClass to streamline token handling and improve clarity. - Ensure proper setting of the `x-auth-token` header in the WebHookAPI class.
58c5027 to
1383c3d
Compare
|
/jira ARCH-1935 |
There was a problem hiding this comment.
2 issues found across 4 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/meteor/app/api/server/ApiClass.ts">
<violation number="1" location="apps/meteor/app/api/server/ApiClass.ts:832">
P1: `String(authToken)` coerces `null` to `"null"` when the header is absent, causing `Accounts._hashLoginToken` to hash the literal string `"null"` and assign it to `this.token` for all non-authenticated routes. Guard against a missing header before hashing.</violation>
<violation number="2" location="apps/meteor/app/api/server/ApiClass.ts:838">
P1: Debug `console.log` statement left in production code. This will log internal authentication state (`shouldPreventAnonymousRead`) on every unauthorized request.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| const shouldPreventUserRead = !this.user && options.authRequired; | ||
|
|
||
| if (shouldPreventAnonymousRead || shouldPreventUserRead) { | ||
| console.log('shouldPreventAnonymousRead', shouldPreventAnonymousRead); |
There was a problem hiding this comment.
P1: Debug console.log statement left in production code. This will log internal authentication state (shouldPreventAnonymousRead) on every unauthorized request.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/meteor/app/api/server/ApiClass.ts, line 838:
<comment>Debug `console.log` statement left in production code. This will log internal authentication state (`shouldPreventAnonymousRead`) on every unauthorized request.</comment>
<file context>
@@ -825,18 +825,17 @@ export class APIClass<TBasePath extends string = '', TOperations extends Record<
const shouldPreventUserRead = !this.user && options.authRequired;
if (shouldPreventAnonymousRead || shouldPreventUserRead) {
+ console.log('shouldPreventAnonymousRead', shouldPreventAnonymousRead);
const result = api.unauthorized('You must be logged in to do this.');
// compatibility with the old API
</file context>
| this.user = user!; | ||
| this.userId = this.user?._id; | ||
| const authToken = this.request.headers.get('x-auth-token'); | ||
| this.token = Accounts._hashLoginToken(String(authToken))!; |
There was a problem hiding this comment.
P1: String(authToken) coerces null to "null" when the header is absent, causing Accounts._hashLoginToken to hash the literal string "null" and assign it to this.token for all non-authenticated routes. Guard against a missing header before hashing.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/meteor/app/api/server/ApiClass.ts, line 832:
<comment>`String(authToken)` coerces `null` to `"null"` when the header is absent, causing `Accounts._hashLoginToken` to hash the literal string `"null"` and assign it to `this.token` for all non-authenticated routes. Guard against a missing header before hashing.</comment>
<file context>
@@ -825,18 +825,17 @@ export class APIClass<TBasePath extends string = '', TOperations extends Record<
+ this.user = user!;
+ this.userId = this.user?._id;
+ const authToken = this.request.headers.get('x-auth-token');
+ this.token = Accounts._hashLoginToken(String(authToken))!;
const shouldPreventAnonymousRead = !this.user && options.authOrAnonRequired && !settings.get('Accounts_AllowAnonymousRead');
</file context>
| this.token = Accounts._hashLoginToken(String(authToken))!; | |
| this.token = authToken ? Accounts._hashLoginToken(authToken) : undefined; |
This pull request refactors authentication handling and user type definitions in the API, and updates related test utilities for improved clarity and consistency. The main focus is on simplifying how user authentication and user objects are managed, especially in API route contexts and test setup.
API authentication and user handling improvements:
APIClass.tsto always hash the auth token and setthis.token, regardless of authentication requirements. Removed the conditional block that previously tied token assignment to authentication checks.uservariable instead ofthis.user, and improved error handling and response construction to reference this variable. [1] [2]API type definitions and context typing:
definition.tsto remove unnecessary null/undefined states, and adjustedTypedThisto use more accurate conditional typing foruseranduserIdbased on authentication requirements. [1] [2] [3]Integration API improvements:
x-auth-tokenheader in the route context, ensuring the integration token is properly propagated for downstream authentication.Test utility and test suite updates:
registerUsertest utility to accept nullable credentials and only set them if provided, improving flexibility for test scenarios. Adjusted test cases to passnullcredentials where anonymous registration is required. [1] [2] [3] [4] [5]Task: ARCH-1980