-
Notifications
You must be signed in to change notification settings - Fork 0
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
Add operationName to generateGraphQLDocument for use in custom authorization #437
Comments
Hey,👋 thanks for raising this! I'm going to transfer this over to our API repository for better assistance 🙂 |
Hi @bradyrichmond 👋 thanks for raising this issue. If I understand correctly, you want to be able to use an operation name to allow/deny users to perform an operation in a custom authorizer lambda. I imagine you're referring to using the deniedFields like so:
However, the operation name is set in client clode and can include any qraphql queries within it. If you intend to block a user from accessing a query by operation name, wouldn't the user be able to access the same query under a different operation name? The custom authorizer example we have uses the name of the query/mutation to determine access instead. Would that fulfill your use case? If I'm misunderstanding, please provide a more complete example to explain your use case. |
Thanks for taking a look at this, @chrisbonifacio! I realize that Amplify doesn’t explicitly support a Multi Tenant SaaS configuration, but that’s what I’m trying to set up. Since the authorization function on the schema uses or for the allow array, I need to use a custom authorizer to make sure that users only perform operations for their organization and that they have been provided permission to do an action within their organization. Right now, I am just using Roles for this, but have a path forward on more granular permissions control. So what I want my flow to be, just focusing on the organization + role authentication this:
The docs reference Am I missing something for how to authorize individual users for individual actions? |
Just wanted to bump this. Let me know if you need something else from me. :) |
Hi @bradyrichmond I've marked this as a feature request for the team to consider. I'm still not convinced that this is the appropriate way to determine authorization in a multi-tenant context because operation names are controlled by the client but I'll discuss with the team and provide feedback. As an example of what I mean: # Intended query with restricted access
query GetSensitiveUserData {
getSensitiveUserInfo(userId: "123") {
id
socialSecurityNumber
bankAccountDetails
}
} Your custom auth logic would need to prevent queries with arbitrary operation names from clients: # Malicious query with a misleading operation name
query GetPublicUserProfile {
getSensitiveUserInfo(userId: "123") {
id
socialSecurityNumber
bankAccountDetails
}
} # Another malicious query without an operation name
{
getSensitiveUserInfo(userId: "123") {
id
socialSecurityNumber
bankAccountDetails
}
} |
Okay. That does make sense. Query string could be altered on the client side, making it pass my auth checks. Can So, user makes a request to use
The api gets this auth object and says "This user is authorized, but not to use |
You can use deniedFields to nullify fields in the response that the user shouldn't have access to. For instance, maybe a user has access to I would recommend initializing example: exports.handler = async (event) => {
const { authorizationToken, requestContext } = event;
try {
// Verify and decode the JWT (implementation details omitted for brevity)
const decodedToken = verifyAndDecodeJWT(authorizationToken);
const { sub: userId, 'custom:orgId': orgId, 'custom:role': role } = decodedToken;
// Initialize deniedFields array
let deniedFields = [];
// Example: Restrict access to sensitive fields for non-admin users
if (role !== 'ADMIN') {
deniedFields.push('UserProfile.ssn');
deniedFields.push('UserProfile.bankAccountDetails');
}
// Example: Restrict access to other organizations' data
if (requestContext?.arguments?.orgId !== orgId) {
deniedFields.push('Organization');
}
return {
isAuthorized: true,
resolverContext: {
userId,
orgId,
role,
},
deniedFields: deniedFields,
};
} catch (error) {
console.error('Authorization failed:', error);
return { isAuthorized: false };
}
}; This is just an example but you can send also extra context to the resolver through the In the resolver you can perform checks against a user's identity (or "tenant") like so: import { util, Context } from '@aws-appsync/utils';
import { get } from '@aws-appsync/utils/dynamodb';
export function request(ctx) {
return get({ key: { ctx.args.id } });
}
export function response(ctx) {
if (ctx.result.orgId === ctx.orgId) {
return ctx.result;
}
return null;
} |
Okay. Gave that a try. I am able to deny access to Queries and Mutations, but not to individual fields. For example, I have this
I tested denying access to
I get this message back when I attempt to run
|
Hi @bradyrichmond 👋 can you try making a request with a modified selection set so that it's omitting the |
That does get it authorized, but seems to conflict with the documentation IMO. My thought was that it could take the exact same query from different users, and it would still be authorized, but the value would be wiped out from the response. Is this a bug? Can you verify the expected behavior? |
Environment information
Description
Right now query strings are generated without an operation name. e.g
"query ($id: ID!) {\n getUserProfile(id: $id) {\n id\n email\n firstName\n lastName\n createdAt\n orgId\n updatedAt\n }\n}\n"
. When using custom resolvers, the requestContext is supposed to include anoperationName
property. Right now it ends up undefined. I would like to be able to use the operationName property in my custom authorizer.I dug around for a while and found code in @aws-amplify/data-schema that I believe is responsible for generating the queryStrings that get used my V6Client.
The operation name already exists in the function, although in a slightly different case. I believe that the only change that would need to happen is to create a pascal case of
graphQLFieldName
and update the graphQLDocument string literal like this:Right now what I'm doing is just searching the
queryString
fromrequestContext
for operation names I'm expecting to handle. However, this runs into collisions quite easily. For example, something as simple as aUserProfile
with anorgId
, and a secondary index oforgId
. This will generatelistUserProfile
, andlistUserProfileByOrgId
. If I search the queryString forlistUserProfile
, it would be true for thelistUserProfileByOrgId
, as well aslistUserProfile
. I may want to handle those two things differently.Is there a technical reason that I don't know about that makes this a bigger issue than I think it is?
The text was updated successfully, but these errors were encountered: