feat: OAuth 2.0 functionality #290

berezovskyicom · 2020-07-10T13:34:55Z

This PR adds OAuth2.0 into Abstract SDK.
Link to OAUTH2.0 RFC
Link to OAuth interface for SDK

Adds add getToken() for making a POST request to auth/tokens with authorizationCode.
And setToken() for storing returned accessToken in client.
generateAuthorizeurl() is used for returning an oauth url to Abstract.

Example:

let abstract = new Abstract.Client({
  clientId,
  clientSecret,
  redirectUri,
});

async function oAuth(code) {
  return await abstract.oauth
    .getToken({
      authorizationCode: code
    })
    .then(token => {
      abstract = abstract.oauth.setToken(token);

      return abstract;
    })
    .catch(error => console.log(error));
}

const authorizedAbstract = oAuth("authentication_code");

authorizedAbstract.then(() => {
  // use sdk as usual
});

Things to consider:
client.oauth.setToken() returns new Client({}) with the same options + accessToken.
Is it really possible to change options for Client and all other endpoints without returning a new class?
I've tried a few ways, but the only options get changed are in oauth endpoint. Any thoughts?

src/endpoints/Oauth.js

src/errors.js

src/endpoints/Endpoint.js

Co-authored-by: Andrew McCloud <[email protected]>

tommoor · 2020-07-13T19:07:45Z

Looks like a bunch of unrelated work from master is showing up in the PR

amccloud

Thanks for consolidating the PRs 🙏

I think once you get the commits in this PR cleaned up you should be all set for review

amccloud · 2020-07-15T03:05:13Z

src/endpoints/OAuth.js

+    body.append("client_id", clientId);
+    // $FlowFixMe
+    body.append("client_secret", clientSecret);
+    // $FlowFixMe


We typically try to avoid type suppressions unless it is absolutely necessary. If flow is warning here it's likely you have an unaccounted for state here

You can guard appending things to body by either providing a default

body.append("redirect_uri", window.location.href);

or returning early before you pass empty values to the body:

if (!clientId || !clientSecret) { throw new Error('OAuthOnAuthorizeToken required'); }

amccloud · 2020-07-15T03:13:18Z

src/types.js

