splitwise icon indicating copy to clipboard operation
splitwise copied to clipboard

Added oauth2 to authenticate and added documentation for it.

Open omkarsk98 opened this issue 3 years ago • 4 comments

Added option oauth2. Initialize the Splitwise object using useOauth2: true and redirect_uri. Additional documentation added in the README.

omkarsk98 avatar Jul 11 '22 05:07 omkarsk98

Hi Keri! I have resolved the comments and replied to the ones that needed answers. Please let me know if anything else needs to be changed.

omkarsk98 avatar Jul 19 '22 02:07 omkarsk98

hi! @omkarsk98 sorry for the delay - I was on vacation.

I implemented a test oauth application, and it more or less worked so fantastic job!! Couple things:

  1. it seems like splitwise ignores whatever redirect uri you pass via query params, and uses the one you supplied when you registered your oauth app on splitwise.com, so if I'm correct, we should just not both with collecting a redirect_uri in our code here
  2. it can be really useful for the application using our library to manage it's own state parameter. We should allow the library user to pass in their own state parameter, in which case we can probably omit the state verification check? (but we should warn the user that they need to verify the state themself).
  3. you have return resolve(true); inside of getAccessTokenFromAuthCode. Is that supposed to be return resolve(accessToken); ? If not, why not.

keriwarr avatar Aug 02 '22 07:08 keriwarr

Hi @keriwarr. Thanks for the update. Happy to modify!

  1. I did run into an issue while developing this that threw "invalid_grant" when I used tried to get the access token. Although, you get the login window, it throws this error while fetching the access token. It is also mentioned here.
  2. Updated to have the state optional. One can supply it in the constructor and the authorization url will include it if supplied. I have updated the docs with an example for this.
  3. Resolved this. Here I realised the the IIFE issue you mentioned earlier. I have fixed that too.

omkarsk98 avatar Aug 02 '22 09:08 omkarsk98

@keriwarr I have this query here

omkarsk98 avatar Aug 08 '22 01:08 omkarsk98