Merge branch 'feature/user_avatars' into live

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,20 @@
+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 || :large
+
+      unless user.avatar_filename == "#{sha256_hash}.#{format}"
+        http_status :not_found and return
+      end
+
+      send_file user.avatar.service.path_for(user.avatar.key),
+        disposition: "inline", type: "image/#{format}"
+    else
+      http_status :not_found and return
+    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,10 @@ class SettingsController < ApplicationController
       end
 
       if @user.avatar_new.present?
-        LdapManager::UpdateAvatar.call(dn: @user.dn, file: @user.avatar_new)
+        @user.avatar.attach(@user.avatar_new)
+        @user.avatar.blob.update(filename: @user.avatar_filename)
+        @user.save!
+        LdapManager::UpdateAvatar.call(user: @user)
       end
 
       if @user.pgp_pubkey && (@user.pgp_pubkey != @user.ldap_entry[:pgp_key])
@@ -162,7 +165,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

app/helpers/application_helper.rb

@@ -14,4 +14,12 @@ 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)
+    if Setting.s3_enabled?
+      s3_image_url(attachment)
+    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
   #
@@ -159,6 +165,32 @@ class User < ApplicationRecord
     @display_name ||= ldap_entry[:display_name]
   end
 
+  def avatar_base64(size: :medium)
+    return nil unless avatar.attached?
+    variant = avatar_variant(size: size)
+    data = ActiveStorage::Blob.service.download(variant.key)
+    Base64.strict_encode64(data)
+  end
+
+  def avatar_filename
+    return nil unless avatar.attached?
+    data = ActiveStorage::Blob.service.download(avatar.key)
+    hash = Digest::SHA256.hexdigest(data)
+    ext = avatar.content_type == "image/png" ? "png" : "jpg"
+    "#{hash}.#{ext}"
+  end
+
+  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 +218,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 +255,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,40 @@ require "image_processing/vips"
 
 module LdapManager
   class UpdateAvatar < LdapManagerService
-    def initialize(dn:, file:)
-      @dn = dn
-      @img_data = process(file)
+    def initialize(user:)
+      @user = user
+      @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(img_data)
+
+      result = replace_attribute(@dn, :jpegPhoto, jpg_data)
+      result == 0
     end
 
     private
 
-    def process(file)
-      processed = ImageProcessing::Vips
-        .resize_to_fill(512, 512)
-        .source(file)
-        .convert("jpeg")
-        .saver(strip: true)
-        .call
-
-      Base64.strict_encode64 processed.read
+    def process(data)
+      @user.avatar.blob.open do |file|
+        processed = ImageProcessing::Vips
+          .source(file)
+          .resize_to_fill(256, 256)
+          .convert("jpeg")
+          .saver(strip: true)
+          .call
+        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? %>
-                <img src="data:image/jpeg;base64,<%= @avatar %>" class="h-48 w-48" />
+              <% if @ldap_avatar.present? %>
+                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| %>
@@ -33,21 +33,19 @@
 
     <% if Flipper.enabled?(:avatar_upload, current_user) %>
     <label class="block">
-      <p class="font-bold mb-1">
-        Avatar
-      </p>
-      <p class="text-gray-500">
-        Default profile picture
-      </p>
+      <p class="font-bold mb-1">Avatar</p>
+      <p class="text-gray-500">Default profile picture</p>
       <div class="flex items-center gap-6">
-        <% if current_user.avatar.present? %>
-        <p class="flex-none">
-          <%= image_tag "data:image/jpeg;base64,#{current_user.avatar}", class: "h-24 w-24 rounded-lg" %>
-        </p>
+        <% if @user.avatar.attached? %>
+          <p class="flex-none">
+            <%= image_tag @user.avatar_variant(size: :medium),
+              class: "h-24 w-24 rounded-lg" %>
+          </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>

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,20 @@ 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
+    Flipper.enable "avatar_upload"
+
     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 +59,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)
-        .with(user.dn, :jpegPhoto, avatar_base64).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
@@ -86,7 +91,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",