+  state: string
+};
+
+export type OAuthOnAuthorizeToken = {


Since this is the input for getToken

Suggested change

export type OAuthOnAuthorizeToken = {

export type OAuthTokenInput = {

OAuthOnAuthorizeToken seems like what authorizationCode is and that's only part of the input

…w warnings

tommoor · 2020-07-17T18:01:45Z

Please move out of draft and re-ping when you're ready for final review.

amccloud

Looks great 👍

Before merging can we start to stub out documentation for this feature? Would be nice to cover what this PR includes

tommoor

Almost there, I hope to get chance to spin up a sample app to really test before final approval.

abstract-sdk.d.ts

tommoor · 2020-07-21T22:25:21Z

src/Client.js

-    this.stars = new Stars(this, options);
-    this.users = new Users(this, options);
-    this.webhooks = new Webhooks(this, options);
+    this._analyticsCallback = this.options.analyticsCallback;


I think we should go ahead and just change Endpoint to only accept one argument, this – we can read this.options in the constructor and avoid passing it in essentially twice here.

Done! I left Endpoint options param as optional, because tests were behaving weird.

src/endpoints/Endpoint.js

src/endpoints/OAuth.js

tommoor · 2020-07-21T22:30:37Z

src/endpoints/OAuth.js

+    const state = input.state;
+    const redirectUri = input.redirectUri || this.options.redirectUri;
+
+    if (!clientId || !state || !redirectUri) {


Is state strictly required?

Nope. Will fix that 👍

tommoor · 2020-07-21T22:31:46Z

src/endpoints/OAuth.js

+
+    return `https://app.abstract.com/signin/auth/authorize?client_id=${clientId}&redirect_uri=${encodeURIComponent(
+      redirectUri
+    )}&response_type=code&scope=all&state=${state}`;


Can we allow input.scope here to future proof and just set it to "all" as the default?

tommoor · 2020-07-21T22:32:45Z

src/types.js

+  access_token: string,
+  client_id: string,
+  created_at: string,
+  id: string,


What is this id?

According to back end, it's not used anywhere right now and not much needed for client. Looks like an id in database.

tommoor · 2020-07-21T22:36:31Z

src/Client.js

+
+  setToken(accessToken: string) {
+    this.options.accessToken = accessToken;
+    return this.options;


Did you have a reason for returning options here in mind? Might be confusing, could be better to return this to allow for chaining or nothing at all…

Yes, 100% agree. Returning nothing lgtm. Will fix that in new commit.

Fixed and edited tests.

Co-authored-by: Tom Moor <[email protected]>

…if options param is not specified

berezovskyicom · 2020-07-22T18:18:25Z

src/endpoints/Endpoint.js

    this.client = client;
-    this.options = options;
+    this.options = options ? options : this.client.options;


Not really sure if that's the best idea, but test are feeling fine.

berezovskyicom · 2020-07-22T18:20:24Z

Me & @Geek-1001 are preparing docs for OAuth, we will let you know when it gets finished!

src/endpoints/OAuth.js

Co-authored-by: Tom Moor <[email protected]>

Vladimir Berezovsky added 7 commits July 6, 2020 14:17

feat: add clientSecret and redirectUri

31787ae

chore: add types to CommandOptions

8a3e48e

chore: remove promise value from redirectUri

a334a20

feat: new oauth endpoint with generateUrlMethod()

438cd91

feat: Add getToken & setToken methods for OAuth

3889280

fix: Oauth --> OAuth

612b7a1

chore: remove comment line

47ee0b8

berezovskyicom requested review from amccloud and tommoor July 10, 2020 13:34

amccloud reviewed Jul 10, 2020

View reviewed changes

Vladimir Berezovsky added 3 commits July 13, 2020 13:17

feat: add generateAuthorizeUrl() to OAuth

9c4066b

chore: remove useless error

a747827

fix: add input as param instead of descriptor in getToken()

98a23b0

berezovskyicom changed the title ~~feat(oauth): add getToken() and setToken() methods~~ feat: OAuth 2.0 functionality Jul 13, 2020

berezovskyicom mentioned this pull request Jul 13, 2020

feat(oauth): add OAuth endpoint with generateUrl() method #289

Closed

chore: configure request - info --> getToken

b3fe128

Co-authored-by: Andrew McCloud <[email protected]>

berezovskyicom requested a review from amccloud July 13, 2020 10:58

amccloud reviewed Jul 15, 2020

View reviewed changes

berezovskyicom force-pushed the feat/oauth/handle-token branch from 634605e to 333021e Compare July 15, 2020 13:40

feat: add tests for getToken and setToken

a98f43a

berezovskyicom force-pushed the feat/oauth/handle-token branch from 333021e to a98f43a Compare July 15, 2020 13:43

Vladimir Berezovsky added 2 commits July 16, 2020 00:47

feat: rename rename OAuthOnAuthorizeToken to OAuthTokenInput; fix flo…

efcfe27

…w warnings

feat: add tests for checking if proper Input is used

68ba371

berezovskyicom force-pushed the feat/oauth/handle-token branch from df6b1a8 to 68ba371 Compare July 15, 2020 21:48

chore: improve tests

0c43ea9

berezovskyicom force-pushed the feat/oauth/handle-token branch from 7afcfeb to 570dcd6 Compare July 15, 2020 22:43

feat: check if Content-Type is not application/x-www-form-urlencoded

9f02dc9

berezovskyicom force-pushed the feat/oauth/handle-token branch from 570dcd6 to 9f02dc9 Compare July 20, 2020 22:11

Vladimir Berezovsky added 2 commits July 21, 2020 01:46

chore: remove setToken from OAuth endpoint

c45bc8e

feat: add options as this.options and add setToken() as client method

1f97cc9

berezovskyicom marked this pull request as ready for review July 20, 2020 22:54

berezovskyicom requested a review from amccloud July 20, 2020 22:54

chore: add Organization.features

369209c

amccloud reviewed Jul 20, 2020

View reviewed changes

Vladimir Berezovsky added 2 commits July 21, 2020 12:41

chore: remove useless assignment

56ad9b6

fix: corrent link when generating url

7cdd1d4

tommoor suggested changes Jul 21, 2020

View reviewed changes

Vladimir Berezovsky and others added 10 commits July 22, 2020 19:35

chore: remove support for promises in clientSecret

274e7b7

Co-authored-by: Tom Moor <[email protected]>

chore: remove support for promises in redirectUri

6900756

Co-authored-by: Tom Moor <[email protected]>

chore: edit rule when headers are present but Content-Type isn’t

3b5069e

Co-authored-by: Tom Moor <[email protected]>

fix: typo (double check)

7071e1c

Co-authored-by: Tom Moor <[email protected]>

fix: typo in authorize_url

de85e68

fix: remove state as required

81c2c03

fix: lint error for checking content-type statement in apiRequest

341b32c

feat: add scope as a variable for future proof

f6b8697

chore: don't return anything in Client.setToken()

0d56460

chore: add options param as optional in Endpoint; use client.options …

49fe426

…if options param is not specified

berezovskyicom commented Jul 22, 2020

View reviewed changes

tommoor reviewed Jul 23, 2020

View reviewed changes

src/endpoints/OAuth.js Outdated Show resolved Hide resolved

tommoor approved these changes Jul 23, 2020

View reviewed changes

chore: use input.scope or "all" when choosing scope value in url

abbc152

Co-authored-by: Tom Moor <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: OAuth 2.0 functionality #290

feat: OAuth 2.0 functionality #290

berezovskyicom commented Jul 10, 2020 •

edited

Loading

tommoor commented Jul 13, 2020

amccloud left a comment

amccloud Jul 15, 2020

amccloud Jul 15, 2020

tommoor commented Jul 17, 2020

amccloud left a comment

tommoor left a comment

tommoor Jul 21, 2020

berezovskyicom Jul 22, 2020

tommoor Jul 21, 2020

berezovskyicom Jul 22, 2020

berezovskyicom Jul 22, 2020

tommoor Jul 21, 2020

berezovskyicom Jul 22, 2020

berezovskyicom Jul 22, 2020

tommoor Jul 21, 2020

berezovskyicom Jul 22, 2020

tommoor Jul 21, 2020

berezovskyicom Jul 22, 2020

berezovskyicom Jul 22, 2020

berezovskyicom Jul 22, 2020

berezovskyicom commented Jul 22, 2020

	export type OAuthOnAuthorizeToken = {
	export type OAuthTokenInput = {

feat: OAuth 2.0 functionality #290

Are you sure you want to change the base?

feat: OAuth 2.0 functionality #290

Conversation

berezovskyicom commented Jul 10, 2020 • edited Loading

tommoor commented Jul 13, 2020

amccloud left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tommoor commented Jul 17, 2020

amccloud left a comment

Choose a reason for hiding this comment

tommoor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

berezovskyicom commented Jul 22, 2020

berezovskyicom commented Jul 10, 2020 •

edited

Loading