Merge pull request 'User avatars' (#223) from feature/user_avatars into master

Reviewed-on: #223
Reviewed-by: Greg <greg@noreply.kosmos.org>

app/components/app_catalog/web_app_icon_component.rb

@@ -9,13 +9,5 @@ module AppCatalog
         @image_url = image_url_for(web_app.apple_touch_icon)
       end
     end
-
-    def image_url_for(attachment)
-      if Setting.s3_enabled?
-        s3_image_url(attachment)
-      else
-        Rails.application.routes.url_helpers.rails_blob_path(attachment, only_path: true)
-      end
-    end
   end
 end

app/controllers/admin/users_controller.rb

@@ -22,7 +22,7 @@ class Admin::UsersController < Admin::BaseController
 
     @services_enabled = @user.services_enabled
 
-    @avatar = LdapManager::FetchAvatar.call(cn: @user.cn)
+    @ldap_avatar = LdapManager::FetchAvatar.call(cn: @user.cn)
   end
 
   # POST /admin/users/:username/invitations

app/controllers/avatars_controller.rb

@@ -0,0 +1,27 @@
+class AvatarsController < ApplicationController
+  def show
+    if user = User.find_by(cn: params[:username])
+      http_status :not_found and return unless user.avatar.attached?
+
+      sha256_hash = params[:hash]
+      format = params[:format]&.to_sym || :png
+      # size = params[:size]&.to_sym || :original
+
+      unless user.avatar.filename.to_s == "#{sha256_hash}.#{format}"
+        http_status :not_found and return
+      end
+
+      # TODO See note for avatar_variant in user model
+      # blob = if size == :original
+      #          user.avatar.blob
+      #        else
+      #          user.avatar_variant(size: size)&.blob
+      #        end
+
+      data = user.avatar.blob.download
+      send_data data, type: "image/#{format}", disposition: "inline"
+    else
+      http_status :not_found
+    end
+  end
+end

app/controllers/discourse/sso_controller.rb

@@ -8,6 +8,9 @@ class Discourse::SsoController < ApplicationController
     sso.email = current_user.email
     sso.username = current_user.cn
     sso.name = current_user.display_name
+    if current_user.avatar.attached?
+      sso.avatar_url = helpers.image_url_for(current_user.avatar)
+    end
     sso.admin = current_user.is_admin?
     sso.sso_secret = secret
 

app/controllers/settings_controller.rb

@@ -25,7 +25,7 @@ class SettingsController < ApplicationController
   def update
     @user.preferences.merge!(user_params[:preferences] || {})
     @user.display_name = user_params[:display_name]
-    @user.avatar_new   = user_params[:avatar]
+    @user.avatar_new   = user_params[:avatar_new]
     @user.pgp_pubkey   = user_params[:pgp_pubkey]
 
     if @user.save
@@ -34,7 +34,12 @@ class SettingsController < ApplicationController
       end
 
       if @user.avatar_new.present?
-        LdapManager::UpdateAvatar.call(dn: @user.dn, file: @user.avatar_new)
+        if store_user_avatar
+          LdapManager::UpdateAvatar.call(user: @user)
+        else
+          @validation_errors = @user.errors
+          render :show, status: :unprocessable_entity and return
+        end
       end
 
       if @user.pgp_pubkey && (@user.pgp_pubkey != @user.ldap_entry[:pgp_key])
@@ -162,7 +167,7 @@ class SettingsController < ApplicationController
 
     def user_params
       params.require(:user).permit(
-        :display_name, :avatar, :pgp_pubkey,
+        :display_name, :avatar_new, :pgp_pubkey,
         preferences: UserPreferences.pref_keys
       )
     end
@@ -184,4 +189,40 @@ class SettingsController < ApplicationController
       salt = BCrypt::Engine.generate_salt
       BCrypt::Engine.hash_secret(password, salt)
     end
+
+    def store_user_avatar
+      io = @user.avatar_new.tempfile
+      img_data = process_avatar(io)
+      tempfile = Tempfile.create
+      tempfile.binmode
+      tempfile.write(img_data)
+      tempfile.rewind
+
+      hash = Digest::SHA256.hexdigest(img_data)
+      ext = @user.avatar_new.content_type == "image/png" ? "png" : "jpg"
+      filename = "#{hash}.#{ext}"
+
+      if filename == @user.avatar.filename.to_s
+        @user.errors.add(:avatar, "must be a new file/picture")
+        false
+      else
+        key = "users/#{@user.cn}/avatars/#{filename}"
+        @user.avatar.attach io: tempfile, key: key, filename: filename
+        @user.save
+      end
+    end
+
+    def process_avatar(io)
+      processed = ImageProcessing::Vips
+        .source(io)
+        .resize_to_fill(400, 400)
+        .saver(strip: true)
+        .call
+      io.rewind
+      processed.read
+    rescue Vips::Error => e
+      Sentry.capture_exception(e) if Setting.sentry_enabled?
+      Rails.logger.error { "Image processing failed for avatar: #{e.message}" }
+      nil
+    end
 end

app/controllers/webfinger_controller.rb

@@ -33,6 +33,10 @@ class WebfingerController < WellKnownController
       links: []
     }
 
