feat(logs): POST /logs/search #2063
Conversation
|
| GitGuardian id | GitGuardian status | Secret | Commit | Filename | |
|---|---|---|---|---|---|
| 10531125 | Triggered | Username Password | 30e760b | packages/logs/lib/es/client.ts | View secret |
🛠 Guidelines to remediate hardcoded secrets
- Understand the implications of revoking this secret by investigating where it is used in your code.
- Replace and store your secret safely. Learn here the best practices.
- Revoke and rotate this secret.
- If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.
To avoid such incidents in the future consider
- following these best practices for managing and storing secrets including API keys and other credentials
- install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.
🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.
packages/logs/lib/models/messages.ts
Outdated
| @@ -26,19 +27,24 @@ export async function createMessage(row: MessageRow): Promise<void> { | |||
| /** | |||
| * List operations | |||
| */ | |||
| export async function listOperations(opts: { limit: number }): Promise<ListOperations> { | |||
| export async function listOperations(opts: { accountId: number; environmentId?: number; limit: number }): Promise<ListOperations> { | |||
| const q: opensearchtypes.QueryDslQueryContainer = { | |||
There was a problem hiding this comment.
Never a fan of single letter variables. If you call it query you can then just do
body: { query }
later
There was a problem hiding this comment.
yeah good point 👍🏻
| @@ -1,10 +1,11 @@ | |||
| import { client } from '../es/client.js'; | |||
| import type { MessageRow } from '../types/messages.js'; | |||
| import type { MessageRow, OperationRow } from '@nangohq/types'; | |||
There was a problem hiding this comment.
Can you explain the reasoning for calling this directory models?
There was a problem hiding this comment.
it's the data accessors, e.g all access to DBs or Opensearch are here. I initially planned to have more but in the end it's a single model so less useful
| @@ -46,7 +46,7 @@ services: | |||
| - '${SERVER_PORT:-3003}:${SERVER_PORT:-3003}' | |||
| depends_on: | |||
| - nango-db | |||
| - nanog-opensearch | |||
There was a problem hiding this comment.
sounds like a pokemon name. I like it
|
|
||
| export const searchLogs = asyncWrapper<SearchLogs>(async (req, res) => { | ||
| if (!envs.NANGO_LOGS_ENABLED) { | ||
| res.status(422).send({ error: { code: 'feature_disabled' } }); |
There was a problem hiding this comment.
yeah I wasn't sure because 404 means it the URL is invalid (basically), it's kind of invalid but also not and doesn't convey the same initial message. Happy to put 404
There was a problem hiding this comment.
404 means resource doesn't exist so it fits quite well I think. The error message conveys that is a feature that can be enabled
| return; | ||
| } | ||
|
|
||
| const emptyQuery = requireEmptyQuery(req, { withEnv: true }); |
There was a problem hiding this comment.
not sure why requireEmptyQuery is a function where below we do the validation of the body inline, or do you mean to use requireEmptyBody below?
There was a problem hiding this comment.
requireEmptyQuery is a utils to avoid all the endpoints that have no query params to rewrite this logic.
the body is not empty and specific to this endpoint so no shared logic to reuse.
Don't know if that makes sense
| ? [passport.authenticate('session'), authMiddleware.sessionAuth.bind(authMiddleware), rateLimiterMiddleware] | ||
| : isBasicAuthEnabled | ||
| ? [passport.authenticate('basic', { session: false }), authMiddleware.basicAuth.bind(authMiddleware), rateLimiterMiddleware] | ||
| : [authMiddleware.noAuth.bind(authMiddleware), rateLimiterMiddleware]; | ||
|
|
||
| if (isTest) { | ||
| webAuth = apiAuth; | ||
| } |
There was a problem hiding this comment.
is it for convenience so we don't have to do web auth in the tests?
There was a problem hiding this comment.
yes exactly, so we are "closer" to true e2e tests. That's what motivated me to have the unified context, so there is no difference between the auth
There was a problem hiding this comment.
sounds good. Can you add a comment before the if, so in 6months I remember why we have done that
There was a problem hiding this comment.
yeah good point
Describe your changes
Fixes NAN-846
Note
This is the proposed change for new API routes, discussed last week
New route
POST /api/v1/logs/searchPOST because it will require a lot of params that are easier to send with a json body.
Integration test
This is the first e2e API test. To achieve it I needed an auth method not tied to a session, so when it's in test mode we are using SecretKey auth, let me know if that's alright.
Add async wrapper to catch errors
Note that it won't be useful anymore after express 5 but it's still a long way before release