dwyl / hapi-auth-google

:lollipop: Let people authenticate with your application/website using their Google Account (OAuth2)

GNU General Public License v2.0

41 stars 6 forks source link

Hapi v17 Compatible #54

Open Y-LyN-10 opened 7 years ago

Y-LyN-10 commented 7 years ago

Third attempt to propose these code changes :sweat_smile: :pray: Refers to: #51 and #55

Although, let's not rush to deliver. This is work-in-progress and code reviews are highly welcome.

Also, I have some additional comments / suggestions / questions:

[x] 1. One of the original test cases was changed. The idea of the test case is that the server should not crash when a bad code is passed in the google auth callback. I agree with that, but the original version returned status 200 (OK), which is not the expected behavior in my opinion. Instead, it returns status 400 (Bad Request) now (no crashing!). Do you agree?
[ ] 2. I know that env2 dev-dependency is created / maintained by dwyl organization, but how about replacing it with dotenv?
[ ] 3. How about using Lab for testing as it's a part of the Hapi ecosystem?
[x] 5. Readme.md should be updated with examples as well.
[x] 6. Looks-like more tests are needed now for 100% test coverage.

codecov[bot] commented 7 years ago

Codecov Report

Merging #54 into master will not change coverage. The diff coverage is 100%.

@@          Coverage Diff          @@
##           master    #54   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           3      1    -2     
  Lines          59     10   -49     
=====================================
- Hits           59     10   -49

Impacted Files	Coverage Δ
example/google_oauth_handler.js	`100% <100%> (ø)`	:arrow_up:

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 9499d49...9a38ee8. Read the comment docs.

Y-LyN-10 commented 7 years ago

Well, the uncovered part was in the example handler (auth callback) where the authentication has been successful, but there was no profile data from google plus (and there were no errors). I am not sure how to reproduce such a scenario in real-life and what is the desired result. :thinking:

In the example I returned 503 status code, which is like ... service is temporary unavailable or try again later. I added a test for that, but is that a real case at all? What are your thoughts? I'll do some research on that.

Y-LyN-10 commented 7 years ago

Hi, the latest merge commits were to update Readme.md docs with examples how to register and use this plugin with Hapi 17.

Also, important update: Hapi v17 is the latest and default version on NPM now and I assume that (very soon) people will try to use hapi-auth-google with it and they will run into non-compatibility issues. Is this PR good enough to solve this problem?

cola4u commented 6 years ago

so are you planning on merging this soon?

nelsonic commented 6 years ago

Tests pass on localhost (as expected given that they pass on Travis-CI):

Running the example/google.server.js on localhost:

nelsonic commented 6 years ago

Travis-CI is showing a "False Positive": https://travis-ci.org/dwyl/hapi-auth-google/builds/385615089 The tests are not passing. we're seeing the error:

{ Error: Line 16: Unexpected token function ...

This is because Google have changed their API in the latest version of googleapis ... 🙄

nelsonic commented 6 years ago

We need to update our dependency on googleapis ... https://github.com/dwyl/hapi-auth-google/issues/55

Y-LyN-10 commented 6 years ago

Yeah, it's been a while since I opened this PR. I am not currently using google auth in my projects, but I will take a look later today and update what's needed. Thank you for the review, @nelsonic!

Y-LyN-10 commented 6 years ago

So, the latest googleapis works natively with async/await :tada: I was using util.promisify to achieve that flow before.

nelsonic commented 6 years ago

@Y-LyN-10 thank you for making those updates! 🎉 Sadly, there is still a "false positive" test that requires further investigation: 🔍 https://travis-ci.org/dwyl/hapi-auth-google/builds/386194728#L1302

# test/google.test.js > /googleauth?code=oauth2codehere
{ Error: invalid_request
    at createError (/home/travis/build/dwyl/hapi-auth-google/node_modules/axios/lib/core/createError.js:16:15)
    at settle (/home/travis/build/dwyl/hapi-auth-google/node_modules/axios/lib/core/settle.js:18:12)
    at IncomingMessage.handleStreamEnd (/home/travis/build/dwyl/hapi-auth-google/node_modules/axios/lib/adapters/http.js:201:11)