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.

Sign up for GitHub

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

Jump to bottom

Two factor authentication support #630

Merged
merged 11 commits into from
Jan 16, 2017
Merged

Two factor authentication support #630

merged 11 commits into from
Jan 16, 2017

Conversation

minecrafter
Copy link
Contributor

@minecrafter minecrafter commented Jan 10, 2017 •
edited
Loading

This PR is still a work in progress and therefore I do not recommend pulling this just yet.

This adds two-factor authentication support to Gitea, resolving #179. As I'm new to Gitea and fairly new to Go, I'm looking for friendly input.

There are a few issues that need to be looked at:

  • Hashing the scratch code: at the moment, scratch codes are unhashed. This may be fine as you'd need to break the password first, which is relatively difficult, but another layer of security would not hurt.

@minecrafter
Initial commit for 2FA support
d810cc4 
Signed-off-by: Andrew <write@imaginarycode.com>
@lunny lunny added this to the 1.1.0 milestone Jan 10, 2017
@lunny lunny added type/feature Completely new functionality. Can only be merged if feature freeze is not active. topic/security Something leaks user information or is otherwise vulnerable. Should be fixed! pr/wip This PR is not ready for review labels Jan 10, 2017
@metalmatze
Copy link
Contributor

Looking good so far. I'll fetch and checkout this branch later. Merging this will be awesome!

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

lunny commented Jan 10, 2017

@minecrafter Is this ready for review? not WIP?

BlackVoid
BlackVoid reviewed Jan 10, 2017
View reviewed changes
cmd/web.go
m.Get("/2fa", user.ShowTwofa)
m.Post("/2fa", bindIgnErr(auth.TwofaAuthForm{}), user.TwofaPost)
m.Get("/2fa_scratch", user.TwofaScratch)
m.Post("/2fa_scratch", bindIgnErr(auth.TwofaScratchAuthForm{}), user.TwofaScratchPost)
Copy link

Choose a reason for hiding this comment

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

Since there are many endpoints it might be better to use the following structure and change 2fa_scratch to be 2fa/scratch.

m.Group("/2fa", func() {
    m.Get("", user.ShowTwofa)
    [...]
})

cmd/web.go
m.Post("/2fa/regenerate_scratch", user.SettingsTwofaRegenerateScratch)
m.Post("/2fa/disable", user.SettingsTwofaDisable)
m.Get("/2fa/enroll", user.SettingsTwofaEnroll)
m.Post("/2fa/enroll", bindIgnErr(auth.TwofaAuthForm{}), user.SettingsTwofaEnrollPost)
Copy link

Choose a reason for hiding this comment

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

Same here

@minecrafter
Copy link
Contributor Author

@lunny It's a WIP still. I will probably finish it up this week.

@minecrafter
Copy link
Contributor Author

Have some time to work on this - going to be improving the code and finalizing everything.

@minecrafter
Copy link
Contributor Author

Ready to get this pull request reviewed.

@lunny lunny removed the pr/wip This PR is not ready for review label Jan 15, 2017
@lunny
Copy link
Member

lunny commented Jan 15, 2017

LGTM, Good Job!

@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 Jan 15, 2017
Bwko
Bwko reviewed Jan 15, 2017
View reviewed changes
routers/user/auth.go

handleSignInFull(ctx, u, remember, false)
ctx.Flash.Info(ctx.Tr("auth.twofa_scratch_used"))
ctx.Redirect(setting.AppSubURL + "/user/settings/twofa")
Copy link
Member

Choose a reason for hiding this comment

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

Should be 2fa instead of twofa

Copy link
Member

Choose a reason for hiding this comment

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

^

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@bkcsoft
Copy link
Member

bkcsoft commented Jan 15, 2017 •
edited
Loading

I would prefer if you changed "twofa" to "twofactor" instead, it makes it easier to read since we can't do "2fa" or "TwoFA" :)

@metalmatze
Copy link
Contributor

Just tried it locally and it seems to be working just fine.
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 Jan 16, 2017
@minecrafter
Copy link
Contributor Author

Thanks! 👍

@bkcsoft I might consider doing so, but otherwise it works well so it's more of a cosmetic issue.

@lunny
Copy link
Member

lunny commented Jan 16, 2017

@minecrafter can't wait to merge it. When would you like to push a commit for @bkcsoft's advice?

lunny
lunny requested changes Jan 16, 2017
View reviewed changes
Copy link
Member

@lunny lunny left a comment

Choose a reason for hiding this comment

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

otherwise LGTM

models/twofactor.go
// Twofa represents a two-factor authentication token.
type Twofa struct {
// TwoFactor represents a two-factor authentication token.
type TwoFactor struct {
ID int64 `xorm:"pk autoincr"`
UID int64 `xorm:"UNIQUE INDEX"`
Copy link
Member

Choose a reason for hiding this comment

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

It's no need for both UNIQUE and INDEX, chose one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@lunny
Copy link
Member

lunny commented Jan 16, 2017

Let L-G-T-M work

@lunny lunny merged commit 6dd096b into go-gitea:master Jan 16, 2017
@lunny
Copy link
Member

lunny commented Jan 16, 2017

Thanks!

@minecrafter
Copy link
Contributor Author

No problem! 😄

@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.
Reviewers

@Bwko Bwko Bwko left review comments

@BlackVoid BlackVoid BlackVoid left review comments

@lunny lunny lunny approved these changes

Assignees
No one assigned
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. topic/security Something leaks user information or is otherwise vulnerable. Should be fixed! type/feature Completely new functionality. Can only be merged if feature freeze is not active.
Projects
None yet
Milestone
1.1.0
Development

Successfully merging this pull request may close these issues.
7 participants
@minecrafter @metalmatze @lunny @bkcsoft @BlackVoid @Bwko @tboerger