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

Remove gqlSuffix from creatAuth #4968

Merged
merged 2 commits into from
Mar 1, 2021
Merged

Remove gqlSuffix from creatAuth #4968

merged 2 commits into from
Mar 1, 2021

Conversation

timleslie
Copy link
Contributor

While trying to document this option in #4963 I found it very hard to justify its existence. Arguably it can be useful if you want to call createAuth() twice with the same listKey. This use case, having two different ways to authenticate against the same list seems fairly rare (though definitely not inconceivable). It's not clear to me that all the other functionality of the package would work seamlessly in such a case. Indeed we don't currently have good visibility on using two instances of createAuth against different lists in the system. e.g. the type definition union AuthenticatedItem = User probably won't work.

I would prefer to remove this option for now, and loop back to it in a more comprehensive way if we decide that supporting this use case is something we explicitly want to do. If that's the case, it should be explicitly tested and documented as a use case.

@vercel
Copy link

vercel bot commented Mar 1, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/keystonejs/keystone-next-docs/2kApLdbRe9mRaN1auQsvDhin1JVo
✅ Preview: https://keystone-next-docs-git-remove-gql-suffix-keystonejs.vercel.app

@changeset-bot
Copy link

changeset-bot bot commented Mar 1, 2021

🦋 Changeset detected

Latest commit: 612a6a4

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 6 packages
Name Type
@keystone-next/auth Major
@keystone-next/example-auth Patch
@keystone-next/app-basic Patch
@keystone-next/example-ecommerce Patch
@keystone-next/example-roles Patch
@keystone-next/example-todo Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@timleslie timleslie requested a review from molomby March 1, 2021 04:23
@gautamsi
Copy link
Member

gautamsi commented Mar 1, 2021

The way withAuth work is making it complex to use multiple list to sign in with, for example User list and Admin list

@timleslie
Copy link
Contributor Author

The way withAuth work is making it complex to use multiple list to sign in with, for example User list and Admin list

Could you expand on this? Have you tried to do this and run into problems? Were you able to get it to work at all?

@gautamsi
Copy link
Member

gautamsi commented Mar 1, 2021

I have tried some time ago and using withAuth multiple time messes up with the UI config. this would result in last one winning in the ui. I have tried changing few things but no success.

@JedWatson
Copy link
Member

I have tried some time ago and using withAuth multiple time messes up with the UI config. this would result in last one winning in the ui.

That's to be expected, and something we could probably warn about (special ___withAuth prop on config for example, that withAuth checks for and bails if it exists, with a warning message about how you can't use it twice)

You can totally use createAuth twice and add fields / mutations / etc from both to your keystone schema, but the Admin UI would only support one set of auth pages as currently written; so the idea was if you want that advanced use-case you get all the bits you need to implement your own withAuth binding logic that decides how to merge the two strategies.

If you really seriously needed auth pages in the Admin UI that lets people sign in as one list or another, that's also a thing you could implement yourself in user land leveraging the same APIs that the auth package itself uses - this was specifically a thing we designed for, and why auth is implemented the way it is

It also means we can offer a more opinionated behaviour (with init screens, a forgotten password flow, etc) because you can replace or extend it yourself properly now if you want a different set of behaviours, without needing to make the built-in functionality super complex or generic.


All that said I'm 👍 for this @timleslie, until a more specific use case for multiple auth configs against the same list emerges, and then we can test for it properly rather than having something incomplete, but implying it's possible.

@vercel vercel bot temporarily deployed to Preview March 1, 2021 20:55 Inactive
@timleslie timleslie enabled auto-merge (squash) March 1, 2021 20:55
@timleslie timleslie merged commit 9ea0328 into master Mar 1, 2021
@timleslie timleslie deleted the remove-gql-suffix branch March 1, 2021 21:05
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

Successfully merging this pull request may close these issues.

3 participants