search

pocketbase / pocketbase

Open Source realtime backend in 1 file
https://pocketbase.io
MIT License
40.54k stars 1.9k forks source link

Merge users and profiles (request for feedback) #376

Closed ganigeorgiev closed 2 years ago

ganigeorgiev commented 2 years ago

This was partially discussed in https://github.com/pocketbase/pocketbase/discussions/225#discussioncomment-3511148 and other similar older discussions.

The current approach of having 2 separate entities (users and profiles) to manage the application users is not very convenient and has several drawbacks:

Considering the above shortcomings, I propose to merge the user models and the profiles collection into a new single system collection called users.

This basically means that:

While I'll try to automate as much as possible the required DB changes, this unfortunately will not be fully backward compatible. Of course, there will be a detailed migration guide, but still users will have to update manually their existing custom app code.

This issue is with high prirority because it will not be backward compatible and the quicker we introduce the changes the "less damage" it will cause.

Please comment with your feedback if you have any different use case or idea how to handle better the current implementation shortcomings (pseudo code and examples are also welcomed).

theMackabu commented 2 years ago

Honestly it sounds pretty good, as I don't need to create a a profile after user creation, but small question. Does this mean the user has to edit via the user's endpoint and user list had to be from that as well?

jimafisk commented 2 years ago

This sounds reasonable to me @ganigeorgiev, I like the plan as you have proposed it. It would be nice to have API rules for the user itself and full "manager-subordinate" permissions would be important for an app I'm working on. I also think working off a single user.id and not having to worry about user.profile.id could help simplify things from a dev perspective.

While I'll try to automate as much as possible the required DB changes, this unfortunately will not be fully backward compatible.

For my specific app this shouldn't be a problem at this point. Happy to delete my users and manually recreate them, but don't want to speak for others.

This issue is with high priority because it will not be backward compatible and the quicker we introduce the changes the "less damage" it will cause.

I definitely appreciate this. Would love to start building on the new user architecture sooner rather than later.

ganigeorgiev commented 2 years ago

@theMackabu: Does this mean the user has to edit via the user's endpoint and user list had to be from that as well?

Good question. I haven't decided on that yet. Since the users will be a system collection, I think we can allow access from both services in order to minimize the breaking changes and for consistency with the admins service.

Or in other words, the following SDK calls would be equivalent:

client.users.getList();                   <=>   client.records.getList("users");
client.users.getOne("USER_ID");           <=>   client.records.getOne("users", "USER_ID");
client.users.create({ ... });             <=>   client.records.create("users", { ... });
client.users.update("USER_ID", { ... });  <=>   client.records.update("users", "USER_ID", { ... });
client.users.delete("USER_ID");           <=>   client.records.delete("users", "USER_ID");

Internally the users api action could "redirect" to the records api so that the actual handling will happen in a single place.

The create and update user handling will be almost identical as for a regular record, with the exception for the special system fields like email and password.

jimafisk commented 2 years ago

Personally I'd rather have one way of doing things over backwards compatibility (assuming it doesn't limit functionality). PocketBase is new enough that hopefully it wouldn't be unduly burdensome for folks to rewrite these sections of their apps. Allowing both is something you'll carry forward and new users will continually question which way they should do it and the difference between them. Maybe this is an unpopular opinion so thumbs down this if you disagree.

What would the update look like for changing passwords?

await client.records.update('users', 'RECORD_ID', {
  password: '123456',
  passwordConfirm: '123456',
});
ganigeorgiev commented 2 years ago

@jimafisk Maybe you are right. I will think a little more on this when I start working on the proposed changes, because supporting both apis may cause also confusion which request event hook will be triggered (user or record), so maybe it would be better to throw an error if someone tries to update the users collection records via the regular records api (or the other way arround).

About the password change - in my local notes I have outlined 2 options to consider:

  1. Don't allow users to directly update their email and password, requiring them to explicitly call /api/users/request-password-reset and /api/users/request-email-change (at it is now).

  2. Allow users to update their email and password, but require to send also the old password for verification (when changing the password also require 2 new fields to read the new password from). Here are couple of examples how this could look like:

    // change only the email
await client.users.update('USER_ID', {
    email:    'new@example.com',
    password: '123456',
});

// change only the password
await client.users.update('USER_ID', {
    password:           '123456',
    newPassword:        '654321',
    newPasswordConfirm: '654321',
});

// change the email and password at the same time
await client.users.update('USER_ID', {
    email:              'new@example.com',
    password:           '123456',
    newPassword:        '654321',
    newPasswordConfirm: '654321',
});

// change the email and password at the same time + other fields
await client.users.update('USER_ID', {
    email:              'new@example.com',
    password:           '123456',
    newPassword:        '654321',
    newPasswordConfirm: '654321',
    // other regular fields:
    name: "test",
});

For admins we will have a separate endpoint which will allow changing all user system fields (email, password, verified) from the Admin UI, without requiring to enter the current user password beforehand.

theMackabu commented 2 years ago

Honestly the second option seems more in-line with how a lot of other services do it, require old password to set new one.

ollema commented 2 years ago

These changes sounds great to me!

I also agree that keeping both methods for the sake of backwards compatibility is not a good idea - just remove the old method. Users should expect breaking changes before 1.0

ganigeorgiev commented 2 years ago

I was able to experiment a little with the proposal and the suggestions from above and here are a little more details about the planned changes.

I think the user model and it related services could be completely removed.

Instead we will introduce the following collection types:

(the names of the types are subject to change; suggestions are welcomed)

Or in other words, the "users" will be replaced with auth collection(s).

