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

Add Gitea Oauth #2622

Open
wants to merge 12 commits into
base: master
from

Conversation

Projects
None yet
6 participants
@techknowlogick
Copy link

techknowlogick commented Mar 8, 2019

With fallback to basic auth if Gitea server does not support oauth

Linked to drone/go-login#3

Add Gitea Oauth
With fallback to basic auth if Gitea server does not support oauth
@CLAassistant

This comment has been minimized.

Copy link

CLAassistant commented Mar 8, 2019

CLA assistant check
All committers have signed the CLA.

@techknowlogick techknowlogick changed the title WIP: Add Gitea Oauth Add Gitea Oauth Mar 8, 2019

@bradrydzewski

This comment has been minimized.

Copy link
Member

bradrydzewski commented Mar 8, 2019

awesome! one question: how does gitea handle the scenario where two separate go routines try to refresh the access token at the same time? this is something that caused issues in early gitlab oauth implementations, so just want to understand a bit more :)

@techknowlogick

This comment has been minimized.

Copy link
Author

techknowlogick commented Mar 8, 2019

Tests fail because the above linked PR isn't merged

@bradrydzewski

This comment has been minimized.

Copy link
Member

bradrydzewski commented Mar 8, 2019

I just merged :)

@techknowlogick

This comment has been minimized.

Copy link
Author

techknowlogick commented Mar 8, 2019

Thanks @bradrydzewski 😄

@techknowlogick

This comment has been minimized.

Copy link
Author

techknowlogick commented Mar 8, 2019

@jonasfranz could you answer @bradrydzewski's question above?

techknowlogick added some commits Mar 8, 2019

@techknowlogick techknowlogick changed the title Add Gitea Oauth WIP: Add Gitea Oauth Mar 8, 2019

@techknowlogick techknowlogick changed the title WIP: Add Gitea Oauth Add Gitea Oauth Mar 8, 2019

@techknowlogick

This comment has been minimized.

Copy link
Author

techknowlogick commented Mar 8, 2019

@bradrydzewski looking through Gitea OAuth code, refreshing token isn't implemented in Gitea (yet). I was able to compile drone and login with this PR and it pulled all the repos. I've also been able to submit an updated PR to go-login with being able to use RedirectURL.

@appleboy

This comment has been minimized.

Copy link
Member

appleboy commented Mar 9, 2019

also update the provideClient func:

    case config.Github.ClientID != "":
        return provideGithubClient(config)
-    case config.Gitea.Server != "":
+    case config.Gitea.ClientID != "":
        return provideGiteaClient(config)
    case config.GitLab.ClientID != "":
        return provideGitlabClient(config)
@techknowlogick

This comment has been minimized.

Copy link
Author

techknowlogick commented Mar 9, 2019

@appleboy I kept config.Gitea.ClientID != "" codition so this doesn't break for Gitea users < 1.8 so they can still use username/password

@appleboy

This comment has been minimized.

Copy link
Member

appleboy commented Mar 9, 2019

@techknowlogick Good catch. Thanks for the PR. LGTM.

@jonasfranz

This comment has been minimized.

Copy link
Contributor

jonasfranz commented Mar 9, 2019

Refresh tokens are implemented. You can only use an access token or refresh token once to redeem a new token.

@bradrydzewski

This comment has been minimized.

Copy link
Member

bradrydzewski commented Mar 9, 2019

For context, GitLab had a similar restriction a few years ago that they since removed. Here are some issues that we ran into as a result #855

There are some other documented issues described in these threads:
doorkeeper-gem/doorkeeper#815
doorkeeper-gem/doorkeeper#449

It is very exciting to see oauth support land in Gitea (congrats!) and I look forward to taking advantage of this in Drone. I think we'll just need to work through some of these details before we can fully enable.

@tboerger

This comment has been minimized.

Copy link
Member

tboerger commented Mar 9, 2019

Great to see that this gets finally integrated

Server string `envconfig:"DRONE_GITEA_SERVER"`
SkipVerify bool `envconfig:"DRONE_GITEA_SKIP_VERIFY"`
Debug bool `envconfig:"DRONE_GITEA_DEBUG"`
Server string `envconfig:"DRONE_GITEA_SERVER" default:"https://try.gitea.io"`

This comment has been minimized.

@bradrydzewski

bradrydzewski Mar 13, 2019

Member

this default needs to be removed. An zero value server string is the only way we know if gitea is configured or not.

This comment has been minimized.

@techknowlogick

techknowlogick Mar 13, 2019

Author

Good catch. Thanks :)

I've updated.

techknowlogick added some commits Mar 13, 2019

@appleboy

This comment has been minimized.

Copy link
Member

appleboy commented Mar 14, 2019

@techknowlogick conflicts.

@appleboy
Copy link
Member

appleboy left a comment

LGTM, Can't wait.

techknowlogick added some commits Mar 14, 2019

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.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.