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

Add new afterLogin cloud code hook #6387

Merged
merged 2 commits into from
Feb 11, 2020

Conversation

davesters
Copy link
Contributor

This PR adds a new afterLogin cloud code hook. I referenced the pull request adding the beforeLogin hook done by @omairvaiyani quite heavily for this PR, so it was implemented in a very similar manner.

Currently there is no method to do any work after a user has successfully logged in, and Parse Server does not allow hooks on _Session objects.

Use cases:

  • Sending events to other parts of a system when a user logs in.
  • Recording a successful login event for analytics.
  • Performing any other necessary post login actions like sending emails, etc.

Motivation:

In my specific case, we have an API gateway and authentication service in other areas of our system. This uses Parse authentication and user sessions when necessary for Parse users. We have a need to keep track of Parse Sessions in other areas of our system, so with this new hook we are able to send events when a new session is created on login that can be consumed by our other services. We also use the afterLogout hook when sessions are deleted as well.

Example usage:

// In cloud code
Parse.Cloud.afterLogin(request => { 
  const { object: user } = request;
  // do stuff with user or session here.
});

@davesters
Copy link
Contributor Author

This is the PR for the beforeLogin hook I mentioned in my description.

#5445

@omairvaiyani omairvaiyani self-requested a review February 4, 2020 10:02
@omairvaiyani
Copy link
Contributor

Hey @davesters this is good stuff, I definitely see the use for it, and it rounds off the user's expectation of before/after hook pairing. I've only reviewed the test-cases so far and would like clarification on one point

Your afterLogin hook expects request.user to be undefined, which is okay given that the request was not initiated by an authenticated user, however, given that at this point in the flow, the user will have their session created, I can see why some developers would like access to this here.

I'm not sure if the user as set on request.object should have the sessionToken, but if it doesn't, we should consider making an exception by setting the user on request.user. Thoughts @davimacedo ?

@davesters
Copy link
Contributor Author

Thanks @omairvaiyani for the feedback.

Currently, the user is available on the request.object field, and I made user null at this time since it is available on the object field, but that is a good point in that the user is authenticated, so it does make some amount of sense to have it on the req.user field.

In terms of the session token, that is an interesting point. I left it in there because I need it for my specific use case, but I can see a good argument for taking it out. In which case, I could do a quick query inside the hook to look up the new session token for the user for my use case.

I will leave it alone for right now and wait for some additional feedback, but I am open to any of these changes as I think they do make sense.

Thanks!

@omairvaiyani
Copy link
Contributor

Sorry I think I might have given the wrong idea! My final point was that having the session token within afterLogin is actually a good idea, despite it somewhat going against the expected pattern of authenticated requests. However, I think it should be under the req.user, as all other requests in parse require developers to check within there to access the session.

@dplewis
Copy link
Member

dplewis commented Feb 8, 2020

Nicely done. Super simple. req.user should be there as it is the user that made the requests. Even if it is a new user.

@codecov
Copy link

codecov bot commented Feb 9, 2020

Codecov Report

Merging #6387 into master will decrease coverage by 0.13%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6387      +/-   ##
==========================================
- Coverage   93.98%   93.85%   -0.14%     
==========================================
  Files         169      169              
  Lines       11687    11694       +7     
==========================================
- Hits        10984    10975       -9     
- Misses        703      719      +16
Impacted Files Coverage Δ
src/Routers/UsersRouter.js 94.3% <100%> (+0.03%) ⬆️
src/cloud-code/Parse.Cloud.js 98.18% <100%> (+0.22%) ⬆️
src/triggers.js 93.36% <100%> (+0.41%) ⬆️
src/Routers/PushRouter.js 93.1% <0%> (-3.45%) ⬇️
src/Adapters/Storage/Mongo/MongoStorageAdapter.js 91.14% <0%> (-2.34%) ⬇️
src/batch.js 89.79% <0%> (-2.05%) ⬇️
src/ParseServerRESTController.js 96.36% <0%> (-1.82%) ⬇️
src/RestWrite.js 93.81% <0%> (-0.33%) ⬇️
src/Controllers/DatabaseController.js 94.95% <0%> (-0.17%) ⬇️
src/Adapters/Storage/Mongo/MongoTransform.js 88.67% <0%> (-0.15%) ⬇️
... and 1 more

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 3c46117...a474644. Read the comment docs.

@davesters
Copy link
Contributor Author

Thanks @omairvaiyani and @dplewis for the feedback. I have made a quick change to include the user object on the req.user field for the hook. I was unsure if I should remove it from the req.object field, but I left it there as it seems to match the pattern.

Thanks!

Copy link
Member

@davimacedo davimacedo left a comment

Choose a reason for hiding this comment

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

@dplewis it looks good to me. Do you also want to take a look?

@dplewis dplewis merged commit 09a1dca into parse-community:master Feb 11, 2020
UnderratedDev pushed a commit to UnderratedDev/parse-server that referenced this pull request Mar 21, 2020
* add new afterLogin cloud code hook

* include user on req.user for afterLogin hook
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.

4 participants