fix: refactor auth package

[nityanandagohain]

Fixes https://github.com/SigNoz/platform-pod/issues/458

---

Important

Refactors authentication package by introducing authtypes.JWT for JWT handling, updating context management, and modifying middleware and server setup accordingly.

• JWT Refactor:
  • Introduces authtypes.JWT for JWT handling, replacing old JWT logic.
  • Updates Login, Register, and other auth functions to use authtypes.JWT.
  • Removes auth0/go-jwt-middleware and github.com/golang-jwt/jwt dependencies.
• Context Management:
  • Adds ContextFromRequest in authtypes.JWT for extracting JWT from request headers.
  • Introduces authtypes.UUID for managing UUIDs in context.
• Middleware and Server:
  • Updates middleware/auth.go to use new JWT handling.
  • Modifies server.go to integrate new JWT and UUID context management.
• Tests:
  • Updates integration tests to use new JWT handling.
  • Adds tests for authtypes.JWT in jwt_test.go.

^{This description was created by for 28f4dfa. It will automatically update as commits are pushed.}

[ellipsis-dev]

❌ Changes requested. Reviewed everything up to 5b73c0d in 2 minutes and 34 seconds

More details

• Looked at 1942 lines of code in 31 files
• Skipped 0 files when reviewing.
• Skipped posting 4 drafted comments based on config settings.

1. pkg/types/authtypes/jwt.go:65

• Draft comment:
  Consider handling the case where the token is not valid more robustly - the error returned by jwt.Parse is wrapped into a generic error string. This might affect debugging and error logging.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50%
  While returning raw errors might preserve more detail, wrapping errors with context is generally good practice in Go. The current error wrapping provides useful context about where/why the error occurred. The TODO suggests this code will be refactored anyway. The suggestion seems more like a style preference than a clear improvement.
  The comment could be right that preserving raw JWT errors might help with debugging specific JWT parsing issues. Error wrapping does hide the underlying error type.
  However, error wrapping with context is an established Go pattern. The current approach follows good practices by adding context while still including the underlying error details via %v.
  The comment suggests a style change that's not clearly better than the current approach. The code is already flagged for refactoring with ParseWithClaims.

2. pkg/types/authtypes/jwt.go:152

• Draft comment:
  The getStringClaim helper function returns an empty string if the key is missing. Consider whether it's appropriate or if it should return an error to distinguish between a missing key and an empty string value.
• Reason this comment was not posted:
  Confidence changes required: 50% <= threshold 50%
  None

3. pkg/types/authtypes/jwt.go:48

• Draft comment:
  When splitting the Authorization header by "Bearer ", check that the resulting slice has at least 2 elements to avoid an index out of range panic if the header is malformed.
• Reason this comment was not posted:
  Marked as duplicate.

4. pkg/types/authtypes/jwt.go:48

• Draft comment:
  The Authorization header is fetched twice; consider storing its value in a variable to avoid redundant calls.
• Reason this comment was not posted:
  Confidence changes required: 50% <= threshold 50%
  None

Workflow ID: wflow_vud220wkbvCLrLev

---

Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

[ellipsis-dev]

👍 Looks good to me! Incremental review on 20e16af in 1 minute and 15 seconds

More details

• Looked at 6048 lines of code in 66 files
• Skipped 0 files when reviewing.
• Skipped posting 9 drafted comments based on config settings.

1. pkg/query-service/tests/integration/signoz_integrations_test.go:29

• Draft comment:
  Test file is comprehensive and clearly covers the lifecycle of SigNoz integrations, dashboards, and pipelines. The use of require assertions is appropriate and error messages are helpful.
• Reason this comment was not posted:
  Confidence changes required: 0% <= threshold 50%
  None

2. pkg/query-service/tests/integration/signoz_integrations_test.go:114

• Draft comment:
  The tests for log pipelines and dashboard integrations are well-structured, verifying ordering, enabling/disabling, and uninstallation behavior.
• Reason this comment was not posted:
  Confidence changes required: 0% <= threshold 50%
  None

3. pkg/query-service/tests/integration/signoz_integrations_test.go:297

• Draft comment:
  Dashboard tests correctly assert that dashboards for installed integrations appear and are removed upon uninstallation.
