Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: improve metrics/logs for checkUserAccess/checkAdminAccess #139

Merged
merged 4 commits into from
May 22, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/)
and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.html).

## [Unreleased]
### Changed
- Improved metrics and logging for checkUserAccess and checkAdminAccess directives

## [1.40.4] - 2024-04-29

Expand Down
10 changes: 10 additions & 0 deletions node/clients/IdentityClient.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,17 @@
})
}

public async validateToken({ token }: { token: string }): Promise<any> {

Check warning on line 14 in node/clients/IdentityClient.ts

View workflow job for this annotation

GitHub Actions / QE / Lint Node.js

Unexpected any. Specify a different type
return this.http.post('/api/vtexid/credential/validate', { token })
}

public async getToken({
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The storefront-permissions app was not properly handling appkey/apptokens. Now we can get a token from identity for a given appkey/apptoken and allow/block requests properly.

appkey,
apptoken,
}: {
appkey: string
apptoken: string
}): Promise<any> {

Check warning on line 24 in node/clients/IdentityClient.ts

View workflow job for this annotation

GitHub Actions / QE / Lint Node.js

Unexpected any. Specify a different type
return this.http.post('/api/vtexid/apptoken/login', { appkey, apptoken })
}
}
93 changes: 48 additions & 45 deletions node/directives/checkAdminAccess.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import type { GraphQLField } from 'graphql'
import { defaultFieldResolver } from 'graphql'

import sendAuthMetric, { AuthMetric } from '../metrics/auth'
import { validateAdminToken, validateApiToken } from './helper'

