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

Allow set user mapped from JWT directly on request #6411

Merged
merged 2 commits into from
Apr 3, 2020

Conversation

sunshineo
Copy link
Contributor

A not ideal workaround for people really need it
#6390 (comment)

@davimacedo
Copy link
Member

Could you please add some tests cases? @dplewis what do you think about it?

@sunshineo
Copy link
Contributor Author

About tests, I had a look at https://github.com/parse-community/parse-server/blob/master/spec/Middlewares.spec.js . It seems to me that a lot of the existing code branch and logic are not covered. I cannot find a good test example that I can copy paste then modify for the code I just added.

@davimacedo
Copy link
Member

I think you could inspire the tests in the session token tests of the following file: https://github.com/parse-community/parse-server/blob/fd0b535159877fa9bc74fb645169f56814c67c60/spec/ParseUser.spec.js

@sunshineo
Copy link
Contributor Author

Thank you @davimacedo . I see the tests in that file mimic a request to test. For example with sessionToken in the headers to get proper access. But I'm not sure how should I write this test. If I have the JWT token on the header it won't work because the step of map from JWT to a Parse user is done by the user themselves before they set the user onto the request. It's a middleware before the Parse middleware. I considered if the mapping should be part of Parse but as I said in the original issue, I'm not sure #6390 (comment)

This workaround still requires user to wrap Parse server in express and map the JWT to a Parse server themselves before pass it down to Parse middleware. But is this the final stop? Should Parse take over the JWT decoding and user mapping? I can see that introduce a ton of configurations to support all the different decoding usecases? And I'm not even sure if the user mapping is possible without code. I'd like to hear your thoughts.

@davimacedo
Copy link
Member

Maybe it can be part of Parse Server. Do you want to suggest how it could be included? Anyways, in the case of the tests, you can create an express app and add the middleware then Parse (just like you are doing in your application) in the beginning of your test case.

@codecov
Copy link

codecov bot commented Mar 10, 2020

Codecov Report

Merging #6411 into master will decrease coverage by 0.02%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6411      +/-   ##
==========================================
- Coverage   93.95%   93.93%   -0.03%     
==========================================
  Files         169      169              
  Lines       11687    11812     +125     
==========================================
+ Hits        10981    11096     +115     
- Misses        706      716      +10
Impacted Files Coverage Δ
src/middlewares.js 97.26% <100%> (+0.09%) ⬆️
src/GraphQL/loaders/schemaDirectives.js 88.88% <0%> (-5.56%) ⬇️
src/GraphQL/loaders/parseClassMutations.js 98.96% <0%> (-1.04%) ⬇️
src/Adapters/Storage/Mongo/MongoStorageAdapter.js 92.79% <0%> (-0.69%) ⬇️
src/ParseServer.js 91.07% <0%> (-0.5%) ⬇️
...dapters/Storage/Postgres/PostgresStorageAdapter.js 96.74% <0%> (-0.28%) ⬇️
src/Controllers/index.js 96.66% <0%> (ø) ⬆️
src/triggers.js 92.94% <0%> (ø) ⬆️
src/Config.js 89.6% <0%> (ø) ⬆️
src/RestWrite.js 93.64% <0%> (ø) ⬆️
... and 14 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 8e195ef...8906ee3. Read the comment docs.

@sunshineo
Copy link
Contributor Author

sunshineo commented Mar 10, 2020

@davimacedo and @dplewis , I've added a test. I've no clue how the code coverage could drop that much. Probably something wrong with the CI system

@dplewis
Copy link
Member

dplewis commented Mar 22, 2020

@acinader Thoughts?

@davimacedo
Copy link
Member

I think we are good with this coverage and we can merge.

@sunshineo
Copy link
Contributor Author

Thank you @davimacedo ! Could someone please merge it?

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