Added oauth2 to authenticate and added documentation for it. #29

omkarsk98 · 2022-07-11T05:35:52Z

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

keriwarr

mostly small stuff. I don't expect you to necessarily agree with everything but if you choose not to address a comment, please briefly explain why not.

Thanks so much for this PR, I really appreciate it ;)

src/splitwise.js

keriwarr · 2022-07-19T01:18:35Z

src/splitwise.js

+    if (!useOauth2) {
+      // earlier an IIFE. But now, this is called only in case of client_credentials
+      accessTokenPromise();
+    }


I think this code would really benefit from having a clear separation between the client credentials code and the authorization code code. maybe something like a switch statement over the new grantType argument which contains the relevant pieces.

Called by default while initializing so as to acquire a token right away. This is just a single case. We cant get the token right away in case of authorization_code

src/splitwise.js

keriwarr · 2022-07-19T01:19:48Z

src/splitwise.js

+      }
+    }
+    else 
+      this.getAccessToken = () => accessTokenPromise;


would this still work sinceit's not an IIFE anymore?

Yes this works perfectly

omkarsk98 · 2022-07-19T02:07:12Z

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.

keriwarr · 2022-08-02T07:59:02Z

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:

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
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).
you have return resolve(true); inside of getAccessTokenFromAuthCode. Is that supposed to be return resolve(accessToken); ? If not, why not.

omkarsk98 · 2022-08-02T09:56:40Z

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

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.
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.
Resolved this. Here I realised the the IIFE issue you mentioned earlier. I have fixed that too.

keriwarr

thanks for the update!

keriwarr · 2022-08-08T01:20:27Z

README.md

  consumerSecret: 'your secret here',
  grant_type: 'authorization_code',
  redirect_uri: 'your redirect_uri'
+  state: 'your state' // OPTIONAL. But we strongly recomend to have a state parameter and verify it later on as mentioned below 


ah, this won't work, the state must be distinct for every attempted authentication

@keriwarr oh yaa! I get it. so how about the client supplies a function that will generate state everytime? But do we expose this state to the client so that they can verify it? Because passing a static state will always result in having the same state for each auth request.

I think the client should just supply the state individually for each request. We would have to expose it - not just so that they can verify it - it might be used to store information about how the user initiated the original request (such as which page to redirect back @omkarsk98

@keriwarr Got it. So this is my understanding. Correct me if I am wrong. we add a state key in getAuthorizationUrl. If that is present, we use that, else we generate our state and we expose getState for both the cases?

That's what i was originally thinking, but i re-read the OAuth spec for the "state" parameter, and my new understanding is that in order to verify the state, it's insufficient to check that it matches the outgoing state, you actually have to check that the user for whom you generated the auth url is the same user who visited the callback. Our library isn't in a position to do that so we have to leave it to the client, meaning that we shouldn't generate a state, we should require the client to generate one. Moreover, the client will receive the state from the inbound callback request, so we shouldn't need to provide a getState mechanism. Indeed, we might not be able to as in theory there could be many users doing oauth authentication at the same time - how would we store all their different states and keep them disentangled without knowing who they are?

keriwarr · 2022-08-08T01:21:41Z

src/splitwise.js

-      return this.state;
-    }
-
    this.getAuthorizationUrl = () => {


i would probably do something like adding state as an argument to this method

keriwarr · 2022-08-08T01:21:53Z

src/splitwise.js

        redirect_uri,
        scope: '',
-        state: generateState(),
+        state,


~~and then this could be state || generateState()~~

nvm, as above, we probably don't want to generate state

omkarsk98 · 2022-08-08T01:48:11Z

@keriwarr I have this query here

keriwarr · 2022-08-10T16:59:53Z

src/splitwise.js

+      return new Promise((resolve, reject) => {
+        if(!this.authCode) 
+          return reject(`No auth code generated yet. Visit ${this.getAuthorizationUrl()} and login. You need a callback url in your project as mentioned in callback url in your registered splitwise app. Then call \`getAccessToken()\` to register the access token.`);
+        if(this.accessToken) return resolve(this.accessToken);


Ah sorry, I realize now this might not work either. What we have to remember is that with the client_credentials flow, the client will be receiving authorization on behalf of exactly one person (the person owning the credentials) so it makes sense to store data on this, however in the authorization_code flow, we have 1 client <=> 1 instance of this library <=> many users, which means that there isn't just one access token, there are many. This line here would give away the access token of the first user who authenticates to whoever comes next, a security problem! In general, we should probably entirely avoid using this for any code that handles authorization_code flow

@keriwarr Yaa I thought about this. With this, our clients will need to use the authorization_code to allow multiple users interaction. Because, with client_credentials, client can just use his/her account only. As it logs into his own account be default. So authorization_code would become mandatory in case of multiple users.
Like wise, talking about the state, it should be supplied in getAuthorizationUrl and then once logged in, we would give away the access token and we wont have track of it at all as multiple users would be interacting with the client.
So, summing up,

We can remove the use of this but we wont have the access_token and then the client will have to supply the access_token for each function. Not a good idea

Client can generate and share the state and we wont have to deal with it at all. Better for both, client as well as the library.

Else, to track the state, we can have a redis connection to persist the state. But to which user does it belong to?

Also, as per you, will this library be used for a project of multiple users?

omkarsk98 added 2 commits July 10, 2022 16:37

Added oauth2 to authenticate

a8785f9

Documented oauth2 in readme

5de1775

keriwarr requested changes Jul 19, 2022

View reviewed changes

Updated as per PR comments

c412a3e

State verification upto the client. Returning access token

2dbfdd2

keriwarr requested changes Aug 8, 2022

View reviewed changes

keriwarr reviewed Aug 10, 2022

View reviewed changes

Added oauth2 to authenticate and added documentation for it. #29

Are you sure you want to change the base?

Added oauth2 to authenticate and added documentation for it. #29

Conversation

omkarsk98 commented Jul 11, 2022

Uh oh!

keriwarr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

omkarsk98 commented Jul 19, 2022

Uh oh!

keriwarr commented Aug 2, 2022

Uh oh!

omkarsk98 commented Aug 2, 2022

Uh oh!

keriwarr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

omkarsk98 Aug 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

keriwarr Aug 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

keriwarr Aug 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

omkarsk98 commented Aug 8, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

omkarsk98 Aug 8, 2022 •

edited

Loading

keriwarr Aug 10, 2022 •

edited

Loading

keriwarr Aug 8, 2022 •

edited

Loading