Closed beeplin closed 6 years ago
Thanks. I'll look into this when I'm free.
You may want to look into
https://github.com/eddyystop/feathers-test-populate-etc
https://github.com/feathersjs/feathers-hooks-common/issues/42
to see if that could be useful to you. Your comments would be appreciated.
On Thu, Nov 10, 2016 at 5:14 AM, Beep LIN notifications@github.com wrote:
Currently 1.0.0 supports short verification token for SMS/social media, but email is still the essential way to identify a user. For example, code here https://github.com/eddyystop/feathers-service-verify-reset/blob/master/src/client.js$L82
this.authenticate = (email, password, cb) => { let cbCalled = false;
app.authenticate({ type: 'local', email, password }) .then((result) => { ... });
};
Here we still suppose email and password is the only way to login. And code here https://github.com/eddyystop/feathers-service-verify-reset/blob/master/src/client.js#L72 :
this.emailChange = (password, email, user, cb) => { verifyReset.create({ action: 'emailChange', value: { password, email }, }, { user }, cb); };
The API only supports changing email and then sending a notification message to the old email.
But consider a project in which most users don't even have emails ... like my on-going project for teenagers and elders in China. Our survey shows most of our potential teenage customers were born at the era of cellphone and QQ and wechat (just whatsapp and facebook and snapchat for teenagers in the US), and most of our elder customers just missed the email era -- they don't know how to register an email account and how to check email, but all of them have smart phones and wechat accounts.
So perhaps we could just treat email as just one of the multiple possible communication channels to make contact with our users, just like others like cellphone, snapchat etc. I noticed now there is no more emailer but a new userNotifier, so likewise, I think email should be no more treated as a specialty in our API, either.
My current way is to add a primaryCommunication field in the users table in database. It determines which way is the primary communication way to contact with the user, like email or sms or other channels. It is chosen by users when they sign up. Of course we could make email the default, but people have other options. They could choose to sign up with their cellphone number instead of email, and then every communication with them (verify, reset, notification of cellphone number/pasword change) should be sent through sms.
And for login or authenticate, it is unnecessary to force uses to login with email/password. Many apps let people log in with a unique username. Or, if a user's primaryCommuniation is not email but cellphone or sms, he/she could log in with his/her cellphone number.
That's to say, when the user is logging in, instead of the two params named email and password, an options map should be passed to app.authenticate. The developer can decide whether a username is required, or an email, or a cellphone number, or any of them works. So the API at here https://github.com/eddyystop/feathers-service-verify-reset/blob/master/src/client.js$L79 can be changed into this:
this.authenticate = (options, cb) => { let cbCalled = false; options.type = 'local'; // add type field to options app.authenticate(options) // pass options directly to app.authenticate() .then((result) => { ... });
And it's up to developer which fields should be included in options.
Similarly, this.emailChange might be rename to this. PrimaryCommunicationChange or something alike. If the user's primaryCommunication is not 'email' but 'snapchat', this method would patch the new value to the 'snapchat' field, not the 'email' field, and then notify the user via snapchat instead of email.
Some other methods like sendResetPwd and passwordChange also need to be changed a bit.
So to wrap up, my whole idea is to treat email equally with other possible communiation channels. People who aren't familiar with emails would appreciate that. This is not only about the long/short token when verify/reset, but also about the way for users to sign up, log in, etc.
Sorry I didn't bring this up before 1.0.0 is published. This idea just came up later than that. :)
— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/eddyystop/feathers-service-verify-reset/issues/18, or mute the thread https://github.com/notifications/unsubscribe-auth/ABezn97oEvEWlcrQmKV_1nj-f5fZNu-gks5q8u6cgaJpZM4KufRL .
hmmm, currently resendVerifySignup
also only accept email or token as user identifier. It's better to also allow username
, cellphone
etc.
There is an option in option
which specifies what user
props may be used in some APIs in order to identify the user. I'll be looking at either using that option or at introdcuing a new one for this issue.
The configuration will determine what props are permitted.
yes, and the key point is that in a project some users may sign up with email and others with sms. so we need not only an extended option when configuration but also am additional field in the users database to determine which is the user's primary communication (say, the identifier)
@beeplin Would you mind commenting on the new populate & serialize hooks if you do have an interest in them? The README has all the information needed. Thanks.
the new populate hook seems amazing~ I will read the doc and comment in issues of that repo.
I don't handle feathers-authenticate which is key to the interesting points you raise. I have informed @ekryski of this post and asked for his comments.
The README for feathers-authenticate
has been updated for the new version.
I'll look into the other related issues.
Really interesting @beeplin. That makes total sense. The new auth supports anything and doesn't require an email. The params required are dependent on the passport auth strategy that you decide to use so it now supports what you were suggesting:
The developer can decide whether a username is required, or an email, or a cellphone number, or any of them works. And it's up to developer which fields should be included in options.
I'll need to dig into this module a bit after I'm done with the new auth. I have yet to try it. I've implemented something similar in proprietary projects so @eddyystop and I have talked about collaborating on this.
"You need to configure the usernameField
on the old auth to support something other than email. The new auth is completely flexible. I’m pushing the new client this afternoon."
@ekryski I believe the following might be possible based on your comment:
username
is what the authenticate
is configured to use.users
with what was entered to identify the user.authenticate
with the password and user.username
to sign in.and this can be done with both the old and the new authenticate?
Come to think of it, instead of username
you could use a field populated with a randomly generated string. Perhaps even use _id
.
@beeplin I've tried to extract what you propose into concrete changes. Could you please see if these are complete?
Potential changes for v1.1.0.
checkUniqueness(uniques, ownId, meta)
OKresendVerifySignup(emailOrToken, notifierOptions)
userNotifer
uses user.primaryCommunication
to route notification.verifySignupWithLongTokenverifySignupWithLongToken(verifyToken)
OKverifySignupWithShortToken(verifyShortToken, findUser)
OKsendResetPwd(email, notifierOptions)
userNotifer
uses user.primaryCommunication
to route notification.resetPwdWithLongToken(resetToken, password)
OK
-resetPwdWithShortToken(resetShortToken, findUser, password)
OKpasswordChange(user, oldPassword, password)
[edit]
user.email
is used. [edit]emailChange(user, password, email)
user.verifyProp
to { prop: value }userNotifier
to send the verify tokens like resendVerifySignup does now.prop
!== user.primaryCommunication
prop
=== user.primaryCommunication
user.primaryCommunication
one. verifySignup(query, tokens)
: called by verifySignupWithLongToken and verifySignupWithShortToken
user.verifyProp
to change the appropriate field.user.verifyProp
along with verifyToken, verifyTokenShort and verifyExpires.user.verifyProp
on initial creation of user so that case is not different than emailChange.sanitizeUserForEmail
to sanitizeUserForNotifier
. [edit]options.userPropsForShortToken
.Perhaps we should have a sign in wrapper and/or API which helps with the following.
username
is what the authenticate
is configured to use.users
with what was entered to identify the user.authenticate
with the password and user.username
to sign in.username
you could use a field populated with a randomly generated string. Perhaps even use _id
.feathers.authenticate
itself.The present wrapper authenticate
may have to change for authenticate 1.0
[edited with beeplin's comment below]
Wow the new feathers-authentication
is really a huge change. I will take some time to dig into it.
All potential changes seem OK except one:
note: I don't see passwordChange needing any changes.
Currently in index.js
the passwordChange
function uses user.email
to identify the user, which needs to be changed:
function passwordChange(user, oldPassword, password) {
debug('passwordChange', oldPassword, password);
return Promise.resolve()
.then(() => {
ensureValuesAreStrings(oldPassword, password);
return users.find({
query: {
email: user.email // ** HERE email is used **
}
})
.then(data => (Array.isArray(data) ? data[0] : data.data[0]));
})
@beeplin @eddyystop basically with the new auth to support this there are 2 ways you could approach this:
Register separate strategies (either existing passport ones or your own custom ones). So depending on what field the user fills in you make a call to authenticate with the correct strategy. The client would call authenticate
like so:
// email auth
app.authenticate({
strategy: 'local',
email: 'me@mydomain.com',
password: 'password'
});
// phone auth
app.authenticate({
strategy: 'mobile',
phone: '15551234567',
});
// wechat username
app.authenticate({
strategy: 'wechat',
username: 'beeplin',
});
and on the backend you would register 3 separate strategies which each look up the user the correct way, verify them, and return them. You can see how feathers-authentication-local does it.
app.configure(auth({secret: 'mysecret' })
.configure(jwt())
.configure(local());
app.passport.use('mobile', new CustomMobileStrategy());
app.passport.use('wechat', new CustomWeChatStrategy());
app.service('authentication').hooks({
before: {
// attempt all the strategies in that order. If one succeeds proceed. Fail if they all fail
create: auth.hooks.authenticate(['local', 'mobile', 'wechat'])
}
});
Overload the username
field and extend the feathers-authentication-local
Verifier
class:
On the client:
// email auth
app.authenticate({
strategy: 'local',
username: 'me@mydomain.com',
password: 'password',
query: { method: 'local' }
});
// phone auth
app.authenticate({
strategy: 'local',
username: '15551234567',
password: 'temp password or empty',
query: { method: 'phone' }
});
// wechat username
app.authenticate({
strategy: 'local',
username: 'wechat handle',
password: 'temp password or empty',
query: { method: 'wechat' }
});
On the server:
const Verifier = require('feathers-authentication-local').Verifier;
class MyVerifier extends Verifier {
verify(req, username, password, done) {
switch(req.query.method) {
case 'local':
// verify by username/password
break;
case 'phone':
// verify by sms
break;
case 'wechat':
// verify by wechat
break;
}
done(null, user);
}
}
app.configure(auth({secret: 'mysecret' })
.configure(jwt())
.configure(local({ Verifier: MyVerifier }));
app.service('authentication').hooks({
before: {
create: auth.hooks.authenticate('local')
}
});
Note: I think this second option would be dependent on the auth client passing query params as part of authentication, which I think is totally doable.
Regarding your comments @eddyystop:
Perhaps we should have a sign in wrapper and/or API which helps with the following.... However it may be wise for the wrapper not to call feathers.authenticate itself.
You now need to fetch the entity (ie. user) explicitly on the client so I don't think we need the authenticate
wrapper anymore. I think checking whether the user is verified should be up to the developer and if they are using sockets they can listen for a patch
and check that the user has now become verified.
@eddyystop a couple suggestions I have:
user
it just defaults to one but you can customize the service
and the entity
options. I think this likely should do the same. 95% of the time it will be a user but this service could easily be applied to other entities (such as an organization).ownId
field more flexible by using service.id
on this line. See how I do it with feathers-authentication-jwt. All datastore backed service adapters now have an id
field.index.js
to separate files to make the files smaller and easier to trace throughrestrictToVerified
hook to isVerified
so that it follows the same naming convention as feathers-permissions and feathers-authentication.userNotifier
to just notifier
auth.hooks.authenticate('jwt')
(or whatever strategy you want) and imho should be registered by the developer instead of hidden away in the service.setup
method you don't need to do this here and instead can set this.service
inside the setup method.checkUniqueness
for pre-checking before set? In many cases I think this could/should be a before
hook that is a validator? Maybe I'm missing the use case.Happy to help out with this effort next week. This is really sweet and supports more use cases than the other ones I have implemented.
@eddyystop would you be up for moving this to core Feathers? I think it warrants being a core module and I think it's pretty darn close to being API stable with some minor changes. We could name it feathers-verify
or feathers-confirmation
and create an epic around it to track all these tasks.
@ekryski I'll look into your comments. It may take a while for me to grok them.
checkUniqueness
is primarily intended for use by the UI. It helps the UI display messages like Username already in use. Try another name
. That being the case it's API has to be something typically expected by UIs.
I agree this should be in core.
Potential names keep getting dated as the scope of the repo expands. (Thanks @beeplin. LOL) I think we should soon'ish expand notifier
with suggested HTML-based text for each message, for each comm method. That would save dev time.
Perhaps we should have a generic feathers repo that routes messages to email, SMS, whatsapp, facebook, qq, wechat, etc. and notifier
could use that module.
Because of this perhaps we should consider a name suggesting what problem the repo solves rather than what it does. Maybe feathers-authentication-local-extend
-expand
-extension
-addon
though they all impinges on feathers-authentication-local
. Any suggestions @beeplin?
user
it just defaults to one but you can customize the service
and the entity
options. I think this likely should do the same. 95% of the time it will be a user but this service could easily be applied to other entities (such as an organization).ownId
field more flexible by using service.id
on this line. See how I do it with feathers-authentication-jwt. All datastore backed service adapters now have an id
field.index.js
to separate files to make the files smaller and easier to trace throughrestrictToVerified
hook to isVerified
so that it follows the same naming convention as feathers-permissions and feathers-authentication.userNotifier
to just notifier
auth.hooks.authenticate('jwt')
(or whatever strategy you want) and imho should be registered by the developer instead of hidden away in the service.setup
method you don't need to do this here and instead can set this.service
inside the setup method.@ekryski I plan to deprecate this repo. I need a name to open a new repo with these changes, then add what @Beeplin has suggested. How about feathers-auth-services
? Its generic enough to add more things to it.
@eddyystop may as well stick with the same convention as with the other repos. How about feathers-authentication-management
or feathers-authentication-manager
? I want to stay away from services
because there is a service inside auth already.
I figure we might end up including a password
service or a authorization
service or something in the future to support more complex authorization schemes.
I think the core functionality of this repo is "verify" -- verifying the identity of the user before signing up, resetting password, changing communication channel. So something like "feathers-authentication-verify" should be ok.
Thanks for your reply beeplin. Unfortunately I have already created feathers-authentications-management.
I will be doing any future work on that repo.
never mind. "management" is much more general then "verify" so we are allowed to extend it to more functionalities in the future.
@beeplin @林
You may want to review https://github.com/feathersjs/feathers-authentication-management/issues/1 which hopefully implements your recommendations, and lists the migration issues coming from verify-reset. The README is yet to be updated. @ekryski is yet to comment.
I'm done with https://github.com/feathersjs/feathers-authentication-management . Waiting on bugs, short comings, comments.
Hello @beeplin @林 .
All of my FeathersJS repos will be scoped under @feathers-plus in v3. I have a Vuejs-based theme for their documentation, and I was wondering if you have the time to offer me your feedback. I need to finalize the look before rewriting all the docs, including the common hooks.
Fork feathers-plus/docs.feathers-plus.org npm install hexo generate (redo if things go wonky) hexo server Point a browser at localhost:4000 Only Extensions ... batch-loader is available.
Feel free to email me at johnsz9999 (at) g mail (dot) com if that is more convenient.
Thanks.
sure~
I am reading the new v3 api docs. Will check it asap. :)
Currently 1.0.0 supports short verification token for SMS/social media, but email is still the essential way to identify a user. For example, code here
Here we still suppose
email
andpassword
is the only way to login. And code here:The API only supports changing
email
and then sending a notification message to the old email.But consider a project in which most users don't even have emails ... like my on-going project for teenagers and elders in China. Our survey shows most of our potential teenage customers were born at the era of cellphone and QQ and wechat (just whatsapp and facebook and snapchat for teenagers in the US), and most of our elder customers just missed the email era -- they don't know how to register an email account and how to check email, but all of them have smart phones and wechat accounts.
So perhaps we could just treat
email
as just one of the multiple possible communication channels to make contact with our users, just like others like cellphone, snapchat etc. I noticed now there is no moreemailer
but a newuserNotifier
, so likewise, I thinkemail
should be no more treated as a specialty in our API, either.My current way is to add a
primaryCommunication
field in theusers
table in database. It determines which way is the primary communication way to contact with the user, likeemail
orsms
or other channels. It is chosen by users when they sign up. Of course we could makeemail
the default, but people have other options. They could choose to sign up with their cellphone number instead of email, and then every communication with them (verify, reset, notification of cellphone number/pasword change) should be sent through sms.And for
login
orauthenticate
, it is unnecessary to force uses to login withemail/password
. Many apps let people log in with a uniqueusername
. Or, if a user'sprimaryCommuniation
is notemail
butcellphone
orsms
, he/she could log in with his/her cellphone number.That's to say, when the user is logging in, instead of the two params named
email
andpassword
, an options map should be passed toapp.authenticate
. The developer can decide whether a username is required, or an email, or a cellphone number, or any of them works. So the API at here can be changed into this:And it's up to developer which fields should be included in
options
.Similarly,
this.emailChange
might be rename tothis.PrimaryCommunicationChange
or something alike. If the user'sprimaryCommunication
is not 'email' but 'snapchat', this method would patch the new value to the 'snapchat' field, not the 'email' field, and then notify the user via snapchat instead of email.Some other methods like
sendResetPwd
andpasswordChange
also need to be changed a bit.So to wrap up, my whole idea is to treat
email
equally with other possible communiation channels. People who aren't familiar with emails would appreciate that. This is not only about the long/short token when verify/reset, but also about the way for users to sign up, log in, etc.Sorry I didn't bring this up before 1.0.0 is published. This idea just came up later than that. :)