All existing user auth routes will be replaced with a corresponding collection route, for example:

/api/users/auth-via-email  =>  /api/collections/users/auth-via-email

The SDKs will no longer have a client.users and client.records services and instead a new collection service will be created that will be accessible through client.collection(ID_OR_NAME).*. For example:

// base crud:
client.records.getFullList("test", )           => client.collection("test").getFullList()
client.records.getList("test", )               => client.collection("test").getList()
client.records.getOne("test", "RECORD_ID")     => client.collection("test").getOne("RECORD_ID")
client.records.create("test", {})              => client.collection("test").create({})
client.records.update("test", "RECORD_ID", {}) => client.collection("test").update("RECORD_ID", {})
client.records.delete("test", "RECORD_ID")     => client.collection("test").delete("RECORD_ID")

// auth collection only:
client.users.authViaEmail(...)         => client.collection("test").authViaEmail(...)
client.users.authViaOAuth2(...)        => client.collection("test").authViaOAuth2(...)
client.users.refresh()                 => client.collection("test").authRefresh()
client.users.requestPasswordReset(...) => client.collection("test").requestPasswordReset(...)
client.users.confirmPasswordReset(...) => client.collection("test").confirmPasswordReset()
client.users.getAuthMethodsList()      => client.collection("test").getAuthMethodsList()
client.users.getExternalAuthsList(...) => client.collection("test").getExternalAuthsList(...)
client.users.unlinkExternalAuth(...)   => client.collection("test").unlinkExternalAuth(...)

// deleted:
[X] client.users.requestEmailChange(...)
[X] client.users.confirmEmailChange(...)

The special @request.user.* filter rule will be replaced with @request.auth.*. You will be able to query the auth collection name (@request.auth.collectionName) allowing you to even use different auth collections for roles/groups separation.

A new manage action and API rule will be added for the auth collections to allow manager/supervisor auth models to be able to change the special subordinate fields like email, password, verified, etc. without providing the original user password.

The owner of the auth model will be able to change their email or password via the update action if authorizationPassword is provided, for example:

// change only the email (the verified state will be auto set to `false` after this call)
await client.collection('users').update('RECORD_ID', {
    email: 'new_email@example.com',
    authorizationPassword: 'old_pass',
});

// change only the password
await client.collection('users').update('RECORD_ID', {
    password: 'new_pass',
    passwordConfirm: 'new_pass',
    authorizationPassword: 'old_pass',
});

// change the email and password at the same time + some other regular field(s)
await client.collection('users').update('RECORD_ID', {
    email: 'new_email@example.com',
    password: 'new_pass',
    passwordConfirm: 'new_pass',
    authorizationPassword: 'old_pass',
    // regular fields:
    name: 'test',
});

The authorization token schema will be included in the token, meaning that you no longer will have to prefix the token with Admin or User:

Authorization: Admin TOKEN  =>  Auhtorization: TOKEN
Authorization: User TOKEN   =>  Auhtorization: TOKEN

There will be also a lot of other changes to the internals, for example to simplify the Record model accessors we will rename:

   SetDataValue            -> Set
   GetDataValue            -> Get
   GetBoolDataValue        -> GetBool
   GetStringDataValue      -> GetString
   GetIntDataValue         -> GetInt
   GetFloatDataValue       -> GetFloat
   GetTimeDataValue        -> GetTime
   GetDateTimeDataValue    -> GetDateTime
   GetStringSliceDataValue -> GetStringSlice

For example, you'll be able to access the user email via record.GetString("email") (this is to unify the record data access API when PocketBase is used as framework).

Overall this will be a major refactoring and a lot of components will change. As mentioned in https://github.com/pocketbase/pocketbase/discussions/123#discussioncomment-3539859, new features will be pushed back for a while, so please be patient. In short, the next couple of weeks I'll try to work on:

This probably will be the last major breaking change before v1.0.0 and if you have any other ideas or suggestions, please add them as comment, so that I could consider them during the refactoring.

ollema commented 2 years ago

@ganigeorgiev some feedback:

in the example above you used "test" as the collection name for both the catalogue type collection and the auth type collection right:

// base crud:
client.collection("test").getFullList()
client.collection("test").getList()
...

// auth collection only:
client.collection("test").authViaEmail(...)
client.collection("test").authViaOAuth2(...)

but this would not be the case for a real app right? you could not have the same name for two different collections - so maybe you would use "test" for the catalogue collection and maybe "users" for the auth collection?

just want to make sure I'm following along.

as for the collection type names, I like both auth and single but not sure about catalogue. I don't have a better suggestion though :) just does not feel very natural to me

when it comes to the naming changes, I'm also not sure.

previously, we had:

client.records.getList("test", )               // get list of records
client.records.getOne("test", "RECORD_ID")     // get one record
client.records.create("test", {})              // create a record
client.records.update("test", "RECORD_ID", {}) // update a record
client.records.delete("test", "RECORD_ID")     // delete a record

with the new naming changes:

client.collection("test").getList()               // does not get list of collections?
client.collection("test").getOne("RECORD_ID")     // does not get a collection?
client.collection("test").create({})              // does not create a collection?
client.collection("test").update("RECORD_ID", {}) // does not update a collection?
client.collection("test").delete("RECORD_ID")     // does not delete a collection?

to me this makes less sense! but I guess it's personal preference

maybe:

client.collections("test").records.getList()               // get list of records
client.collections("test").records.getOne("RECORD_ID")     // get one record
client.collections("test").records.create({})              // create a record
client.collections("test").records.update("RECORD_ID", {}) // update a record
client.collections("test").records.delete("RECORD_ID")     // delete a record

