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

Add token-less re-scrape with request signature #937

Merged
merged 4 commits into from Jun 28, 2018

Conversation

Projects
None yet
3 participants
@diegoquinteiro
Copy link
Collaborator

diegoquinteiro commented May 13, 2018

This PR aims to do trigger a token-less re-scrape of the URLs being updated to trigger a new Instant Article ingestion even if the user does not have a configured app ID/app secret/access token.

  • Create key generation/exposure logic
  • Gracefully degrade to require a token when OpenSSL lib is not available

@diegoquinteiro diegoquinteiro requested a review from everton-rosario May 13, 2018

@diegoquinteiro diegoquinteiro added this to the 4.1.0 milestone May 13, 2018

@jerclarke

This comment has been minimized.

Copy link

jerclarke commented May 13, 2018

Wow, so this is possible!? Great! Good luck getting it fully working!

@diegoquinteiro

This comment has been minimized.

Copy link
Collaborator Author

diegoquinteiro commented May 14, 2018

Hi @jerclarke!

That's one of the changes we're testing to solve the problem, based on your feedback. But we're still rolling out the API side of it and we can only review/land this PR afterwards.

Would you like/be able to help testing it?

Thank you!

Diego Moreno Quinteiro and others added some commits May 13, 2018

@diegoquinteiro diegoquinteiro force-pushed the feature/tokenless-rescrape branch from 6421c87 to 85f6c6c Jun 25, 2018

@diegoquinteiro diegoquinteiro changed the title [WIP] Add token-less re-scrape with request signature Add token-less re-scrape with request signature Jun 25, 2018

@diegoquinteiro

This comment has been minimized.

Copy link
Collaborator Author

diegoquinteiro commented Jun 25, 2018

@everton-rosario This PR is now complete and ready for review.

@everton-rosario
Copy link
Collaborator

everton-rosario left a comment

Check if my comments require changes.

There are some small style-nit things as well.
Other than that seems really good to me. Nice work!

Ping me if you want a second run, if changes are more than small style-nit.

$adapter = new Instant_Articles_Post( $post );
$old_slugs = get_post_meta( $post_id, '_wp_old_slug' );
if ( $adapter->should_submit_post() ) {

This comment has been minimized.

Copy link
@everton-rosario

everton-rosario Jun 25, 2018

Collaborator

Remove extra empty line.

This comment has been minimized.

Copy link
@diegoquinteiro

diegoquinteiro Jun 27, 2018

Author Collaborator

💅

global $wp;
if ( $wp->request == self::PUBLIC_KEY_PATH ) {
status_header(200);
echo self::get_public_key();

This comment has been minimized.

Copy link
@everton-rosario

everton-rosario Jun 25, 2018

Collaborator

Shouldn't we add proper content-type?

This comment has been minimized.

Copy link
@everton-rosario

everton-rosario Jun 25, 2018

Collaborator

Check if we shouldn't allow more text into this response. So the_content should not have anything else but the key.

This comment has been minimized.

Copy link
@diegoquinteiro

diegoquinteiro Jun 27, 2018

Author Collaborator
  • We die() aftwards to ensure nothing else is coming in out the response.
  • We have no requirements for a specific content-type.
const PUBLIC_KEY_OPTION = 'instant-articles-rescrape-public-key';
const PRIVATE_KEY_OPTION = 'instant-articles-rescrape-private-key';
const PUBLIC_KEY_PATH = '.well-known/graph-api/apikey.pub';

This comment has been minimized.

Copy link
@everton-rosario

everton-rosario Jun 25, 2018

Collaborator

Is this a fixed place? Should we make it by default this path, but allow configuration in the settings page.

This comment has been minimized.

Copy link
@diegoquinteiro

diegoquinteiro Jun 27, 2018

Author Collaborator

This is a fixed place, yes. It's not configurable on the FB side.

@diegoquinteiro diegoquinteiro merged commit 7b5feb3 into master Jun 28, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.