-
-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Add beforeLogin trigger with support for auth providers #5445
Add beforeLogin trigger with support for auth providers #5445
Conversation
Codecov Report
@@ Coverage Diff @@
## master #5445 +/- ##
==========================================
+ Coverage 93.93% 93.94% +<.01%
==========================================
Files 123 123
Lines 9024 9035 +11
==========================================
+ Hits 8477 8488 +11
Misses 547 547
Continue to review full report at Codecov.
|
@stage88 or @georgesjamous would either of you be willing to look at this pull request and give feedback on:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
Would it be worth guarding against maintainers accidentally adding beforeLogin
to classes other then _User
?
See function validateClassNameForTriggers
in https://github.com/parse-community/parse-server/blob/master/src/triggers.js.
Also see this test: 'should validate triggers correctly'
.
Thanks for taking a look @stage88 I've added the assertions as you suggested with one caveat. The test case |
Great effort 👏, thanks @omairvaiyani. @georgesjamous this looks good to me. CC @acinader |
@@ -41,6 +42,11 @@ function validateClassNameForTriggers(className, type) { | |||
// TODO: Allow proper documented way of using nested increment ops | |||
throw 'Only afterSave is allowed on _PushStatus'; | |||
} | |||
if (type === Types.beforeLogin && className !== '_User') { | |||
// TODO: check if upstream code will handle `Error` instance rather |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
@omairvaiyani I've started a draft PR for the documentation of this features in #617 on the docs repo. It would be great if you could take a look. |
@TomWFox that was quick! I've added some suggestions but nothing major. |
…ity#5445) * Add beforeLogin trigger with support for auth providers * adjust comment that boxed off beforeLogin to a negative use-case only * add internal error to help future maintainers regarding use of beforeLogin * let beforeLogin accept className or constructor like other hook types * add assertions for beforeLogin trigger className validation
@omairvaiyani @TomWFox Trying to only run a verification check when a user logs-in with a specific auth provider. |
This has been attempted before a couple of times but never seen to completion. I used some of the ideas by @carlosapgomes in his now closed PR, however most of the work was too out of sync with the current state of the library.
Currently:
User sign ups can be validated, rejected and side-effects issued in
beforeSave
onParse.User
. There is no similar functionality for user logins.Use cases:
Motivation / Bias:
We have a multi-tenanted platform wherein each client has their own subdomain. They share the same database and so, theoretically users from subdomain-a can legally login to subdomain-b. This new trigger allows us to check the request
headers
against the user to verify if the login is appropriate.Example usage:
Considerations:
This trigger behaves in a similar manner to other triggers:
TriggerRequest
that containsobject
,headers
,ip
,installationId
, etcafterSave
onParse.User
, it will not save mutations to the user unless explicitlysave
d.Parse.Error
if an exception is thrownThe trigger has some unique behaviours not seen in other triggers:
className
orParse.Object
constructor as the first argument; it is only meant forParse.User
user
on the request object - technically, the user has not yet been provided asession
until afterbeforeLogin
is successfully completedSome specific behaviours with regards to signup/login:
authProvider
loginsPlease let me know if there are side-effects that I have not accounted for in the added unit tests.