Skip to content

fix .netrc authentication #2700

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

Merged
merged 7 commits into from
Oct 15, 2017
Merged

Conversation

daviian
Copy link
Member

@daviian daviian commented Oct 14, 2017

Fixes #2480

Reverted a part of #2184 to allow token usage without the need of 2FA.
But token is required if 2FA is enabled!

@codecov-io
Copy link

codecov-io commented Oct 14, 2017

Codecov Report

Merging #2700 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #2700   +/-   ##
======================================
  Coverage    27.1%   27.1%           
======================================
  Files          87      87           
  Lines       17191   17191           
======================================
  Hits         4659    4659           
  Misses      11852   11852           
  Partials      680     680

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 1ec4dc6...15998be. Read the comment docs.

@tboerger tboerger added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Oct 14, 2017
@lunny
Copy link
Member

lunny commented Oct 14, 2017

@daviian but since harness/harness#2241 merged. This PR will broke it again?

Copy link
Member

@lafriks lafriks left a comment

Choose a reason for hiding this comment

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

Yes I think that would break fixed drone

@daviian
Copy link
Member Author

daviian commented Oct 14, 2017

@lunny @lafriks Yes, but this would be the correct behaviour as defined for token authentication by github. IMHO the drone fix was an urgent hot fix and the drone shouldn't be the only target of the netrc capability.

But yes I see that it will be hard to do that change again :/ Sadly...

@daviian
Copy link
Member Author

daviian commented Oct 14, 2017

And current drone only works with latest gitea release... and that's not good at all. Every gitea user that uses drone is now forced to upgrade

@lafriks
Copy link
Member

lafriks commented Oct 14, 2017

Drone support only 1.2 anyway as only 1.2 has status api

@daviian
Copy link
Member Author

daviian commented Oct 14, 2017

But as you can see in issues users use drone in 1.1.4 and earlier as well.

@daviian daviian force-pushed the bugfix/netrc-behaviour branch from 391f941 to bed668c Compare October 14, 2017 10:01
@daviian
Copy link
Member Author

daviian commented Oct 14, 2017

@lunny @lafriks I changed it to support both options. Username as token as well as password as token.

So no break in drone and backwards compatibility.

Signed-off-by: David Schneiderbauer <[email protected]>
@lafriks lafriks added backport/v1.2 issue/regression Indicates a previously functioning feature or behavior that has broken or regressed after a change labels Oct 14, 2017
@lafriks lafriks added this to the 1.3.0 milestone Oct 14, 2017
@lunny
Copy link
Member

lunny commented Oct 14, 2017

@daviian gitea 1.1.4 could use Gogs driver of drone.

@lafriks
Copy link
Member

lafriks commented Oct 14, 2017

@daviian I still think it's not a good idea to do this as .netrc anyway supports both username&password and drone is only that use this and now that it has already changed to use new way, let's than keep just one way not to have problems later.

@lunny
Copy link
Member

lunny commented Oct 15, 2017

@lafriks but that ask people upgrade their drone to latest. So I will give this LGTM

@tboerger tboerger 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 Oct 15, 2017
@lafriks
Copy link
Member

lafriks commented Oct 15, 2017

LGTM

@tboerger tboerger 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 Oct 15, 2017
if err != nil {
ctx.Handle(http.StatusInternalServerError, "GetUserByID", err)
}
} else {
Copy link
Member

Choose a reason for hiding this comment

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

else if

if isUsernameToken {
authUser, err = models.GetUserByID(token.UID)
if err != nil {
ctx.Handle(http.StatusInternalServerError, "GetUserByID", err)
Copy link
Member

Choose a reason for hiding this comment

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

missing return?

if isUsernameToken {
// Assume username is token
authToken = authUsername
} else {
Copy link
Member

Choose a reason for hiding this comment

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

// Assume username is token
authToken := authUsername

if !isUsernameToken {
	// Assume password is token
	authToken = authPasswd

Signed-off-by: David Schneiderbauer <[email protected]>
@daviian
Copy link
Member Author

daviian commented Oct 15, 2017

Make LG-TM work

@lafriks
Copy link
Member

lafriks commented Oct 15, 2017

Make it work again

@lafriks lafriks merged commit 4ccb0fe into go-gitea:master Oct 15, 2017
@lafriks
Copy link
Member

lafriks commented Oct 15, 2017

@daviian please backport to 1.2 branch

@daviian daviian deleted the bugfix/netrc-behaviour branch October 15, 2017 15:37
daviian added a commit to daviian/gitea that referenced this pull request Oct 15, 2017
* provide both possible authentication solutions

Signed-off-by: David Schneiderbauer <[email protected]>
@lafriks lafriks added the backport/done All backports for this PR have been created label Oct 15, 2017
lafriks pushed a commit that referenced this pull request Oct 15, 2017
* provide both possible authentication solutions

Signed-off-by: David Schneiderbauer <[email protected]>
@go-gitea go-gitea locked and limited conversation to collaborators Nov 23, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
backport/done All backports for this PR have been created issue/regression Indicates a previously functioning feature or behavior that has broken or regressed after a change lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

netrc x-oauth-basic no longer works since 1.2.0-rc1
6 participants