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

Change drone token name to let users know to use oauth2 #6912

Merged
merged 3 commits into from May 12, 2019

Conversation

6 participants
@techknowlogick
Copy link
Member

commented May 11, 2019

This changes the name of the drone token so that basic auth is still allowed (but points user in direction of using oauth2)

@techknowlogick techknowlogick added this to the 1.9.0 milestone May 11, 2019

@codecov-io

This comment has been minimized.

Copy link

commented May 11, 2019

Codecov Report

Merging #6912 into master will decrease coverage by <.01%.
The diff coverage is 40%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6912      +/-   ##
==========================================
- Coverage   41.42%   41.42%   -0.01%     
==========================================
  Files         440      440              
  Lines       59738    59744       +6     
==========================================
- Hits        24749    24747       -2     
- Misses      31752    31760       +8     
  Partials     3237     3237
Impacted Files Coverage Δ
routers/api/v1/user/app.go 60.57% <40%> (-2.69%) ⬇️
models/unit.go 62.16% <0%> (-5.41%) ⬇️
routers/repo/view.go 42.02% <0%> (-1.02%) ⬇️
modules/log/event.go 65.98% <0%> (+1.52%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 13583a6...0fdd44c. Read the comment docs.

@zeripath
Copy link
Contributor

left a comment

Lgtm but I know nothing about Drone

@GiteaBot GiteaBot added lgtm/need 1 and removed lgtm/need 2 labels May 12, 2019

@GiteaBot GiteaBot added lgtm/done and removed lgtm/need 1 labels May 12, 2019

@lafriks

This comment has been minimized.

Copy link
Member

commented May 12, 2019

Is this really only option?

@techknowlogick

This comment has been minimized.

Copy link
Member Author

commented May 12, 2019

What users should be doing is using OAuth2, however some don't. So this will resolve the broken connection when they try to use basic auth with drone, however it'll let them know they should be using OAuth2.

Basic auth connection with drone is already not working due with users that use 2FA, and so if users want to be secure they should be using OAuth2. And as we've seen from recent attacks against SCM providers, 2FA is essentially mandatory if you want to keep your code safe.

I see this PR as temporary, and once 1.9.0 is final we can put in blog post that users should switch to OAuth2 for Drone, and then we can remove it for 1.10.0

@techknowlogick techknowlogick merged commit 5ffc965 into go-gitea:master May 12, 2019

1 of 2 checks passed

continuous-integration/drone/pr Build is running
Details
approvals/lgtm this commit looks good

@techknowlogick techknowlogick deleted the techknowlogick:techknowlogick-patch-3 branch May 12, 2019

@lafriks

This comment has been minimized.

Copy link
Member

commented May 12, 2019

But wouldn't generate lots of tokens this way?

@techknowlogick

This comment has been minimized.

Copy link
Member Author

commented May 12, 2019

@lafriks yes, it would. This encourages users to switch to OAuth2 (basic auth for Gitea will be removed in next release of Drone)

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.