Add support for master key clients to create user sessions #7406

GormanFletcher · 2021-05-28T18:39:22Z

New Pull Request Checklist

I am not disclosing a vulnerability.
I am creating this PR in reference to an issue.

Issue Description

This PR proposes an implementation for #6641: a way for Cloud Code (or any master key client) to create user sessions without access to the user's credentials, and without presumption of an existing user session. This is useful for user impersonation (customer support), or for creating a standalone API that can act with a user's authority after validating the user's identity some other way (e.g., API key).

Related issue: #6641

Approach

I've created a new REST endpoint that creates a new session for the given user ID if the master key is supplied. I elided the before/after login triggers since it's not really a login - I assume we want a clear semantic distinction between normal user activity and e.g., a user making an API call against their account that creates a session, or a customer support representative logging in as the user to investigate a ticket. Maybe a parameter in the API call should toggle hooks on/off?

TODOs before merging

Still needs docs updates, and I'll submit a PR for the JS SDK, both after I'm given the thumbs-up on the new API.

Add test cases
Add entry to changelog
Add changes to documentation (guides, repository pages, in-code descriptions)
Add new Parse Error codes to Parse JS SDK?
- Not sure if I'm using the existing error codes correctly.
What authProvider should the session be flagged with? Is password okay, or do we need a new value?

sadortun

This PR is awesome!

I suggest the REST endpoint to be renamed to impersonateUser

src/Routers/UsersRouter.js

mtrezza · 2021-05-29T11:02:28Z

Add security check
I wasn't sure what to do here; the existing auth routes don't use CheckGroups, but I did see they check req.isMaster, so I used that as a starting point.

I don't think a security check is needed here. You are not adding a configurable feature / new option to Parse Server. Security checks evaluate options that the developer has configured and that may create points of attacks if not configured properly.

If you agree that no security check is needed, you may well delete the TODO from the template.

Still needs docs updates, and I'll submit a PR for the JS SDK, both after I'm given the thumbs-up on the new API.

This should be a TODO in the related issue, not in this PR. Since the issue has not been created by you (and is using an old template), you could just add it as a comment or create a new issue and reference the existing issue. The issue itself should contain all TODOs that are necessary for this feature, such as:

Parse Server PR
REST API doc change

Any implementation in the Parse JS SDK (or any other Parse client SDK) could be regarded as a separate undertaking, each with its own required TODO in the respective SDK guide.

Should the fn return the Session, or the session token?

The REST response should be the same response as for the /login endpoint. This allows to leverage existing logic in Parse Server and for the developer in custom code.

src/Routers/UsersRouter.js

GormanFletcher · 2021-06-02T17:26:54Z

I've created a pull request to the docs repo covering the new endpoint.

GormanFletcher · 2021-06-02T17:30:44Z

What's the appropriate way to handle locking out a user from the /loginAs endpoint? If a user's account is locked out, we wouldn't want them being allowed to hit a custom API endpoint that performs a /loginAs. I could make /loginAs perform the same ACL check that /login makes, but then we'd need a way to signify whether the request should override the lockout. /login marks the difference by the presence of the master key, but that won't work for /loginAs since it always requires the master key.

Should /loginAs simply take another parameter, honorLockout, defaulting to true?

mtrezza · 2021-06-02T17:38:34Z

If a user's account is locked out, we wouldn't want them being allowed to hit a custom API endpoint that performs a /loginAs.

I hope I understand your question correctly. Security of a custom API endpoint is up to the developer, whatever their use case requires.

Which account do you mean would be locked - the user who invokes loginAs or the user that is returned by the request?

GormanFletcher · 2021-06-02T17:44:06Z

I hope I understand your question correctly. Security of a custom API endpoint is up to the developer, whatever their use case requires.

Which account do you mean would be locked - the user who invokes loginAs or the user that is returned by the request?

Should /loginAs forbid logging into a user account whose ACL is master-key only, the same as /login does, since they're both named 'login'? Or should /loginAs allow logging into a locked-out account, since the master key is always supplied?

I could go either way - I just need to know whether I should make a code change to this PR to allow supplying a parameter to /loginAs to honor or bypass a lockout, or an annotation in the docs PR that developers need to manually check for a locked-out user when using /loginAs.

mtrezza · 2021-06-02T17:49:15Z

The master key always trumps any restriction and ACL.

loginAs should always succeed with master key, because if it does not, how else would one override that restriction, if not - again - with the master key? If there is any check required, then the developer can create a Cloud Function that conditionally calls loginAs.

GormanFletcher · 2021-06-02T18:01:36Z

Got it. I've updated my REST docs PR with that information.

This PR still has two outstanding questions:

Confirmation that I'm using the correct error codes in each situation
What's the appropriate authProvider value to use? I don't know if this behavior is a match for any existing values. The PR currently uses anonymous.

Other than that, are there any outstanding concerns before this PR could be considered for merge?

mtrezza · 2021-06-02T18:24:13Z

Confirmation that I'm using the correct error codes in each situation

~~Can you list the error codes and the scenarios?~~ I'll take a look in the changes.

What's the appropriate authProvider value to use? I don't know if this behavior is a match for any existing values. The PR currently uses anonymous.

I think we can introduce a new one, otherwise we cannot differentiate the session origin. Any suggestions?

GormanFletcher · 2021-06-02T18:50:56Z

Can you list the error codes and the scenarios?

If the master key isn't supplied, OPERATION_FORBIDDEN is thrown.

If the REST call doesn't supply the userId parameter, MISSING_OBJECT_ID is thrown.

If there isn't a user with the supplied userId, OBJECT_NOT_FOUND is thrown. This seemed more specific than INVALID_VALUE.

What's the appropriate authProvider value to use? I don't know if this behavior is a match for any existing values. The PR currently uses anonymous.

I think we can introduce a new one, otherwise we cannot differentiate the session origin. Any suggestions?

How about:

masterkey
admin
impersonate

mtrezza · 2021-06-02T19:17:56Z

Looking at the current auth provider terms for an indication, I think masterkey most accurately describes what facilitated the authentication. Compare to password for example.

However, masterkey is also a rather primitive, technical term. It does not suggest that the master key was used as an override, with would be more analogous to anonymous.

So for lack of a better term (admin and impersonation being too abstract I think) we could go with masterkey or override (or maybe something along that line).

GormanFletcher · 2021-06-02T22:43:53Z

I've pushed a change setting the value to override.

mtrezza · 2021-06-03T12:33:27Z

Let's give the auth provider naming some more thought, since this is unlikely because difficult to change in the future.

mtrezza

🙌 for cleaning up and code documentation

Just some corrections for the error codes and testing.

spec/ParseUser.spec.js

src/Routers/UsersRouter.js

spec/ParseUser.spec.js

src/Routers/UsersRouter.js

spec/ParseUser.spec.js

src/Routers/UsersRouter.js

codecov · 2021-06-03T17:25:01Z

Codecov Report

Merging #7406 (3a2726d) into master (754c127) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #7406      +/-   ##
==========================================
- Coverage   93.95%   93.93%   -0.02%     
==========================================
  Files         181      181              
  Lines       13228    13245      +17     
==========================================
+ Hits        12428    12442      +14     
- Misses        800      803       +3

Impacted Files	Coverage Δ
src/Routers/UsersRouter.js	`94.38% <100.00%> (+0.59%)`	⬆️
src/batch.js	`91.37% <0.00%> (-1.73%)`	⬇️
src/Adapters/Files/GridFSBucketAdapter.js	`79.50% <0.00%> (-0.82%)`	⬇️
src/Adapters/Storage/Mongo/MongoStorageAdapter.js	`92.59% <0.00%> (-0.66%)`	⬇️
src/RestWrite.js	`93.92% <0.00%> (-0.16%)`	⬇️
...dapters/Storage/Postgres/PostgresStorageAdapter.js	`95.51% <0.00%> (+0.15%)`	⬆️
src/ParseServerRESTController.js	`98.50% <0.00%> (+1.49%)`	⬆️

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 754c127...3a2726d. Read the comment docs.

mtrezza · 2021-06-03T19:22:29Z

Could you take a look at the coverage report to see whether this PR requires more tests?

GormanFletcher · 2021-06-03T19:34:25Z

The coverage report seems off to me. Codecov thinks all coverage for the Redis adapter has been removed, as well as some cases for the Mongo adapter (e.g., handling DB connection/auth errors) and some RestWrite cases (username taken, email taken). Reviewing the PR diff, I don't see how it could be affecting coverage for those subsystems.

Coverage for UsersRouter is up 0.59%.

But the CI checks don't mark the coverage as a requirement. It looks like CI failed on the self-check with this error:

Error: CI environments are not up-to-date with the latest Node.js versions.

mtrezza · 2021-06-03T20:14:03Z

The coverage report seems off to me.

Could be false positives.

Error: CI environments are not up-to-date with the latest Node.js versions.

I'll submit a PR to fix this.

mtrezza · 2021-06-03T20:41:03Z

If you rebase on master, the CI tests should pass.

…n log in as any user, without access to the user's credentials, and without presuming the user already has a session

GormanFletcher · 2021-06-03T22:39:54Z

I've rebased onto master.

mtrezza · 2021-06-03T23:01:58Z

Can you please delete the package-lock file and rebuild it? It's out of sync with package.json.

GormanFletcher · 2021-06-03T23:17:47Z

Done.

mtrezza

LGTM, thanks for this great feature!

GormanFletcher · 2021-06-04T18:07:49Z

Thanks so much for all your help!

dplewis

Can you revert the package-lock file? Otherwise this looks good to me!

mtrezza · 2021-06-04T20:08:28Z

Why revert? I rebuilt it from package json.

dplewis · 2021-06-04T20:19:11Z

It doesn't have anything to do with the PR and we don't know what those dozens of dependencies updates will cause.

mtrezza · 2021-06-04T20:24:55Z

I see, I agree it makes sense to do this in a separate PR.

What is our current practice for this? I can't remember an explicit PR where we only updated package-lock in the past. I do remember PRs however were we regenerated the package-lock as a side effect, even though it had nothing to do with the PR.

dplewis · 2021-06-04T20:37:11Z

This usually happens because a user deleted the package-lock file then regenerated it, thats my issue.

In this PR rearranging the package.json (npm install) shouldn't have caused a change in the lock file (I just tried it locally and no change).

mtrezza · 2021-06-04T21:47:03Z

makes sense, reverted package-lock

So should I regenerate the package-lock file in a dedicated PR or how to we rebuild the package-lock file?

dplewis

LGTM!

dplewis · 2021-06-04T23:54:50Z

So should I regenerate the package-lock file in a dedicated PR or how to we rebuild the package-lock file?

Let Synx manage the package-lock file or if someone installs a new package the lock file will update without regenerating.