could work but I'm guessing you have thought of this already and probably had a good reason not to choose this (maybe too verbose) 😅

regarding the changes to the auth collection:

client.users.authViaEmail(...)         => client.collection("test").authViaEmail(...)
client.users.authViaOAuth2(...)        => client.collection("test").authViaOAuth2(...)
client.users.refresh()                 => client.collection("test").authRefresh()
client.users.requestPasswordReset(...) => client.collection("test").requestPasswordReset(...)
client.users.confirmPasswordReset(...) => client.collection("test").confirmPasswordReset()
client.users.getAuthMethodsList()      => client.collection("test").getAuthMethodsList()
client.users.getExternalAuthsList(...) => client.collection("test").getExternalAuthsList(...)
client.users.unlinkExternalAuth(...)   => client.collection("test").unlinkExternalAuth(...)

is there a specific reason why you want to specify the name of the collection here (in this example "test") versus it being a internal constant value? do you want to support people having multiple auth collections?

if not, then maybe it could still make sense to use something like client.users even though it now uses a collection under the hood rather than another model.

why I personally would prefer client.users over something like client.collection("users") comes down to three reasons:

1) it is shorter. 

const { token, user } = await locals.pocketbase.users.authViaEmail(email, password);

is already pretty long and

const { token, user } = await locals.pocketbase.collection("users").authViaEmail(email, password);

would be even longer

2) more convention over configuration makes for more readable code whenever you look at other peoples pocketbase auth code

3) the difference between client.users and client.collections("test") makes it obvious in the code that they have different uses/APIs (the former has .authViaEmail(...))

should be noted that 1), 2) and 3) are not major issues but minor nitpicks.

finally, regarding

if you have any other ideas or suggestions, please add them as comment, so that I could consider them during the refactoring.

not sure if this is what you meant, but I would like to make another push for https://github.com/pocketbase/pocketbase/issues/312 if you are anyways refactoring collections. I've used pocketbase in two web apps so far and indirect expand would have been very useful in both of them!

sorry for the long post, hopefully some of it might have been useful.

again, these changes look great and I'm sure they will be great for the pocketbase project regardless of what function names are used.

thanks!

ganigeorgiev commented 2 years ago

@ollema Thanks for the feedback.

in the example above you used "test" as the collection name for both the catalogue type collection and the auth type collection right:

The idea is that the auth collection will have everything what the default catalog colleciton has + additional auth endpoints and some special system fields (email, verified, passwordHash, etc.). Or in other words, you could treat the above example as if the "test" colleciton is auth.

as for the collection type names, I like both auth and single but not sure about catalogue. I don't have a better suggestion though :) just does not feel very natural to me

I also don't like catalog (or catalogue) but I can't think of anything better. Initially I considered "list", but then in the code it kinda read strange when collection.IsList() is called (eg. someone may think that this is slice or similar).

why I personally would prefer client.users over something like client.collection("users") comes down to three reasons:

The idea of introducing client.collection(ID_OR_NAME).* is to unify the crud and auth actions in a single service and to allow multiple auth collections (eg. you could want the manager auth collection to have different fields than the subordinate auth collection).

