Skip to content
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(gatsby-source-wordpress): allow users to obtain JWT Token to make authenticated requests #9509

Merged
merged 15 commits into from Nov 4, 2018

Conversation

ghost
Copy link

@ghost ghost commented Oct 28, 2018

This fixes #6879.

@ghost ghost changed the title Feat: Allow users to specify a JWT Token to authenticate and update README feat(gatsby-source-wordpress) Allow users to specify a JWT Token to authenticate and update README Oct 28, 2018
@pieh
Copy link
Contributor

pieh commented Oct 29, 2018

Something is wrong with formatting here - seems like you pasted built file to source file? Changes itself are fine, but formatting need to be fixed

@ghost
Copy link
Author

ghost commented Oct 29, 2018

@pieh Whoops! I noticed that I accidently worked on the built file, pasted it into the source file, saw no linting errors and assumed it was fine. I'm gonna fix that.

@pieh
Copy link
Contributor

pieh commented Oct 29, 2018

Hmmm, just reading more about it - we should hit /wp-json/jwt-auth/v1/token to grab a token to use (similiar to what we use for wp.com hosting) and not set this token in config as it will expire?

@ghost
Copy link
Author

ghost commented Oct 29, 2018

We could do so, too. I'll look into it and try to implement getting the token as well.

@pieh
Copy link
Contributor

pieh commented Oct 29, 2018

We could do so, too. I'll look into it and try to implement getting the token as well.

Please check how this is done for wordpress.com:

/**
* Gets wordpress.com access token so it can fetch private data like medias :/
*
* @returns
*/
async function getWPCOMAccessToken(_auth) {
let result
const oauthUrl = `https://public-api.wordpress.com/oauth2/token`
try {
let options = {
url: oauthUrl,
method: `post`,
data: querystring.stringify({
client_secret: _auth.wpcom_app_clientSecret,
client_id: _auth.wpcom_app_clientId,
username: _auth.wpcom_user,
password: _auth.wpcom_pass,
grant_type: `password`,
}),
}
result = await axios(options)
result = result.data.access_token
} catch (e) {
httpExceptionHandler(e)
}
return result
}

This probably will be very similar for jwt-authentication-for-wp-rest-api plugin (maybe same function for getting access token can work for both)

@ghost
Copy link
Author

ghost commented Oct 29, 2018

@pieh Thanks! I have already found that function as a starting point. 👍

@ghost ghost requested a review from pieh October 29, 2018 22:49
@watzing
Copy link

watzing commented Nov 1, 2018

Thanks for this! We need to expose drafts for a preview server. I'm struggling to reference this pull request in my package json to begin using it now, is that even possible? Appreciate if someone can point me in the right direction. Cheers.

@pieh
Copy link
Contributor

pieh commented Nov 1, 2018

@watzing it's not possible to reference PR like that, but you can npm pack inside packages/gatsby-source-wordpress to create .tgz file that you can reference in your project (either upload it somewhere or commit to your repo)

@pieh
Copy link
Contributor

pieh commented Nov 1, 2018

@watzing also one thing - this adds support for jwt authorization - but it doesn't change endpoints that this plugin use.

F.e. we grab posts using /wp-json/wp/v2/posts route (which even with authorization will only show published posts). To get also drafts we would need to use /wp-json/wp/v2/posts?status=any or /wp-json/wp/v2/posts?status=publish,future,draft,pending (not really that familiar with some available options that wp-json shows):

publish, future, draft, pending, private, trash, auto-draft, inherit, request-pending, request-confirmed, request-failed, request-completed, any

But right now wordpress plugin doesn't support applying filters like that - this would need to be implemented separately

@pieh
Copy link
Contributor

pieh commented Nov 1, 2018

@openmedi I thought (hoped) that grabbing token function/request wouldn't need special cases like that. I think it makes sense to have separate functions after all for wp.com on jwt plugin. Sorry for adding more work :(

@watzing
Copy link

watzing commented Nov 1, 2018

but you can npm pack inside packages/gatsby-source-wordpress to create .tgz

@pieh Thanks for the help, I'll give that a go.

But right now wordpress plugin doesn't support applying filters like that - this would need to be implemented separately

Ahh, I thought I would be able to add the filters to the includedRoutes config item - I'll dig a bit deeper into the plugin code

@pieh
Copy link
Contributor

pieh commented Nov 1, 2018

Ahh, I thought I would be able to add the filters to the includedRoutes config item - I'll dig a bit deeper into the plugin code

includedRoutes only filters routes we get from main /wp-json endpoint (it can't add new routes).

Check https://github.com/gatsbyjs/gatsby/blob/master/packages/gatsby-source-wordpress/src/fetch.js in particular getPages which is function that actually make content requests - we would want to modify this

url: `${url}?${querystring.stringify({
per_page: _perPage,
page: page,
})}`,

to apply status filter. But we would only do this for content types (posts, pages and any custom post types) and not for other endpoints (tags, categories, media/images(?), users/authors, ACF (if someone is using that)). And also we would only want to apply that filter if user want us too (if you apply this filter without authorization you will get 400 forbidden response)

---edit:
sorry @openmedi for hijacking this PR comments. @watzing we should move our discussion elsewhere - if you will have further questions please create new issue and reference our comments here

@ghost
Copy link
Author

ghost commented Nov 2, 2018

What do you think, @pieh?

@pieh
Copy link
Contributor

pieh commented Nov 2, 2018

I will be looking to test it locally today, already created wordpress instance with JWT plugin.

One thing that I think we could simplify is change

    if (_useJWT && _accessToken) {
      options.headers = {
        Authorization: `Bearer ${_accessToken}`,
      }
    }

    if (_hostingWPCOM && _accessToken) {
      options.headers = {
        Authorization: `Bearer ${_accessToken}`,
      }
    }

to just

    if (_accessToken) {
      options.headers = {
        Authorization: `Bearer ${_accessToken}`,
      }
    }

We want to apply authorization header same way if there is any access token and at that point it doesn't matter if it's wp.com or site with jwt plugin (there are 2 places where we can change that)

@ghost
Copy link
Author

ghost commented Nov 2, 2018

@pieh Alright. I implemented the changes that you suggested.

@pieh
Copy link
Contributor

pieh commented Nov 2, 2018

I think we can also get away with explicit useJWT flag (if user has defined auth.jwt_user and/or auth.jwt_pass - I'll just apply that change and get this merged in (already checked with my test wordpress instance and working as expected :) )

@pieh
Copy link
Contributor

pieh commented Nov 2, 2018

@openmedi enjoy first PR against your gatsby fork :) https://github.com/openmedi/gatsby/pull/1

@pieh pieh changed the title feat(gatsby-source-wordpress) Allow users to specify a JWT Token to authenticate and update README feat(gatsby-source-wordpress): allow users to obtain JWT Token to make authenticated requests Nov 4, 2018
Copy link
Contributor

@pieh pieh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @openmedi!

@sarahannnicholson
Copy link
Contributor

---edit:
sorry @openmedi for hijacking this PR comments. @watzing we should move our discussion elsewhere - if you will have further questions please create new issue and reference our comments here

Created a new feature request for adding drafted content #10982

gpetrioli pushed a commit to gpetrioli/gatsby that referenced this pull request Jan 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Need gatsby-source-wordpress to support jwt authentication
3 participants