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.

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

New Field Request: Ability to set Profile Pic with LDAP #4144

Closed
benyanke opened this issue Jun 6, 2018 · 34 comments · Fixed by #16851
Closed

New Field Request: Ability to set Profile Pic with LDAP #4144

benyanke opened this issue Jun 6, 2018 · 34 comments · Fixed by #16851
Labels
type/enhancement An improvement of existing functionality type/proposal The new feature has not been accepted yet but needs to be discussed first.

Comments

@benyanke
Copy link
Contributor

benyanke commented Jun 6, 2018

It would be neat to be able to set profile picture with LDAP. I suppose this would be best implemented as a URL, but I know there are multiple ways of implementing images in ldap.

@lunny lunny added type/enhancement An improvement of existing functionality type/proposal The new feature has not been accepted yet but needs to be discussed first. labels Jun 6, 2018
@tcs-ulli
Copy link

gogs can this maybe its easy to copy that feature ?

@lunny
Copy link
Member

lunny commented Nov 21, 2018

@tcs-ulli could you point where is the code base?

@mq2035
Copy link

mq2035 commented Mar 14, 2019

In openldap attribute name is jpegPhoto (from inetOrgPerson), it is binary and it is stored in LDAP.

working code example (in java)
https://github.com/xwiki-contrib/ldap/tree/master/ldap-authenticator

@dl5rcw
Copy link

dl5rcw commented Jan 18, 2020

Using fusiondirectory, the jpegPhoto as mentioned by @mq2035 really makes sense. It should be updated like the other entries to pull changes from ldap.

@helmut72
Copy link

Yes, would be good, as I have stored jpegPhoto in my Samba4/LDAP server. Many Applications use the photo from LDAP server, which makes sense.

@lafriks
Copy link
Member

lafriks commented Feb 12, 2020

Only problem could be with sync, how to check and download images only if they have been changed to not download all user images on every sync

@benyanke
Copy link
Contributor Author

benyanke commented Feb 13, 2020

That would be an additional piece needed. Most apps I've seen do it on individual login, and then refetch on an administrator's cache clear, or when a scheduled job runs. Ideally it's something you could cache a day or two, since pictures don't change often, especially if you're updating on the login event.

@helmut72
Copy link

Only problem could be with sync, how to check and download images only if they have been changed to not download all user images on every sync

Let the user decide in his settings.

@lafriks
Copy link
Member

lafriks commented Feb 19, 2020

I was just thinking if there is more optimal way to check if image has changed

@helmut72
Copy link

helmut72 commented Sep 1, 2020

It would be a good start if there is just a sync every x day/hour setting with 0 to disable. Then it's the administrators choice. Better than no ldap photo sync.

@nedvedad
Copy link

Or check/download avatar on each user login.

@SonGokussj4
Copy link

Any progress with this? I just got LDAP to work after days of fighting with it but no pictures sadly.

@zeripath
Copy link
Contributor

I have put up a PR. It could do with some testing from people with working LDAPs.

zeripath added a commit to zeripath/gitea that referenced this issue Aug 27, 2021
Add setting to LDAP source to allow it to provide an Avatar.

Currently this is required to point to the image bytes.

Fix go-gitea#4144

Signed-off-by: Andrew Thornton <art27@cantab.net>
@SonGokussj4
Copy link

Any chance for me to try that in Docker? I'm using v1.15.0 docker image. (centos 7) Can't try this as local installation.

@zeripath
Copy link
Contributor

zeripath commented Sep 1, 2021

OK I've pushed up a docker image based on 1.16 (v1.16.0-dev-195-gd01836f4f) - this PR can't be backported to 1.15 without significant work (and as it would be a feature would be unlikely to be merged.)

https://www.eldritchkitty.com/~andrew/gitea-1.16.0-dev-195-pr-16851.tar.gz

if the image needs updating please discuss with me on discord or matrix. Similarly if a backport to 1.15 is wanted.

@SonGokussj4
Copy link

Okay so if I want to just try that and then go back I should not just rename image in docker-compose.yml from gitea:1.15 to gitea:1.16-dev (as I renamed your docker) (and there back to 1.15)

I should stop my current Gitea instance, copy folder with data and try your docker image on that?

@zeripath
Copy link
Contributor

zeripath commented Sep 2, 2021

