search

soudis / discoursesso

Nextcloud App providing Discourse SSO
GNU Affero General Public License v3.0
15 stars 5 forks source link

Add avatar_url #23

Closed fabiiretro closed 3 years ago

fabiiretro commented 3 years ago

I added support for avatar_urls in the format url/{username}. I tried to add comments for things I added. I also fixed a small typo in your php /template/admin.php said on L28 (now 29) "whitepsaces".

Small addition, but in my opinion nice to have: There's now this "i" Icon next to the header similar to Two-Factor Authentication which redirects to your repo (borrowed from the nextcloud/server repo)

Fully tested with NC 21 and latest Discourse

fabiiretro commented 3 years ago

I accidentally re-committed some of your older commits while trying to correctly sign off my pull request. Sorry for that

soudis commented 3 years ago

Thx for the contribution. I merged the branch and tested it myself. However, before I release it, can you explain what the purpose of this is? I don't know about that avatar URL thing in discourse and how it works. Specifically: why to you need a fixed URL for every user be set by the discoursesso plugin? Can this not be configured in Discourse settings?

fabiiretro commented 3 years ago

With avatar_url set, Discourse downloads the avatars and displays them as if they were uploaded directly by the user to the forum. Without avatar_url, you're able to enable an external avatar service (Admin settings > Files > external system avatars url). However, with that you simply embed the avatar and every request for an avatar is redirected to that service with all parameters you might have set.

For example, we currently have "external system avatars url" enabled and configured rocket.chat as our avatar provider. Because we don't want to have this endpoint publicly available, we appended an auth token in Discourse. (This is propably also possible with NC but our LDAP avatar sync won't work there...) However, as the avatars are simply embedded, this token is visible to the enduser (not THAT bad in our case because this token is useless besides fetching avatars and our forum is private anyway).

avatar_url via this plugin probably also reduces traffic as the avatars get downloaded on login instead of being fetched with every request in Discourse. Besides that, this enables you to implement a (in my opinion) more streamlined form of LDAP avatar sync compared to the external avatar url.

Hope I could make my arguments clear. Feel free to ask for clarification, if needed.

soudis commented 3 years ago

Thx for clarification. I still think it is a little bit of a fringe requirement, but as you contributed the code yourself, I'm happy to release it. Will take a couple of hours to be online.