Add support for master key clients to create user sessions (parse-community#7406)

mtrezza · 2021-06-05T09:05:52Z

Let Synx manage the package-lock file or if someone installs a new package the lock file will update without regenerating.

I am not sure about that approach. I opened #7417 to discuss that.

vince1995 · 2021-06-29T10:19:41Z

Cool PR! But

I elided the before/after login triggers since it's not really a login

But we would use this to login users with our own login provider, so for us it's a real login. So, wouldn't it be better if it's optional to call the triggers?

mtrezza · 2021-06-30T08:52:08Z

@vince1995 If you want to use a custom login provider, you would create a custom auth adapter.

Please feel free to open a new issue where you explain what you suggest to change. This PR is closed.

sadortun · 2021-06-30T15:29:20Z

@vince1995 @mtrezza This is related : #6641

tehsunnliu · 2021-09-27T03:11:25Z

Hi, any chances of this PR being released? I'm using the currently using master branch to use this feature.

dblythy · 2021-09-27T04:15:14Z

@tehsunnliu the next major release will be V5, available in the coming months.

See: #7492

tehsunnliu · 2021-09-27T04:51:59Z

Thank you for the info.

parseplatformorg · 2021-11-01T16:28:57Z

🎉 This change has been released in version 5.0.0-beta.1

Ref: parse-community#7406

parseplatformorg · 2022-03-14T13:00:56Z

🎉 This change has been released in version 5.0.0

GormanFletcher mentioned this pull request May 28, 2021

Create a session token in cloud code without password #6641

Closed

sadortun suggested changes May 28, 2021

View reviewed changes

src/Routers/UsersRouter.js Outdated Show resolved Hide resolved

mtrezza changed the title ~~6641: Implement support for master key clients to create user sessions~~ Add support for master key clients to create user sessions May 29, 2021

sadortun approved these changes Jun 1, 2021

View reviewed changes

src/Routers/UsersRouter.js Outdated Show resolved Hide resolved

GormanFletcher mentioned this pull request Jun 2, 2021

Parse Server 6641: document the new /loginAs REST endpoint parse-community/docs#832

Merged

mtrezza requested changes Jun 3, 2021

View reviewed changes

src/Routers/UsersRouter.js Outdated Show resolved Hide resolved

GormanFletcher force-pushed the create-session-without-authentication branch from 8924dff to 783681b Compare June 3, 2021 22:24

6641: Implement support for user impersonation: master key clients ca…

303c58d

…n log in as any user, without access to the user's credentials, and without presuming the user already has a session

GormanFletcher force-pushed the create-session-without-authentication branch from 783681b to 303c58d Compare June 3, 2021 23:17

mtrezza approved these changes Jun 4, 2021

View reviewed changes

mtrezza requested review from davimacedo and dplewis June 4, 2021 18:09

dplewis suggested changes Jun 4, 2021

View reviewed changes

revert package-lock

3a2726d

dplewis approved these changes Jun 4, 2021

View reviewed changes

dplewis merged commit 129f7bf into parse-community:master Jun 4, 2021

sakibguy added a commit to sakibguy/parse-server that referenced this pull request Jun 5, 2021

Merge pull request #6 from parse-community/master

58a8e67

Add support for master key clients to create user sessions (parse-community#7406)

parseplatformorg added the state:released-beta Released as beta version label Nov 1, 2021

jenwich added a commit to buddyreview/parse-server that referenced this pull request Dec 15, 2021

feat: support loginAs using master key

7c62fd4

Ref: parse-community#7406

mtrezza mentioned this pull request Mar 12, 2022

build: release 5.0 #7851

Merged

parseplatformorg added the state:released Released as stable version label Mar 14, 2022

isaacaskew mentioned this pull request Apr 11, 2023

feat: Add ParseUser::logInAs method parse-community/parse-php-sdk#486

Merged

2 tasks

dplewis mentioned this pull request Apr 28, 2023

feat: Add Parse.User.loginAs parse-community/Parse-SDK-JS#1875

Merged

2 tasks

Add support for master key clients to create user sessions #7406

Add support for master key clients to create user sessions #7406

Conversation

GormanFletcher commented May 28, 2021 • edited by mtrezza Loading

New Pull Request Checklist

Issue Description

Approach

TODOs before merging

sadortun left a comment • edited Loading

Choose a reason for hiding this comment

mtrezza commented May 29, 2021 • edited Loading

GormanFletcher commented Jun 2, 2021

GormanFletcher commented Jun 2, 2021

mtrezza commented Jun 2, 2021

GormanFletcher commented Jun 2, 2021 • edited Loading

mtrezza commented Jun 2, 2021

GormanFletcher commented Jun 2, 2021

mtrezza commented Jun 2, 2021 • edited Loading

GormanFletcher commented Jun 2, 2021

mtrezza commented Jun 2, 2021 • edited Loading

GormanFletcher commented Jun 2, 2021

mtrezza commented Jun 3, 2021 • edited Loading

mtrezza left a comment • edited Loading

Choose a reason for hiding this comment

codecov bot commented Jun 3, 2021 • edited Loading

Codecov Report

mtrezza commented Jun 3, 2021

GormanFletcher commented Jun 3, 2021

mtrezza commented Jun 3, 2021 • edited Loading

mtrezza commented Jun 3, 2021

GormanFletcher commented Jun 3, 2021

mtrezza commented Jun 3, 2021 • edited Loading

GormanFletcher commented Jun 3, 2021

mtrezza left a comment

Choose a reason for hiding this comment

GormanFletcher commented Jun 4, 2021

dplewis left a comment

Choose a reason for hiding this comment

mtrezza commented Jun 4, 2021

dplewis commented Jun 4, 2021

mtrezza commented Jun 4, 2021 • edited Loading

dplewis commented Jun 4, 2021 • edited Loading

mtrezza commented Jun 4, 2021 • edited Loading

dplewis left a comment

Choose a reason for hiding this comment

dplewis commented Jun 4, 2021

mtrezza commented Jun 5, 2021 • edited Loading

vince1995 commented Jun 29, 2021

mtrezza commented Jun 30, 2021 • edited Loading

sadortun commented Jun 30, 2021 • edited Loading

tehsunnliu commented Sep 27, 2021

dblythy commented Sep 27, 2021 • edited Loading

tehsunnliu commented Sep 27, 2021

parseplatformorg commented Nov 1, 2021

parseplatformorg commented Mar 14, 2022

GormanFletcher commented May 28, 2021 •

edited by mtrezza

Loading

sadortun left a comment •

edited

Loading

mtrezza commented May 29, 2021 •

edited

Loading

GormanFletcher commented Jun 2, 2021 •

edited

Loading

mtrezza commented Jun 2, 2021 •

edited

Loading

mtrezza commented Jun 2, 2021 •

edited

Loading

mtrezza commented Jun 3, 2021 •

edited

Loading

mtrezza left a comment •

edited

Loading

codecov bot commented Jun 3, 2021 •

edited

Loading

mtrezza commented Jun 3, 2021 •

edited

Loading

mtrezza commented Jun 3, 2021 •

edited

Loading

mtrezza commented Jun 4, 2021 •

edited

Loading

dplewis commented Jun 4, 2021 •

edited

Loading

mtrezza commented Jun 4, 2021 •

edited

Loading

mtrezza commented Jun 5, 2021 •

edited

Loading

mtrezza commented Jun 30, 2021 •

edited

Loading

sadortun commented Jun 30, 2021 •

edited

Loading

dblythy commented Sep 27, 2021 •

edited

Loading