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

Validate webhook's payload #4940

Merged
merged 53 commits into from Feb 27, 2019
Merged

Conversation

@stsewd
Copy link
Member

@stsewd stsewd commented Nov 30, 2018

I tested it with github and gitlab, it works, I'm returning a 403 when the payload isn't valid.

screenshot_2018-12-04 settings santos gallegos rtd-test 1

screenshot_2018-12-04 settings santos gallegos rtd-test

Close #4886

Copy link
Member

@humitos humitos left a comment

Looks good for now.

readthedocs/restapi/views/integrations.py Outdated Show resolved Hide resolved
readthedocs/restapi/views/integrations.py Outdated Show resolved Hide resolved
readthedocs/integrations/models.py Outdated Show resolved Hide resolved
readthedocs/integrations/utils.py Show resolved Hide resolved
readthedocs/integrations/utils.py Outdated Show resolved Hide resolved
@codecov
Copy link

@codecov codecov bot commented Dec 3, 2018

Codecov Report

Merging #4940 into master will decrease coverage by 0.03%.
The diff coverage is 89.7%.

@@            Coverage Diff             @@
##           master    #4940      +/-   ##
==========================================
- Coverage   76.85%   76.82%   -0.04%     
==========================================
  Files         158      158              
  Lines        9991     9998       +7     
  Branches     1254     1248       -6     
==========================================
+ Hits         7679     7681       +2     
- Misses       1978     1984       +6     
+ Partials      334      333       -1
Impacted Files Coverage Δ
readthedocs/restapi/views/integrations.py 93.33% <100%> (+1.37%) ⬆️
readthedocs/oauth/services/base.py 47.31% <100%> (+1.15%) ⬆️
readthedocs/projects/forms.py 79.44% <50%> (-0.56%) ⬇️
readthedocs/oauth/services/gitlab.py 52.53% <50%> (-0.04%) ⬇️
readthedocs/integrations/models.py 81.56% <60%> (-0.73%) ⬇️
readthedocs/integrations/utils.py 43.75% <75%> (-18.75%) ⬇️
readthedocs/oauth/services/github.py 43.31% <83.33%> (-0.28%) ⬇️
readthedocs/vcs_support/backends/svn.py 29.5% <0%> (-9.89%) ⬇️
readthedocs/core/utils/__init__.py 74.73% <0%> (-5.68%) ⬇️
... and 14 more

@readthedocs readthedocs deleted a comment from codecov bot Dec 4, 2018
@stsewd
Copy link
Member Author

@stsewd stsewd commented Dec 4, 2018

So, there are a couple of decisions here. I'm generating a secret by default, when applying the migrations, all the integrations will have a secret, and we don't want that, because those don't have a secret set on github/gitlab.

Also, when users create a webhook, a secret is generated, but they can't see the secret, so the can't create a webhook manually.

I can suggest these things:

  • Only generate a secret when creating a webhook using the provider's api
  • Let the user see the secret (it can't be changed by the user), or we can let the user change this value too, so they can set up a secret on their own, or don't let the user see the secret and only generate one when creating an integration using the provider's api.

@stsewd
Copy link
Member Author

@stsewd stsewd commented Dec 4, 2018

So, I changed the code to only create a secret when rtd create the webhook, if the user creates the webhook manually it can't set a secret. Also, users can't see the secrets right now.

@stsewd stsewd requested a review from Jan 17, 2019
@stsewd stsewd requested a review from Jan 24, 2019
@agjohnson agjohnson removed this from the 3.2 milestone Jan 25, 2019
@agjohnson agjohnson added this to the 3.3 milestone Jan 25, 2019
Copy link
Member

@ericholscher ericholscher left a comment

Believe this is ready 👍 We likely need to update the migration names tho.

@stsewd
Copy link
Member Author

@stsewd stsewd commented Feb 26, 2019

Merged with master, migrations are still valid

@stsewd stsewd requested review from humitos and agjohnson and removed request for humitos Feb 26, 2019
@ericholscher ericholscher merged commit 9f9d240 into readthedocs:master Feb 27, 2019
1 check passed
@stsewd stsewd deleted the validate-payload-webhooks branch Feb 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants