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

Extract updateSession function to reduce repetition #21735

Merged
merged 11 commits into from
Nov 10, 2022
Merged

Extract updateSession function to reduce repetition #21735

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
71fb03d
refactor: updateSession
wolfogre Nov 9, 2022
e2e570a
fix: rename to updateSession
wolfogre Nov 9, 2022
589e973
fix: keep comments
wolfogre Nov 9, 2022
c17a8e6
fix: use string as key
wolfogre Nov 9, 2022
400265d
Merge branch 'main' into feature/refactor_update_session
lunny Nov 9, 2022
0978b18
Merge branch 'main' into feature/refactor_update_session
lunny Nov 9, 2022
abb85e5
Merge branch 'main' into feature/refactor_update_session
zeripath Nov 9, 2022
72c0483
Merge branch 'main' into feature/refactor_update_session
lunny Nov 10, 2022
febbb6c
Merge branch 'main' into feature/refactor_update_session
wolfogre Nov 10, 2022
9079153
Merge branch 'main' into feature/refactor_update_session
wolfogre Nov 10, 2022
0fe6fd5
Apply suggestions from code review
wolfogre Nov 10, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
117 changes: 52 additions & 65 deletions routers/web/auth/auth.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,19 +82,12 @@ func AutoSignIn(ctx *context.Context) (bool, error) {

isSucceed = true

if _, err := session.RegenerateSession(ctx.Resp, ctx.Req); err != nil {
return false, fmt.Errorf("unable to RegenerateSession: Error: %w", err)
}

// Set session IDs
if err := ctx.Session.Set("uid", u.ID); err != nil {
return false, err
}
if err := ctx.Session.Set("uname", u.Name); err != nil {
return false, err
}
if err := ctx.Session.Release(); err != nil {
return false, err
if err := updateSession(ctx, nil, map[string]interface{}{
// Set session IDs
"uid": u.ID,
"uname": u.Name,
}); err != nil {
return false, fmt.Errorf("unable to updateSession: %w", err)
}

if err := resetLocale(ctx, u); err != nil {
Expand Down Expand Up @@ -252,32 +245,17 @@ func SignInPost(ctx *context.Context) {
return
}

if _, err := session.RegenerateSession(ctx.Resp, ctx.Req); err != nil {
ctx.ServerError("UserSignIn: Unable to set regenerate session", err)
return
}

// User will need to use 2FA TOTP or WebAuthn, save data
if err := ctx.Session.Set("twofaUid", u.ID); err != nil {
ctx.ServerError("UserSignIn: Unable to set twofaUid in session", err)
return
}

if err := ctx.Session.Set("twofaRemember", form.Remember); err != nil {
ctx.ServerError("UserSignIn: Unable to set twofaRemember in session", err)
return
updates := map[string]interface{}{
// User will need to use 2FA TOTP or WebAuthn, save data
"twofaUid": u.ID,
"twofaRemember": form.Remember,
}

if hasTOTPtwofa {
// User will need to use WebAuthn, save data
if err := ctx.Session.Set("totpEnrolled", u.ID); err != nil {
ctx.ServerError("UserSignIn: Unable to set WebAuthn Enrolled in session", err)
return
}
updates["totpEnrolled"] = u.ID
}

if err := ctx.Session.Release(); err != nil {
ctx.ServerError("UserSignIn: Unable to save session", err)
if err := updateSession(ctx, nil, updates); err != nil {
ctx.ServerError("UserSignIn: Unable to update session", err)
return
}

Expand Down Expand Up @@ -308,29 +286,23 @@ func handleSignInFull(ctx *context.Context, u *user_model.User, remember, obeyRe
setting.CookieRememberName, u.Name, days)
}

if _, err := session.RegenerateSession(ctx.Resp, ctx.Req); err != nil {
if err := updateSession(ctx, []string{
// Delete the openid, 2fa and linkaccount data
"openid_verified_uri",
"openid_signin_remember",
"openid_determined_email",
"openid_determined_username",
"twofaUid",
"twofaRemember",
"linkAccount",
}, map[string]interface{}{
"uid": u.ID,
"uname": u.Name,
}); err != nil {
ctx.ServerError("RegenerateSession", err)
return setting.AppSubURL + "/"
}

// Delete the openid, 2fa and linkaccount data
_ = ctx.Session.Delete("openid_verified_uri")
_ = ctx.Session.Delete("openid_signin_remember")
_ = ctx.Session.Delete("openid_determined_email")
_ = ctx.Session.Delete("openid_determined_username")
_ = ctx.Session.Delete("twofaUid")
_ = ctx.Session.Delete("twofaRemember")
_ = ctx.Session.Delete("linkAccount")
if err := ctx.Session.Set("uid", u.ID); err != nil {
log.Error("Error setting uid %d in session: %v", u.ID, err)
}
if err := ctx.Session.Set("uname", u.Name); err != nil {
log.Error("Error setting uname %s session: %v", u.Name, err)
}
if err := ctx.Session.Release(); err != nil {
log.Error("Unable to store session: %v", err)
}

// Language setting of the user overwrites the one previously set
// If the user does not have a locale set, we save the current one.
if len(u.Language) == 0 {
Expand Down Expand Up @@ -762,22 +734,15 @@ func handleAccountActivation(ctx *context.Context, user *user_model.User) {

log.Trace("User activated: %s", user.Name)

if _, err := session.RegenerateSession(ctx.Resp, ctx.Req); err != nil {
if err := updateSession(ctx, nil, map[string]interface{}{
"uid": user.ID,
"uname": user.Name,
}); err != nil {
log.Error("Unable to regenerate session for user: %-v with email: %s: %v", user, user.Email, err)
ctx.ServerError("ActivateUserEmail", err)
return
}

if err := ctx.Session.Set("uid", user.ID); err != nil {
log.Error("Error setting uid in session[%s]: %v", ctx.Session.ID(), err)
}
if err := ctx.Session.Set("uname", user.Name); err != nil {
log.Error("Error setting uname in session[%s]: %v", ctx.Session.ID(), err)
}
if err := ctx.Session.Release(); err != nil {
log.Error("Error storing session[%s]: %v", ctx.Session.ID(), err)
}

if err := resetLocale(ctx, user); err != nil {
ctx.ServerError("resetLocale", err)
return
Expand Down Expand Up @@ -821,3 +786,25 @@ func ActivateEmail(ctx *context.Context) {
// Should users be logged in automatically here? (consider 2FA requirements, etc.)
ctx.Redirect(setting.AppSubURL + "/user/settings/account")
}

func updateSession(ctx *context.Context, deletes []string, updates map[string]interface{}) error {
if _, err := session.RegenerateSession(ctx.Resp, ctx.Req); err != nil {
return fmt.Errorf("regenerate session: %w", err)
}
sess := ctx.Session
sessID := sess.ID()
for _, k := range deletes {
if err := sess.Delete(k); err != nil {
return fmt.Errorf("delete %v in session[%s]: %w", k, sessID, err)
}
}
for k, v := range updates {
if err := sess.Set(k, v); err != nil {
return fmt.Errorf("set %v in session[%s]: %w", k, sessID, err)
}
}
if err := sess.Release(); err != nil {
return fmt.Errorf("store session[%s]: %w", sessID, err)
}
return nil
}
22 changes: 6 additions & 16 deletions routers/web/auth/linkaccount.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ import (
"code.gitea.io/gitea/modules/log"
"code.gitea.io/gitea/modules/mcaptcha"
"code.gitea.io/gitea/modules/recaptcha"
"code.gitea.io/gitea/modules/session"
"code.gitea.io/gitea/modules/setting"
"code.gitea.io/gitea/modules/web"
auth_service "code.gitea.io/gitea/services/auth"
Expand Down Expand Up @@ -156,25 +155,16 @@ func linkAccount(ctx *context.Context, u *user_model.User, gothUser goth.User, r
return
}

if _, err := session.RegenerateSession(ctx.Resp, ctx.Req); err != nil {
if err := updateSession(ctx, nil, map[string]interface{}{
// User needs to use 2FA, save data and redirect to 2FA page.
"twofaUid": u.ID,
"twofaRemember": remember,
"linkAccount": true,
}); err != nil {
ctx.ServerError("RegenerateSession", err)
return
}

// User needs to use 2FA, save data and redirect to 2FA page.
if err := ctx.Session.Set("twofaUid", u.ID); err != nil {
log.Error("Error setting twofaUid in session: %v", err)
}
if err := ctx.Session.Set("twofaRemember", remember); err != nil {
log.Error("Error setting twofaRemember in session: %v", err)
}
if err := ctx.Session.Set("linkAccount", true); err != nil {
log.Error("Error setting linkAccount in session: %v", err)
}
if err := ctx.Session.Release(); err != nil {
log.Error("Error storing session: %v", err)
}

// If WebAuthn is enrolled -> Redirect to WebAuthn instead
regs, err := auth.GetWebAuthnCredentialsByUID(u.ID)
if err == nil && len(regs) > 0 {
Expand Down
50 changes: 15 additions & 35 deletions routers/web/auth/oauth.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@ import (
"code.gitea.io/gitea/modules/context"
"code.gitea.io/gitea/modules/json"
"code.gitea.io/gitea/modules/log"
"code.gitea.io/gitea/modules/session"
"code.gitea.io/gitea/modules/setting"
"code.gitea.io/gitea/modules/timeutil"
"code.gitea.io/gitea/modules/util"
Expand Down Expand Up @@ -1027,17 +1026,12 @@ func setUserGroupClaims(loginSource *auth.Source, u *user_model.User, gothUser *
}

func showLinkingLogin(ctx *context.Context, gothUser goth.User) {
if _, err := session.RegenerateSession(ctx.Resp, ctx.Req); err != nil {
ctx.ServerError("RegenerateSession", err)
if err := updateSession(ctx, nil, map[string]interface{}{
"linkAccountGothUser": gothUser,
}); err != nil {
ctx.ServerError("updateSession", err)
return
}

if err := ctx.Session.Set("linkAccountGothUser", gothUser); err != nil {
log.Error("Error setting linkAccountGothUser in session: %v", err)
}
if err := ctx.Session.Release(); err != nil {
log.Error("Error storing session: %v", err)
}
ctx.Redirect(setting.AppSubURL + "/user/link_account")
}

Expand Down Expand Up @@ -1075,21 +1069,14 @@ func handleOAuth2SignIn(ctx *context.Context, source *auth.Source, u *user_model
// If this user is enrolled in 2FA and this source doesn't override it,
// we can't sign the user in just yet. Instead, redirect them to the 2FA authentication page.
if !needs2FA {
if _, err := session.RegenerateSession(ctx.Resp, ctx.Req); err != nil {
ctx.ServerError("RegenerateSession", err)
if err := updateSession(ctx, nil, map[string]interface{}{
"uid": u.ID,
"uname": u.Name,
}); err != nil {
ctx.ServerError("updateSession", err)
return
}

if err := ctx.Session.Set("uid", u.ID); err != nil {
log.Error("Error setting uid in session: %v", err)
}
if err := ctx.Session.Set("uname", u.Name); err != nil {
log.Error("Error setting uname in session: %v", err)
}
if err := ctx.Session.Release(); err != nil {
log.Error("Error storing session: %v", err)
}

// Clear whatever CSRF cookie has right now, force to generate a new one
middleware.DeleteCSRFCookie(ctx.Resp)

Expand Down Expand Up @@ -1138,22 +1125,15 @@ func handleOAuth2SignIn(ctx *context.Context, source *auth.Source, u *user_model
}
}

if _, err := session.RegenerateSession(ctx.Resp, ctx.Req); err != nil {
ctx.ServerError("RegenerateSession", err)
if err := updateSession(ctx, nil, map[string]interface{}{
// User needs to use 2FA, save data and redirect to 2FA page.
"twofaUid": u.ID,
"twofaRemember": false,
}); err != nil {
ctx.ServerError("updateSession", err)
return
}

// User needs to use 2FA, save data and redirect to 2FA page.
if err := ctx.Session.Set("twofaUid", u.ID); err != nil {
log.Error("Error setting twofaUid in session: %v", err)
}
if err := ctx.Session.Set("twofaRemember", false); err != nil {
log.Error("Error setting twofaRemember in session: %v", err)
}
if err := ctx.Session.Release(); err != nil {
log.Error("Error storing session: %v", err)
}

// If WebAuthn is enrolled -> Redirect to WebAuthn instead
regs, err := auth.GetWebAuthnCredentialsByUID(u.ID)
if err == nil && len(regs) > 0 {
Expand Down
26 changes: 7 additions & 19 deletions routers/web/auth/openid.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@ import (
"code.gitea.io/gitea/modules/log"
"code.gitea.io/gitea/modules/mcaptcha"
"code.gitea.io/gitea/modules/recaptcha"
"code.gitea.io/gitea/modules/session"
"code.gitea.io/gitea/modules/setting"
"code.gitea.io/gitea/modules/util"
"code.gitea.io/gitea/modules/web"
Expand Down Expand Up @@ -232,27 +231,16 @@ func signInOpenIDVerify(ctx *context.Context) {
}
}

if _, err := session.RegenerateSession(ctx.Resp, ctx.Req); err != nil {
ctx.ServerError("RegenerateSession", err)
return
}

if err := ctx.Session.Set("openid_verified_uri", id); err != nil {
log.Error("signInOpenIDVerify: Could not set openid_verified_uri in session: %v", err)
}
if err := ctx.Session.Set("openid_determined_email", email); err != nil {
log.Error("signInOpenIDVerify: Could not set openid_determined_email in session: %v", err)
}

if u != nil {
nickname = u.LowerName
}

if err := ctx.Session.Set("openid_determined_username", nickname); err != nil {
log.Error("signInOpenIDVerify: Could not set openid_determined_username in session: %v", err)
}
if err := ctx.Session.Release(); err != nil {
log.Error("signInOpenIDVerify: Unable to save changes to the session: %v", err)
if err := updateSession(ctx, nil, map[string]interface{}{
"openid_verified_uri": id,
"openid_determined_email": email,
"openid_determined_username": nickname,
}); err != nil {
ctx.ServerError("updateSession", err)
return
}

if u != nil || !setting.Service.EnableOpenIDSignUp || setting.Service.AllowOnlyInternalRegistration {
Expand Down