Ah you want to use your current data!

You would need to backup or copy your db as there are some database migrations.

(There's a small bug in 1.15.0 dump which will break login_source so you would need to use your db backups or wait for 1.15.1 - which is imminent)

@SonGokussj4
Copy link

I would try that in production in my company so... To be safe, I will wait :-) So when 1.15.1 is out, I will update to that, then try to use your 1.16.0-dev, report functionality, then I can go back to 1.15.1?

@SonGokussj4
Copy link

1.15.1 here. Should I wait for your docker build and go back to 1.15.1 after I try it? Or I have to duplicate my environment and try that on duplicated DB to be sure.

@zeripath
Copy link
Contributor

zeripath commented Sep 3, 2021

You will need to back up your db, duplicate it and use the image I provided above.

The reason is that once you use a 1.16-dev build your db is migrated and so you would not be a able to go back to 1.15.

@SonGokussj4
Copy link

Finally at work. Did try your image but error happened. Are you sure (did you try it?) it works?

# I've imported your image
docker import https://www.eldritchkitty.com/~andrew/gitea-1.16.0-dev-195-pr-16851.tar.gz

# Renamed
docker image tag b63236d14157 gitea/gitea:1.16.0-dev

# Modified docker-compose.yml
services:
  server:
    image: gitea/gitea:1.16.0-dev

# Started
$ docker-compose up
gitea_ldap_test_db_1 is up-to-date
Creating gitea_ldap_test_server_1 ... error

ERROR: for gitea_ldap_test_server_1  Cannot create container for service server: b'No command specified'

ERROR: for server  Cannot create container for service server: b'No command specified'
ERROR: Encountered errors while bringing up the project.

@zeripath
Copy link
Contributor

zeripath commented Sep 6, 2021

Sigh.

Works for me.

Not being a docker expert I don't know how to fix this or why it has occurred - at worst the image will contain a gitea binary which at worst you can copy over another image.

@SonGokussj4
Copy link

SonGokussj4 commented Sep 7, 2021

Docker inspect gitea/gitea:1.16.0-dev, it will give me {"Config": {"Cmd": null, ...}, ...}
When I googled the problem it gave me "you have no Cmd in dockerfile".

Edit: after inspecting original 1.15 image, it is way too different. So my thoughts are it was not built correctly.

@zeripath
Copy link
Contributor

zeripath commented Sep 8, 2021

It'll be the way it was exported. Clearly docker export did not work and is not the correct thing to do. I'm not certain how to make that work properly.

I have pushed up an xz compressed binary to https://www.eldritchkitty.com/~andrew/gitea-v1.16.0-dev-219-g4b6e023a1-pr-16851-linux-amd64.xz which once decompressed you can use to replace the /app/gitea/gitea binary within a working docker.

@SonGokussj4
Copy link

Thanks. I copied it to a running container and there is a new field "Avatar Attribute" and entered "thumbnailPhoto"

My ldapsearch -x -W -D xxx shows:

mail: mymail@MYCOMPANY.CZ
mobile: +420123456789
thumbnailPhoto:: /9j/4AAQSkZJRgABAQEA8ADwAAD/4R1kRXhpZgAATU0AKgAAAAgACwEPAAIAA
 AASAAAAkgEQAAIAAAAKAAAApAEaAAUAAAABAAAArgEbAAUAAAABAAAAtgEoAAMAAAABAAIAAAExAA
...
  • Updated Authentication Source.
  • Tried logout, login
  • Tried Admin --> Monitoring --> Synchronize external user data
  • Tried to change from User Profile --> Avatar --> Look Up Avatar by Email Address to Use Custom Avatar and back

Still the random avatar. What should I do so that it will pull it out from LDAP?

https://i.imgur.com/zA2LxYh.png

@zeripath
Copy link
Contributor

zeripath commented Sep 8, 2021

hmm... the avatar was only being updated if something else was updated on the user - I've changed that.

The other question is whether your ldap is giving us the data as []byte or as a base64 encoded string. If it's the latter then it's not currently handling that.

https://www.eldritchkitty.com/~andrew/gitea-v1.16.0-dev-225-g929885e0c-pr-16851-linux-amd64.xz

@SonGokussj4
Copy link

