Store OAuth2 session data in database #3660

lafriks · 2018-03-12T19:37:15Z

Currently OAuth2 sessions are saved only on filesystem in sessions directory independently of where gitea session is actually saved

codecov-io · 2018-03-12T19:46:08Z

Codecov Report

Merging #3660 into master will decrease coverage by <.01%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master    #3660      +/-   ##
==========================================
- Coverage    23.3%   23.29%   -0.01%     
==========================================
  Files         126      126              
  Lines       24999    25002       +3     
==========================================
  Hits         5825     5825              
- Misses      18286    18289       +3     
  Partials      888      888

Impacted Files	Coverage Δ
models/oauth2.go	`0% <0%> (ø)`	⬆️

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 8d5f58d...542efa7. Read the comment docs.

lunny · 2018-03-13T01:25:58Z

vendor/github.com/lafriks/xormstore/xormstore.go

+	}
+
+	if !st.opts.SkipCreateTable {
+		st.e.Sync2(&xormSession{tableName: st.opts.TableName})


The error should be handled.

lunny · 2018-03-13T01:31:19Z

@lafriks why save OAuth session to database not in file system?

lafriks · 2018-03-13T07:07:04Z

@lunny probably currently there is no real reason. I wrote this code when trying to fix oath2 bug but error that file session storage was throwing about trying to delete unexisting file was just a consequence not a real source for that bug.
But more generally speaking this could be useful in future as currently we can specify to store Gitea session data in redis for example but oauth2 sessions will still be saved in file system so Gitea can not be load balanced because of that as if first request before redirecting to auth2 authorization service would hit one node and create session, if second request after authorization would hit other node, it would not know about such session and would fail with error.

lunny · 2018-03-13T07:10:55Z

@lafriks seems good enough. Please follow my review, otherwise LGTM.

jonasfranz

I get the following error if I try to login via github oauth at my local gitea instance:

2018/03/18 20:49:52 [...routers/user/auth.go:411 handleOAuth2SignIn()] [E] UserSignIn: could not find a matching session for this request

jonasfranz · 2018-03-18T19:44:21Z

modules/auth/oauth2/oauth2.go

-	store := sessions.NewFilesystemStore(sessionDir, []byte(sessionUsersStoreKey))
+func Init(x *xorm.Engine) {
+	store := xormstore.NewOptions(x, xormstore.Options{
+		TableName: "oauth2_sessions",


IMHO we should use oauth2_session instead of oauth2_sessions because we currently use the singular for table names.

lafriks · 2018-03-18T22:31:10Z

@JonasFranzDEV ~~~did you clear cookies (could be that some old cookie still be left that was stored in file system)?~~~ try again, there was not updated goth libs that actually fixed github authorization

jonasfranz · 2018-03-19T10:22:49Z

vendor/github.com/lafriks/xormstore/xormstore.go

+}
+
+type xormSession struct {
+	ID          string         `xorm:"VARCHAR(400) PK NAME 'id'"`


VARCHAR(400) results in an error when I use mysql:

panic: Error 1071: Specified key was too long; max key length is 767 bytes goroutine 1 [running]: code.gitea.io/gitea/vendor/github.com/lafriks/xormstore.NewOptions(0xc4206680b0, 0x113ce57, 0xf, 0xc42067d600, 0xc4206fb8f0, 0x1, 0x1, 0x0) /mnt/local_storage/jonasfranz/Go/src/code.gitea.io/gitea/vendor/github.com/lafriks/xormstore/xormstore.go:108 +0x2d2 code.gitea.io/gitea/modules/auth/oauth2.Init(0xc4206680b0) /mnt/local_storage/jonasfranz/Go/src/code.gitea.io/gitea/modules/auth/oauth2/oauth2.go:44 +0xb9 code.gitea.io/gitea/models.InitOAuth2() /mnt/local_storage/jonasfranz/Go/src/code.gitea.io/gitea/models/oauth2.go:101 +0x3f code.gitea.io/gitea/routers.GlobalInit() /mnt/local_storage/jonasfranz/Go/src/code.gitea.io/gitea/routers/init.go:63 +0x56e code.gitea.io/gitea/cmd.runWeb(0xc420344000, 0x0, 0x0) /mnt/local_storage/jonasfranz/Go/src/code.gitea.io/gitea/cmd/web.go:83 +0xa8 code.gitea.io/gitea/vendor/github.com/urfave/cli.HandleAction(0xfb0900, 0x117e690, 0xc420344000, 0xc42020f440, 0x0) /mnt/local_storage/jonasfranz/Go/src/code.gitea.io/gitea/vendor/github.com/urfave/cli/app.go:471 +0xad code.gitea.io/gitea/vendor/github.com/urfave/cli.(*App).Run(0xc4200d5040, 0xc420030190, 0x1, 0x1, 0x0, 0x0) /mnt/local_storage/jonasfranz/Go/src/code.gitea.io/gitea/vendor/github.com/urfave/cli/app.go:246 +0x542 main.main() /mnt/local_storage/jonasfranz/Go/src/code.gitea.io/gitea/main.go:52 +0x40e

The error is returned at line 106 and is actually not handled!

When I change this to VARCHAR(200) for example, no error is thrown.

jonasfranz · 2018-03-19T10:24:50Z

@lafriks Now the authentication works (except the error reported (mysql)) but the table is empty. Is that an excepted behaviour?

lafriks · 2018-03-19T11:04:57Z

@JonasFranzDEV it depends on goth lib usage and in how it is currently done in gitea it seams to store session only between when you redirect to oauth2 provider and when you get session back.

…zation error

lafriks · 2018-04-09T10:54:15Z

@lunny @JonasFranzDEV fixed

lafriks · 2018-04-29T06:04:11Z

Make LG-TM work

lunny · 2018-04-29T06:06:01Z

@lafriks this pr is update to latest master commit. Code Review is passed already.

lafriks added the type/enhancement An improvement of existing functionality label Mar 12, 2018

lafriks added this to the 1.5.0 milestone Mar 12, 2018

tboerger added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Mar 12, 2018

lunny requested changes Mar 13, 2018

View reviewed changes

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 Mar 13, 2018

jonasfranz suggested changes Mar 18, 2018

View reviewed changes

jonasfranz suggested changes Mar 19, 2018

View reviewed changes

Store OAuth2 session data in database

6858da3

lafriks force-pushed the feat/xormstore_oath2_sessions branch from eb61efd to 6858da3 Compare April 9, 2018 10:48

Rename table to oauth2_session and do not skip xormstorage initiali…

645229b

…zation error

lunny approved these changes Apr 9, 2018

View reviewed changes

jonasfranz approved these changes Apr 10, 2018

View reviewed changes

bkcsoft 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 Apr 10, 2018

lunny and others added 3 commits April 11, 2018 08:54

Merge branch 'master' into feat/xormstore_oath2_sessions

49dc3e4

Merge branch 'master' into feat/xormstore_oath2_sessions

01cc606

Merge branch 'master' into feat/xormstore_oath2_sessions

542efa7

lafriks merged commit 5a62eb3 into go-gitea:master Apr 29, 2018

lafriks deleted the feat/xormstore_oath2_sessions branch April 29, 2018 06:09

go-gitea locked and limited conversation to collaborators Nov 24, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Store OAuth2 session data in database #3660

Store OAuth2 session data in database #3660

Uh oh!

lafriks commented Mar 12, 2018

Uh oh!

codecov-io commented Mar 12, 2018 •

edited

Loading

Uh oh!

lunny Mar 13, 2018

Uh oh!

lunny commented Mar 13, 2018

Uh oh!

lafriks commented Mar 13, 2018

Uh oh!

lunny commented Mar 13, 2018

Uh oh!

jonasfranz left a comment

Uh oh!

jonasfranz Mar 18, 2018

Uh oh!

lafriks commented Mar 18, 2018 •

edited

Loading

Uh oh!

jonasfranz Mar 19, 2018

Uh oh!

jonasfranz commented Mar 19, 2018

Uh oh!

lafriks commented Mar 19, 2018

Uh oh!

lafriks commented Apr 9, 2018

Uh oh!

lafriks commented Apr 29, 2018

Uh oh!

lunny commented Apr 29, 2018

Uh oh!

Uh oh!

Uh oh!

Store OAuth2 session data in database #3660

Store OAuth2 session data in database #3660

Uh oh!

Conversation

lafriks commented Mar 12, 2018

Uh oh!

codecov-io commented Mar 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

lunny Mar 13, 2018

Choose a reason for hiding this comment

Uh oh!

lunny commented Mar 13, 2018

Uh oh!

lafriks commented Mar 13, 2018

Uh oh!

lunny commented Mar 13, 2018

Uh oh!

jonasfranz left a comment

Choose a reason for hiding this comment

Uh oh!

jonasfranz Mar 18, 2018

Choose a reason for hiding this comment

Uh oh!

lafriks commented Mar 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonasfranz Mar 19, 2018

Choose a reason for hiding this comment

Uh oh!

jonasfranz commented Mar 19, 2018

Uh oh!

lafriks commented Mar 19, 2018

Uh oh!

lafriks commented Apr 9, 2018

Uh oh!

lafriks commented Apr 29, 2018

Uh oh!

lunny commented Apr 29, 2018

Uh oh!

Uh oh!

codecov-io commented Mar 12, 2018 •

edited

Loading

lafriks commented Mar 18, 2018 •

edited

Loading