Synchronize SSH keys on login with LDAP + Fix SQLite deadlock on ldap ssh key deletion

[zeripath]

This PR synchronises SSH keys on login with LDAP.

It could do with a test from an LDAP user.

[zeripath]

Should fix #5545

[NomAnor]

I did a quick test and removed my user from gitea and then logged in. The ssh key was added.
Then I removed the key from my account and relogged and it was added again.
Also works with multiple keys. It looks to be doing everything I want. Thanks for the fast response.

Maybe add the 'sshPublicKey' attribute to the log.Trace("Fetching attributes '%v', '%v', '%v', '%v', '%v' with filter %s and base %s", ...) call

[zeripath]

@NomAnor cool.

It looks like the changes have broken some tests so I need to look at those and re-push.
I'll look at changing the debug note too.

[zeripath]

OK, so this needs a bit more thought... There's a few issues with it.

1. models/user.go:deleteKeysMarkedForDeletion(...) creates a new Xorm session - this causes a hang on sqlite - so I think I will need to change that and figure out why it's felt that a new session is required.
2. @NomAnor, @sapk and @lafriks - Is it intended that if you're using the LDAP to provide SSH Keys that any changes you make to SSH Keys in gitea will be overwritten with the LDAP ones? That is, even if you put an SSH key in to gitea, if it's not there in the LDAP, next sync it will disappear - this is the current code it's just somewhat opaque because you don't see it on next login and it only happens when the next resynchronization occurs.

If 2. is not the expected behaviour then there is a bug in Gitea, and if it is expected then our testcases are actually wrong because they rely on a race (albeit a race that should never be lost.)

I'll assume that 2. is the intended behaviour and fix the bug in 1 and the respective testcases.

[zeripath]

OK, problem 1. is definitely a bug with the current implementation - Any sqlite installation will hang if they have to remove keys during a synchronisation.

[NomAnor]

I changed the full name of my account an relogged and it was not overwritten. Then I started the synchronization as administrator and it was changed. So the current behaviour of gitea seem to be that account details are only overwritten when the synchronization runs and initially filled out on first login without an existing account (Maybe document that relogging does currently not synchronize account information).

The current ssh key implementation does not adhear to this behaviour and synchronizes on every login. I think it would be better if all external authentication plugins and all account attributes work the same way.

If someone uses external account management then all information available should be overwritten so the account management is the definite source of information. From a user perspective I think it would be good that this synchronization also happens on every login (that might have performance issues on big installations; make it configurable?). In this case the overwritten user settings should be disabled (like the username for external accounts).

I'm not in a corporate environment so their requirements might be different.

Another problem might occour if someone does not want to store ssh keys in the ldap directory. Then the keys are deleted by the synchronization because the sshPublicKey Attribute is absent. Only delete the keys if the sshPublicKey Attribute is present but empty?

[codecov-io]

Codecov Report

Merging #5557 into master will increase coverage by 0.08%.
The diff coverage is 70.96%.

@@            Coverage Diff             @@
##           master    #5557      +/-   ##
==========================================
+ Coverage   37.51%   37.59%   +0.08%     
==========================================
  Files         322      322              
  Lines       47323    47337      +14     
==========================================
+ Hits        17751    17797      +46     
+ Misses      27021    26983      -38     
- Partials     2551     2557       +6

Impacted Files	Coverage Δ
modules/auth/ldap/ldap.go	51.62% <100%> (+0.39%)	⬆️
models/ssh_key.go	40.43% <100%> (+1.39%)	⬆️
models/user.go	48.03% <50%> (+2.06%)	⬆️
models/login_source.go	26.3% <50%> (+0.38%)	⬆️
modules/util/compare.go	45.45% <0%> (+30.3%)	⬆️

---

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 2058c36...b9ddddc. Read the comment docs.

[zeripath]

Hi @NomAnor. I think you're right - if you're going to have externally managed sshkeys then they should be externally managed. I could imagine a use-case whereby you want to allow people to add their own, but then if you go down that route I can imagine a use-case whereby you want you to be able to feed back in to the LDAP. And so on, and so on...

I guess the thing is, although this PR doesn't actually change the current policy it certainly makes it more apparent...

(I've fixed the problem with the test cases and the deadlock bug in sqlite now.)

[zeripath]

OK, I was wrong about gitea removing all your keys - just those that have names "ldap-" which are no longer in the ldap. If you delete an ldap key when you log back in it will be restored so that's fine too.

I guess that means this is ready for review.