feat: api management #4557
feat: api management #4557
Conversation
|
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
WalkthroughThe recent update refines the management of Personal Access Tokens (PATs) in the query-service, enhancing capabilities for updating and revoking PATs, improving authentication processes, and refining data handling. Changes include renaming endpoints for consistency, adding new fields and validation functions for PATs, reorganizing authentication-related imports, and extending the database schema to support these new functionalities. Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
|
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
1 similar comment
|
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
|
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
1 similar comment
|
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 11
Configuration used: CodeRabbit UI
Files selected for processing (9)
- ee/query-service/app/api/api.go (1 hunks)
- ee/query-service/app/api/pat.go (5 hunks)
- ee/query-service/app/server.go (6 hunks)
- ee/query-service/auth/auth.go (1 hunks)
- ee/query-service/dao/interface.go (1 hunks)
- ee/query-service/dao/sqlite/modelDao.go (4 hunks)
- ee/query-service/dao/sqlite/pat.go (4 hunks)
- ee/query-service/model/pat.go (1 hunks)
- pkg/query-service/telemetry/telemetry.go (3 hunks)
Additional comments: 7
ee/query-service/app/api/api.go (1)
- 155-158: The route registration for PAT management uses plural forms (
/api/v1/pats) which aligns with the PR objectives. However, ensure that all client-side references to these endpoints are also updated to reflect the new endpoint URLs.pkg/query-service/telemetry/telemetry.go (3)
- 155-155: The addition of
patTokenUserto theTelemetrystruct aligns with the PR's objective to enhance telemetry with PAT token user information.- 247-249: Resetting
patTokenUsertofalseafter sending telemetry data ensures that the flag is only true for the current telemetry event where a PAT token user is involved. This is a good practice for state management within the telemetry system.- 352-354: The
SetPatTokenUsermethod is straightforward and correctly sets thepatTokenUserfield totrue. This method provides a clear way to mark telemetry events as being triggered by a PAT token user.ee/query-service/app/server.go (3)
- 23-27: Changing the import alias from
authtobaseauthfor thequery-service/authpackage clarifies that it's the base authentication functionality being used, distinguishing it from potentially extended authentication logic in the enterprise edition. This change improves code readability and maintainability.- 429-429: Using
baseauth.GetEmailFromJwtto extract the user email from the JWT in the request context is a direct application of the updated import alias. This usage is correct and aligns with the PR's objectives related to enhancing authentication handling.- 450-456: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [453-472]
The inclusion of
baseauth.AttachJwtToContextin theanalyticsMiddlewarefunction ensures that the JWT token, if present, is attached to the request context for downstream use. This is a good practice for middleware that requires access to authenticated user information. However, ensure that theAttachJwtToContextmethod properly handles cases where a JWT token is not present or is invalid.
|
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
1 similar comment
|
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
|
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (4)
- ee/query-service/app/api/pat.go (5 hunks)
- ee/query-service/dao/interface.go (1 hunks)
- ee/query-service/dao/sqlite/pat.go (5 hunks)
- ee/query-service/model/pat.go (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- ee/query-service/dao/interface.go
- ee/query-service/dao/sqlite/pat.go
Additional comments: 7
ee/query-service/model/pat.go (2)
- 3-10: The
Userstruct introduces fields for user details. Ensure that theNotFoundfield is appropriately used in the application logic to handle cases where a user is not found, rather than throwing an error or exception.- 13-25: The
PATstruct now includes fields for managing and tracking PATs more effectively. Verify that theUpdatedAt,LastUsed, andRevokedfields are updated correctly in all relevant operations within the application to ensure accurate tracking and management of PATs.ee/query-service/app/api/pat.go (5)
- 45-48: As previously mentioned, consider moving the
validatePATRequestcall after authorization checks to avoid unnecessary processing for unauthorized requests.- 51-60: In the
createPATfunction, ensure that theExpiresAtfield calculation from days to Unix timestamp is correct and accounts for leap years and time zones if necessary.- 73-83: The
validatePATRequestfunction checks for valid roles,expiresAt, and name. Ensure that all possible roles (ViewerGroup,EditorGroup,AdminGroup) are correctly defined and used throughout the application to prevent unauthorized access or incorrect role assignments.- 86-121: In the
updatePATfunction, verify that theUpdatedByUserIDis correctly set and used in all relevant database operations to track who last updated the PAT. Also, ensure that theUpdatedAtfield is correctly updated to reflect the current time at the point of update.- 158-171: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [142-170]
In the
revokePATfunction, renamed fromdeletePAT, ensure that the revocation logic correctly updates theRevokedfield in the database and that this change is reflected in all queries that should exclude revoked PATs.
|
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- ee/query-service/app/api/pat.go (5 hunks)
- ee/query-service/model/pat.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- ee/query-service/model/pat.go
Additional comments: 8
ee/query-service/app/api/pat.go (8)
- 15-15: The addition of
baseconstantsimport is appropriate for the use of constants in role validation within thevalidatePATRequestfunction. This change aligns with the PR objectives of enhancing validation and introducing role-based access control.- 32-32: Changing the type from
model.PATtomodel.CreatePATRequestBodyin thecreatePATfunction is a good practice. It likely reflects a more accurate representation of the data being handled at this stage of the PAT creation process, enhancing code readability and maintainability.- 45-49: The initialization of the
patvariable with fields fromreqis correctly done. However, it's important to ensure thatExpiresInDaysfrom the request is correctly converted to a Unix timestamp later in the code to maintain the correct data type and value forExpiresAt.- 50-54: The call to
validatePATRequestbefore proceeding with PAT creation is a good practice for ensuring input validation. This aligns with the PR objectives of enhancing validation and security. It's crucial that this validation logic is comprehensive and correctly implemented.- 63-66: The logic for converting
ExpiresAtfrom days to a Unix timestamp is correct and necessary for the functionality described in the PR. This ensures that the expiration time is stored in a consistent and usable format in the database.- 78-88: The
validatePATRequestfunction correctly checks for the presence and validity of required fields (Role,ExpiresAt,Name). However, ensure that the role validation logic (req.Role != baseconstants.ViewerGroup && req.Role != baseconstants.EditorGroup && req.Role != baseconstants.AdminGroup) is comprehensive and aligns with the application's role definitions and requirements.- 91-125: The
updatePATfunction correctly decodes the request, validates the input, and updates the PAT information. It's important to ensure that theUpdatePATmethod in the data access layer correctly handles the update logic, especially concerning fields that can be updated and how theUpdatedAttimestamp is managed.- 163-176: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [147-175]
Renaming
deletePATtorevokePATand updating the corresponding logic to revoke a PAT instead of deleting it is in line with the PR objectives. This change allows for a more nuanced management of PATs, where tokens can be disabled without being removed from the database. Ensure that theRevokePATmethod in the data access layer correctly implements the revocation logic, particularly in setting therevokedstatus without actually deleting the record.
Summary
Updates PAT crud APIs.
Endpoint updated to
/patsAdd new columns to PAT table
Update delete API to revoke
Introduce role for API
Add telemetry event in heart beat for PAT usage
PRD: https://www.notion.so/signoz/PRD-Ability-to-manage-API-keys-in-the-UI-4b0f64d6f5c4434c8abf082d15c702a5#8bb1f72327d24aa89315ef4017e37577
Design: https://www.figma.com/file/EgkXVTQCosEQBfNB27YNKo/Signoz-Interface?type=design&mode=design
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Improvements
Refactor
Database Changes
personal_access_tokenstable to accommodate new columns for improved PAT management.