+    if @user.avatar.attached?
+      jrd[:links] += avatar_link
+    end
+
     if Setting.mastodon_enabled && @user.service_enabled?(:mastodon)
       # https://docs.joinmastodon.org/spec/webfinger/
       jrd[:aliases] += mastodon_aliases
@@ -47,6 +51,16 @@ class WebfingerController < WellKnownController
     jrd
   end
 
+  def avatar_link
+    [
+      {
+        rel: "http://webfinger.net/rel/avatar",
+        type: @user.avatar.content_type,
+        href: helpers.image_url_for(@user.avatar)
+      }
+    ]
+  end
+
   def mastodon_aliases
     [
       "#{Setting.mastodon_public_url}/@#{@user.cn}",

app/helpers/application_helper.rb

@@ -14,4 +14,19 @@ module ApplicationHelper
   def badge(text, color)
     tag.span text, class: "inline-flex items-center rounded-full bg-#{color}-100 px-2.5 py-0.5 text-xs font-medium text-#{color}-800"
   end
+
+  def image_url_for(attachment)
+    return s3_image_url(attachment) if Setting.s3_enabled?
+
+    if attachment.record.is_a?(User) && attachment.name == "avatar"
+      hash, format = attachment.blob.filename.to_s.split(".", 2)
+      user_avatar_url(
+        username: attachment.record.cn,
+        hash: hash,
+        format: format
+      )
+    else
+      Rails.application.routes.url_helpers.rails_blob_path(attachment, only_path: true)
+    end
+  end
 end

app/jobs/create_ldap_user_job.rb

@@ -4,7 +4,7 @@ class CreateLdapUserJob < ApplicationJob
   def perform(username:, domain:, email:, hashed_pw:, confirmed: false)
     dn = "cn=#{username},ou=#{domain},cn=users,dc=kosmos,dc=org"
     attr = {
-      objectclass: ["top", "account", "person", "extensibleObject"],
+      objectclass: ["top", "account", "person", "inetOrgPerson", "extensibleObject"],
       cn: username,
       sn: username,
       uid: username,

app/models/user.rb

@@ -4,8 +4,8 @@ class User < ApplicationRecord
   include EmailValidatable
 
   attr_accessor :current_password
-  attr_accessor :avatar_new
   attr_accessor :display_name
+  attr_accessor :avatar_new
   attr_accessor :pgp_pubkey
 
   serialize :preferences, coder: UserPreferences
@@ -27,6 +27,12 @@ class User < ApplicationRecord
 
   has_many :accounts, through: :lndhub_user
 
+  #
+  # Attachments
+  #
+
+  has_one_attached :avatar
+
   #
   # Validations
   #
@@ -50,6 +56,7 @@ class User < ApplicationRecord
   validates_length_of :display_name, minimum: 3, maximum: 35, allow_blank: true,
                                      if: -> { defined?(@display_name) }
 
+
   validate :acceptable_avatar
 
   validate :acceptable_pgp_key_format, if: -> { defined?(@pgp_pubkey) && @pgp_pubkey.present? }
@@ -77,6 +84,10 @@ class User < ApplicationRecord
          :timeoutable,
          :rememberable
 
+  #
+  # Methods
+  #
+
   def ldap_before_save
     self.email = Devise::LDAP::Adapter.get_ldap_param(self.cn, "mail").first
     self.ou    = dn.split(',')
@@ -159,6 +170,20 @@ class User < ApplicationRecord
     @display_name ||= ldap_entry[:display_name]
   end
 
+  # TODO Variant keys are currently broken for some reason
+  # (They use the same key as the main blob, when it should be
+  # "/variants/#{key)"
+  # def avatar_variant(size: :medium)
+  #   dimensions = case size
+  #                when :large  then [400, 400]
+  #                when :medium then [256, 256]
+  #                when :small  then [64, 64]
+  #                else [256, 256]
+  #                end
+  #   format = avatar.content_type == "image/png" ? :png : :jpeg
+  #   avatar.variant(resize_to_fill: dimensions, format: format)
+  # end
+
   def nostr_pubkey
     @nostr_pubkey ||= ldap_entry[:nostr_key]
   end
@@ -186,10 +211,6 @@ class User < ApplicationRecord
     ZBase32.encode(Digest::SHA1.digest(cn))
   end
 
-  def avatar
-    @avatar_base64 ||= LdapManager::FetchAvatar.call(cn: cn)
-  end
-
   def services_enabled
     ldap_entry[:services_enabled] || []
   end
@@ -227,7 +248,7 @@ class User < ApplicationRecord
       return unless avatar_new.present?
 
       if avatar_new.size > 1.megabyte
-        errors.add(:avatar, "file size is too large")
+        errors.add(:avatar, "must be less than 1MB file size")
       end
 
       acceptable_types = ["image/jpeg", "image/png"]

app/services/ldap_manager/fetch_avatar.rb

@@ -5,12 +5,12 @@ module LdapManager
     end
 
     def call
-      treebase   = ldap_config["base"]
+      treebase = ldap_config["base"]
       attributes = %w{ jpegPhoto }
-      filter     = Net::LDAP::Filter.eq("cn", @cn)
+      filter = Net::LDAP::Filter.eq("cn", @cn)
 
       entry = client.search(base: treebase, filter: filter, attributes: attributes).first
-      entry.try(:jpegPhoto) ? entry.jpegPhoto.first : nil
+      entry[:jpegPhoto].present? ? entry.jpegPhoto.first : nil
     end
   end
 end

app/services/ldap_manager/update_avatar.rb

@@ -2,26 +2,41 @@ require "image_processing/vips"
 
 module LdapManager
   class UpdateAvatar < LdapManagerService
-    def initialize(dn:, file:)
+    def initialize(user:)
-      @dn = dn
+      @user = user
-      @img_data = process(file)
+      @dn = user.dn
     end
 
     def call
-      replace_attribute @dn, :jpegPhoto, @img_data
+      unless @user.avatar.attached?
+        Rails.logger.error { "Cannot store empty jpegPhoto for user #{@user.cn}" }
+        return false
+      end
+
+      img_data = @user.avatar.blob.download
+      jpg_data = process_avatar
+
+      Rails.logger.debug { "Storing new jpegPhoto for user #{@user.cn} in LDAP" }
+      result = replace_attribute(@dn, :jpegPhoto, jpg_data)
+      result == 0
     end
 
     private
 
-    def process(file)
+    def process_avatar
-      processed = ImageProcessing::Vips
+      @user.avatar.blob.open do |file|
-        .resize_to_fill(512, 512)
+        processed = ImageProcessing::Vips
-        .source(file)
+          .source(file)
-        .convert("jpeg")
+          .resize_to_fill(256, 256)
-        .saver(strip: true)
+          .convert("jpeg")
-        .call
+          .saver(strip: true)
-
+          .call
-      Base64.strict_encode64 processed.read
+        processed.read
+      end
+    rescue Vips::Error => e
+      Sentry.capture_exception(e) if Setting.sentry_enabled?
+      Rails.logger.error { "Image processing failed for LDAP avatar: #{e.message}" }
+      nil
     end
   end
 end

app/views/admin/users/show.html.erb

@@ -95,8 +95,8 @@
           <tr>
             <th>Avatar</th>
             <td>
-              <% if @avatar.present? %>
+              <% if @ldap_avatar.present? %>
-                <img src="data:image/jpeg;base64,<%= @avatar %>" class="h-48 w-48" />
+                JPEG size: <%= @ldap_avatar.size %>
               <% else %>
                 &mdash;
               <% end %>

app/views/settings/_profile.html.erb

@@ -20,7 +20,7 @@
       </button>
     </p>
     <p class="text-sm text-gray-500">
-      Your user address for Chat and Lightning Network.
+      Your account's address on the Internet
     </p>
   </div>
   <%= form_for(@user, url: setting_path(:profile), html: { :method => :put }) do |f| %>
@@ -31,23 +31,19 @@
       <% end %>
     <% end %>
 
-    <% if Flipper.enabled?(:avatar_upload, current_user) %>
     <label class="block">
-      <p class="font-bold mb-1">
+      <p class="font-bold mb-1">Avatar</p>
-        Avatar
+      <p class="text-gray-500">Default profile picture</p>
-      </p>
-      <p class="text-gray-500">
-        Default profile picture
-      </p>
       <div class="flex items-center gap-6">
-        <% if current_user.avatar.present? %>
+        <% if @user.avatar.attached? %>
-        <p class="flex-none">
+          <p class="flex-none">
-          <%= image_tag "data:image/jpeg;base64,#{current_user.avatar}", class: "h-24 w-24 rounded-lg" %>
+            <%= image_tag image_url_for(@user.avatar), class: "h-24 w-24 rounded-lg" %>
-        </p>
+          </p>
         <% end %>
         <div class="grow">
           <p class="mb-2">
-          <%= f.file_field :avatar, class: "" %>
+            <%= f.file_field :avatar_new, accept: "image/jpeg,image/png" %>
+          </p>
           <p class="text-sm text-gray-500">
             JPEG or PNG image, not larger than 1 megabyte
           </p>
@@ -57,7 +53,6 @@
         </div>
       </div>
     </label>
-    <% end %>
 
     <p class="mt-8 pt-6 border-t border-gray-200 text-right">
       <%= f.submit 'Save', class: "btn-md btn-blue w-full md:w-auto" %>

config/routes.rb

@@ -15,6 +15,9 @@ Rails.application.routes.draw do
   match 'signup/:step', to: 'signup#steps', as: :signup_steps, via: [:get, :post]
   post 'signup_validate', to: 'signup#validate'
 
+
+  get "users/:username/avatars/:hash", to: "avatars#show", as: :user_avatar
+
   namespace :contributions do
     root to: 'donations#index'
     resources :donations, only: ['index', 'create'] do

spec/features/settings/profile_spec.rb

@@ -2,18 +2,18 @@ require 'rails_helper'
 
 RSpec.describe 'Profile settings', type: :feature do
   let(:user) { create :user, cn: "mwahlberg" }
-  let(:avatar_base64) { File.read("#{Rails.root}/spec/fixtures/files/avatar-base64.txt") }
+  let(:avatar_jpeg) { File.binread("#{Rails.root}/spec/fixtures/files/taipei.jpg") }
 
   before do
     login_as user, :scope => :user
     allow(user).to receive(:display_name).and_return("Mark")
-    allow_any_instance_of(User).to receive(:dn).and_return("cn=mwahlberg,ou=kosmos.org,cn=users,dc=kosmos,dc=org")
-    allow_any_instance_of(User).to receive(:ldap_entry).and_return({
-      uid: user.cn, ou: user.ou, display_name: "Mark", pgp_key: nil
-    })
-    allow_any_instance_of(User).to receive(:avatar).and_return(avatar_base64)
 
-    Flipper.enable "avatar_upload"
+    allow_any_instance_of(User).to receive(:dn)
+      .and_return("cn=mwahlberg,ou=kosmos.org,cn=users,dc=kosmos,dc=org")
+    allow_any_instance_of(User).to receive(:ldap_entry)
+      .and_return({
+        uid: user.cn, ou: user.ou, display_name: "Mark", pgp_key: nil
+      })
   end
 
   feature "Update display name" do
@@ -57,21 +57,24 @@ RSpec.describe 'Profile settings', type: :feature do
     end
 
     scenario "fails with validation error for file size too large" do
+      expect_any_instance_of(LdapManager::UpdateAvatar)
+        .not_to receive(:replace_attribute).and_return(true)
+
       visit setting_path(:profile)
       attach_file "Avatar", "#{Rails.root}/spec/fixtures/files/fsociety-irc.png"
       click_button "Save"
 
       expect(current_url).to eq(setting_url(:profile))
       within ".error-msg" do
-        expect(page).to have_content("file size is too large")
+        expect(page).to have_content("must be less than 1MB")
       end
     end
 
     scenario 'works with valid JPG file' do
       file_path = "#{Rails.root}/spec/fixtures/files/taipei.jpg"
 
-      expect_any_instance_of(LdapManager::UpdateAvatar).to receive(:replace_attribute)
+      expect_any_instance_of(LdapManager::UpdateAvatar)
-        .with(user.dn, :jpegPhoto, avatar_base64).and_return(true)
+        .to receive(:replace_attribute).and_return(true)
 
       visit setting_path(:profile)
       attach_file "Avatar", file_path
@@ -86,7 +89,8 @@ RSpec.describe 'Profile settings', type: :feature do
     scenario 'works with valid PNG file' do
       file_path = "#{Rails.root}/spec/fixtures/files/bender.png"
 
-      expect(LdapManager::UpdateAvatar).to receive(:call).and_return(true)
+      expect_any_instance_of(LdapManager::UpdateAvatar)
+        .to receive(:replace_attribute).and_return(true)
 
       visit setting_path(:profile)
       attach_file "Avatar", file_path

spec/jobs/create_ldap_user_job_spec.rb

@@ -32,7 +32,7 @@ RSpec.describe CreateLdapUserJob, type: :job do
     expect(ldap_client_mock).to have_received(:add).with(
       dn: "cn=halfinney,ou=kosmos.org,cn=users,dc=kosmos,dc=org",
       attributes: {
-        objectclass: ["top", "account", "person", "extensibleObject"],
+        objectclass: ["top", "account", "person", "inetOrgPerson", "extensibleObject"],
         cn: "halfinney",
         sn: "halfinney",
         uid: "halfinney",
@@ -51,7 +51,7 @@ RSpec.describe CreateLdapUserJob, type: :job do
     expect(ldap_client_mock).to have_received(:add).with(
       dn: "cn=halfinney,ou=kosmos.org,cn=users,dc=kosmos,dc=org",
       attributes: {
-        objectclass: ["top", "account", "person", "extensibleObject"],
+        objectclass: ["top", "account", "person", "inetOrgPerson", "extensibleObject"],
         cn: "halfinney",
         sn: "halfinney",
         uid: "halfinney",

spec/requests/webfinger_spec.rb

@@ -18,6 +18,46 @@ RSpec.describe "WebFinger", type: :request do
       })
     end
 
+    describe "Avatar" do
+      context "not available" do
+        it "does not include an avatar link" do
+          get "/.well-known/webfinger?resource=acct%3Atony%40kosmos.org"
+          expect(response).to have_http_status(:ok)
+          res = JSON.parse(response.body)
+
+          link = res["links"].find{|l| l["rel"] == "http://webfinger.net/rel/avatar"}
+          expect(link).to be_nil
+        end
+      end
+
+      context "available" do
+        let(:fixture_path) { Rails.root.join("spec/fixtures/files/taipei.jpg") }
+        let(:img_data) { File.read(fixture_path) }
+        let(:img_hash) { Digest::SHA256.hexdigest(img_data) }
+
+        before do
+          ActiveStorage::Blob.create_and_upload!(
+            io: File.open(fixture_path),
+            filename: "#{img_hash}.jpg",
+            content_type: "image/jpeg"
+          ).tap do |blob|
+            user.avatar.attach(blob)
+          end
+        end
+
+        it "includes a public avatar link" do
+          get "/.well-known/webfinger?resource=acct%3Atony%40kosmos.org"
+          expect(response).to have_http_status(:ok)
+          res = JSON.parse(response.body)
+
+          link = res["links"].find { |l| l["rel"] == "http://webfinger.net/rel/avatar" }
+          expect(link).to be_present
+          expect(link["type"]).to eq("image/jpeg")
+          expect(link["href"]).to match(%r{users/tony/avatars/#{img_hash}.jpg})
+        end
+      end
+    end
+
     describe "Mastodon entries" do
       context "Mastodon available" do
         it "includes the Mastodon aliases and links for the user" do