feat: add facebook driver #121
Conversation
|
Looks great. Can you please rebase your branch with the latest |
|
Branch has been updated. |
src/Drivers/Facebook/index.ts
Outdated
| protected accessTokenUrl = 'https://graph.facebook.com/v10.0/oauth/access_token' | ||
| protected authorizeUrl = 'https://www.facebook.com/v10.0/dialog/oauth' | ||
| protected graphUrl = 'https://graph.facebook.com' | ||
| protected graphVersion = 'v10.0' |
There was a problem hiding this comment.
I don't think we need separate graphUrl and graphVersion variables. We can just hardcode them inside the userInfoUrl coz the user can overwrite the entire URL anyways
src/Drivers/Facebook/index.ts
Outdated
| * Define params based upon user config | ||
| */ | ||
| if (this.config.popup) { | ||
| request.param('display', this.config.reRequest) |
There was a problem hiding this comment.
It should be this.config.display
There was a problem hiding this comment.
fixed and changed to use the param name on config.
src/Drivers/Facebook/index.ts
Outdated
| * Send state to Facebook when request is not stateles | ||
| */ | ||
| if (!this.isStateless) { | ||
| request.field('state', this.stateCookieValue) |
There was a problem hiding this comment.
Does facebook access token request accepts state?
There was a problem hiding this comment.
Looking at the facebook documentation it should.
state. A string value created by your app to maintain state between the request and callback. This parameter should be used for preventing Cross-site Request Forgery and will be passed back to you, unchanged, in your redirect URI.
https://developers.facebook.com/docs/facebook-login/manually-build-a-login-flow/#logindialog
There was a problem hiding this comment.
But you are passing it to the access token request (not the redirect request).
There was a problem hiding this comment.
you are right, removed this
src/Drivers/Facebook/index.ts
Outdated
| protected async getUserInfo(token: string, callback?: (request: ApiRequestContract) => void) { | ||
| const fields = this.config.userFields || this.userFields | ||
| const fieldList = fields.join(',') | ||
| const requestUrl = `${this.graphUrl}/${this.config.graphVersion || this.graphVersion}/me` |
There was a problem hiding this comment.
We should be using the this.config.userInfoUrl || this.userInfoUrl property here
examples/facebook.ts
Outdated
|
|
||
| Route.get('/facebook/redirect', async ({ ally }) => { | ||
| return ally.use('facebook').redirect((request) => { | ||
| request.scopes(['user', 'repo']) |
There was a problem hiding this comment.
Can you update this example to use facebook scopes?
src/Drivers/Facebook/index.ts
Outdated
| 'last_name', | ||
| 'link', | ||
| 'email', | ||
| // 'picture', |
There was a problem hiding this comment.
Can you please uncomment the picture field?
|
Looks great. Thanks for taking out the time to contribute 🙂 |
Proposed changes
Add Facebook diver
Types of changes
What types of changes does your code introduce?
Put an
xin the boxes that applyChecklist
Put an
xin the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.Further comments
If this is a relatively large or complex change, kick off the discussion by explaining why you chose the solution you did and what alternatives you considered, etc...