Skip to content

Conversation

dplewis
Copy link
Member

@dplewis dplewis commented Jul 3, 2019

Closes: #833

There aren't any CurrentUser in a server environment. This PR allows for use of sessionTokens and prevent errors from being thrown.

Let me know if more tests are needed and any feedback

@codecov
Copy link

codecov bot commented Jul 3, 2019

Codecov Report

Merging #846 into master will increase coverage by 0.02%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #846      +/-   ##
=========================================
+ Coverage   92.08%   92.1%   +0.02%     
=========================================
  Files          54      54              
  Lines        4939    4953      +14     
  Branches     1100    1103       +3     
=========================================
+ Hits         4548    4562      +14     
  Misses        391     391
Impacted Files Coverage Δ
src/CoreManager.js 100% <ø> (ø) ⬆️
src/ParseUser.js 83.14% <100%> (+0.55%) ⬆️

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 e19da0e...5a72b45. Read the comment docs.

@dplewis dplewis requested review from acinader and davimacedo July 3, 2019 23:59
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.

  • "logOut" looks good to me
  • "become" will probably make some developers confused. Currently, when become is called, the following operations automatically use the current user session token. That's not true for the Node.js. My suggestion is to leave the become as is now and create a new function (maybe called "me"?).
  • Shouldn't we take care of "signUp" and "logIn" as well?

@dplewis
Copy link
Member Author

dplewis commented Jul 6, 2019

My suggestion is to leave the become as is now and create a new function (maybe called "me"?).

Parse.User.me(sessionToken) @acinader What do you think?

Shouldn't we take care of "signUp" and "logIn" as well?

I believe those are already taken care of in a node js environment.

@acinader
Copy link
Contributor

acinader commented Jul 8, 2019

ugh. I can't quite wrap my head around this -- probably just soft after my holiday!

Most of the work I've done with the JS-SDK is cloud code, so I am familiar with the territory, but I could use a use case of what we're trying to accomplish (in the form of an integration test or two) to show what the goal of this change is more clearly.

I read #833 twice, but I can't quite piece together what we're solving and why.

@davimacedo
Copy link
Member

@acinader currently, there is no method in the Node.js SDK to log out or validate a session token. That's the problem @dplewis is solving. This PR does two different things:

  • Include the option of passing a session token argument in the logOut() method, so it can be used in Node.js;
  • Changes the become() method to be used in Node.js - for this one, in my opinion, I think we should create a new method maybe called me();

@acinader
Copy link
Contributor

acinader commented Jul 9, 2019

ok. I get the logout fix and I buy it. Not so sure about the become/me.

What use case would you use this become/me for? And where would the session Id be coming from?

When I've wanted server code to be able to 'log in', I have made an auth adapter so I can create a session using login.

And one other thought that may be wrong: would it make sense to make two user controllers: one for node and one for browser? They could inherit from a common ancestor that has the majority of the functionality, but one would be stateful and the other not.

@davimacedo
Copy link
Member

Currently the same UserController is used in two different scenarios:

  • Running in web browser:
  1. Client uses signUp(username, password), logIn(username, password), or become(sessionToken) functions
  2. SDK validates credentials/sessionToken in Parse Server and receive back a valid session token and the current user data
  3. SDK stores the session token, current user data and automatically uses the session token in any future api call
  4. Client uses logOut() to destroy the session token
  • Running in server
  1. Client calls some kind of signUp or logIn function in a custom Node.js application
  2. Node.js application use uses SDK's signUp(username, password) or logIn(username, password) functions
  3. SDK validates credentials in Parse Server and receive back a valid session token and the current user data (it does not store it, differently when used in the web browser)
  4. Node.js application returns session token and current user data to client
  5. Client stores session token and current user data no matter how
  6. For a future request (for instance, find() objects), client send to Node.js the session token
  7. Node.js calls SDK's find({ sessionToken }) function
  8. SDK sends the operation to Parse Server together with sessionToken and receive back the result
  9. Node.js receive result from SDK and send to client

The flow works, except:

  • There is no convenient method for logging out, because current SDK's logOut() function does not have sessionToken option - I understand this point we've already pacified
  • There is no convenient method for validating a session token and receiving back the user data (take a look in this thread here: https://community.parseplatform.org/t/session-token-validation-with-nodejs/87/27) - This is the become/me function; I agree with @dplewis that it should exist, I am not so comfortable with the name become() though, because it can cause some confusion to the developers as they may think that, after using this function, the next api calls will use the sessionToken automatically (and that's not true for the Node.js SDK). I suggested me, because it is actually the name of the REST endpoint that performs the job in the Parse Server side (https://github.com/parse-community/parse-server/blob/master/src/Routers/UsersRouter.js#L424).

I agree with you that we could have two controllers but I'd tackle this on a separate issue.

@acinader
Copy link
Contributor

acinader commented Jul 9, 2019

@davimacedo

whew. Thank you for laying that out so crystal clearly.

the only real purpose of me/become in the node environment would be to validate a sessionToken? or is there any other use case? I can't think of one.

If that's the only use, I'm good with it. My preference is also me over become now that I understand your rationale @davimacedo for that too.

On the two controllers, just asking for your view. After reading your response, the thread you cite and playing with some code, I don't think that breaking into an abstract class with two subclasses would really be worth overhead it would take to figure out what is going on. Though I don't have a strong opinion either way :).

@dplewis dplewis requested a review from davimacedo July 9, 2019 22:25
@davimacedo
Copy link
Member

the only real purpose of me/become in the node environment would be to validate a sessionToken? or is there any other use case? I can't think of one.

I think it is the only one

On the two controllers, just asking for your view. After reading your response, the thread you cite and playing with some code, I don't think that breaking into an abstract class with two subclasses would really be worth overhead it would take to figure out what is going on. Though I don't have a strong opinion either way :).

In my personal opinion, I think it is easier to understand if we go with the subclasses instead of the current approach in which I have to read the whole code and understand the "if" clauses. I think it is a low priority issue though.

@dplewis dplewis merged commit 309790c into master Jul 10, 2019
@dplewis dplewis deleted the stateless-user branch July 10, 2019 00:24
expect(user.existed()).toBe(true);
});

it('can retreive a user with sessionToken and masterKey(me)', async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a use case for this? In other words, how would this request differ from the above? The master key is included, but does it behave it any differently? I looked at the router code, but not sure...

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.

Stateless user controller for server-side user handling

3 participants