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

Login hook #4513

Closed
Closed

Conversation

carlosapgomes
Copy link

Create a login hook that is called just after username/password validation but before creating a session token.
It allows addition of custom logic before login completion, like a custom validation or user logging.
The way it can be used is by adding code to cloud-code file like:

Parse.Cloud.loginHook(function (userObjId) {
 // custom logic here
  return;
});

As I am new to parse-server, I am neither completely sure if this is the correct way of adding a hook nor if it adds any security threats.
Please, correct me if I did it wrong.
Thanks

@codecov
Copy link

codecov bot commented Jan 19, 2018

Codecov Report

Merging #4513 into master will decrease coverage by 0.14%.
The diff coverage is 54.54%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4513      +/-   ##
==========================================
- Coverage   92.86%   92.71%   -0.15%     
==========================================
  Files         118      118              
  Lines        8394     8405      +11     
==========================================
- Hits         7795     7793       -2     
- Misses        599      612      +13
Impacted Files Coverage Δ
src/Routers/UsersRouter.js 93.38% <100%> (+0.09%) ⬆️
src/triggers.js 92.45% <42.85%> (-1.7%) ⬇️
src/cloud-code/Parse.Cloud.js 94.73% <50%> (-2.49%) ⬇️
src/Adapters/Cache/InMemoryCache.js 91.66% <0%> (-8.34%) ⬇️
src/Adapters/Auth/meetup.js 84.21% <0%> (-5.27%) ⬇️
src/Routers/PushRouter.js 92.85% <0%> (-3.58%) ⬇️
src/RestWrite.js 93.28% <0%> (-0.55%) ⬇️
...dapters/Storage/Postgres/PostgresStorageAdapter.js 97.01% <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 33890bb...5535d5c. Read the comment docs.

@addisonElliott
Copy link
Contributor

Not a bad start for sure. I have an issue and even a pull request started trying to do a similar thing.

The one thing that tripped me up is that logging in should be considered from email/password AND from a 3rd party authentication like Facebook, Twitter, etc.

From my understanding, I think the UsersRouter only handled with username and password case. I'll have to take a closer look though.

Not a bad start and I hope we can get something working!

@carlosapgomes
Copy link
Author

carlosapgomes commented Jan 20, 2018

Hi @addisonElliott, thanks for the comment.

What do you think about calling that same hook at this point on src/Adapters/Auth/index.js

It seems that, at this point, a user is authenticated with the provider, but the promise is not resolved yet. A 'try/catch' could call the hook and resolve/reject the promise.
However, what is available here is authData and not a Parse.User object, as in src/Routers/UsersRouter.js.

But loginHookHandler can be set to receive just an object, then it will have to find out what kind of object was received and do queries, if needed.

@addisonElliott
Copy link
Contributor

I'm not sure if that's the best place to put the hook trigger.

Just to give you some more information, here is the issue(#4020) and pull request(#4373) I have.

My idea was to have a beforeSave and afterSave trigger for the Session class. Since a new session is created when logging in via a 3rd party authenticator and via regular login, this would work. However, I'm not sure if there are any other scenarios where sessions are created, such as email verification, etc?

For the login hook, I think it would be a good idea to pass the user along as well as the session variable. This way the person can use that hook to see how the person logged in and who the person is. With that in mind, that means you will need to call the hook after the session is saved and when the user is authenticated.

On another note, I would suggest you write some tests to test your new trigger to make sure it works as expected.

@carlosapgomes
Copy link
Author

I see your point, but one of my use cases is to add extra validation to login and I am not sure if it would be possible to block or abort the login flow after session creation. I will check that and try to find a better place to call this hook.
Thanks for the suggestions,

@addisonElliott
Copy link
Contributor

Good point. I had this same thought when I was implementing it. I wanted to have the hook be able to block the login as well, since I could see that as being a possible event.

I didn't get look into it too much, but I think you might be able to do it after the session creation. Just if the hook declines the login, then the created session might need to be deleted, which is a bit redundant. Also, duplicated sessions are deleted each time the person logs in, so maybe don't even delete the old session and let it be deleted next time?

Another thought off the cuff is to call the login before the session is created and just pass along the information that the session has. Really, the only interesting stuff is what login method was used such as facebook, password, etc.

Wish I had more time to work on this myself. I will definitely monitor this PR and mine to hopefully reach a conclusion that adds this feature.

…meone can run code to do extra validation, notification or logging
@flovilmart
Copy link
Contributor

flovilmart commented Jan 24, 2018

HI @carlosapgomes , thanks for the PR.

Before we go any further I want to point out that if we add a new 'hook' it shoud always follow the same conventions.

Namely:

  • the hook methods should be named Parse.Cloud.afterLogin
  • A beforeLogin hook would be appreciated as well, this one, like all beforeSave's could fail.
  • The payload, should contain the same properties as all other 'hooks' namely object, headers etc...
  • You should probably wrap the call the use user provided code into a Promise / try/catch in order to prevent any error being propagated to the client in case of failure.
  • You should not wait till the function resolves, as for all afterSave hooks.

I'll be closing this PR now, Please take into account all aforementioned advices and reopen the PR when you feel it's more appropriate.

@flovilmart flovilmart closed this Jan 24, 2018
@carlosapgomes
Copy link
Author

@flovilmart , thanks for the directions.
I am still working on it.
I will do my best to comply with those requirements and will reopen the PR when I got more code working.

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.

3 participants