Skip to content

Commit

Permalink
Fix Push API Key validation in diagnose report (#416)
Browse files Browse the repository at this point in the history
The Push API key validation wasn't implemented previously. I've now
implemented a validation by calling the AppSignal.com auth endpoint with
the configured Push API key.

The http(s) library doesn't a way to make synchronous requests that I
could find, so I had to update the diagnose tool and script to be async,
by using the async and await keywords all the way up the chain of
functions.

The rest of the implementation such as the coloring of the output, and
storing of the result on the report, was already implemented.

I've updated the diagnose tests submodule to account for this change in
behavior in the tests.

Fix #408
  • Loading branch information
tombruijn authored Jul 16, 2021
1 parent baa2b7f commit 7bb5398
Show file tree
Hide file tree
Showing 5 changed files with 58 additions and 21 deletions.
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
bump: "patch"
---

Fix the validation of Push API key in the diagnose report. It would always print "valid" even if the key was not set or invalid.
36 changes: 20 additions & 16 deletions packages/nodejs/src/__tests__/diagnose.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,8 @@ describe("DiagnoseTool", () => {
tool = new DiagnoseTool({})
})

it("generates a configuration", () => {
const output = tool.generate()
it("generates a configuration", async () => {
const output = await tool.generate()

expect(output.library.package_version).toEqual(VERSION)
expect(output.library.agent_version).toEqual(AGENT_VERSION)
Expand All @@ -28,31 +28,33 @@ describe("DiagnoseTool", () => {
expect(output.process.uid).toEqual(process.getuid())
})

it("returns the log_dir_path", () => {
expect(tool.generate().paths.log_dir_path.path).toEqual("/tmp")
it("returns the log_dir_path", async () => {
const report = await tool.generate()
expect(report.paths.log_dir_path.path).toEqual("/tmp")
})

describe("appsignal.log path", () => {
it("returns the appsignal.log path", () => {
expect(tool.generate().paths["appsignal.log"].path).toEqual(
"/tmp/appsignal.log"
)
it("returns the appsignal.log path", async () => {
const report = await tool.generate()
expect(report.paths["appsignal.log"].path).toEqual("/tmp/appsignal.log")
})

it("returns all of the file if file is less than 2MiB in size", () => {
it("returns all of the file if file is less than 2MiB in size", async () => {
const fileSize = 1.9 * 1024 * 1024 // Write 1.9 MiB of content
const content = ".".repeat(fileSize)
fs.writeFileSync("/tmp/appsignal.log", content, { flag: "w" })

const contentArray = tool.generate().paths["appsignal.log"].content
const report = await tool.generate()
const contentArray = report.paths["appsignal.log"].content
expect(contentArray?.join("\n").length).toBeCloseTo(fileSize, 0)
})

it("returns maximum 2MiB of content", () => {
it("returns maximum 2MiB of content", async () => {
const content = ".".repeat(2.1 * 1024 * 1024) // Write 2.1 MiB of content
fs.writeFileSync("/tmp/appsignal.log", content, { flag: "w" })

const contentArray = tool.generate().paths["appsignal.log"].content
const report = await tool.generate()
const contentArray = report.paths["appsignal.log"].content
expect(contentArray?.join("\n").length).toEqual(2 * 1024 * 1024)
})
})
Expand All @@ -63,12 +65,14 @@ describe("DiagnoseTool", () => {
tool = new DiagnoseTool({})
})

it("returns the log_dir_path", () => {
expect(tool.generate().paths.log_dir_path.path).toEqual("/path/to")
it("returns the log_dir_path", async () => {
const report = await tool.generate()
expect(report.paths.log_dir_path.path).toEqual("/path/to")
})

it("returns the appsignal.log path", () => {
expect(tool.generate().paths["appsignal.log"].path).toEqual(
it("returns the appsignal.log path", async () => {
const report = await tool.generate()
expect(report.paths["appsignal.log"].path).toEqual(
"/path/to/appsignal.log"
)
})
Expand Down
4 changes: 2 additions & 2 deletions packages/nodejs/src/cli/diagnose.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,8 @@ const util = require("util")
const readline = require("readline")

export class Diagnose {
public run() {
const data = new DiagnoseTool({}).generate()
public async run() {
const data = await new DiagnoseTool({}).generate()

console.log(`AppSignal diagnose`)
console.log(`=`.repeat(80))
Expand Down
32 changes: 30 additions & 2 deletions packages/nodejs/src/diagnose.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
import fs from "fs"
import path from "path"
import https from "https"
import { URL, URLSearchParams } from "url"
import { createHash } from "crypto"

import { Agent } from "./agent"
Expand Down Expand Up @@ -33,7 +35,11 @@ export class DiagnoseTool {
* Reports are serialized to JSON and send to an endpoint that expects
* snake_case keys, thus the keys in the report on this side must be snake cased also.
*/
public generate() {
public async generate() {
let pushApiKeyValidation
await this.validatePushApiKey()
.then(result => (pushApiKeyValidation = result))
.catch(result => (pushApiKeyValidation = result))
return {
library: this.getLibraryData(),
installation: this.getInstallationReport(),
Expand All @@ -44,7 +50,7 @@ export class DiagnoseTool {
options: this.getConfigData(),
sources: {}
},
validation: { push_api_key: "valid" },
validation: { push_api_key: pushApiKeyValidation },
process: {
uid: process.getuid()
},
Expand Down Expand Up @@ -86,6 +92,28 @@ export class DiagnoseTool {
}
}

private async validatePushApiKey() {
return new Promise((resolve, reject) => {
const config = this.#config.data
const params = new URLSearchParams({ api_key: config["apiKey"] })
const url = new URL(`/1/auth?${params.toString()}`, config["endpoint"])
const options = { method: "POST" }

const request = https.request(url, options, function (response) {
const status = response.statusCode
if (status === 200) {
resolve("valid")
} else if (status === 401) {
reject("invalid")
} else {
reject(`Failed with status ${status}`)
}
})
request.write("{}") // Send empty JSON body
request.end()
})
}

private getPathsData() {
const paths: { [key: string]: FileMetadata } = {}

Expand Down
2 changes: 1 addition & 1 deletion test/integration/diagnose

0 comments on commit 7bb5398

Please sign in to comment.