-
Notifications
You must be signed in to change notification settings - Fork 18
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
feat: OAuth 2.0 functionality #290
base: master
Are you sure you want to change the base?
Conversation
Co-authored-by: Andrew McCloud <andrew@abstract.com>
Looks like a bunch of unrelated work from master is showing up in the PR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for consolidating the PRs 🙏
I think once you get the commits in this PR cleaned up you should be all set for review
src/endpoints/OAuth.js
Outdated
body.append("client_id", clientId); | ||
// $FlowFixMe | ||
body.append("client_secret", clientSecret); | ||
// $FlowFixMe |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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');
}
src/types.js
Outdated
state: string | ||
}; | ||
|
||
export type OAuthOnAuthorizeToken = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is the input for getToken
export type OAuthOnAuthorizeToken = { | |
export type OAuthTokenInput = { |
OAuthOnAuthorizeToken
seems like what authorizationCode
is and that's only part of the input
634605e
to
333021e
Compare
333021e
to
a98f43a
Compare
df6b1a8
to
68ba371
Compare
7afcfeb
to
570dcd6
Compare
Please move out of draft and re-ping when you're ready for final review. |
570dcd6
to
9f02dc9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great 👍
Before merging can we start to stub out documentation for this feature? Would be nice to cover what this PR includes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Almost there, I hope to get chance to spin up a sample app to really test before final approval.
this.stars = new Stars(this, options); | ||
this.users = new Users(this, options); | ||
this.webhooks = new Webhooks(this, options); | ||
this._analyticsCallback = this.options.analyticsCallback; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done! I left Endpoint options param as optional, because tests were behaving weird.
src/endpoints/OAuth.js
Outdated
const state = input.state; | ||
const redirectUri = input.redirectUri || this.options.redirectUri; | ||
|
||
if (!clientId || !state || !redirectUri) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is state
strictly required?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope. Will fix that 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
src/endpoints/OAuth.js
Outdated
|
||
return `https://app.abstract.com/signin/auth/authorize?client_id=${clientId}&redirect_uri=${encodeURIComponent( | ||
redirectUri | ||
)}&response_type=code&scope=all&state=${state}`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we allow input.scope
here to future proof and just set it to "all" as the default?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pushed.
access_token: string, | ||
client_id: string, | ||
created_at: string, | ||
id: string, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this id
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to back end, it's not used anywhere right now and not much needed for client. Looks like an id in database.
src/Client.js
Outdated
|
||
setToken(accessToken: string) { | ||
this.options.accessToken = accessToken; | ||
return this.options; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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…
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, 100% agree. Returning nothing lgtm. Will fix that in new commit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed and edited tests.
Co-authored-by: Tom Moor <tom@abstract.com>
Co-authored-by: Tom Moor <tom@abstract.com>
Co-authored-by: Tom Moor <tom@abstract.com>
Co-authored-by: Tom Moor <tom@abstract.com>
…if options param is not specified
this.client = client; | ||
this.options = options; | ||
this.options = options ? options : this.client.options; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not really sure if that's the best idea, but test are feeling fine.
Me & @Geek-1001 are preparing docs for OAuth, we will let you know when it gets finished! |
Co-authored-by: Tom Moor <tom@abstract.com>
This PR adds OAuth2.0 into Abstract SDK.
Link to OAUTH2.0 RFC
Link to OAuth interface for SDK
Adds add
getToken()
for making aPOST
request toauth/tokens
withauthorizationCode
.And
setToken()
for storing returnedaccessToken
in client.generateAuthorizeurl()
is used for returning an oauth url to Abstract.Example:
Things to consider:
client.oauth.setToken()
returnsnew Client({})
with the sameoptions + 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 inoauth
endpoint. Any thoughts?