If I enter my thumbnailPhoto into https://codebeautify.org/base64-to-image-converter, it gives me my picture. So I think (and I can't change that) it's base64.

@zeripath
Copy link
Contributor

@SonGokussj4 did you try the update above?

@SonGokussj4
Copy link

Hi. No I didn't. We use base64 for image and you wrote it doesn't handle the base64 so I didn't even try.

@zeripath
Copy link
Contributor

I don't think you can assume that you're using base64 from that output. If you look at your output above:

thumbnailPhoto:: /9j/4AAQSkZJRgABAQEA8ADwAAD/4R1kRXhpZgAATU0AKgAAAAgACwEPAAIAA
 AASAAAAkgEQAAIAAAAKAAAApAEaAAUAAAABAAAArgEbAAUAAAABAAAAtgEoAAMAAAABAAIAAAExAA
...

There is a double : after the field's name. That could imply that the object is stored as []byte but it's being rendered as base64. My code explicitly asks for the bytes. Now you were previously expecting synchronization to occur - it wasn't because of an oversight - but it does happen now. So it would be helpful to know if my interpretation above is correct or if I have to read the data and detect it's base64 and then do a conversion.

Because if I have to convert for you - it is very likely that there will have to be conversions for others.

If I don't then that's fine and I can save 200 lines of code messing around with dealing with this issue.

@SonGokussj4
Copy link

That makes sense. Thanks for the explanation.
I'll try that right away.

@SonGokussj4
Copy link

SonGokussj4 commented Sep 14, 2021

So I updated the file, restarted gitea docker-compose restart, entered thumbnailPhoto, then tried log-out, log-in, synchronize data, nothing.
Then I downed docker docker-compose down and started it again docker-compose up -d, logs show me this:

server_1  | Downgrading database version from '194' to '189' is not supported and may result in loss of data integrity.
server_1  | If you really know what you're doing, execute `UPDATE version SET version=189 WHERE id=1;`
server_1  | Received signal 15; terminating.
gitea_ldap_test_server_1 exited with code 0

Edit:
Ignore the above. I repeated the copying (I forgot that when I d-c down my container, it will replace all default files...)
I filled the Avatar Attribute field but nothing happens.
Interesting enough, I couldn't log back in.
The log was:

Completed POST /user/login 500 Internal Server Error

Not sure what's happening.

@zeripath
Copy link
Contributor

zeripath commented Sep 15, 2021

OK I think I've got it working completely now! (Discovered our test ldap actually had jpegPhoto attributes already so was able to use that.) I wasn't actually passing the attribute to the LDAP request.

https://www.eldritchkitty.com/~andrew/gitea-v1.16.0-dev-262-g5f7352645-pr-16851-linux-amd64.xz

@SonGokussj4
Copy link

SonGokussj4 commented Sep 20, 2021

Hi, sorry for the delay, I wasn't at work till now. And...

It WORKS!!

I had to do Monitoring -> Synchronize external user data but I think the newly added user should have his picture there. Will through today.

Edit: New user logged in and his picture was automatically applied. So nice!
If this could be a functional PR and have it in the next version, that would be awesome.

techknowlogick added a commit that referenced this issue Sep 27, 2021
* Allow LDAP Sources to provide Avatars

Add setting to LDAP source to allow it to provide an Avatar.

Currently this is required to point to the image bytes.

Fix #4144

Signed-off-by: Andrew Thornton <art27@cantab.net>

* Rename as Avatar Attribute (drop JPEG)

Signed-off-by: Andrew Thornton <art27@cantab.net>

* Always synchronize avatar if there is change

Signed-off-by: Andrew Thornton <art27@cantab.net>

* Actually get the avatar from the ldap

Signed-off-by: Andrew Thornton <art27@cantab.net>

* clean-up

Signed-off-by: Andrew Thornton <art27@cantab.net>

* use len()>0 rather than != ""

Signed-off-by: Andrew Thornton <art27@cantab.net>

* slight shortcut in IsUploadAvatarChanged

Signed-off-by: Andrew Thornton <art27@cantab.net>

Co-authored-by: techknowlogick <techknowlogick@gitea.io>
@go-gitea go-gitea locked and limited conversation to collaborators Apr 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
type/enhancement An improvement of existing functionality type/proposal The new feature has not been accepted yet but needs to be discussed first.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants