Open juni0r opened 1 year ago
I would love to have this.
I built built the User
integration very briefly, just to showcase how anyone could build what you described on top of it.
One question I have though is how should we ship that to the user of our module.
Right now, applying changes on the schema of the user feels out-of-scope for me as I would like that module to stay close to the bare minimal integration of EdgeDB features offering.
Maybe we could provide a toggleable feature that enables that User
feature once it is properly shaped, and use hooks to add additional treatment inside the auth server routes?
What are your thoughts on the User model?
Ok, so it turns out I was mistaken about Identities. I thought that every auth method (email/pwd as well as oauth) represents a different Identity that needs to be linked to a User.
If I'm not mistaken though, there is only one Identity, which is the conceptual equivalent of a User in other frameworks. This one Identity can be authenticated by different means such as EmailPasswordFactor or via OAuth. Do you know how this works exactly? The EdgeDB docs are quite sparse in that regard.
If an Identity is all you need to manage authentication and authorization, I think we shouldn't provide a User model since that's not the scope of a module.
I would say this is a fairly blocking issue, as it's forcing a dbschema that doesn't match in many cases.
User
always will be in the default EdgeDB module.name
is a str
.User
will have some required properties and constraints.I would rather have to write a few hooks and queries to control the creation of the the user myself, than to be forced into a paradigm that doesn't fit my project.
@tdolsen ; could you let me know how you would envision the perfect implementation for auth in your case?
That would help me reflect on the usecases, as I'm not using auth myself on my project using this module. š
I'll give a little input, absolutely. š
Note: I'm using EdgeDB auth and Nuxt for the first time as an experiment on a side project, and my experience with both are fairly new. (Long time Vue and EdgeDB user though.) This is just to mention that I'm still learning the internals, and that I might not know all possibilities in terms of lifecycle hooks and such.
dbschema
definitionThe biggest concern I have is that the creation of a User
is hardcoded, forcing a specific dbschema implementation. As I mention above, there are several use cases that will cause problems with the module implementation:
User
- e.g. account::Client
.current_user
global.Some of these can be solved with simple configuration, and maybe simplifying the module implementation/take account
EdgeDB's auth module has no concept of User
- it only knows about Identity
. This means the consumer is completely free to define their own implementation, and to some extent that's all that I expect from a module like nuxt-edgedb
.
Basically, if api/auth/identity
only returned the current identity, instead of trying to also find a user, that would be OK.
There are cases where I'd want to do some additional computing for the auth API endpoints, and being able to overload any specific endpoint without having to rewrite the module functionality would be desirable.
Whether this would best be done with some lifecycle hooks, or by providing the API endpoint functionality as server composables, am I really not sure about.
With the notes above said, am I still not opposed to sane defaults for a module like this. Many projects and developers are probably very content with using (more or less) what you've started on, and as long as it's possible to override the defaults then all are happy.
To summarize my thoughts, here are a some suggestions I think would be an improvement. My thoughts are not complete on all of this, and some of these suggestions might be somewhat redundant.
select global current_user
query configurableSimply making the query for fetching the user configurable (with your current implementation as default) in the module options.
edgeDb.auth.currentUserQuery = "select global account::ClientUser;"
name
property in User
typeSince the default implementation of User
just inserts an empty name, I suggest removing all references to name
. If an empty name is acceptable in the dbschema
the default should be defined there, and not in the code.
identity
to identities
in User
typeMaking it possible to have multiple identities for a user by default would raise the glass ceiling quite a bit, and the additional complexity is minimal.
global current_user := assert_single((
select User
filter global ext::auth::ClientTokenIdentity in .identities
));
type User {
required multi identities: ext::auth::Identity;
}
insert User
query configurableMaking the query to insert a User configurable would make it possible to account for some deviations from the default.
edgeDb.auth.insertUserQuery = `insert account::ClientUser {
someProp := "...",
identities := ($identityToken)
};`
Maybe there's something to supporting a callback function as well.
api/auth/signup
and/or user creation hookableAdding a way to hook into the user creation flow, giving access to the full request event, allowing collection of additional data, such that a custom user creation flow can be implemented, would be very nice.
As I see it, this should ideally happen upon signup, as that would be the place to collect initial data for the user.
This is a little vague for me at the moment, but it would be very desirable to be able to add my own sugar on top of the auth API endpoints.
I'll await your response to this, and elaborate further if needed. Once we have something we're confident about, am I happy to help out with implementing and test some of the changes myself.
hello @tdolsen ; I'm very sorry I took so much time to answer.
been very busy on my company lately and as our setup with EdgeDB do not include auth, this seem a bit remote to me.
dbschema definition
I am 100% with you on that; what I imagined is to provide these toggleable defaults for people that do not want to do something complex, and this can safely be disabled and built locally in the Nuxt project if you need overloading it yourself.
Like a boilerplate that you can overwrite once it feels needed.
Only reflecting EdgeDB auth
100% with you here as well; I would like a PR that reflects ONLY the things that EdgeDB is sharing from their docs.
The fact is I had to "hardcode" some stuff to make the oauth really easy to setup for newcomers, but now with some reflection on it, I think it should be just bare-defaults, and good documentation to build on top of it.
As for your steps; I agree with every of them; honestly this just feels like you spotlighted every weakness from the current auth approach, I would be very happy to have a PR from you if you are still around as you seem to have a great understanding of everything that's needed!
Don't sweat it @Tahul, I know very well how that goes - had it been a more pressing concern I would've submitted opinonated PRs, but I've been busy myself.
I would like a PR that reflects ONLY the things that EdgeDB is sharing from their docs. [On reflecting EdgeDB auth]
I'll look into doing that. šš¼
[..] I think it should be just bare-defaults, and good documentation to build on top of it.
Totally agree with that.
I would be very happy to have a PR from you if you are still around as you seem to have a great understanding of everything that's needed!
I got a fairly busy calendar the coming weeks, but hopefully this is something I can get done over the sommer.
Currently the authenticated User isn't exposed by the API directly but has to be retrieved separately using
ext::auth::Identity
. EdgeDB doesn't have the notion of a User, it knows only identities and a User is just another type that happens to be associated with an Identity.There is a lot of flexibility in this approach as the User type can be defined by the application and carry any additional information. However, it is more intuitive and practical to have the User as the primary object exposed by auth. Most applications will want to associate a User with other types (like author of a Post).
Currently the User is created as a side-effect when the Identity is retrieved:
https://github.com/Tahul/nuxt-edgedb/blob/37ac5c41833465f3ee9adfcf5ebec6bc20ed237d/src/runtime/api/auth/identity.ts#L15-L24
That doesn't feel right, especially if you want to collect other data (like name) as part of the signup process. It's more intuitive to create the User in the signup handler instead of merely a side-effect in the identity handler. The email and password fields can be used to create the identity and all other fields are assigned to the User.
It might be good to allow for customizing the User type etc. but for now, I'd just require that if auth is enabled, there has to be a type named
User
which has to have at least an identity field.Just an idea so far. Let me know what you think!