• Reason this comment was not posted:
  Confidence changes required: 0% <= threshold 50%
  None

4. pkg/query-service/tests/integration/signoz_integrations_test.go:80

• Draft comment:
  Clarify the unit conversion for testLog.Timestamp (dividing by 1,000,000) so readers know if the log timestamp is in nanoseconds or microseconds.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

5. pkg/query-service/tests/integration/signoz_integrations_test.go:49

• Draft comment:
  Instead of directly indexing availableIntegrations[0] to check installation status, consider searching for the expected integration by its ID to avoid dependency on list order.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

6. pkg/query-service/tests/integration/signoz_integrations_test.go:223

• Draft comment:
  Reversing the pipelines slice with slices.Reverse modifies the order in place; ensure that this mutation does not affect subsequent assertions or rely on implicit sort ordering.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

7. pkg/query-service/tests/integration/signoz_integrations_test.go:373

• Draft comment:
  Multiple helper methods (e.g., GetAvailableIntegrationsFromQS, GetIntegrationDetailsFromQS) repetitively marshal and unmarshal result.Data. Consider abstracting this parsing logic to reduce boilerplate and improve readability.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

8. pkg/query-service/tests/integration/signoz_integrations_test.go:344

• Draft comment:
  The dashboard test assumes the dashboard list order (using dashboards[0]) for timestamp checks. It might be more robust to match dashboards by integration ID rather than relying on index positions.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

9. pkg/query-service/tests/integration/signoz_integrations_test.go:30

• Draft comment:
  Consider adding brief comments at the start of each test (e.g., integration lifecycle, pipeline behavior, dashboard handling) to improve clarity for future maintainers.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

Workflow ID: wflow_pJynTcSAst6pOGg6

---

You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

[ellipsis-dev]

👍 Looks good to me! Incremental review on 8ddc958 in 1 minute and 24 seconds

More details

• Looked at 169 lines of code in 3 files
• Skipped 0 files when reviewing.
• Skipped posting 5 drafted comments based on config settings.

1. pkg/types/authtypes/jwt.go:48

• Draft comment:
  Redundant header retrieval: line 43 gets Authorization header, then line 48 repeats. Consider reusing the value to improve clarity.
• Reason this comment was not posted:
  Comment was on unchanged code.

2. pkg/types/authtypes/jwt.go:51

• Draft comment:
  Consider renaming the local variable 'jwt' to avoid confusion with the jwt package name.
• Reason this comment was not posted:
  Confidence changes required: 33% <= threshold 50%
  None

3. pkg/types/authtypes/jwt.go:43

• Draft comment:
  Redundant header retrieval in GetJwtFromRequest: the Authorization header is fetched twice. Consider reusing the already retrieved value.
• Reason this comment was not posted:
  Confidence changes required: 33% <= threshold 50%
  None

4. pkg/types/authtypes/jwt.go:50

• Draft comment:
  Local variable 'jwt' shadows the imported jwt package. Consider renaming it (e.g., tokenStr) to avoid confusion.
• Reason this comment was not posted:
  Confidence changes required: 33% <= threshold 50%
  None

5. pkg/types/authtypes/jwt.go:105

• Draft comment:
  In signToken, consider using your custom Claims type (e.g., &Claims{...}) instead of jwt.MapClaims for consistency with GetJwtClaims.
• Reason this comment was not posted:
  Confidence changes required: 33% <= threshold 50%
  None

Workflow ID: wflow_AXC355YsfMA10ykc

---

You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

[ellipsis-dev]

👍 Looks good to me! Incremental review on be3f329 in 1 minute and 59 seconds

More details

• Looked at 808 lines of code in 19 files
• Skipped 0 files when reviewing.
• Skipped posting 6 drafted comments based on config settings.

1. pkg/types/authtypes/jwt_test.go:29

• Draft comment:
  TestGetJwtFromRequest correctly extracts the token from the Authorization header. Consider adding a test case to verify extraction from the Sec-WebSocket-Protocol header as documented in GetJwtFromRequest.
• Reason this comment was not posted:
  Confidence changes required: 30% <= threshold 50%
  None

2. pkg/types/authtypes/jwt_test.go:80

• Draft comment:
  The tests for invalid token, expired token, and invalid signature are comprehensive. Note that error messages could vary when the underlying jwt library updates. Consider using error type assertions or constants for more robust tests.
• Reason this comment was not posted:
  Confidence changes required: 30% <= threshold 50%
  None

3. pkg/query-service/rules/db.go:227

• Draft comment:
  Consider using parameterized queries instead of fmt.Sprintf for building SQL queries. Even though the ID is numeric, using placeholders (e.g. $1) avoids potential SQL injection risks and improves consistency.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

4. pkg/types/authtypes/jwt.go:42

• Draft comment:
  Avoid fetching the same header value twice. Store the value of the 'Authorization' header in a variable to improve clarity and avoid redundant calls.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

5. pkg/query-service/auth/auth.go:77

• Draft comment:
  In the Invite function, if claims extraction fails, the code wraps a nil error. Instead, return a new error (e.g. errors.New("failed to extract admin user id")) rather than wrapping an undefined 'err'.
• Reason this comment was not posted:
  Comment was on unchanged code.

6. pkg/query-service/rules/db.go:151

• Draft comment:
  There is commented‐out transaction code in EditRuleTx. Consider addressing the commented code (or removing it) so that transaction handling is consistent and database locking issues can be properly diagnosed.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

Workflow ID: wflow_7ew4GQSfeqUItA3l

---

You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

[ellipsis-dev]

👍 Looks good to me! Incremental review on 71c16fe in 1 minute and 34 seconds

More details

• Looked at 13 lines of code in 1 files
• Skipped 0 files when reviewing.
• Skipped posting 3 drafted comments based on config settings.

1. pkg/query-service/rules/db.go:271

• Draft comment:
  Using authtypes.NewClaimsFromContext(ctx) instead of GetClaimsFromContext(ctx) appears correct. Ensure that NewClaimsFromContext returns the expected claims (with proper Email field) for subsequent use.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50%
  This comment violates several rules:

1. It asks for verification/confirmation ("Ensure that...")
2. It's speculative about potential issues
3. The change from GetClaimsFromContext to NewClaimsFromContext appears to be an internal refactor that the author would have tested
4. The comment doesn't point out a clear issue requiring code changes
   The comment could be valid if there was evidence that NewClaimsFromContext behaves differently than GetClaimsFromContext in terms of Email field handling.
   However, this appears to be an internal refactor where the author would have tested the basic functionality. Without clear evidence of a problem, asking for verification adds unnecessary noise.
   Delete this comment as it asks for verification without identifying a clear issue requiring code changes.

2. pkg/query-service/rules/db.go:271

• Draft comment:
  Replacing GetClaimsFromContext with NewClaimsFromContext is consistent with the auth refactor. Ensure that the new function returns the expected claims structure.
• Reason this comment was not posted:
  Confidence changes required: 0% <= threshold 50%
  None

3. pkg/query-service/rules/db.go:271

• Draft comment:
  The PR title and description mention a product feedback button fix, but this diff only refactors auth claim extraction. Confirm that all necessary changes for the issue are included.
• Reason this comment was not posted:
  Confidence changes required: 33% <= threshold 50%
  None

Workflow ID: wflow_Y9hvXMpo1saF7Ydu

---

You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

[ellipsis-dev]

👍 Looks good to me! Incremental review on b142efb in 58 seconds

More details

• Looked at 806 lines of code in 13 files
• Skipped 0 files when reviewing.
• Skipped posting 13 drafted comments based on config settings.

1. pkg/query-service/app/server.go:120

• Draft comment:
  Consider checking the error returned from json.Unmarshal in extractQueryRangeData for proper error handling.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

2. pkg/query-service/app/server.go:264

• Draft comment:
  Use descriptive log messages for server start-up; avoid using hardcoded messages if possible.
• Reason this comment was not posted:
  Confidence changes required: 30% <= threshold 50%
  None

3. pkg/http/middleware/analytics.go:97

• Draft comment:
  Pre-compiling regular expressions might improve performance over calling regexp.MatchString repeatedly.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

4. pkg/http/middleware/analytics.go:91

• Draft comment:
  Consider logging and handling errors from request body reading, rather than silently returning false.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

5. pkg/types/alertmanagertypes/channel_test.go:44

• Draft comment:
  Good use of assert.JSONEq to compare JSON strings; ensure test covers edge cases.
• Reason this comment was not posted:
  Confidence changes required: 20% <= threshold 50%
  None

6. frontend/src/components/CeleryTask/CeleryTaskDetail/CeleryTaskDetail.tsx:80

• Draft comment:
  Avoid inline styling in React components; consider moving style properties (e.g., background based on isDarkMode) to a CSS or styled component.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

7. frontend/src/components/CeleryTask/CeleryTaskGraph/CeleryTaskBar.tsx:168

• Draft comment:
  Instead of hardcoding color values (e.g., Color.BG_CHERRY_500), verify these are design tokens. Prefer using design tokens or CSS variables for consistency.
• Reason this comment was not posted:
  Confidence changes required: 50% <= threshold 50%
  None

8. frontend/src/components/CeleryTask/CeleryTaskGraph/CeleryTaskGraph.tsx:100

• Draft comment:
  Ensure that usage of default panel type falls back to the design token defined in constants rather than a literal.
• Reason this comment was not posted:
  Confidence changes required: 30% <= threshold 50%
  None

9. frontend/src/pages/Celery/CeleryTask/CeleryTask.tsx:14

• Draft comment:
  The task filter logging using logEvent is implemented correctly. Make sure event names are consistent across telemetry usage.
• Reason this comment was not posted:
  Confidence changes required: 20% <= threshold 50%
  None

10. pkg/query-service/app/server.go:483

• Draft comment:
  In makeRulesManager, consider adding error details from pqle.FromReader to aid in debugging pql engine creation failures.
• Reason this comment was not posted:
  Confidence changes required: 40% <= threshold 50%
  None

11. frontend/src/components/CeleryTask/CeleryTaskDetail/CeleryTaskDetail.tsx:80

• Draft comment:
  Avoid using inline styles in the Drawer component. Move these styles into a CSS class or use a styled component to maintain consistency and easier theming.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

12. frontend/src/components/CeleryTask/CeleryTaskDetail/CeleryTaskDetail.tsx:86

• Draft comment:
  Inline style is used for the closeIcon prop. Prefer defining a CSS class (or using styled components) for layout-related styles to improve maintainability.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

13. frontend/src/container/GridCardLayout/GridCard/index.tsx:259

• Draft comment:
  Avoid using inline styles (e.g. setting height and width to 100%) in the container div. Use a CSS class instead to ensure consistency with theming and future maintenance.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

Workflow ID: wflow_6F6sit1db7ixNsmK

---

You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

[ellipsis-dev]

👍 Looks good to me! Incremental review on 958b0cf in 1 minute and 20 seconds

More details

• Looked at 755 lines of code in 23 files
• Skipped 0 files when reviewing.
• Skipped posting 2 drafted comments based on config settings.

1. pkg/types/authtypes/uuid.go:26

• Draft comment:
  Consider revising the error message on line 27. Since this UUID context attachment is likely for API keys (e.g., PAT tokens) rather than a typical "Authorization header", a more descriptive message (e.g. "missing UUID or API key header") might improve clarity during debugging.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50%
  The code is in a package called 'authtypes' and handles UUIDs, but there's no clear evidence that this is specifically for API keys or PAT tokens. The current error message mentions "Authorization header" which could be correct - we don't have enough context to know what values are being passed in. The suggestion is speculative without more evidence about how this code is used.
  The code is in an auth package and handles UUIDs, so the reviewer's assumption about API keys could be correct. The current error message might be misleading if this isn't actually checking an Authorization header.
  Without seeing how this code is used or what values are being passed in, we can't be certain that the current message is wrong or that the suggested message is more accurate. We should err on the side of not making speculative changes.
  Delete the comment as it makes assumptions about the code's usage without clear evidence, and changing error messages without certainty could be misleading.

2. pkg/types/authtypes/uuid.go:25

• Draft comment:
  The error returned when no non‐empty header value is found says 'missing Authorization header', which may be misleading if this method is intended to extract a custom header (e.g. SIGNOZ-API-KEY). Consider updating the error message to better reflect the expected header value.
• Reason this comment was not posted:
  Marked as duplicate.

Workflow ID: wflow_x2jGpZGIbgDLqBhm

---

You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

[ellipsis-dev]

👍 Looks good to me! Incremental review on 28f4dfa in 1 minute and 38 seconds

More details

• Looked at 750 lines of code in 26 files
• Skipped 0 files when reviewing.
• Skipped posting 10 drafted comments based on config settings.

1. frontend/src/components/CeleryOverview/CeleryOverviewTable/CeleryOverviewTable.tsx:90

• Draft comment:
  Avoid inline styles (e.g. padding: 8) in the filter dropdown container. Consider moving such styling to a CSS class to maintain consistency with design tokens.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

2. frontend/src/container/CloudIntegrationPage/ServicesSection/ServicesTabs.tsx:67

• Draft comment:
  Avoid using inline style for width (e.g. width: '100%') on the Select component. Use a CSS class or design token for consistent theming.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

3. frontend/src/hooks/integration/aws/useAccountSettingsModal.ts:105

• Draft comment:
  Ensure region comparison uses sorted arrays when checking equality to avoid false negatives. The use of isEqual with sorted arrays is a good practice – just verify that accountRegions and selectedRegions are always normalized.
• Reason this comment was not posted:
  Confidence changes required: 30% <= threshold 50%
  None

4. frontend/src/pages/MessagingQueues/MQDetails/MessagingQueueOverview.tsx:90

• Draft comment:
  The nested ternary for determining 'detailType' is a bit hard to follow. Consider refactoring into a separate helper function for better readability.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

5. frontend/src/pages/MessagingQueues/MQDetails/MQTables/MQTables.tsx:267

• Draft comment:
  Avoid using JSON.stringify as a dependency in useEffect; consider deriving a stable hash or using proper dependencies to avoid unnecessary re-renders.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50%
  While using JSON.stringify in useEffect deps can cause performance issues, in this case it seems intentional - they want to detect any changes to the table data structure for analytics. The code is already using useRef to prevent duplicate logs. A more optimal solution might exist but the current approach works for this use case.
  The comment raises a valid performance concern. JSON.stringify can be expensive for large objects and could cause unnecessary re-renders.
  However, this is an analytics logging use case where we want to detect any data changes, and the code already has safeguards against duplicate logs. The performance impact is likely negligible given the context.
  The comment should be deleted as the current implementation, while not perfect, is intentional and serves its purpose for analytics tracking without major drawbacks.

6. frontend/src/components/CeleryOverview/CeleryOverviewTable/CeleryOverviewTable.tsx:90

• Draft comment:
  Avoid inline styles in the filter dropdown (e.g. padding: 8). Use a CSS class and design tokens for consistency.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

7. frontend/src/components/CeleryOverview/CeleryOverviewTable/CeleryOverviewTable.tsx:488

• Draft comment:
  Using JSON.stringify(tableData) in the dependency array can lead to unnecessary re-renders. Consider using a more stable dependency (or a deep-comparison hook) to track changes in tableData.
• Reason this comment was not posted:
  Marked as duplicate.

8. frontend/src/pages/MessagingQueues/MQDetails/DropRateView/DropRateView.tsx:236

• Draft comment:
  Similar to the table in CeleryOverviewTable, using JSON.stringify(tableData) as a dependency may be suboptimal. Consider a more efficient way to detect changes in tableData (e.g. a stable identifier or deep equality check).
• Reason this comment was not posted:
  Marked as duplicate.

9. frontend/src/pages/MessagingQueues/MQDetails/MQTables/MQTables.tsx:271

• Draft comment:
  The useEffect that logs table changes relies on JSON.stringify(tableData) as well. For large datasets this may be inefficient. Consider a more stable dependency or a memoized hash of the table data.
• Reason this comment was not posted:
  Marked as duplicate.

10. frontend/src/pages/MessagingQueues/MQDetails/MetricPage/MetricPage.tsx:128

• Draft comment:
  Good use of a ref to log the first graph render event. Just ensure that the condition for logging (newCount === 1) accurately reflects when data becomes available.
• Reason this comment was not posted:
  Confidence changes required: 20% <= threshold 50%
  None

Workflow ID: wflow_JkkGTVqIzQUVKajQ

---

You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.