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

Microsoft Graph Authentication #6051

Merged
merged 12 commits into from
Sep 26, 2019
Merged

Conversation

alann-maulana
Copy link
Contributor

Add authentication using Microsoft according Microsoft Graph REST API v1.0

@codecov
Copy link

codecov bot commented Sep 15, 2019

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6051      +/-   ##
==========================================
- Coverage   93.73%   93.71%   -0.03%     
==========================================
  Files         164      165       +1     
  Lines       11151    11161      +10     
==========================================
+ Hits        10452    10459       +7     
- Misses        699      702       +3
Impacted Files Coverage Δ
src/Adapters/Auth/index.js 93.22% <100%> (+0.11%) ⬆️
src/Adapters/Auth/microsoft.js 100% <100%> (ø)
src/Adapters/Storage/Mongo/MongoStorageAdapter.js 92.27% <0%> (-0.71%) ⬇️
src/RestWrite.js 93.56% <0%> (ø) ⬆️

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 1c8e814...7c855b8. Read the comment docs.

@davimacedo
Copy link
Member

@alann-maulana thanks for tackling this. Could you please add some test cases?

@alann-maulana
Copy link
Contributor Author

@davimacedo ok, I'll try it

@davimacedo
Copy link
Member

The CI is failing both for the lint and for the tests. Can you please take a look here and fix it?

@alann-maulana
Copy link
Contributor Author

@davimacedo fixed it (I think) but still there is an error 😐

@davimacedo
Copy link
Member

@alann-maulana yes... this test is still failing:
Failures:

  1. Microsoft Auth should fail to validate Microsoft Graph auth with bad token
    Message:
    Unhandled promise rejection: ParseError: 101 Microsoft Graph auth is invalid for this user.
    Stack:
    error properties: Object({ code: 101 })
    Error: Microsoft Graph auth is invalid for this user.
    at request.then.response (/home/travis/build/parse-community/parse-server/lib/Adapters/Auth/microsoft.js:3:431)
    at process._tickCallback (internal/process/next_tick.js:68:7)

@alann-maulana
Copy link
Contributor Author

Yup, now the error goes to Parse.Push

1) Parse.Push does not get stuck with _PushStatus 'running' on 1 installation added
  Message:
    Expected 'running' to equal 'succeeded'.
  Stack:
    Error: Expected 'running' to equal 'succeeded'.
        at <Jasmine>
        at reconfigureServer.then.then.then.then.then.results (/home/travis/build/parse-community/parse-server/spec/Parse.Push.spec.js:402:38)
        at process._tickCallback (internal/process/next_tick.js:68:7)

never touch them btw 😅

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.

This test is actually hanging. It has just run again and passed. It looks good to me! @dplewis do you want to take a look at this one here?

@dplewis dplewis merged commit 38e0ff9 into parse-community:master Sep 26, 2019
UnderratedDev pushed a commit to UnderratedDev/parse-server that referenced this pull request Mar 21, 2020
* add microsoft graph auth

* change mail to id

* add graph user id and email

* add microsoft graph auth test case

* remove validating auth data using mail

* add test case to AuthenticationAdapters

* fix indentation

* fix httpsRequest and fakeClaim not found

* add newline eof last

* fix test in auth adapter

* fix unhandled promise rejection
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