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 new Authentication Source: GitHub, including GitHub Enterprise #5340

Merged
merged 3 commits into from Dec 18, 2018

Conversation

5 participants
@luhaixun
Copy link
Contributor

luhaixun commented Jul 15, 2018

This is a reopen of pull request #5306 as closed by mistake. The PR will support GitHub Authentication both for github.com and any customized GitHub Enterprise.

The related issues are:
#2927
#3033

@luhaixun luhaixun force-pushed the luhaixun:github_enterprise_authentication branch from 7381c1c to 0a365f8 Jul 15, 2018

@luhaixun luhaixun force-pushed the luhaixun:github_enterprise_authentication branch from 0a365f8 to 4173ae7 Jul 15, 2018

@cupen

This comment has been minimized.

Copy link

cupen commented Jul 25, 2018

Looks good to me.

@cupen

This comment has been minimized.

Copy link

cupen commented Jul 25, 2018

I register local gogs account with my email which same as my github account used, then I meet a problem with github login.
Error log:

2018/07/25 16:38:02 [ERROR] [...g/context/context.go:175 ServerError()] UserLogin: e-mail has been used [email: me@xxmail.com]
@cupen

This comment has been minimized.

Copy link

cupen commented Jul 25, 2018

How about use oauth2.0 ? You know, put user's password in the memory of your computer wasn't very safety. And using oauth2.0 means that other third-party services can be integrated easily and quickly, not just github.

@luhaixun

This comment has been minimized.

Copy link
Contributor

luhaixun commented Jul 25, 2018

@cupen for the first email issue, that's because Gogs has already has that email linked to another account, you should change to another one and try again.

As of safety concerns, although the credential token will be encrypted before storing it in the database, I feel they share the same security risk.

OAuth2.0 is a good idea, but it is not friendly if the same user uses different browser or clean the browser cache before login, he has to authorize it every time.

@cupen

This comment has been minimized.

Copy link

cupen commented Jul 26, 2018

@luhaixun Thanks for your feedback. It looks like we have two different viewpoint.

  1. security issue
    I'm not just meaning "password store". We know, if not https, the username/password is transmitted in clear text, from user's machine to your(or him or mine) gogs machine. It's a long-range could be attacked if the username/password is the third-part services account of yours. e.g.: github, google? double-kill, triple-kill. So it's not a little problem, though it's not your code's mistake. Maybe we can refactor a little code of gogs account system for adapte oauth2.0 ?

  2. oauth2.0 is not friendly ?
    Yes or no. But I tend to think the vast majority of usercases is: one browser <--> one service.
    e.g. I use gmail in chrome, weixin chat in firefox, not use gmail or weixin chat both in chrome and firefox.

@luhaixun

This comment has been minimized.

Copy link
Contributor

luhaixun commented Jul 26, 2018

@cupen For [1] security concern, I agree with you that it's a good chance for Gogs think more about this. 💯

As of [2], yes we have a different view indeed because my Chrome profile is set to clear all cache every time after I close the browser, which is hard set by our Dev IT. :(

@Unknwon Unknwon force-pushed the gogs:develop branch from 7bcdc94 to 7a7e07a Sep 15, 2018

@ibearth2025

This comment has been minimized.

Copy link

ibearth2025 commented Oct 2, 2018

Yes it seems good

@ibearth2025

This comment has been minimized.

Copy link

ibearth2025 commented Oct 2, 2018

But obviously oAuth is needed , @Unknwon can you please re-implement oAuth ?

@Unknwon Unknwon changed the title Add new Authentication Source: GitHub, including GitHub Enterprise. Add new Authentication Source: GitHub, including GitHub Enterprise. Dec 11, 2018

@Unknwon Unknwon changed the title Add new Authentication Source: GitHub, including GitHub Enterprise. Add new Authentication Source: GitHub, including GitHub Enterprise Dec 11, 2018

@Unknwon Unknwon changed the base branch from develop to feature/login-source-github Dec 18, 2018

@Unknwon

This comment has been minimized.

Copy link
Member

Unknwon commented Dec 18, 2018

I'm merging this into a feature branch because of some required fixes/changes. Thanks for the PR!

@Unknwon Unknwon merged commit 311df9c into gogs:feature/login-source-github Dec 18, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Unknwon

This comment has been minimized.

Copy link
Member

Unknwon commented Dec 18, 2018

security issue
I'm not just meaning "password store". We know, if not https, the username/password is transmitted in clear text, from user's machine to your(or him or mine) gogs machine. It's a long-range could be attacked if the username/password is the third-part services account of yours. e.g.: github, google? double-kill, triple-kill. So it's not a little problem, though it's not your code's mistake. Maybe we can refactor a little code of gogs account system for adapte oauth2.0 ?

Just tested and proved that use personal access token as password with user profile permission actually works. So no password transmission at all.

Unknwon added a commit that referenced this pull request Dec 18, 2018

@Unknwon

This comment has been minimized.

Copy link
Member

Unknwon commented Dec 18, 2018

Merged into develop branch.

@Unknwon Unknwon referenced this pull request Dec 18, 2018

Open

Github Authentication #2927

@yocorn

This comment has been minimized.

Copy link

yocorn commented Dec 19, 2018

Getting this error

Unknown error: RangeError
@Unknwon

This comment has been minimized.

Copy link
Member

Unknwon commented Dec 19, 2018

@CodingPurple thanks your feedback! But your feedback is as vague as said nothing. Can you provide detailed steps?

@yocorn

This comment has been minimized.

Copy link

yocorn commented Dec 19, 2018

Hello @Unknwon! I'm getting this error while creating new auth

admin > auths > new

screenshot 1

@Unknwon

This comment has been minimized.

Copy link
Member

Unknwon commented Dec 19, 2018

@codeskyblue Hi, I cannot reproduce this error. I suspect you are using a modified version of Gogs and changed validation rule to make a "Range" binding rule to be required.

@luhaixun

This comment has been minimized.

Copy link
Contributor

luhaixun commented Dec 19, 2018

@codeskyblue could you help run below query in your gogs database for a quick check?

select id,type from login_source;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment