Fix SSH auth lfs locks #3152

sapk · 2017-12-11T13:34:27Z

Mostly don't reinvent the wheel to fix #3084 by re-using func of lfs.

sapk · 2017-12-11T23:11:55Z

I extract a function parseToken of original authenticateToken and add user reference to token claim to be able to fill ctx.User if needed for creating or deleting locks.

codecov-io · 2017-12-11T23:27:21Z

Codecov Report

Merging #3152 into master will decrease coverage by 0.08%.
The diff coverage is 42.95%.

@@            Coverage Diff             @@
##           master    #3152      +/-   ##
==========================================
- Coverage   35.66%   35.57%   -0.09%     
==========================================
  Files         281      281              
  Lines       40552    40579      +27     
==========================================
- Hits        14463    14437      -26     
- Misses      23957    24001      +44     
- Partials     2132     2141       +9

Impacted Files	Coverage Δ
cmd/serv.go	`0% <0%> (ø)`	⬆️
modules/lfs/server.go	`39.07% <26.47%> (+1.4%)`	⬆️
modules/lfs/locks.go	`47.73% <58.53%> (-8%)`	⬇️
models/lfs_lock.go	`63.21% <65%> (-6.55%)`	⬇️
models/error.go	`32.73% <85.71%> (+0.3%)`	⬆️
modules/indexer/indexer.go	`70% <0%> (-7.5%)`	⬇️
models/repo_indexer.go	`41.15% <0%> (-5.76%)`	⬇️
models/repo_list.go	`65.62% <0%> (-1.57%)`	⬇️
models/repo.go	`43.25% <0%> (-0.19%)`	⬇️
... and 1 more

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 97fe773...2036540. Read the comment docs.

lafriks · 2017-12-11T23:36:39Z

LGTM

flozz · 2017-12-12T10:36:52Z

The authentication seems to work for the lock feature, but I am not able to push LFS object anymore:

$ git lfs push origin master
Git LFS: (0 of 1 files) 0 B / 8 B                                                                        
batch response: Fatal error: Server error: http://git2.XXX.YYY/ZZZ/test-lfs.git/info/lfs/objects/batch

Errors logged to /tmp/test-lfs/.git/lfs/objects/logs/20171212T112833.900447349.log
Use `git lfs logs last` to view the log.

From gitea log:

Dec 12 11:28:33 XXX gitea[44851]: [Macaron] 2017-12-12 11:28:33: Completed POST /ZZZ/test-lfs.git/info/lfs/objects/batch 500 Internal Server Error in 1.880036ms

lafriks

See comment above :)

sapk · 2017-12-12T12:25:12Z

@flozz thanks for the testing. Will look at it

ethantkoenig · 2017-12-14T05:30:20Z

models/lfs_lock.go

 	l.Path = cleanPath(l.Path)
 }

 // AfterLoad is invoked from XORM after setting the values of all fields of this object.
 func (l *LFSLock) AfterLoad() {
 	l.Owner, _ = GetUserByID(l.OwnerID)
+	l.Repo, _ = GetRepositoryByID(l.RepoID)


Two thoughts:

We shouldn't ignore errors here; we should at least log them.

We should double-check that loading the Owner and Repo every time we read from the database is necessary. IMO, it seems a bit wasteful

I added repo during dev because I needed it at one time but this could be revert to only use repoID if it to much additional workload.

ethantkoenig · 2017-12-14T05:31:00Z

models/lfs_lock.go

@@ -71,15 +74,15 @@ func CreateLFSLock(lock *LFSLock) (*LFSLock, error) {
 }

 // GetLFSLock returns release by given path.
-func GetLFSLock(repoID int64, path string) (*LFSLock, error) {
+func GetLFSLock(repo *Repository, path string) (*LFSLock, error) {


Why this change? Is it really worth passing the repository if the only thing we care about it the id?

Now I remember it is to use HasAccess that need a repo reference.

Maybe I could make a func accessLevel and hasAccess that use repoID more generally for better perf ? (I think in a other PR ?)

Ah, I must have missed the call to CheckLFSAccessForRepo. Yes, perhaps in another PR

In fact after reviewing code of accessLevel, it use repo.IsPrivate that need a repo reference so.

ethantkoenig · 2017-12-14T05:34:57Z

modules/lfs/locks.go

 )

-func checkRequest(req macaron.Request, post bool) int {
+func checkRequest(ctx *context.Context, post bool) int {


Would it be cleaner to write responses to ctx, instead of returning the response status? Right now, the person calling this function needs to know that 200 is a special return value (which isn't documented anywhere)

I have wrote the method.

ethantkoenig · 2017-12-14T05:43:20Z

models/lfs_lock.go

@@ -123,24 +126,20 @@ func DeleteLFSLockByID(id int64, u *User, force bool) (*LFSLock, error) {
 }

 //CheckLFSAccessForRepo check needed access mode base on action
-func CheckLFSAccessForRepo(u *User, repoID int64, action string) error {
+func CheckLFSAccessForRepo(u *User, repo *Repository, reqWrt bool) error {


Maybe it's cleaner to pass the AccessMode directly, instead of a bool?

lafriks · 2017-12-22T06:09:43Z

integrations/pgsql.ini.tmpl

@@ -26,7 +26,8 @@ SSH_DOMAIN       = localhost
 HTTP_PORT        = 3002
 ROOT_URL         = http://localhost:3002/
 DISABLE_SSH      = false
-SSH_PORT         = 22
+SSH_LISTEN_HOST  = localhost
+SSH_PORT         = 2222


Do not use same port for multiple database tests as they are run in parallel

lafriks · 2017-12-22T12:51:21Z

seems that ssh port change requires update in tests

ethantkoenig · 2017-12-22T13:22:23Z

models/lfs_lock.go

@@ -12,27 +12,38 @@ import (
 	"time"

 	api "code.gitea.io/sdk/gitea"
+	"github.com/ngaut/log"


import format: see https://github.com/go-gitea/gitea/blob/master/CONTRIBUTING.md#styleguide

@ethantkoenig sorry I pass it as WIP seen I want to setup test for various git access to make that I don't break anything in future. It is my editor that setup this import, I will review my code once it works and remove WIP when ready. Sorry for the distrurb.

sapk · 2017-12-30T17:10:01Z

@flozz can you re-test? It should be good now

sapk · 2017-12-30T17:27:00Z

Ok found my mistake in 2723ca5d82dee097e8fa4c0080a5492ec0bb44b8
This PR add integration test from git cli for http and ssh url for normal clone/commit but also lfs file and lfs lock.

sapk · 2018-01-16T08:48:57Z

Wait for #3377 to have tests before

* test: integration add git cli tests Extracted form for easing review process and debug #3152 * test: integration add git cli big file commit * fix: Don't rewrite key if internal server

sapk · 2018-01-16T21:31:14Z

All is working now. What fix pgsql tests should be inside #3377 or #3346.

lafriks · 2018-01-17T21:53:54Z

@ethantkoenig need your approval

lafriks · 2018-01-20T20:10:31Z

@ethantkoenig we need to release 1.4.0 soon, can you please review this?

ethantkoenig

Apologies for delay

ethantkoenig · 2018-01-20T20:30:47Z

models/lfs_lock.go

 	l.Path = cleanPath(l.Path)
 }

 // AfterLoad is invoked from XORM after setting the values of all fields of this object.
 func (l *LFSLock) AfterLoad() {
-	l.Owner, _ = GetUserByID(l.OwnerID)
+	var err error
+	l.Owner, err = GetUserByID(l.OwnerID)


Will performing database queries inside AfterLoad lead to deadlock problems if the query that triggered AfterLoad is part of a transaction? See #1813

@ethantkoenig queries should not be affected by locking and will load last committed data even if update/insert is in progress

I see. In #1813 the offending SQL was a write which blocked on an ongoing transaction, but if you're sure that reading won't block on ongoing transactions then this is okay.

You should use

func (l *LFSLock) AfterLoad(sess *xorm.Session) { }

to avoid that.

lunny

Please follow my suggestion.

lafriks · 2018-01-26T20:41:51Z

@lunny need your approval

sapk force-pushed the fix-ssh-auth-lfs-locks branch 3 times, most recently from a1f9481 to 7205109 Compare December 11, 2017 22:55

sapk changed the title ~~[WIP] Fix SSH auth lfs locks~~ Fix SSH auth lfs locks Dec 11, 2017

tboerger added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Dec 11, 2017

sapk force-pushed the fix-ssh-auth-lfs-locks branch from 3b60454 to 894d1ea Compare December 11, 2017 23:20

lafriks added the type/bug label Dec 11, 2017

lafriks added this to the 1.4.0 milestone Dec 11, 2017

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 Dec 11, 2017

lafriks requested changes Dec 12, 2017

View reviewed changes

ethantkoenig suggested changes Dec 14, 2017

View reviewed changes

sapk changed the title ~~Fix SSH auth lfs locks~~ [WIP] Fix SSH auth lfs locks Dec 18, 2017

sapk force-pushed the fix-ssh-auth-lfs-locks branch 2 times, most recently from 32caa14 to 2155d57 Compare December 20, 2017 22:50

lafriks requested changes Dec 22, 2017

View reviewed changes

ethantkoenig suggested changes Dec 22, 2017

View reviewed changes

sapk force-pushed the fix-ssh-auth-lfs-locks branch 2 times, most recently from 71eb418 to 8116752 Compare December 30, 2017 16:49

sapk changed the title ~~[WIP] Fix SSH auth lfs locks~~ Fix SSH auth lfs locks Dec 30, 2017

sapk force-pushed the fix-ssh-auth-lfs-locks branch 2 times, most recently from 31c8e64 to 5d8d6c6 Compare December 30, 2017 18:55

sapk force-pushed the fix-ssh-auth-lfs-locks branch 4 times, most recently from cfbd6e3 to e658af2 Compare January 16, 2018 20:39

Fix SSH auth LFS locks

ac9e714

sapk force-pushed the fix-ssh-auth-lfs-locks branch from e658af2 to ac9e714 Compare January 16, 2018 20:40

sapk changed the title ~~[WIP] Fix SSH auth lfs locks~~ Fix SSH auth lfs locks Jan 16, 2018

Activate SSH/lock test

bad1991

sapk force-pushed the fix-ssh-auth-lfs-locks branch from c0c0fda to bad1991 Compare January 16, 2018 21:10

Remove debug

ef09252

lafriks approved these changes Jan 16, 2018

View reviewed changes

ethantkoenig approved these changes Jan 20, 2018

View reviewed changes

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 20, 2018

ethantkoenig reviewed Jan 20, 2018

View reviewed changes

Merge branch 'master' into fix-ssh-auth-lfs-locks

faf2196

lunny requested changes Jan 22, 2018

View reviewed changes

Follow @lunny recommendation for AfterLoad method

5064568

lunny approved these changes Jan 27, 2018

View reviewed changes

lafriks added 2 commits January 27, 2018 13:53

Merge branch 'master' into fix-ssh-auth-lfs-locks

c8fce15

Merge branch 'master' into fix-ssh-auth-lfs-locks

2036540

lafriks merged commit 9e842c8 into go-gitea:master Jan 27, 2018

sapk deleted the fix-ssh-auth-lfs-locks branch May 19, 2018 23:56

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

Fix SSH auth lfs locks #3152

Fix SSH auth lfs locks #3152

Conversation

sapk commented Dec 11, 2017 • edited Loading

sapk commented Dec 11, 2017

codecov-io commented Dec 11, 2017 • edited Loading

Codecov Report

lafriks commented Dec 11, 2017

flozz commented Dec 12, 2017 • edited Loading

lafriks left a comment

Choose a reason for hiding this comment

sapk commented Dec 12, 2017

ethantkoenig Dec 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lafriks commented Dec 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sapk commented Dec 30, 2017

sapk commented Dec 30, 2017

sapk commented Jan 16, 2018

sapk commented Jan 16, 2018 • edited Loading

lafriks commented Jan 17, 2018

lafriks commented Jan 20, 2018

ethantkoenig left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lunny left a comment

Choose a reason for hiding this comment

lafriks commented Jan 26, 2018

sapk commented Dec 11, 2017 •

edited

Loading

codecov-io commented Dec 11, 2017 •

edited

Loading

flozz commented Dec 12, 2017 •

edited

Loading

ethantkoenig Dec 14, 2017 •

edited

Loading

sapk commented Jan 16, 2018 •

edited

Loading