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.

Sign up for GitHub

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

feat: add directive to validate auth token and feature flag #122

Closed
wants to merge 2 commits into from
Closed

feat: add directive to validate auth token and feature flag #122

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
49e4711
feat: add directive to validate auth token and feature flag
Rudge Nov 15, 2023
d8055a7
fix: adjust check user logs.
Rudge Nov 27, 2023
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
4 changes: 4 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.

## [Unreleased]

### Added

- add new directive to validate auth token, with feature flag to enable the validation

## [1.37.2] - 2023-11-10

### Fixed
Expand Down
1 change: 1 addition & 0 deletions graphql/directives.graphql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,3 +4,4 @@ directive @withSession on FIELD_DEFINITION
directive @withSender on FIELD_DEFINITION
directive @withUserPermissions on FIELD_DEFINITION
directive @auditAccess on FIELD | FIELD_DEFINITION
directive @checkAccessWithFeatureFlag on FIELD | FIELD_DEFINITION
38 changes: 33 additions & 5 deletions graphql/schema.graphql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ type Query {
@settings(settingsType: "workspace")
@withSender
@auditAccess
@checkAccessWithFeatureFlag

getRole(id: ID!): Role @cacheControl(scope: PRIVATE, maxAge: SHORT)

Expand All @@ -15,6 +16,7 @@ type Query {
@withSession
@withSender
@auditAccess
@checkAccessWithFeatureFlag

getFeaturesByModule(module: String!): Feature
@settings(settingsType: "workspace")
Expand All @@ -25,6 +27,7 @@ type Query {
@cacheControl(scope: PRIVATE, maxAge: SHORT)
@withSender
@auditAccess
@checkAccessWithFeatureFlag

getUser(id: ID!): User @cacheControl(scope: PRIVATE)
getB2BUser(id: ID!): User @cacheControl(scope: PRIVATE)
Expand All @@ -33,22 +36,27 @@ type Query {
@cacheControl(scope: PRIVATE)
@withSender
@auditAccess
@checkAccessWithFeatureFlag

getUserByEmail(email: String!): [User]
@cacheControl(scope: PRIVATE)
@withSender
@auditAccess
@checkAccessWithFeatureFlag

listAllUsers: [User]
@cacheControl(scope: PRIVATE, maxAge: SHORT)
@withSender
@auditAccess
@checkAccessWithFeatureFlag

listUsers(organizationId: ID, costCenterId: ID, roleId: ID): [User]
@cacheControl(scope: PRIVATE, maxAge: SHORT)
@deprecated(
reason: "This query is deprecated, use listUsersPaginated query instead."
)
@auditAccess
@checkAccessWithFeatureFlag

listUsersPaginated(
organizationId: ID
Expand All @@ -59,7 +67,10 @@ type Query {
search: String
sortOrder: String
sortedBy: String
): UserPagination @cacheControl(scope: PRIVATE, maxAge: SHORT)
): UserPagination
@cacheControl(scope: PRIVATE, maxAge: SHORT)
@auditAccess
@checkAccessWithFeatureFlag

checkImpersonation: UserImpersonation
@settings(settingsType: "workspace")
Expand All @@ -72,14 +83,25 @@ type Query {
@withSender
@cacheControl(scope: PRIVATE)

getSessionWatcher: Boolean @cacheControl(scope: PRIVATE)
getSessionWatcher: Boolean
@cacheControl(scope: PRIVATE)
@auditAccess
@checkAccessWithFeatureFlag

getUsersByEmail(email: String!): [User] @cacheControl(scope: PRIVATE)
getUsersByEmail(email: String!): [User]
@cacheControl(scope: PRIVATE)
@auditAccess
@checkAccessWithFeatureFlag

getActiveUserByEmail(email: String!): User @cacheControl(scope: PRIVATE)
getActiveUserByEmail(email: String!): User
@cacheControl(scope: PRIVATE)
@auditAccess
@checkAccessWithFeatureFlag

getOrganizationsByEmail(email: String!): [Organization]
@cacheControl(scope: PRIVATE)
@auditAccess
@checkAccessWithFeatureFlag
}

type Mutation {
Expand All @@ -90,12 +112,17 @@ type Mutation {
name: String!
slug: String
features: [FeatureInput]
): MutationResponse @cacheControl(scope: PRIVATE) @withSender @auditAccess
): MutationResponse
@cacheControl(scope: PRIVATE)
@withSender
@auditAccess
@checkAccessWithFeatureFlag

deleteRole(id: ID!): MutationResponse
@cacheControl(scope: PRIVATE)
@withSender
@auditAccess
@checkAccessWithFeatureFlag

saveUser(
id: ID
Expand Down Expand Up @@ -160,6 +187,7 @@ type Mutation {
@cacheControl(scope: PRIVATE)
@withSender
@auditAccess
@checkAccessWithFeatureFlag
}

type UserImpersonation {
Expand Down
34 changes: 34 additions & 0 deletions node/directives/checkAccessWithFeatureFlag.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
import type { GraphQLField } from 'graphql'
import { defaultFieldResolver } from 'graphql'
import { SchemaDirectiveVisitor } from 'graphql-tools'

import { checkUserOrAdminTokenAccess } from './checkUserAccess'

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

field.resolve = async (
root: any,
args: any,
context: Context,
info: any
) => {
const {
clients: { masterdata },
} = context

const config: { enable: boolean } = await masterdata.getDocument({
dataEntity: 'auth_validation_config',
fields: ['enable'],
id: 'storefront-permissions',
})

if (config?.enable) {
await checkUserOrAdminTokenAccess(context, field.astNode?.name?.value)
}

return resolve(root, args, context, info)
}
}
}
97 changes: 56 additions & 41 deletions node/directives/checkUserAccess.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,61 @@ import type { GraphQLField } from 'graphql'
import { defaultFieldResolver } from 'graphql'
import { SchemaDirectiveVisitor } from 'graphql-tools'

export async function checkUserOrAdminTokenAccess(
ctx: Context,
operation?: string
) {
const {
vtex: { adminUserAuthToken, storeUserAuthToken, logger },
clients: { identity, vtexId },
} = ctx

if (!adminUserAuthToken && !storeUserAuthToken) {
logger.warn({
message: `CheckUserAccess: No admin or store token was provided`,
operation,
})
throw new AuthenticationError('No admin or store token was provided')
}

if (adminUserAuthToken) {
try {
await identity.validateToken({ token: adminUserAuthToken })
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this cause us to send duplicated checks for validating these tokens? Or are we not already doing this validation on any of these fields today?

Copy link
Contributor Author

@Rudge Rudge Nov 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

It was implemented previously. I refactored it into a new function to reuse in another directive.

Copy link
Contributor

Choose a reason for hiding this comment

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

I saw that, but I mean if there are other files that are doing these checks inside field resolvers. Some of the fields where you added this new checkAccessFeatureFlag directive may already be doing these checks internally, so we'd do it twice now, duplicating the work. If the only place doing this was the function you refactored than we should be good.

} catch (err) {
logger.warn({
error: err,
message: `CheckUserAccess: Invalid admin token`,
operation,
})
throw new ForbiddenError('Unauthorized Access')
}
} else if (storeUserAuthToken) {
let authUser = null

try {
authUser = await vtexId.getAuthenticatedUser(storeUserAuthToken)
if (!authUser?.user) {
logger.warn({
message: `CheckUserAccess: No valid user found by store user token`,
operation,
})
authUser = null
}
} catch (err) {
logger.warn({
error: err,
message: `CheckUserAccess: Invalid store user token`,
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
Expand All @@ -13,47 +68,7 @@ export class CheckUserAccess extends SchemaDirectiveVisitor {
context: Context,
info: any
) => {
const {
vtex: { adminUserAuthToken, storeUserAuthToken, logger },
clients: { identity, vtexId },
} = context

if (!adminUserAuthToken && !storeUserAuthToken) {
throw new AuthenticationError('No admin or store token was provided')
}

if (adminUserAuthToken) {
try {
await identity.validateToken({ token: adminUserAuthToken })
} catch (err) {
logger.warn({
error: err,
message: 'CheckUserAccess: Invalid admin token',
token: adminUserAuthToken,
})
throw new ForbiddenError('Unauthorized Access')
}
} else if (storeUserAuthToken) {
let authUser = null

try {
authUser = await vtexId.getAuthenticatedUser(storeUserAuthToken)
if (!authUser?.user) {
authUser = null
}
} catch (err) {
logger.warn({
error: err,
message: 'CheckUserAccess: Invalid store user token',
token: adminUserAuthToken,
})
authUser = null
}

if (!authUser) {
throw new ForbiddenError('Unauthorized Access')
}
}
await checkUserOrAdminTokenAccess(context, field.astNode?.name?.value)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think I'm missing something, here, but from what I understood we now have both directives:

  1. @auditAccess. Always checks the token and throws errors when it's not present or valid.
  2. @checkAccessWithFeatureFlag. Does the same as the other one, but only when the feature flag is enabled.

Why have both? The auditAccess will already do the check and throw before checkAccessWithFeatureFlag ever runs right? Shouldn't we only have checkAccessWithFeatureFlag, so that we can control when the check is made? And having both will make us call vtexid/identity twice when the feature flag is enabled and the check passes, since we'll check the same thing that has been checked by the first directive.

I think I misunderstood something though, can you explain the idea of the two directives with more details?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think you said about the @checkUserAccess instead of @auditAccess, is it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@checkUserAccess - Validates the storeUserToken or adminToken, but is not applied for all operations.
@auditAccess - Generate metrics about the use of the operation and if it uses the token or not.
@checkAccessWithFeatureFlag - It was created for temporary use. We put in some operations with @auditAccess, without token validation, and mapped in the task. The intention is to control whether we can enable or disable token validation for these operations in production.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, got it! I was confusing auditAccess with checkUserAccess, then. I get it now!

Copy link
Contributor

Choose a reason for hiding this comment

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

Final question then: will we be able to remove this auditAccess directive after all the missing access checks are fully launched? We won't need those metrics anymore when that happens right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this is the intention. I almost removed it in this PR, but I let it go for the next one.


return resolve(root, args, context, info)
}
Expand Down
2 changes: 2 additions & 0 deletions node/directives/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,10 @@ import { WithUserPermissions } from './withUserPermissions'
import { CheckAdminAccess } from './checkAdminAccess'
import { CheckUserAccess } from './checkUserAccess'
import { AuditAccess } from './auditAccess'
import { CheckAccessWithFeatureFlag } from './checkAccessWithFeatureFlag'

export const schemaDirectives = {
checkAccessWithFeatureFlag: CheckAccessWithFeatureFlag as any,
checkAdminAccess: CheckAdminAccess as any,
checkUserAccess: CheckUserAccess as any,
withSession: WithSession,
Expand Down
Loading