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

Update hookUrl to https #16

Open
zimme opened this issue Apr 28, 2015 · 16 comments
Open

Update hookUrl to https #16

zimme opened this issue Apr 28, 2015 · 16 comments

Comments

@zimme
Copy link
Member

zimme commented Apr 28, 2015

We can't toggle new web hooks right now. Pretty critical bug.

@zimme
Copy link
Member Author

zimme commented Apr 28, 2015

I at least think it's hookUrl in settings.json which needs an update to https

@splendido
Copy link
Member

AaaaAAaarg!!!
That's really bad: hooks do no work with force-ssl because the answer they get is a 302 redirect to https://autopublish.meteor.com/publish which GitHub does not handle!!!

We've potentially lost all triggers received in the last 4 days :-(

I removed force-ssl for now...
Then I'll try to see whether using user tokens stored in the DB the server can modify the hook settings with some migration routine, otherwise I see the use of force-ssl unfeasible.

...we could redirect by hands to https for the few existing user routes, while keeping http://autopublish.meteor.com/publish (which is a server side route...) working as is.

Thoughts?

@splendido
Copy link
Member

Thanks a lot for reporting!!!

@splendido
Copy link
Member

...actually, latest release of [email protected] was gone :(

@zimme
Copy link
Member Author

zimme commented Apr 29, 2015

Change http://autopublish.meteor.com/publish to https://autopublish.meteor.com/publish in the webhook requests and request new webhooks for the ones already setup?

@splendido
Copy link
Member

I have to check whether I can change/create new webhooks using the user tokens present in the DB.
If not, we'd need to create Issues on all repositories having registered hooks to ask people to fix them or come back to useraccounts.meteor.com and untoggle/toggle their repo.

Is this what you mean with

request new webhooks for the ones already setup

?

@zimme
Copy link
Member Author

zimme commented Apr 29, 2015

Yeah that's what I meant, just assumed github had a way of updating webhooks.

I don't have any better ideas at the moment.

@splendido
Copy link
Member

In principle yes! You can use the API to modify whatever you wish (based on the permission scopes you asked on signup...)
The only thing I'm not sure is whether the token you get can be used also when your user is 'offline' from your app or not.

I'll try to confirm this during the weekend.
If the token is fine, I'll try an automatic update of all hooks ;-)

@zimme
Copy link
Member Author

zimme commented May 11, 2015

Did you get any time to look into if it was possible to use the stored api key to request new webhooks with https instead?

@splendido
Copy link
Member

no, I'm sorry :-(

@zimme
Copy link
Member Author

zimme commented May 11, 2015

No problem at all, I just wondered.

If I get some time to spare I might try and check if I'm able to request new webhook with a previous api key.

@splendido
Copy link
Member

Good news!
I've just played a bit with this, and with:

var hookDetails = {
  user: "MeteorPackaging",
  repo: "autopublish-test",
  id: 4797246,
};

github.authenticate({
  type: "token",
  token: "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx"
});
github.repos.getHook(hookDetails, function(err, res){
  console.log('GET HOOK:');
  console.log(err);
  console.log(res);
  if (!err) {
    hookDetails.name = res.name;
    hookDetails.config = res.config;
    hookDetails.events = ['release'];
    hookDetails.config.url = res.config.url.replace('http://', 'https://');
    github.repos.updateHook(hookDetails, function(err, res){
      console.log('UPDATE HOOK:');
      console.log(err);
      console.log(res);
    });
  }
});

I was able to succesfully update the hook even without having the user logged in! :-)

I've also confimed that the same doesn't work in case the user had revoked the app authorization and that we get an error in case the hook does not exists anymore...

I'd say there's everything we need to do a batch update, but we should be careful to notice which hooks we're not able to update so to ask repo maintainers to update them manually.

This said, now it's not the right time to write such a migration procedure... ;-)
I'll try to write something robut and bug free in the next days...
...perhaps even automatically creating new issues to request manual updates for those hooks we won't be able to update programmatically.

Thanks for your support!

@zimme
Copy link
Member Author

zimme commented May 12, 2015

This is great news! Yes ofc, as people are depending on the service we need to be careful on how and when we do the update, we don't want another blunder like the last one 👍

@splendido
Copy link
Member

I've just switched again to https by using a modified version of force-ssl which also accepts non secure requests to /update.

This allows to get users on https while accepting webhook triggers both on http and https.

I've also updated the hook address accordingly so that new registrations will trigger on https.
I'll try to update already existing hook in the nex days..

@splendido
Copy link
Member

Just checked again and unfortunately I won't be able to automatically update existing hooks.
This is because within the knownhooks objects I've kept no reference to the user who originally created them. So there's no way to retrieve a user token to be used to authorize the hook update :(

Nevermind, I'll try to automatically generate an issue on source repositories to kindly ask to manually update the hook address to https or untoggle/toggle again the repo so to get a fresh new hook! :-)

@zimme
Copy link
Member Author

zimme commented May 16, 2015

👍 Great work

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

No branches or pull requests

2 participants