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

beforeLogin Hook #4524

Closed
wants to merge 17 commits into from
Closed

Conversation

carlosapgomes
Copy link

Hello!

I am reopening PR #4513 with a beforeLogin Hook name.
I think it relates to similar PRs: #4020 and #1016.

In my use case I need a way to add extra validation to the login process. To do that I think it is important to know who the user is but still be able to abort the whole login flow.

So, biased by my use case, I think that the definition of beforeLogin is not before the login process begins, but before it is completed, that is, the hook should be placed in a point on the login flow where the user has already had her credentials verified by parse-server, but before token creation.

With that in mind, I added code to allow the following snippet to be called from the login process and still be able to throw errors that would abort it:

Parse.Cloud.beforeLogin(function (userLoginData) {
 // custom logic here
 if(condition){
           throw new Parse.Error(Parse.Error.OTHER_CAUSE, 'some msg to the client');
 }else{
  return;
 }
});

where userLoginData is an object:

{
  'objectId': user.objectId,
  'username': user.username,
  'email': user.email,
  'name':user.name,
  'createdAt':user.createdAt,
  'updatedAt':user.updatedAt,
  'authProvider': '',
  'authData': {}
}

authProvider is a string with the provider name (for login with authProvider) or just password (for login with username/password).
authData is the same authData object available in the authProvider login flow and is an empty object for login with username/password.

I wrote some tests in spec/BeforeLogin.spec.js, but I am pretty sure that they do not cover all possible side effects.

So I would like to ask for opinions and suggestions especially about the choices I made defining beforeLogin, the way it was implemented and about what other tests should I do.

Thanks

@codecov
Copy link

codecov bot commented Jan 30, 2018

Codecov Report

Merging #4524 into master will increase coverage by <.01%.
The diff coverage is 96.15%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4524      +/-   ##
==========================================
+ Coverage    92.9%   92.91%   +<.01%     
==========================================
  Files         118      118              
  Lines        8448     8473      +25     
==========================================
+ Hits         7849     7873      +24     
- Misses        599      600       +1
Impacted Files Coverage Δ
src/RestWrite.js 93.89% <100%> (+0.24%) ⬆️
src/cloud-code/Parse.Cloud.js 97.36% <100%> (+0.14%) ⬆️
src/triggers.js 94.47% <100%> (+0.23%) ⬆️
src/Routers/UsersRouter.js 93.05% <87.5%> (-0.33%) ⬇️
...dapters/Storage/Postgres/PostgresStorageAdapter.js 97.12% <0%> (-0.1%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update db8594d...323b5ee. Read the comment docs.

src/RestWrite.js Outdated
return validateAuthData(authData[provider]).then(() => {
if (this.response) {
// it is a login call
runBeforeLoginHandler({
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As mentioned previously, the object passed to the beforeLogin hook should match the ones provided in other hooks like beforeSave http://parseplatform.org/Parse-SDK-JS/api/v1.11.0/Parse.Cloud.html#.TriggerRequest

The req.object property should be an instance of Parse.User, all the key types should match their proper types (createdAt/updatedAt as dates), authProvider don’t make sense on req.object but may be left on the top level.

We also expect all other keys like installationId, headers etc.. to be set

Copy link
Author

@carlosapgomes carlosapgomes Feb 1, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @flovilmart, sorry for the incomplete PR, I misunderstood your previous message. I am fixing it.
Thanks,

@flovilmart
Copy link
Contributor

flovilmart commented Feb 1, 2018 via email

@omairvaiyani
Copy link
Contributor

@carlosapgomes May I ask why this PR was closed? Could you summarise what you've achieved and which areas you need help with? I have a significant use-case for this in my application and look forward to seeing this accepted.

@addisonElliott
Copy link
Contributor

@omairvaiyani If it's any assistance, this is on my TODO list that I hopefully will get to soon. I have a similar PR with before/after Session triggers that I hope to work on. Maybe even this week or the next if all goes well.

@omairvaiyani
Copy link
Contributor

@addisonElliott excellent, I'll watch this PR and help where possible

@carlosapgomes
Copy link
Author

Hi @omairvaiyani ,

I got to parse-server while looking for a nodejs framework to rewrite one of my web apps. One of my main needs was to have login hooks, and I thought that it could be implemented with parse-server's hooks infrastructure.
While I was trying to do that I got to know better feathersjs, and realised that, for my use case, it has a hooks structure a little easier to use. That's why I chose to close this PR and move on.
Sorry for the incomplete work.

@omairvaiyani
Copy link
Contributor

@carlosapgomes Not a problem - I'm sure @addisonElliott will still find use in your commits

@haroonKhan-10p
Copy link

@omairvaiyani did you give it a go?
I need this feature too.

@haroonKhan-10p
Copy link

@flovilmart can you please say whats left in this PR? So, I can finish it asap, and use it in my current project? Its kinda urgent feature.

@haroonKhan-10p
Copy link

@carlosapgomes please grant me access on your fork, so I can push commits.

@flovilmart
Copy link
Contributor

@haroonKhan-10p please open a new pull request.

@addisonElliott
Copy link
Contributor

@haroonKhan-10p I would suggest opening a new pull request and if you use code from an existing PR, link to it and credit the author.

As an FYI, I have a similar PR here #4020 that uses a different approach. I would recommend trying to merge and pick and place these two methods to get beforeLogin/afterLogin working.

@ferrinho
Copy link

Any news on this? This would be a really great feature.

@NicksonYap
Copy link

+1

@omairvaiyani
Copy link
Contributor

Re-attempted in this PR

@SebC99
Copy link
Contributor

SebC99 commented Apr 30, 2020

Hello @omairvaiyani, if I understand well, the #5445 is not the same PR, as it's not allowing you to have the auth providers @carlosapgomes was talking about in the first message here.
Have you achieved this in another way?
I would love to see this PR merged too!

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.

8 participants