export class CheckAdminAccess extends SchemaDirectiveVisitor {
public visitFieldDefinition(field: GraphQLField<any, any>) {
Expand All @@ -16,68 +17,70 @@ export class CheckAdminAccess extends SchemaDirectiveVisitor {
info: any
) => {
const {
vtex: { adminUserAuthToken, logger },
clients: { identity },
vtex: { adminUserAuthToken, storeUserAuthToken, logger },
} = context

const { hasAdminToken, hasValidAdminToken, hasCurrentValidAdminToken } =
await validateAdminToken(context, adminUserAuthToken as string)

const { hasApiToken, hasValidApiToken } = await validateApiToken(context)

const hasStoreToken = !!storeUserAuthToken // we don't need to validate store token

// now we emit a metric with all the collected data before we proceed
const operation = field.astNode?.name?.value ?? context.request.url
const metric = new AuthMetric(
context.vtex.account,
const userAgent = context?.request?.headers['user-agent'] as string
const caller = context?.request?.headers['x-vtex-caller'] as string
const forwardedHost = context?.request?.headers[
'x-forwarded-host'
] as string

const auditMetric = new AuthMetric(
context?.vtex?.account,
{
operation,
forwardedHost: context.request.header['x-forwarded-host'] as string,
caller: context.request.header['x-vtex-caller'] as string,
userAgent: context.request.header['user-agent'] as string,
hasAdminToken: !!adminUserAuthToken,
hasStoreToken: false,
hasApiToken: false,
forwardedHost,
caller,
userAgent,
hasAdminToken,
hasValidAdminToken,
hasApiToken,
hasValidApiToken,
hasStoreToken,
},
'CheckAdminAccess'
'CheckAdminAccessAudit'
)

if (!adminUserAuthToken) {
metric.error = 'No admin token provided'
sendAuthMetric(logger, metric)
sendAuthMetric(logger, auditMetric)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we validate all possible tokens above so we can emit a metric here with all the necessary data, and below we actually apply the validations and allow/block the request.


if (!hasAdminToken) {
logger.warn({
message: 'CheckAdminAccess: No admin token provided',
userAgent: context.request.header['user-agent'],
vtexCaller: context.request.header['x-vtex-caller'],
forwardedHost: context.request.header['x-forwarded-host'],
message: 'CheckAdminAccess: No token provided',
userAgent,
caller,
forwardedHost,
operation,
hasAdminToken,
hasValidAdminToken,
hasApiToken,
hasValidApiToken,
hasStoreToken,
})
throw new AuthenticationError('No token was provided')
}

try {
const authUser = await identity.validateToken({
token: adminUserAuthToken,
})

// This is the first step before actually enabling this code.
// For now we only log in case of errors, but in follow up commits
// we should also throw an exception inside this if in case of errors
if (!authUser?.audience || authUser?.audience !== 'admin') {
metric.error = 'Token is not an admin token'
sendAuthMetric(logger, metric)
logger.warn({
message: `CheckUserAccess: Token is not an admin token`,
userAgent: context.request.header['user-agent'],
vtexCaller: context.request.header['x-vtex-caller'],
forwardedHost: context.request.header['x-forwarded-host'],
operation,
})
}
} catch (err) {
metric.error = 'Invalid token'
sendAuthMetric(logger, metric)
if (!hasCurrentValidAdminToken) {
logger.warn({
error: err,
message: 'CheckAdminAccess: Invalid token',
userAgent: context.request.header['user-agent'],
vtexCaller: context.request.header['x-vtex-caller'],
forwardedHost: context.request.header['x-forwarded-host'],
userAgent,
caller,
forwardedHost,
operation,
token: adminUserAuthToken,
hasAdminToken,
hasValidAdminToken,
hasApiToken,
hasValidApiToken,
hasStoreToken,
})
throw new ForbiddenError('Unauthorized Access')
}
Expand Down
213 changes: 73 additions & 140 deletions node/directives/checkUserAccess.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,161 +3,94 @@ import type { GraphQLField } from 'graphql'
import { defaultFieldResolver } from 'graphql'
import { SchemaDirectiveVisitor } from 'graphql-tools'

import { getActiveUserByEmail } from '../resolvers/Queries/Users'
import sendAuthMetric, { AuthMetric } from '../metrics/auth'
import {
validateAdminToken,
validateApiToken,
validateStoreToken,
} from './helper'

export async function checkUserOrAdminTokenAccess(
ctx: Context,
operation?: string
) {
const {
vtex: { adminUserAuthToken, storeUserAuthToken, logger },
clients: { identity, vtexId },
} = ctx
export class CheckUserAccess extends SchemaDirectiveVisitor {
public visitFieldDefinition(field: GraphQLField<any, any>) {
const { resolve = defaultFieldResolver } = field

const metric = new AuthMetric(
ctx.vtex.account,
{
operation: operation ?? ctx.request.url,
forwardedHost: ctx.request.header['x-forwarded-host'] as string,
caller: ctx.request.header['x-vtex-caller'] as string,
userAgent: ctx.request.header['user-agent'] as string,
hasAdminToken: !!adminUserAuthToken,
hasStoreToken: !!storeUserAuthToken,
hasApiToken: false,
},
'CheckUserAccess'
)
field.resolve = async (
root: any,
args: any,
context: Context,
info: any
) => {
const {
vtex: { adminUserAuthToken, storeUserAuthToken, logger },
} = context

if (!adminUserAuthToken && !storeUserAuthToken) {
metric.error = 'No admin or store token was provided'
sendAuthMetric(logger, metric)
logger.warn({
message: `CheckUserAccess: No admin or store token was provided`,
userAgent: ctx.request.header['user-agent'],
vtexCaller: ctx.request.header['x-vtex-caller'],
forwardedHost: ctx.request.header['x-forwarded-host'],
operation,
})
throw new AuthenticationError('No admin or store token was provided')
}
const { hasAdminToken, hasValidAdminToken, hasCurrentValidAdminToken } =
await validateAdminToken(context, adminUserAuthToken as string)

const { hasApiToken, hasValidApiToken } = await validateApiToken(context)

const { hasStoreToken, hasValidStoreToken, hasCurrentValidStoreToken } =
await validateStoreToken(context, storeUserAuthToken as string)

// now we emit a metric with all the collected data before we proceed
const operation = field?.astNode?.name?.value ?? context?.request?.url
const userAgent = context?.request?.headers['user-agent'] as string
const caller = context?.request?.headers['x-vtex-caller'] as string
const forwardedHost = context?.request?.headers[
'x-forwarded-host'
] as string

if (adminUserAuthToken) {
try {
const authUser = await identity.validateToken({
token: adminUserAuthToken,
})
const auditMetric = new AuthMetric(
context?.vtex?.account,
{
operation,
forwardedHost,
caller,
userAgent,
hasAdminToken,
hasValidAdminToken,
hasApiToken,
hasValidApiToken,
hasStoreToken,
hasValidStoreToken,
},
'CheckUserAccessAudit'
)

sendAuthMetric(logger, auditMetric)

// This is the first step before actually enabling this code.
// For now we only log in case of errors, but in follow up commits
// we should also throw an exception inside this if in case of errors
if (!authUser?.audience || authUser?.audience !== 'admin') {
metric.error = 'Token is not an admin token'
sendAuthMetric(logger, metric)
if (!hasAdminToken && !hasStoreToken) {
logger.warn({
message: `CheckUserAccess: Token is not an admin token`,
userAgent: ctx.request.header['user-agent'],
vtexCaller: ctx.request.header['x-vtex-caller'],
forwardedHost: ctx.request.header['x-forwarded-host'],
message: 'CheckUserAccess: No token provided',
userAgent,
caller,
forwardedHost,
operation,
hasAdminToken,
hasValidAdminToken,
hasApiToken,
hasValidApiToken,
hasStoreToken,
})
throw new AuthenticationError('No token was provided')
}
} catch (err) {
metric.error = 'Invalid admin token'
sendAuthMetric(logger, metric)
logger.warn({
error: err,
message: `CheckUserAccess: Invalid admin token`,
userAgent: ctx.request.header['user-agent'],
vtexCaller: ctx.request.header['x-vtex-caller'],
forwardedHost: ctx.request.header['x-forwarded-host'],
operation,
})
throw new ForbiddenError('Unauthorized Access')
}
} else if (storeUserAuthToken) {
let authUser = null

try {
authUser = await vtexId.getAuthenticatedUser(storeUserAuthToken)
if (!authUser?.user) {
metric.error = 'No valid user found by store user token'
sendAuthMetric(logger, metric)
if (!hasCurrentValidAdminToken && !hasCurrentValidStoreToken) {
logger.warn({
message: `CheckUserAccess: No valid user found by store user token`,
userAgent: ctx.request.header['user-agent'],
vtexCaller: ctx.request.header['x-vtex-caller'],
forwardedHost: ctx.request.header['x-forwarded-host'],
message: `CheckUserAccess: Invalid token`,
userAgent,
caller,
forwardedHost,
operation,
hasAdminToken,
hasValidAdminToken,
hasApiToken,
hasValidApiToken,
hasStoreToken,
hasValidStoreToken,
})
authUser = null
} else {
// This is the first step before actually enabling this code.
// For now we only log in case of errors, but in follow up commits
// we will remove this additional try/catch and set authUser = null
// in case of errors
try {
const user = (await getActiveUserByEmail(
null,
{ email: authUser?.user },
ctx
)) as { roleId: string } | null

if (!user?.roleId) {
metric.error = 'No active user found by store user token'
sendAuthMetric(logger, metric)
logger.warn({
message: `CheckUserAccess: No active user found by store user token`,
userAgent: ctx.request.header['user-agent'],
vtexCaller: ctx.request.header['x-vtex-caller'],
forwardedHost: ctx.request.header['x-forwarded-host'],
operation,
})
}
} catch (err) {
metric.error = 'Error getting user by email'
sendAuthMetric(logger, metric)
logger.warn({
error: err,
message: `CheckUserAccess: Error getting user by email`,
userAgent: ctx.request.header['user-agent'],
vtexCaller: ctx.request.header['x-vtex-caller'],
forwardedHost: ctx.request.header['x-forwarded-host'],
operation,
})
}
throw new ForbiddenError('Unauthorized Access')
}
} catch (err) {
metric.error = 'Invalid store user token'
sendAuthMetric(logger, metric)
logger.warn({
error: err,
message: `CheckUserAccess: Invalid store user token`,
userAgent: ctx.request.header['user-agent'],
vtexCaller: ctx.request.header['x-vtex-caller'],
forwardedHost: ctx.request.header['x-forwarded-host'],
operation,
})
authUser = null
}

if (!authUser) {
throw new ForbiddenError('Unauthorized Access')
}
}
}

export class CheckUserAccess extends SchemaDirectiveVisitor {
public visitFieldDefinition(field: GraphQLField<any, any>) {
const { resolve = defaultFieldResolver } = field

field.resolve = async (
root: any,
args: any,
context: Context,
info: any
) => {
await checkUserOrAdminTokenAccess(context, field.astNode?.name?.value)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed this extra function call as there was no real gain with it and to keep same style as checkAdminAccess directive


return resolve(root, args, context, info)
}
Expand Down
Loading
Loading