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
Merged

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

merged 3 commits into from
May 12, 2019

Conversation

techknowlogick
Copy link
Member

@techknowlogick techknowlogick 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 the status/blocked This PR cannot be merged yet, i.e. because it depends on another unmerged PR label May 11, 2019
@techknowlogick techknowlogick added this to the 1.9.0 milestone May 11, 2019
@codecov-io
Copy link

codecov-io 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.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label May 11, 2019
@techknowlogick techknowlogick added type/bug and removed status/blocked This PR cannot be merged yet, i.e. because it depends on another unmerged PR labels May 11, 2019
Copy link
Contributor

@zeripath zeripath left a comment

Choose a reason for hiding this comment

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

Lgtm but I know nothing about Drone

@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels May 12, 2019
@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels May 12, 2019
@lafriks
Copy link
Member

lafriks commented May 12, 2019

Is this really only option?

@techknowlogick
Copy link
Member Author

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
@techknowlogick techknowlogick deleted the techknowlogick-patch-3 branch May 12, 2019 17:29
@lafriks
Copy link
Member

lafriks commented May 12, 2019

But wouldn't generate lots of tokens this way?

@techknowlogick
Copy link
Member Author

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

@go-gitea go-gitea locked and limited conversation to collaborators Nov 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants