Open breytex opened 5 years ago
@breytex This issue is partially related to #200.
But don't use context for placing metadata, especially the security-related 😱
Resolvers are executed in parallel, without maintaining the order.
So if you send a document with two queries (one public, one private), you may place the context.public = true
first and then execute the private query resolver.
With this public approach of auth guards, you should wait for #124 to place arbitrary metadata on fields/resolvers and then just access it in your middlewares/guards.
In the meantime, as a temporary solution you can create an alias @Public() = @Authorized("PUBLIC")
and in authChecker
just do the inverse check 😞
Hey @19majkel94 , thanks for your quick answer :) Will definitely subscribe to updates on #200 and #124.
Two questions regarding your suggestion:
@Public() = @Authorized("PUBLIC")
? Can't find any examples on that and also couldn't figure that out myself :(@Authorized("PUBLIC")
, how can I block access to all resolvers as default? Doesn't that loop-back to my opening question? @Authorized
is a guard and "blocking all resolvers for not logged-in user" would require a middleware to run after the @Authorized
guard, right?how to create a guard alias
const Public = () => Authorized("PUBLIC")
class SampleResolver { @Query() @Public() samplePublicQuery(): boolean { return true; } }
> how can I block access to all resolvers as default?
I think about making some changes in `applyAuthChecker` - I can pass `null` for the fields not annotated with `@Authorized`, so it would be easy to make the inverse check and would not harm so much other users but it's a breaking change so I have to think twice and group this feature in one new release.
I don't know if its worth it to introduce a breaking change like that just to save a few lines
(e.g. 20x @Authorized()
vs 3x @Public()
for my project).
I published my little test project for the sake of discussing, and pushed the unsecure state to a different branch:
https://github.com/breytex/typegraphql-typeorm-auth/tree/feature/public-guard-auth
It still uses the insecure context.public = true
in the authChecker approach. I added a warning in the readme.
And you are right:
mutation{
createTodo(text: "test") # not-public mutation
requestSignIn(user:{email:"foo@test.de"}) # public mutation
}
results in accessDenied
while
mutation{
requestSignIn(user:{email:"foo@test.de"}) # public mutation
createTodo(text: "test") # not-public mutation
}
results in a success
and broken records in the database (empty owner relations since no user is logged-in). Thanks for pointing me to that issue.
Relevant code excerpts:
Tried some stuff, but I don't see a quick fix here. Probably have to wait for the next major version before continuing this. Or I switch to using@Authorized()
as it is intended to work :D
Or do you see a possibly working approach instead of using context
?
I don't know if its worth it to introduce a breaking change like that just to save a few lines
It's definitely worth, your use case is not so rarely.
I just have to think about a good API for it:
@Public()
decoratornull
to the authChecker for fields without roles (no @Authorized()
decorator placed)authCheckerMode
or something, where you can define if fields by default are authorized or publicOr do you see a possibly working approach instead of using context?
The ideal and universal solution for custom rules will be #124 and middlewares 😉 The built-in authorization feature would be only a sample implementation of it.
In this same vein, would it be possible to mark an entire resolver set as @Public or @Authorized?
This way we can break our public resolvers into a separate resolver object that is all combined and buildSchema.
In this same vein, would it be possible to mark an entire resolver set as @public or @Authorized?
This way we can break our public resolvers into a separate resolver object that is all combined and buildSchema.
I like the idea of marking an entire resolver as @Public or @Authorized It gives much more room for separation of concerns
Hi,
I was looking for a solution for marking my queries and mutations authorized by default, and came up with the following:
import { getMetadataStorage } from "type-graphql/dist/metadata/getMetadataStorage";
// Loop through registered queries and mutations, and mark
// each one as needing authorization
const metadata = getMetadataStorage();
const { mutations, queries } = metadata;
[...mutations, ...queries].forEach(({ methodName, target }) => {
metadata.collectAuthorizedFieldMetadata({
fieldName: methodName,
roles: [],
target,
});
});
I then added the public decorator as directed and adjusted my auth checker to check for the public role. As long as the code above is ran after the public decorators, it should work :)
@jleck where are you running that code?
@tafelito anywhere after resolvers are loaded and before a request is executed would work.
I believe currently the best way to achieve custom authorization logic is to use extensions to declare which fields are public and then write a guard that gonna read the extensions
value and check authorization or just allow access to public method.
thanks @MichalLytek, that worked!
Maybe I said it worked too soon....
the problem I found with this approach is that if you have a public mutation like a login mutation that returns a User type, then the guard will also run for every field on the User and, in my case, the auth guard not only checks if the user is login but also if the user account is active. So then something like this will always fail
@Extensions({ roles: ['PUBLIC'] })
@Mutation(() => User, {
nullable: true,
})
async login(
@Arg('input', () => LoginUserInput) input: LoginUserInput,
@Ctx() { req }: Context,
): Promise<User> {
const { userName, password } = input;
const user = // find user in db
if (!user) {
throw new AuthenticationError('Invalid username or password');
}
const valid = // validate pwd
if (!valid) {
throw new AuthenticationError('Invalid username or password');
}
add user the session
req.session.user = { id: user.id, accountStatus: user.accountStatus };
return user;
}
and this is the AuthMiddleware that runs globally
export const AuthMiddleware: MiddlewareFn<Context> = async (
{ context: { req }, info },
next,
) => {
const { roles } =
info.parentType.getFields()[info.fieldName].extensions || {};
if (roles?.includes('PUBLIC')) {
return next();
}
const user = req.session.user;
if (!user) {
throw new AuthenticationError('Not authenticated');
}
if (user.accountStatus !== UserAccountStatus.ACTIVE) {
// if user account is not active, restrict access
throw new AuthenticationError('User is not active');
}
return next();
};
When I call the login mutation and the returned user is not active, then the middleware will pass the mutation but then throw an exception for the first field of the User
Ideally, when setting a an extension to a resolver like in this case, maybe the extension value should be passed to the field resolvers as well, unless a field resolver has an specific @Authorized
decorator where in that case the decorator will have a higher priority
if you have a public mutation like a login mutation that returns a User type, then the guard will also run for every field on the User
So you need to use info
data to detect when the middleware is run for query/mutation and when for the object type field. This way no matter what public operation returns, it will be available to the user.
Be aware about reaching some confidental field resolvers, like publicMutation -> User -> friends -> payments
👀
@MichalLytek in the docs says that you can add the @Extensions
on top of a @ObjectType
. If you do that, where are you supposed to get the value in that case?. I added it on top of one of my ObjectTypes but when I run the middleware I don't see the value. I do see the ones that are on top of the @Query
@Mutation
@Field
or @FieldResolver
Hello @tafelito 👋
When you add @Extensions
on an @ObjectType
and the corresponding field is being resolved, you will be able to access the extensions data on info.returnType.extensions
.
One additional thing to note is that non-nullable graphql types are wrapped by another type, which has an ofType
property to access the original type.
Personally, I have written a small helper to extract type extensions like so:
export const extractTypeExtensions = (
info: GraphQLResolveInfo
): Record<string, any> => {
// Non nullable GraphQL types are wrapped by a type
// exposing the base type on its "ofType" property
const type =
"ofType" in info.returnType ? info.returnType.ofType : info.returnType;
return type.extensions || {};
};
thanks @hihuz that worked even tho the types of returnType has no extensions in it
I also found that you can access from the fields itself to the parentType extensions by doing info.parentType.extensions
that in my case it seems like a better approach
I'm still trying to find how to access the extensions
from an @InputType
and from a @Field
inside an @InputType
I haven't had an actual use case for extensions on input types yet, so there might be less convoluted ways to access them, but I was able to access @Extensions
added on top of an @InputType
, itself being used as an  @Arg
for a mutation, with the following:
const argumentTypes = Object.values(
info.parentType.toConfig().fields[info.fieldName].args
).map((arg) => arg.type);
const argumentExtensions = argumentTypes.map(
(type) => ("ofType" in type ? type.ofType : type).extensions
);
It's just a dirty example, but maybe you can try to explore a bit more in that direction. Good luck!
@hihuz that worked to get the extensions from the input type but not for the fields of the input type, do you know how to get the fields?
Thanks!
Sorry I meant the fields that you actually sent to the query/mutation, not all the fields that the InputType has
So if you have a mutation like this
myMutation(input: {field1: "value"}) {
....
}
input is an @InputType
that can have many fields, and one of them can have an @Extensions
I only want to know the extension of the fields that are actually sent to the mutation
In my example above you have a reference to the input type(s) of the argument(s), so you can get all the fields of the type(s) with getFields()
, e.g.
const argumentTypes = Object.values(
info.parentType.toConfig().fields[info.fieldName].args
).map((arg) => arg.type);
const argumentFields = argumentTypes.map(
(type) => ("ofType" in type ? type.ofType : type).getFields()
);
Then you can get the details of each field, including extensions. To simplify let's consider that you have only one argument:
const extensions = Object.values(argumentFields[0]).map(field => field.extensions)
As for knowing which fields were actually sent to the mutation, this will be available in the args
object of your middleware function argument, e.g.
export const AuthMiddleware: MiddlewareFn<Context> = async (
{ context: { req }, info, args },
next,
) => {
// "args" here will contain all the arguments that were passed to your mutation
// you can perform some logic with it against whatever extensions you extracted from "info"
}
Does that make sense to you? Once again this is just from experimenting myself so I won't guarantee that this is the best way to achieve what you are trying to do, but I hope it helps.
@tafelito @hihuz This is a feature request issue and we should discuss here only some ideas about how it should work, the use cases, what might be a weak spot, etc. 😕
So could you make a gist with the auth middleware code and move your discussion there? It's a bit spammy and confusing for other users. If you find a good solution, you can make it available on your repo, publish as npm package and link here for easy access by other users 😉
@hihuz I ended up doing something similar, thanks for the help!
@MichalLytek as you pointed out before, this is not an uncommon use case and since is not currently supported (and by supported I mean like having something similar to the @Authorized decorator) by type-graphql, I think it might this could be useful as a workaround for other people with the same requirements
Hello :) Usually, ppl in the GraphQL world use an
@Authorized()
guard to shield resolvers from unauthorized access. I want to build the opposite: a@Public()
guard to flag a few resolvers as "available without login". Reason is, that my SaaS app has like 3 (login-related) mutations which are public, and all other resolvers are guarded with@Authorized()
so far. I would like to turn this upside-down.So I have a Public guard:
and an auth middleware:
My main problem here is, that a middleware is executed before before the guards in type-graphql, which breaks the entire idea of my approach. I want to detect if a request targets a public resolver using the guard and then "skip" the auth middleware. This requires the public guard to be executed before the middlewares.
Is it possible to make a guard execute before the middlewares in general? Or do you see a different approach for implementing
@Public()
as a counterpart to@Authorized()
?