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

Would like a way to require auth without having Claims merged with my props #722

Closed
craig-pyrra opened this issue Jun 7, 2022 · 2 comments

Comments

@craig-pyrra
Copy link

Describe the problem you'd like to have solved

The withPageAuthRequired helper merges a user property into the output of the getServerSideProps function that is passed into it. But in my case I don't want those props available to the page. I am retrieving the session and looking up the matching user from my own database, and passing just the exact set of props needed for the page. Generally I don't think it's a good idea to modify the output of getServerSideProps - it makes it harder to reason about exactly what props the page is getting.

For example

const getServerSideProps = withPageAuthRequired({
  async getServerSideProps(ctx) {
    const { user } = await getUserAndOrg(ctx.req, ctx.res); // My function that uses Prisma to lookup `user` from my database
    return { props: { user: { name: user.name } } }; // Only this data should be available to the page. But this object is merged with a `Claims` object.
  },
});

Describe the ideal solution

It could be as simple as an additional option to withPageAuthRequired:

const getServerSideProps = withPageAuthRequired({
  mergeProps: false,
  async getServerSideProps(ctx) {
    // ...
  }
});

I actually think merging props should be disabled by default, based on the principle of least surprise.

Alternatives and current work-arounds

I don't have a good workaround at the moment, but probably I could wrap withPageAuthRequired and try to strip out the extra props, except it might be tricky, given the type of Claims?

@adamjmcgrath
Copy link
Contributor

Hi @craig-pyrra - thanks for raising this

Merging the user prop is expected behaviour and we don't have any plans to change this.

For your use case, where you don't use the user from the SDK, I suggest you roll your own withPageAuthRequired.

@craig-pyrra
Copy link
Author

craig-pyrra commented Jun 8, 2022

Thanks for the quick reply. Rolling my own withPageAuthRequired is more than I want to deal with maintaining (esp re session caching) so I will just live with this for now. Can I suggest though that the following is a bug, or at least should be documented.

const getServerSideProps = withPageAuthRequired({
  async getServerSideProps(ctx) {
    return {
      props: {
        user: { myCustomProp: "some value"  },
      },
    };
  },
});

In this example I've attempted to return my own user prop, but it's silently replaced by withPageAuthRequired. I think one of the following might be an improvement:

  1. withPageAuthRequired shouldn't inject user if a key with that name exists in the props returned by the user-supplied getServerSideProps function (but this might also be a bit surprising)
  2. withPageAuthRequired should merge user into the props of the getServerSideProps function, and if there's a key conflict the user-supplied value wins (?)
  3. withPageAuthRequired should emit a warning if it's going to replace a prop returned by getServerSideProps so at least it's clear what's happening
  4. withPageAuthRequired namespaces the props it injects under auth0. So it would look like
{
  props: {
    user: { myCustomProp: "some value" },
    auth0: {
      user: { ... }
    }
  }
}

The namespacing idea is my favourite, as it would also make me feel comfortable about letting the auth0 props be passed to my functions, even if I'm not using those props, because they're structurally separated from the props I do care about.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants