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

Inverted authorization mode - @Public() decorator #230

Open
breytex opened this issue Jan 17, 2019 · 24 comments
Open

Inverted authorization mode - @Public() decorator #230

breytex opened this issue Jan 17, 2019 · 24 comments
Labels
Community 👨‍👧 Something initiated by a community Discussion 💬 Brainstorm about the idea Enhancement 🆕 New feature or request
Milestone

Comments

@breytex
Copy link

breytex commented Jan 17, 2019

Original title: Running a guard before the middlewares

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:

export function Public<T extends object>() {
    return UseMiddleware(async ({ args, context }, next: NextFn) => {
        console.log("public field")
        context.public = true   // default set in index.ts is false
        return next()
    })
}

and an auth middleware:

export class CookieAuthMiddleware implements MiddlewareInterface<MyContext> {
    async use({ context, info }: ResolverData<MyContext>, next: NextFn) {
        if (context.public) {
            console.log("public request, authorized")
            await next()

        } else {
              // do some cookie / session magic to check access rights
        }
     }
}

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()?

breytex added a commit to breytex/typegraphql-typeorm-auth that referenced this issue Jan 17, 2019
@MichalLytek
Copy link
Owner

MichalLytek commented Jan 17, 2019

@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 😞

@MichalLytek MichalLytek added the Question ❔ Not future request, proposal or bug issue label Jan 17, 2019
@breytex
Copy link
Author

breytex commented Jan 19, 2019

Hey @19majkel94 , thanks for your quick answer :)
Will definitely subscribe to updates on #200 and #124.

Two questions regarding your suggestion:

  • how to create a guard alias @Public() = @Authorized("PUBLIC")? Can't find any examples on that and also couldn't figure that out myself :(
  • Consider I use the reverse check in @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?

@MichalLytek
Copy link
Owner

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.

@breytex
Copy link
Author

breytex commented Jan 19, 2019

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?

@MichalLytek
Copy link
Owner

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() decorator
  • passing null to the authChecker for fields without roles (no @Authorized() decorator placed)
  • maybe authCheckerMode or something, where you can define if fields by default are authorized or public

Or 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.

@MichalLytek MichalLytek changed the title Running a guard before the middlewares Inverted authorization mode - @Public() decorator Jan 27, 2019
@MichalLytek MichalLytek added Enhancement 🆕 New feature or request Community 👨‍👧 Something initiated by a community Discussion 💬 Brainstorm about the idea and removed Question ❔ Not future request, proposal or bug issue labels Jan 27, 2019
@MichalLytek MichalLytek added this to the 2.0.0 release milestone Jan 27, 2019
@MWhite-22
Copy link

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.

@Gallevy
Copy link

Gallevy commented Dec 6, 2019

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

@jleck
Copy link

jleck commented Aug 25, 2020

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 :)

@tafelito
Copy link

tafelito commented Sep 6, 2020

@jleck where are you running that code?

@jleck
Copy link

jleck commented Sep 7, 2020

@tafelito anywhere after resolvers are loaded and before a request is executed would work.

@MichalLytek
Copy link
Owner

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.

@tafelito
Copy link

tafelito commented Sep 7, 2020

thanks @MichalLytek, that worked!

@tafelito
Copy link

tafelito commented Sep 7, 2020

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

@MichalLytek
Copy link
Owner

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 👀

@tafelito
Copy link

tafelito commented Sep 9, 2020

@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

@hihuz
Copy link
Contributor

hihuz commented Sep 10, 2020

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 || {};
};

@tafelito
Copy link

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

@tafelito
Copy link

I'm still trying to find how to access the extensions from an @InputType and from a @Field inside an @InputType

@hihuz
Copy link
Contributor

hihuz commented Sep 11, 2020

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!

@tafelito
Copy link

tafelito commented Sep 11, 2020

@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!

@tafelito
Copy link

tafelito commented Sep 11, 2020

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

@hihuz
Copy link
Contributor

hihuz commented Sep 11, 2020

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.

@MichalLytek
Copy link
Owner

@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 😉

@tafelito
Copy link

@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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Community 👨‍👧 Something initiated by a community Discussion 💬 Brainstorm about the idea Enhancement 🆕 New feature or request
Projects
None yet
Development

No branches or pull requests

7 participants