Keeping both client.users and client.records could lead to some strange inconsistencies and duplication of the crud services. I think it is a lot easier to reason when everyhing is a collection and can be accessed through the same api (eg. you don't have to know whether a collection is auth, catalog or single in order to call create or update).

Yes, it is a little longer to type, but users coming from Firebase may find this pattern familiar.

jimafisk commented 2 years ago

Initially I considered "list"

Was coming here to make the same suggestion. I thought list made more sense than catalogue but I see your point @ganigeorgiev. Just spitballing, is the "collection" naming open to change? It's a little strange to have a single collection in the first place. Maybe the overall structure could just be "data" instead of "collection" of which you have auth, collection, and single types. Not sure if that conflicts with other PB concepts so sorry to add noise, just wanted to throw other ideas out there.

ganigeorgiev commented 2 years ago

@jimafisk We can change "collections", but "data" is too broad/general as a term and I don't think it is better in this case. In practice, users rarely will have to work directly with the types, since in the Admin UI we will have human readable labels and probably a short description what each collection type do and expect to store.

An alternative to "single" I considered previously also "oneoff" but I dropped the idea because the spelling could be ambigious (one_off, oneOff, etc.).

About the SDK service naming - Instead ofclient.collection(name) we can use client.use(name) (similar to how in SQL you target specific database) as a more shorter version. For example:

client.collection('users').getList()  vs  client.use('users').getList()

client.collection('users').authViaEmail()  vs  client.use('users').authViaEmail()

Personally, I prefer collection() because it is clear what actually the method is referring/targetting.

blackmann commented 2 years ago

// auth collection only: client.users.authViaEmail(...) => client.collection("test").authViaEmail(...) client.users.authViaOAuth2(...) => client.collection("test").authViaOAuth2(...) client.users.refresh() => client.collection("test").authRefresh() client.users.requestPasswordReset(...) => client.collection("test").requestPasswordReset(...) client.users.confirmPasswordReset(...) => client.collection("test").confirmPasswordReset() client.users.getAuthMethodsList() => client.collection("test").getAuthMethodsList() client.users.getExternalAuthsList(...) => client.collection("test").getExternalAuthsList(...) client.users.unlinkExternalAuth(...) => client.collection("test").unlinkExternalAuth(...)

I'm confused with this API. As in, if a collection wasn't an auth collection are these methods going to exist on it? For example, if I did 

client.collection("products").authViaEmail(...) // <- where products is obviously not an auth collection

Is that valid?

ganigeorgiev commented 2 years ago

@blackmann Yes, you will be able to call that, but you'll get an API error telling you that the referred collection is not an auth.

blackmann commented 2 years ago

TLDR; Mixing auth methods with the base collection methods is an overextension of its responsibilities

IMO, I think this design/API will become convoluted. Because as auth collections get extended with new methods, they'll be applied to collections in general. I (personally) find that weird.

How about, say a more explicit, name like client.auth("managers") which will just return a result with these methods as an extension of [the base] collections.

blackmann commented 2 years ago

I also want to use this opportunity to say, this is really great work and I'm using this in my next SaaS project starting this weekend. It's providence that I found this.

Thank you!

Stumbled on it from some Youtube shorts!

ganigeorgiev commented 2 years ago

Mixing auth methods with the base collection methods is an overextension of its responsibilities

But this is currently how the client.users.* service is organized.

client.users.getFullList()
client.users.getList()
client.users.getOne()
client.users.create()
client.users.update()
client.users.delete()
client.users.refresh()
client.users.authViaEmail()
client.users.authViaOAuth2()
...

How about, say a more explicit, name API like client.auth("managers") which will just return a result with these methods as an extension of [the base] collections.

I thought of this, but I don't like that it collides with client.authStore.* and it is not consistent with client.admins.* (the admins will not be a collection since they have different business logic binded to them, like skipping API rules, special super-access middlewares, etc.).

jimafisk commented 2 years ago

An alternative to "single" I considered previously also "oneoff"

I agree that single is better, I don't have a problem with it, just wanted to think it through.

Personally, I prefer collection() because it is clear what actually the method is referring/targetting.

I'd tend to agree that collection('whatever') is more clear than use('whatever').

I think this design/API will become convoluted. Because as auth collections get extended with new methods, they'll be applied to collections in general.

I would assume the extension would apply to auth collections only?

@ganigeorgiev - the API decisions you've made so far have be great, so I'm definitely comfortable with you making executive decisions on what you think is best.

ganigeorgiev commented 2 years ago

@jimafisk I would assume the extension would apply to auth collections only?

Yes, auth related methods will be available only for auth collections. Obviously, since the collection is accessed via its name or id, nothing will stop you to us an auth method with a non-auth collection, as @blackmann mentioned, but you'll get an API error.

The same will be true if we move the auth related methods in a separate auth(name) service. We will have a type checker on the server-side no matter the SDK API. The whole complication around client.collection(name).* is because I want to support multiple auth collections (at work I have a use for that for a small internal company knowedge base, where there is expected to have 2 different auth models - staff and client and both have different set of fields).

blackmann commented 2 years ago

@ganigeorgiev what I mean is...

How the current client.users.* is great. Because you could only access those methods .authVia*, .confirmPassword, etc on just the client.users but not other collections.

In other words, intentions should be clear while reading code instead of cross-checking with admin panel or during runtime.

But as @jimafisk said, you'll have to make the executive decision. I'm just a troublemaker, haha.

ganigeorgiev commented 2 years ago

@blackmann : In other words, intentions should be clear while reading code instead of cross-checking with admin panel or during runtime.

I get what you mean but I think even if we ended up choosing the client.collection().* API, the name of the collection should be enough to make it clear while reading the code what is the intention of the collection (eg. "users", "clients", etc.). Tomorrow after work I'll research other libraries for ideas/inspirations and will post here if I stumble on something.

In the meantime, the TLDR version of the above discussions, seems to be around:

  1. The names of the 3 new collection types - auth, catalog, single.

  2. The changes in the SDKs API. Based on the feedback, I think we can narrow the options to these 2:

    client.records.getList("users");                              vs   client.collection("users").getList();
client.records.getOne("users", "RECORD_ID");                  vs   client.collection("users").getOne("RECORD_ID");
client.records.create("users", {});                           vs   client.collection("users").create({});
client.records.update("users", "RECORD_ID", {});              vs   client.collection("users").update("RECORD_ID", {});
client.records.delete("users", "RECORD_ID", {});              vs   client.collection("users").delete("RECORD_ID", {});
client.auth.viaEmail("users", "test@example.com", "123456");  vs   client.collection("users").authViaEmail("test@example.com", "123456");
client.auth.viaOAuth2("users", ...);                          vs   client.collection("users").authViaOAuth2(...);
// other auth related methods...

    (for now, I'm inclined more towards the latter because it is consistent with the existing admins service and it also may feel familiar to Firebase users).

denisgurev commented 2 years ago

I'm still a newbie but I like client.collection("users").authViaEmail("test@example.com", "123456") more than client.auth.viaEmail("users", "test@example.com", "123456") (it is also similar to how it is now with client.users).

Could you also add the auth methods to the api preview in the dashboard for the auth collections? I often use the preview to copy paste the code and I think it will be useful for the auth methods too.

ganigeorgiev commented 2 years ago

Could you also add the auth methods to the api preview in the dashboard for the auth collections? I often use the preview to copy paste the code and I think it will be useful for the auth methods too.

@denisgurev Yes, I will consider it when redesigning the Collections UI.

ollema commented 2 years ago

The names of the 3 new collection types - auth, catalog, single.

Other suggestions instead of collections with collections types auth, catalog, single:

Did I miss anything?

I honestly like almost every example listed above here more than catalog, but of course that is just personal preference.

I especially like replacing collection with data, that feels very at home to me!

The changes in the SDKs API. Based on the feedback, I think we can narrow the options to these 2:

If you are looking for user input, I would like to cast my vote for option 1 for reasons stated earlier in the thread

ganigeorgiev commented 2 years ago

@ollema I honestly like almost every example listed above here more than catalog, but of course that is just personal preference.

data is too common and miscellaneous as a term, because everything could be data and will require special naming for local variables when working with the "data" models. collection is a little more specific and doesn't require too much additional context to understand what the model/variable store.

I thought of multiple initially, but I wasn't sure whether it was clear enough. Now that I think on it again, I'm starting to like it - it is a direct antonym of single and it also reads well in code (eg. collection.IsMultiple()). I'll still do a further research, but I think it is a good alternative to catalog.

While the collection type is not critical even if we change them in the future, because most users will use the Admin UI to manage their collections, the SDK public API on the other hand is problematic, because I don't want to introduce another major breaking change in the future. Once I finalize the pure backend changes, I'll spend a little more time on this to explore other options.

ollema commented 2 years ago

thanks @ganigeorgiev - very valid points, sounds great!

mbecker commented 2 years ago

Hi, just became aware of this discussion due to the linking in another issue. It's a long thread with a lot of things discussed. I would like to summarize the most important things at the beginning for me (the following discussions are mostl about naming and conventions for CRUD services) and would like to add some notes:

Hope that matches the most important points; especially the new collection type auth.

I'm not a big fan of mixing the special process of authentication with the underlying resources user / profile / account (however you are calling it) with the normale resources stored in the collection. I do understand that the basic CRUD services like "list all", "get one", "create", and so an are technically the same and can be internally shared in the core app. But the resources "user" / "profile" will ever have some special methods or implementation for example to sanitize sensitive information like password or a method which will update the current password. That's why I would say that the underlying resources for the authentication should be handled separately. PocketBase is not an IAM or a fully identity solution; that's why I would keep it simple. Products like Firebase are separating the resource user and the corresponding process of authentication too (see for example https://firebase.google.com/docs/reference/js/v8/firebase.User ). Plus, in future there will be additional feature requests to include special processes or special attributes like mobile number verification and authentication, OTP code, and very important to have a federation of users. I think it's easier to keep it separately.

Personally, I'm a fan of separating the resources user and profile. The resource user is to manage the authentication process and profile is used to save additional information of the user which may change over the time. I like the idea of having a simple key/value store managed by the user-land code. Looking, for example at Keycloak, the have an additional relation table user_attribute. A technical implementation not have an additional relation may to use the existing table user and have an attribute profile as a JSON document for a key/value store. That would mean I can store more or less anything from user-land code as long as I'm responsible for un/marshalling it.

To be honest, I do not the like the idea of having a special collection type auth. I do not understand the benefits for this. I'm managing all my DB updates with the migrations to have a streamlined CI / CD that results in the same database in the different stages. By using a collection type auth you must ensure that the database migration files will have all necessary attributes / fields like username, password, email, and so on. I think this results in a heavy mess in the future by having new feature requests like for example OTP, federated users, and so on. I do unterstand the technically background to share the underlying methods for the collections (which are basically SQL tables) and to provide the same CRUD services to the SDK API. But again, for me the authentication process and underlying resource are too important and special to mix; plus new feature requests make it more difficult to implement.

You have mentioned two additional benefits for using the special collection type auth as follows

I think that these points are not necessarily linked to a new collection type auth.

Using different collections for different kind of users (like admin, member, staff, etc.) may result in multiple account for the same human person (human person "max" may be a "member" and a "staff", and so on). Linking OAuth provider would be quite tricky; federating user identities as well, SSO would not be possible. I would rather see a new attribute "roles" which may be in the core app or is a user-land managed attribute.

Haven't yet used API rules in details but I'm using Open Policy Agent (OPA) a lot; it's the same idea. As a vision I would LOVE to have OPA integrated in PocketBase API rules. In the meantime it would be great to have full access to the request-input (or the authenticated user; JWT and user-model) to access all user-land attributes like "roles". With that you could implement an RBAC, ABAC, and a full "manager-subordinate".

So, long post, short:

Sorry, to have different oppinion. Hope this helps in the discussion.

ganigeorgiev commented 2 years ago

@mbecker Thanks for the feedback.

Personally, I'm a fan of separating the resources user and profile.

You still will be able to create your own profile collection and create a relation to it if you want to keep your users data in a separate collection.

By using a collection type auth you must ensure that the database migration files will have all necessary attributes / fields like username, password, email, and so on.

The existing user model fields (email, verified, passwordHash, etc.) are system fields and will be automatically created for each auth collection (and underlying table) and they cannot be deleted or renamed. From the Admin UI you will be able to manage only the configuration of your custom fields (the same as it is now with the profiles collection).

To be honest, I do not the like the idea of having a special collection type auth. I do not understand the benefits for this.

The main benefits of treating the users models as a regular collection are:

Using different collections for different kind of users (like admin, member, staff, etc.) may result in multiple account for the same human person (human person "max" may be a "member" and a "staff", and so on). Linking OAuth provider would be quite tricky; federating user identities as well, SSO would not be possible.

If your auth models have the same set of fields, then you could just use a single auth collection (by default "users" will be created). OAuth2 linking and everything related to the authentication will be only within the collection namespace.

Extend API rules to have full access to the JWT and authenticated user model (for example to get the attributes"profile.roles")

This is already supported. You should be able to query the user profile fields by doing @request.user.profile.*.

Implement OPA for API rules and JWT / user model is the input data

I don't plan implementing an extra access layer. It will complicate both the management in the UI and when used as framework. The event hooks are the recommended way to extend and tailor the default PocketBase behavior if users need more fine grained auth controls than what the API rules provide.

lukepighetti commented 2 years ago

While the field type “user” may be identical to a relation field pointing at the user table, I do think it’s a comforting and nice feature to have this dedicated “user” field type.

Also, I do think it’s important to not force PII like email into a public facing record. That appears to be one benefit of the separate “profile” table.

ganigeorgiev commented 2 years ago

@lukepighetti Based on the several raised issues and discussions in the past, most users seems to find it confusing to have both a user model and profiles collection (for example https://github.com/pocketbase/pocketbase/discussions/242).

Having only one entity ("collection") should be a lot easier and intuitive for the developers.

Also, I do think it’s important to not force PII like email into a public facing record. That appears to be one benefit of the separate “profile” table.

I agree, but since the users will be a collection, you will be able to limit the access to the user records with the collection API rules.

Users will always have the option to create their own profiles collection and reference it with a relation field. The only difference from the existing implementation is that it will not be created by default.

jimafisk commented 2 years ago

@ganigeorgiev with the new architecture would there be a way to allow users from different auth type collections to use the same login form? I'd like to have a manager-subordinate relationship between "staff" and "members" where we'd need to collect additional profile information for members, but don't need this for staff. This seems like the correct use-case for two different auth types, but wasn't sure if I'd need to handle this logins separately if I did that as opposed to a single auth type with a custom "role" field. Thanks!

ganigeorgiev commented 2 years ago

with the new architecture would there be a way to allow users from different auth type collections to use the same login form?

On the frontend you can show the same login form, but when submitting the form you'll need to specify the auth collection you are trying to sign-in, eg. client.collection("staff").authViaEmail(...). The idea is that each auth collections will have their own options to configure the auth behavior. Currently this are the options that I'm planning to use based on my needs:

type CollectionAuthOptions struct {
    FullAccessRule          *string  `form:"fullAccessRule" json:"fullAccessRule"`
    AllowOAuth2Auth         bool     `form:"allowOAuth2Auth" json:"allowOAuth2Auth"`
    AllowOAuth2Registration bool     `form:"AllowOAuth2Registration" json:"AllowOAuth2Registration"`
    AllowUsernameAuth       bool     `form:"allowUsernameAuth" json:"allowUsernameAuth"`
    AllowEmailAuth          bool     `form:"allowEmailAuth" json:"allowEmailAuth"`
    RequireEmail            bool     `form:"requireEmail" json:"requireEmail"`
    ExceptEmailDomains      []string `form:"exceptEmailDomains" json:"exceptEmailDomains"`
    OnlyEmailDomains        []string `form:"onlyEmailDomains" json:"onlyEmailDomains"`
    MinPasswordLength       int      `form:"minPasswordLength" json:"minPasswordLength"`
}

The configuration of the individual OAuth2 providers will be global and shared across all auth collections; each auth collection will have only the option to allow/disallow OAuth2 sign-in/up.

Additionally, each auth record will have an optional emailVisibility field, so that users can control on their own whether their email should be visible in listings or expansions (this will be ignored for admins or when the FullAccessRule is satisfied).

Each auth colleciton will have its own table, meaning that you'll be able to have a user registered both in staff and members. We'll ensure only that the auth record id is unique across all auth collections in order to simplify the API rules check when doing something like userId = @request.auth.id.

ganigeorgiev commented 2 years ago

@jimafisk If you don't have a way to identify upfront to which collection you want to submit the login form, you can apply a "failover" flow - first send the request to the staffs collection and if it fail send another request to the members collection. If both fail, show an error:

const email = "...";
const password = "...";

try {
    await client.collection("staff").authViaEmail(email, password);
} catch (_) {
    // failover to "members" authentication
    try {
        await client.collection("members").authViaEmail(email, password);
    } catch(err) {
        alert("Invalid login credentials");
    }
}
JessedeJonge commented 2 years ago

Just came accross this issue after starting to write an application with pocketbasae. I agree with this change wholeheartedly, users and profiles seperated made no sense to me. Do you have an ETA on when this change (0.8) wil release? I will just postpone my implementation so I do not have to rewrite

ganigeorgiev commented 2 years ago

@JessedeJonge It's hard to tell. I've been asked a lot lately for ETAs (especially on the support email) and I understand that it is important, but as I mentioned in https://github.com/pocketbase/pocketbase/discussions/123#discussioncomment-3539859, I work on PocketBase only on my free time and I don't want to constraint my self with hard deadlines.

The "pure" server-side changes (including a data migration command) are already done, but a lot of the existing unit and integration tests need to be rewritten from scratch, which along with the Admin UI redesign, probably will be the most time consuming parts of the refactoring.

Today I've taken a day off from my day job and I've started working on the server-side tests, but still there is a lot of work that remains and I don't want to rush it (especially when users authentication is involved).

If you need some "approximate time estimate", probably I'd say somewhere around mid October.

Threre would be a migration guide that will help with the upgrade, but if you are using the SDKs I expect the changes, excluding the merged users and profiles, to be mostly "cosmetics", eg. renamig client.records.getList("test") to client.collection("test").getList() or client.users.authViaEmail(...) to client.collection("users").authViaEmail(...), etc.

lukepighetti commented 2 years ago

Make sure you take your time so you don't burn out. PocketBase looks like it has a bright future.

osseonews commented 2 years ago

Just went thru this excellent thread, and I probably won't get too many people agreeing with my point of view, but heck I'll add my 2c, as I am a user of many Baas and similar type of software. I am amazed at certain aspects of Pocketbase, which make it a dream platform for certain business uses cases. However, I would say that user authorization is not one of the features on Pocketbase that I think is revolutionary or that will give it mass adoption. There are just too many authorization platforms that are way ahead and it will take an enormous amount of work to get it up to par and then keep up. Do you really want to spend your time developing a new authorization platform? Personally, I see the strengths of Pocketbase in entirely different areas and I think a focus on user authorization tables is a distraction, and that adding other features instead would be a more wise use of time, and would add alot more value to the platform. It's just a guess, but I suspect most people trying out Pocketbase are just using the admin interface, and admin APIs, because they want an easy to use backend for some application, that actually feels like database (personally I dislike how all these Baas try to be like a spreadsheet) and isn't painful to set up and use. I doubt storing user authorization is what most people are looking for, again because there are so many other solutions already out there that work well. Simply having user tokens, which Pocketbase already does well, is probably sufficient.

In terms of 2 other features that would be more beneficial than user authorization:

The above 2 features are honestly the only 2 critical features, I think are lacking right now in Pocketbase. Everything else added on top is obviously amazing, but without those 2, I think the use cases get a bit restricted here. It's doubtful anyone using Pocketbase really wants to spend time developing a front end for their team to add/edit data, and they probably already have serverless functions they can hook into for business logic.

One final feature, given my thoughts on user authorization:

ganigeorgiev commented 2 years ago

@osseonews For the SSO and 3rd party auth platforms - there is no built in solution for that, but you can always implement it on your won if you use PocketBase as framework by passing additional context from the 3rd party auth system. For example you could check https://github.com/pocketbase/pocketbase/issues/136#issuecomment-1186191919 (this will change slightly after the refactoring, but more-or-less the idea will be the same - an auth model will be considered authenticated if exist in the request contex).

osseonews commented 2 years ago

@ganigeorgiev Thanks. Issue 136 answer the question, but my difficulty is that I don't know Go and more importantly don't know how to set up Pocketbase as a framework on Digital Ocean. I've already got it up and running without extending it, and continue to be incredibly impressed. Maybe there is some guide for setting up framework on DO? Or maybe this is something pockethost.io can handle?

In any case, I do agree with the person in Issue 136 where they said as follows: "no more effort into auth. pocketbase already has more than enough for standard use." If you can easily autheticate with a header as in 136 code, then I think auth is solved, and most people, especially enterprises, will almost always use their internal auth provider.

jimafisk commented 2 years ago

It's a cool idea to allow 3rd party auth, but personally I would be sad if the built-in auth was sacrificed for this. The big sell for me is that PocketBase is an all-in-one solution that doesn't require complicated integrations. There are endless BaaS solutions targeting the enterprise market, imo PB carved a niche with practical self-hosting that's resonating with folks.

osseonews commented 2 years ago

@jimafisk I agree 100%, just saying the the auth that is available now is suitable for nearly all cases, and enterprise users, who will ultimately pay for this platform, will never migrate from their current auth platform to a pocketbase auth, so I don't see much benefit in further building out auth. "There are endless BaaS solutions targeting the enterprise market". Yes, that's true, but none are truly portable and all are silly expensive in the long-run with huge lock-in. Plus, most neglect some basic features that are available out of the box with Pocketbase. That's why I think Pocketbase resonates. Upload an executable and you have your database backend with an amazing admin interface + API in minutes! This is a dream! I think most people will use Pocketbase as a Baas to replace many use cases, but will still retain existing auth. Will be interesting to see how this develops.

heckad commented 2 years ago

Small proposal to move authViaEmail and all required data for them into provider table with provider type 'emailAndPassword'. New auth method will get provider type and required values for them. It's unify logic and solve problem with update user data because in user model will be stored only user data (not auth data which required verification in email case and maybe phone number in future).

New users model require only id, created, updated fields. All other fields optional.

New authProviders model with id, created, updated userId, providerType (forignkey to authProviderOptions. Some examples: 'email', 'phoneNumber', 'oauth2.google', 'oauth2.facebook'), lastUsed, and some json fileds provider_data(contains inforamthion from oauth token or for emial provider verified, verification_code if email have been sent and some other fields), meta (additional information like comment or any other)

New authProviderOptions model with id, type, options (type json). Exmaple row for email provider 1, 'emailAndPassword', { 'AllowedDomains': ['gmail'], 'EmailVerificationTokenDuration': 604800, 'templates': {'Verification': {...}, ...}}} and for google provider 2, 'oauth2.google', {'appName': ..., 'appSecret': ..., 'AllowAuth': true, 'AllowRegistration': true, 'AutolinkBy': ['email'], ...}

Api methods examples:

// sign in using email
await client.collection("users").signIn('emailAndPassword', {'email': 'example.com', 'password': 'qwerty1234'});
// sign up using email
await client.collection("users").signUp('emailAndPassword', {'email': 'example.com', 'password': 'qwerty1234'});
// sign in using google
await client.collection("users").signIn('oauth2.google', {'code': '...'}); // insted of code we can send accessToken.
// sign out
await client.signOut()

This model is highly expandable. In it, for example, you can generally remove the auth via email and leave only the auth via google. You can also easily add any options for any user auth method and for each auth provider globaly. What do you think aboud this changes?

ganigeorgiev commented 2 years ago

@heckad

Small proposal to move authViaEmail and all required data for them into provider table with provider type 'emailAndPassword'. New auth method will get provider type and required values for them. It's unify logic and solve problem with update user data because in user model will be stored only user data

But this is basically how currently the user data is managed using the profiles collection. You wouldn't really benefit too much by moving the email and other common user fields to a separate table - it will just complicate the internals requiring always to have a join with the related "auth provider". In addition, there will be always some auth specific field (eg. password) that will have to be shared across multiple auth providers (email, username, phone, etc.).

The idea of this issue is to simplify the users management and to add support for:

heckad commented 2 years ago

@ganigeorgiev

You wouldn't really benefit too much by moving the email and other common user fields to a separate table

What if it is necessary that only users from our google workspace can sign in? If email and password would be a simple provider, we just can off them. I just don't know whether to make it as a separate feature request or it's better to make it into the current changes since backwards compatibility breaks.

it will just complicate the internals requiring always to have a join with the related "auth provider".

Can you explain why does it required always to have a join with the related "auth provider"? Maybe we have an external jwt token generator which generates correct tokens. In such a case, we didn't require any linked auth method at all.

ganigeorgiev commented 2 years ago

@heckad

What if it is necessary that only users from our google workspace can sign in? If email and password would be a simple provider, we just can off them.

But you'll be able to do that (see the auth collection options in https://github.com/pocketbase/pocketbase/issues/376#issuecomment-1243574727). Why moving the email field in a separate table will help with that?

Can you explain why does it required always to have a join with the related "auth provider"?

Because you'll need to perform a search/sort based on the user email, username, etc. Additionally this will increase the complexity for developers using PocketBase as framework, because they will have to manually query or use some special helper to fetch the email every time when they need to read it.

Maybe we have an external jwt token generator which generates correct tokens. In such a case, we didn't require any linked auth method at all.

I don't see issue with that. If all possible auth methods are disabled (email, username, oauth2) you'll still be able to create and manage your users based on your custom auth logic. As mentioned in this https://github.com/pocketbase/pocketbase/issues/376#issuecomment-1250267806, an auth model is considered authenticated as long as it exists in the request context.

heckad commented 2 years ago

But you'll be able to do that (see the auth collection options in #376 (comment)). Why moving the email field in a separate table will help with that?

It's just that these fields only apply to one authorization method. It is very strange to see them in the main model when they may not be useful. Intuitively, I want to take them out to where they will be the very place.

Because you'll need to perform a search/sort based on the user email, username, etc. Additionally this will increase the complexity for developers using PocketBase as framework, because they will have to manually query or use some special helper to fetch the email every time when they need to read it.

Filter example:

client.collection('users').filter({"provider.emailAndPassword.email": "example@email.com"}}

Also, we can filter by another provider if a provider has different email

client.collection('users').filter({"provider.google.email": "example@gmail.com"}} ( email and other data field extracted from token)

I don't see issue with that. If all possible auth methods are disabled (email, username, oauth2) you'll still be able to create and manage your users based on your custom auth logic.

It would be great to have the built-in ability to use third-party tokens as if they were actually issued. For example, configure where to get JWKS and the user id field name. Probably this is also the feature request.)

ganigeorgiev commented 2 years ago

It's just that these fields only apply to one authorization method. It is very strange to see them in the main model when they may not be useful. Intuitively, I want to take them out to where they will be the very place.

That's not always the case. As mentioned previously, there are use cases where you need to share the fields with other auth providers. The password field is one example. Another is for example, linking an oauth2 account to an existing user by its email, or even verifying the user email with the oauth2 sign-in (this is actually already implemented in https://github.com/pocketbase/pocketbase/blob/master/forms/user_oauth2_login.go#L127-L188).

Filter example: ...

The shown example uses the SDK. If we move the individual email field to separate table, internally you will have to join it to the user model and this will not be very convenient when PocketBase is used as framework.

It would be great to have the built-in ability to use third-party tokens as if they were actually issued.

This is too user-land specific and I don't think it can be integrated by default. If you need custom auth logic, you have to use PocketBase as framework and atatch a router middleware similar to the linked example above. For JWKS you may want to check https://github.com/pocketbase/pocketbase/issues/144.

lukepighetti commented 2 years ago

I think discussion is good, but be careful bike shedding on such a small high leverage personal project unless there is a huge benefit to the proposals

lukeed commented 2 years ago

I think a big feature that should come out of (and enable) this is controlling which record fields should be public vs private.

For example, when merging users and profiles, the password, passwordConfirm, userId, and provider fields should be marked as private/hidden. This way, when attaching the profiles relation to a collection (eg articles), none of the sensitive information will be included when ?expand=author is present on the articles list response.

This would be useful if the toggles were available globally as I don't want all fields to be publicly visible in the API response.

ganigeorgiev commented 2 years ago

@lukeed Private/hidden API fields for now are left out of the scope of v0.8.

The implementation in the upcoming v0.8 pre-release hides by default all sensitive auth model fields (eg. password/passwordHash, tokenKey, etc.). The user will have an option only to toggle the visibility of their email using the system emailVisibility bool field. Or in other words, when expanding a user relation, you'll have access to the following props:

{
    // common meta fields available for all records
    "id": "...",
    "created": "...",
    "updated": "...",
    "collectionId": "...",
    "collectionName": "...",
    "expand": {...}

    // fields available only for auth collections
    "username": "...", // all users will have a required username (it is autogenerated by default)
    "verified": true,
    "emailVisibility": true,
    "email": "test@example.com", // visible only if emailVisibility is true

    // other collection fields...
    "name": "..."
}

The email address will be always visible (aka. ignoring emailVisibility) when 1 of the following conditions is met:

(the same applies when you perform search/sort over the email field)

If you need to control the visibility of the other fields, for now they'll have to be moved in a separate collection (aka. similar to the existing profiles workflow).