From 8345f4ac5d97bcfa7c0db2cabfa2d7082ee91576 Mon Sep 17 00:00:00 2001 From: Martin Artnik Date: Tue, 24 Jan 2023 15:57:59 +0100 Subject: [PATCH 1/7] Throw error when authenticating with revoked tokens [SCI-7631] --- .../concerns/token_authentication.rb | 8 +++++ .../doorkeeper/access_tokens_controller.rb | 17 +++++++++ config/routes.rb | 2 ++ .../concerns/token_authentication_spec.rb | 36 +++++++++++++++++++ .../access_tokens_controller_spec.rb | 28 +++++++++++++++ 5 files changed, 91 insertions(+) create mode 100644 app/controllers/doorkeeper/access_tokens_controller.rb create mode 100644 spec/controllers/concerns/token_authentication_spec.rb create mode 100644 spec/controllers/doorkeeper/access_tokens_controller_spec.rb diff --git a/app/controllers/concerns/token_authentication.rb b/app/controllers/concerns/token_authentication.rb index a6ed6d2cd..a856007ee 100644 --- a/app/controllers/concerns/token_authentication.rb +++ b/app/controllers/concerns/token_authentication.rb @@ -17,6 +17,8 @@ module TokenAuthentication @token = request.headers['Authorization']&.sub('Bearer ', '') raise JWT::VerificationError, I18n.t('api.core.missing_token') unless @token + verify_token! + @token_iss = Api::CoreJwt.read_iss(@token) raise JWT::InvalidPayload, I18n.t('api.core.no_iss') unless @token_iss @@ -34,4 +36,10 @@ module TokenAuthentication @current_user = User.find_by(id: payload['sub']) raise JWT::InvalidPayload, I18n.t('api.core.no_user_mapping') unless current_user end + + def verify_token! + if Doorkeeper::AccessToken.where.not(revoked_at: nil).exists?(token: @token) + raise JWT::VerificationError, I18n.t('api.core.expired_token') + end + end end diff --git a/app/controllers/doorkeeper/access_tokens_controller.rb b/app/controllers/doorkeeper/access_tokens_controller.rb new file mode 100644 index 000000000..809f16907 --- /dev/null +++ b/app/controllers/doorkeeper/access_tokens_controller.rb @@ -0,0 +1,17 @@ +# frozen_string_literal: true + +module Doorkeeper + class AccessTokensController < ApplicationController + before_action :find_token + + def revoke + @token.revoke + end + + private + + def find_token + @token = current_user.access_tokens.find(params[:id]) + end + end +end diff --git a/config/routes.rb b/config/routes.rb index 284289e8c..0229a48e2 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -3,6 +3,8 @@ Rails.application.routes.draw do skip_controllers :applications, :authorized_applications, :token_info end + post 'access_tokens/revoke', to: 'doorkeeper/access_tokens#revoke' + # Addons def draw(routes_name) diff --git a/spec/controllers/concerns/token_authentication_spec.rb b/spec/controllers/concerns/token_authentication_spec.rb new file mode 100644 index 000000000..427c1bd87 --- /dev/null +++ b/spec/controllers/concerns/token_authentication_spec.rb @@ -0,0 +1,36 @@ +# frozen_string_literal: true + +require 'rails_helper' + +class TokenAuthenticatedController + attr_accessor :token, :current_user + + include TokenAuthentication + + def request + OpenStruct.new( + headers: { 'Authorization' => "Bearer #{@token}" } + ) + end +end + +describe TokenAuthentication do + let(:test_controller_instance) { TokenAuthenticatedController.new } + + let(:user) { create :user } + let(:access_token) { + user.access_tokens.create( + expires_in: 7500 + ) + } + + describe '#authenticate_request' do + it "rejects revoked token" do + test_controller_instance.token = access_token.token + test_controller_instance.current_user = user + + access_token.revoke + expect { test_controller_instance.send(:authenticate_request!) }.to raise_error(JWT::VerificationError) + end + end +end diff --git a/spec/controllers/doorkeeper/access_tokens_controller_spec.rb b/spec/controllers/doorkeeper/access_tokens_controller_spec.rb new file mode 100644 index 000000000..8de196e84 --- /dev/null +++ b/spec/controllers/doorkeeper/access_tokens_controller_spec.rb @@ -0,0 +1,28 @@ +# frozen_string_literal: true + +require 'rails_helper' + +describe Doorkeeper::AccessTokensController, type: :controller do + login_user + + let!(:access_token) do + subject.current_user.access_tokens.create(expires_in: 7500) + end + + describe 'POST revoke' do + let(:params) do + { + id: access_token.id + } + end + + let(:action) do + put :revoke, params: params + end + + it 'revokes the access token' do + action + expect(access_token.reload.revoked_at).to_not be_nil + end + end +end From 88cc8e3df4fa00b0e2dc0aa1c3b49ad97839d325 Mon Sep 17 00:00:00 2001 From: artoscinote <85488244+artoscinote@users.noreply.github.com> Date: Tue, 7 Feb 2023 11:05:48 +0100 Subject: [PATCH 2/7] Amend token revocation check method name [SCI-7631] --- app/controllers/concerns/token_authentication.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/controllers/concerns/token_authentication.rb b/app/controllers/concerns/token_authentication.rb index a856007ee..46f967bfa 100644 --- a/app/controllers/concerns/token_authentication.rb +++ b/app/controllers/concerns/token_authentication.rb @@ -17,7 +17,7 @@ module TokenAuthentication @token = request.headers['Authorization']&.sub('Bearer ', '') raise JWT::VerificationError, I18n.t('api.core.missing_token') unless @token - verify_token! + check_token_revocation! @token_iss = Api::CoreJwt.read_iss(@token) raise JWT::InvalidPayload, I18n.t('api.core.no_iss') unless @token_iss @@ -37,7 +37,7 @@ module TokenAuthentication raise JWT::InvalidPayload, I18n.t('api.core.no_user_mapping') unless current_user end - def verify_token! + def check_token_revocation! if Doorkeeper::AccessToken.where.not(revoked_at: nil).exists?(token: @token) raise JWT::VerificationError, I18n.t('api.core.expired_token') end From f880ea46cee9f2e1138535a52e9316c4a927b38c Mon Sep 17 00:00:00 2001 From: Martin Artnik Date: Mon, 13 Feb 2023 11:32:18 +0100 Subject: [PATCH 3/7] Add connected device logging [SCI-7632] --- app/models/connected_device.rb | 37 +++++++++++++++++++ config/application.rb | 13 +++++++ ...20230207140811_create_connected_devices.rb | 15 ++++++++ 3 files changed, 65 insertions(+) create mode 100644 app/models/connected_device.rb create mode 100644 db/migrate/20230207140811_create_connected_devices.rb diff --git a/app/models/connected_device.rb b/app/models/connected_device.rb new file mode 100644 index 000000000..2557682c6 --- /dev/null +++ b/app/models/connected_device.rb @@ -0,0 +1,37 @@ +# frozen_string_literal: true + +class ConnectedDevice < ApplicationRecord + belongs_to :oauth_access_token, class_name: 'Doorkeeper::AccessToken' + + after_destroy :revoke_token + + def self.for_user(user) + where(oauth_access_token_id: Doorkeeper::AccessToken.select(:id).where(resource_owner_id: user.id)) + end + + def self.from_request_headers(headers, token = nil) + current_token = Doorkeeper::AccessToken.find_by( + token: headers['Authorization'].gsub(/Bearer\s/, '') + ) + + return unless token || current_token + + connected_device = ConnectedDevice.find_or_initialize_by(uid: headers['Device-Id']) + connected_device.update!( + name: headers['Device-Name'], + metadata: { + os: headers['Device-Os'], + app_version: headers['Device-App-Version'] + }.compact, + last_seen_at: Time.current, + oauth_access_token_id: token&.id || current_token&.id + ) + connected_device + end + + private + + def revoke_token + oauth_access_token.revoke + end +end diff --git a/config/application.rb b/config/application.rb index bf486722c..e8513830d 100644 --- a/config/application.rb +++ b/config/application.rb @@ -69,6 +69,19 @@ module Scinote config.to_prepare do # Only Authorization endpoint Doorkeeper::AuthorizationsController.layout 'sign_in_halt' + + # Add Connected Device logging + Doorkeeper::TokensController.class_eval do + after_action :log_connected_device, only: :create + + private + + def log_connected_device + return if @authorize_response.is_a?(Doorkeeper::OAuth::ErrorResponse) + + ConnectedDevice.from_request_headers(request.headers, @authorize_response&.token) + end + end end config.action_view.field_error_proc = Proc.new { |html_tag, instance| diff --git a/db/migrate/20230207140811_create_connected_devices.rb b/db/migrate/20230207140811_create_connected_devices.rb new file mode 100644 index 000000000..fea978686 --- /dev/null +++ b/db/migrate/20230207140811_create_connected_devices.rb @@ -0,0 +1,15 @@ +# frozen_string_literal: true + +class CreateConnectedDevices < ActiveRecord::Migration[6.1] + def change + create_table :connected_devices do |t| + t.string :uid + t.string :name + t.references :oauth_access_token, null: false, foreign_key: true + t.json :metadata + t.timestamp :last_seen_at + + t.timestamps + end + end +end From 64da4c5368c3ae729fe12f4c389647994ae26b29 Mon Sep 17 00:00:00 2001 From: Andrej Date: Tue, 28 Feb 2023 16:42:56 +0100 Subject: [PATCH 4/7] Fix connected device model for request without device id [SCI-7972] --- app/models/connected_device.rb | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/app/models/connected_device.rb b/app/models/connected_device.rb index 2557682c6..40771bc4a 100644 --- a/app/models/connected_device.rb +++ b/app/models/connected_device.rb @@ -2,6 +2,7 @@ class ConnectedDevice < ApplicationRecord belongs_to :oauth_access_token, class_name: 'Doorkeeper::AccessToken' + validates :uid, presence: true after_destroy :revoke_token @@ -10,8 +11,10 @@ class ConnectedDevice < ApplicationRecord end def self.from_request_headers(headers, token = nil) + return unless headers['Device-Id'] + current_token = Doorkeeper::AccessToken.find_by( - token: headers['Authorization'].gsub(/Bearer\s/, '') + token: headers['Authorization']&.gsub(/Bearer\s/, '') ) return unless token || current_token From 06eeb3b27f9ffd3f62b7afa9d60352e90cb5f741 Mon Sep 17 00:00:00 2001 From: G-Chubinidze <112488503+G-Chubinidze@users.noreply.github.com> Date: Fri, 17 Mar 2023 16:44:41 +0400 Subject: [PATCH 5/7] FE manage devices (table view) + revoking the device [SCI-7633] (#4920) * FE manage devices (table view) + revoking the device [SCI-7633] * revocation code optimisation * hound fixes --------- Co-authored-by: Giga Chubinidze --- .../stylesheets/settings/device_table.scss | 62 +++++++++++++++++++ app/assets/stylesheets/settings/users.scss | 12 ++++ .../_device_revocation_modal.html.erb | 19 ++++++ .../registrations/_manage_devices.html.erb | 46 ++++++++++++++ app/views/users/registrations/edit.html.erb | 6 ++ config/locales/en.yml | 13 ++++ 6 files changed, 158 insertions(+) create mode 100644 app/assets/stylesheets/settings/device_table.scss create mode 100644 app/views/users/registrations/_device_revocation_modal.html.erb create mode 100644 app/views/users/registrations/_manage_devices.html.erb diff --git a/app/assets/stylesheets/settings/device_table.scss b/app/assets/stylesheets/settings/device_table.scss new file mode 100644 index 000000000..210bf4b55 --- /dev/null +++ b/app/assets/stylesheets/settings/device_table.scss @@ -0,0 +1,62 @@ +// scss-lint:disable SelectorDepth NestingDepth IdSelector +#devicesTable { + .devices-table { + display: grid; + grid-auto-rows: 3em 1px; + grid-template-columns: 200px 150px 50px; + min-width: 100%; + + .table-header-cell { + align-items: center; + background-color: $color-concrete; + border: 1px solid $color-white; + display: flex; + height: 3em; + padding: 0 .5em; + z-index: 2; + } + + .table-header { + display: contents; + + &::after { + content: ""; + grid-column: 1/-1; + } + } + + .table-body { + display: contents; + } + + .table-body-cell { + align-items: center; + display: flex; + padding: 0 .5em; + } + + .x-button { + background: 0; + border: 0; + margin-left: 5px; + } + + .table-row { + display: contents; + + &:hover { + .table-body-cell { + background-color: $color-concrete; + } + } + + &::after { + background: $color-concrete; + content: ""; + display: inline-block; + grid-column: 1/-1; + height: 1px; + } + } + } +} diff --git a/app/assets/stylesheets/settings/users.scss b/app/assets/stylesheets/settings/users.scss index f1984281b..ef844a210 100644 --- a/app/assets/stylesheets/settings/users.scss +++ b/app/assets/stylesheets/settings/users.scss @@ -119,6 +119,18 @@ } } +.device-revocation-modal { + .modal-dialog { + height: 216px; + width: 370px; + } +} + +.manage-devices { + position: relative; + top: 30px; +} + @media (max-width: 700px) { .user-settings { .two-factor-container { diff --git a/app/views/users/registrations/_device_revocation_modal.html.erb b/app/views/users/registrations/_device_revocation_modal.html.erb new file mode 100644 index 000000000..77e8121dd --- /dev/null +++ b/app/views/users/registrations/_device_revocation_modal.html.erb @@ -0,0 +1,19 @@ + + diff --git a/app/views/users/registrations/_manage_devices.html.erb b/app/views/users/registrations/_manage_devices.html.erb new file mode 100644 index 000000000..cf223531d --- /dev/null +++ b/app/views/users/registrations/_manage_devices.html.erb @@ -0,0 +1,46 @@ +<%# mobile_app_devices = [['Lenovo ProBook', '07/12/2022'], ['iPhone', '06/12/2022'], ['iMac', '02/12/2022'], ['Acer Air Pro', '01/12/2022']]%> +<% mobile_app_devices = [] %> + + + +
+

<%= t("users.registrations.edit.connected_devices.title") %>

+ <% if mobile_app_devices.present? %> +

<%= t("users.registrations.edit.connected_devices.description") %>

+
+
+
+
+
+ <%= t("users.registrations.edit.devices_table.device_name") %> +
+
+ <%= t("users.registrations.edit.devices_table.added_on") %> +
+
+
+
+
+ <% mobile_app_devices.each do |device| %> +
+
+ <%= device[0] %> +
+
+ <%= device[1] %> +
+
+ +
+
+ <% end %> +
+
+
+
+ <% else %> +

<%= t("users.registrations.edit.connected_devices.empty_state") %>

+ <% end %> +
+ +<%= render partial: 'device_revocation_modal' %> diff --git a/app/views/users/registrations/edit.html.erb b/app/views/users/registrations/edit.html.erb index f7eacf745..918c8d8f5 100644 --- a/app/views/users/registrations/edit.html.erb +++ b/app/views/users/registrations/edit.html.erb @@ -38,6 +38,11 @@ + +
+ <%= render partial: 'users/registrations/manage_devices' %> +
+
@@ -70,6 +75,7 @@
<%= render partial: '2fa_modal' %> + <%= render partial: 'users/shared/user_avatars_modal' %> <%= javascript_pack_tag 'custom/croppie' %> diff --git a/config/locales/en.yml b/config/locales/en.yml index 872e2e07c..b695003f8 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -2326,6 +2326,19 @@ en: 2fa_errors: wrong_submit_code: "Not correct code" wrong_password: "Not correct password" + connected_devices: + title: "Manage connected devices" + description: "When you remove a device, you will be logged out from the selected device." + empty_state: "No connected devices." + devices_table: + device_name: "Device Name" + added_on: "Added on" + device_revocation_modal: + title: "Remove connected device" + description: "This device will lose access to your SciNote account." + validation: "Are you sure you want to remove it?" + cancel: "Cancel" + remove: "Remove" new: head_title: "Sign up" team_name_label: "Team name" From 8eb04c50c2c8ea0f056b506b2a6c65a3cc3e5917 Mon Sep 17 00:00:00 2001 From: ajugo Date: Tue, 21 Mar 2023 16:44:24 +0100 Subject: [PATCH 6/7] Add removing connected device [SCI-8146] (#5174) --- .../javascripts/users/connected_devices.js | 25 ++++++++ .../users/connected_devices_controller.rb | 18 ++++++ .../users/registrations_controller.rb | 5 ++ .../_device_revocation_modal.html.erb | 2 +- .../registrations/_manage_devices.html.erb | 64 +++++++++---------- config/initializers/assets.rb | 1 + config/routes.rb | 2 + 7 files changed, 83 insertions(+), 34 deletions(-) create mode 100644 app/assets/javascripts/users/connected_devices.js create mode 100644 app/controllers/users/connected_devices_controller.rb diff --git a/app/assets/javascripts/users/connected_devices.js b/app/assets/javascripts/users/connected_devices.js new file mode 100644 index 000000000..c99c18e23 --- /dev/null +++ b/app/assets/javascripts/users/connected_devices.js @@ -0,0 +1,25 @@ +/* global I18n */ + +(function() { + let connectedDeviceDescription = $('.connected-devices-description'); + let revocationModal = $('.device-revocation-modal'); + let deleteButtonModal = $('#confirm-device-remove'); + + $('.x-button').on('click', function() { + deleteButtonModal.attr('href', $(this).data('url')); + deleteButtonModal.data('id', $(this).closest('.table-row').data('id')); + }); + + deleteButtonModal + .on('ajax:success', function() { + $(`.table-row[data-id=${deleteButtonModal.data('id')}]`).remove(); + + // Show correct representation if user does not have any connected device anymore + if (connectedDeviceDescription.find('.table-row').length === 0) { + $('.connected-devices-container').remove(); + connectedDeviceDescription.append(`

${I18n.t('users.registrations.edit.connected_devices.empty_state')}

`); + } + + revocationModal.modal('hide'); + }); +}()); diff --git a/app/controllers/users/connected_devices_controller.rb b/app/controllers/users/connected_devices_controller.rb new file mode 100644 index 000000000..59d1daf05 --- /dev/null +++ b/app/controllers/users/connected_devices_controller.rb @@ -0,0 +1,18 @@ +# frozen_string_literal: true + +module Users + class ConnectedDevicesController < ApplicationController + before_action :check_delete_permissions, only: :destroy + + def destroy + @connected_device.destroy + end + + private + + def check_delete_permissions + @connected_device = ConnectedDevice.for_user(current_user).find_by(id: params[:id]) + render_403 if @connected_device.blank? + end + end +end diff --git a/app/controllers/users/registrations_controller.rb b/app/controllers/users/registrations_controller.rb index c1f7e9d11..056d0ee4b 100644 --- a/app/controllers/users/registrations_controller.rb +++ b/app/controllers/users/registrations_controller.rb @@ -184,6 +184,11 @@ class Users::RegistrationsController < Devise::RegistrationsController end end + def edit + @connected_devices = ConnectedDevice.for_user(current_user) + super + end + def two_factor_enable user = current_user || User.find_by(id: session[:otp_user_id]) if user.valid_otp?(params[:submit_code]) diff --git a/app/views/users/registrations/_device_revocation_modal.html.erb b/app/views/users/registrations/_device_revocation_modal.html.erb index 77e8121dd..6a4da209d 100644 --- a/app/views/users/registrations/_device_revocation_modal.html.erb +++ b/app/views/users/registrations/_device_revocation_modal.html.erb @@ -11,7 +11,7 @@
- <%= link_to t("users.registrations.edit.device_revocation_modal.remove"), '#', id: "confirm-device-remove", method: :delete, type: 'button', class: 'btn btn-primary' %> + <%= link_to t("users.registrations.edit.device_revocation_modal.remove"), '#', id: "confirm-device-remove", remote:true ,method: :delete, type: 'button', class: 'btn btn-primary' %>
diff --git a/app/views/users/registrations/_manage_devices.html.erb b/app/views/users/registrations/_manage_devices.html.erb index cf223531d..01e30bd20 100644 --- a/app/views/users/registrations/_manage_devices.html.erb +++ b/app/views/users/registrations/_manage_devices.html.erb @@ -1,39 +1,36 @@ -<%# mobile_app_devices = [['Lenovo ProBook', '07/12/2022'], ['iPhone', '06/12/2022'], ['iMac', '02/12/2022'], ['Acer Air Pro', '01/12/2022']]%> -<% mobile_app_devices = [] %> - - -

<%= t("users.registrations.edit.connected_devices.title") %>

- <% if mobile_app_devices.present? %> -

<%= t("users.registrations.edit.connected_devices.description") %>

-
-
-
-
-
- <%= t("users.registrations.edit.devices_table.device_name") %> -
-
- <%= t("users.registrations.edit.devices_table.added_on") %> -
-
-
-
-
- <% mobile_app_devices.each do |device| %> -
-
- <%= device[0] %> -
-
- <%= device[1] %> -
-
- -
+ <% if @connected_devices.present? %> +
+

<%= t("users.registrations.edit.connected_devices.description") %>

+
+
+
+
+
+ <%= t("users.registrations.edit.devices_table.device_name") %>
- <% end %> +
+ <%= t("users.registrations.edit.devices_table.added_on") %> +
+
+
+
+
+ <% @connected_devices.each do |device| %> +
> +
+ <%= device.name %> +
+
+ <%= l(device.last_seen_at, format: :full_date) %> +
+
+ +
+
+ <% end %> +
@@ -44,3 +41,4 @@
<%= render partial: 'device_revocation_modal' %> +<%= javascript_include_tag 'users/connected_devices' %> diff --git a/config/initializers/assets.rb b/config/initializers/assets.rb index e26e15ff3..d07d3af55 100644 --- a/config/initializers/assets.rb +++ b/config/initializers/assets.rb @@ -97,6 +97,7 @@ Rails.application.config.assets.precompile += %w(reports/save_pdf_to_inventory.j Rails.application.config.assets.precompile += %w(reports/content.js) Rails.application.config.assets.precompile += %w(session_end.js) Rails.application.config.assets.precompile += %w(label_templates/label_templates_datatable.js) +Rails.application.config.assets.precompile += %w(users/connected_devices.js) Rails.application.config.assets.precompile += %w(BrowserPrint-3.0.216.min.js) Rails.application.config.assets.precompile += %w(BrowserPrint-Zebra-1.0.216.min.js) diff --git a/config/routes.rb b/config/routes.rb index 0229a48e2..25de3f85f 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -734,6 +734,8 @@ Rails.application.routes.draw do end end + resources :connected_devices, controller: 'users/connected_devices', only: %i(destroy) + get 'search' => 'search#index' get 'search/new' => 'search#new', as: :new_search From fe37699ba5aaf177b620f31d7a70f55de9c73d1a Mon Sep 17 00:00:00 2001 From: ajugo Date: Wed, 22 Mar 2023 12:15:01 +0100 Subject: [PATCH 7/7] Add switch for connected devices [SCI-8112] (#5179) --- app/views/users/registrations/edit.html.erb | 10 ++++++---- config/application.rb | 2 ++ 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/app/views/users/registrations/edit.html.erb b/app/views/users/registrations/edit.html.erb index 918c8d8f5..02c2e5114 100644 --- a/app/views/users/registrations/edit.html.erb +++ b/app/views/users/registrations/edit.html.erb @@ -38,10 +38,12 @@
- -
- <%= render partial: 'users/registrations/manage_devices' %> -
+<% if Rails.application.config.x.connected_devices_enabled %> + +
+ <%= render partial: 'users/registrations/manage_devices' %> +
+<% end %>
diff --git a/config/application.rb b/config/application.rb index e8513830d..b9b8401c9 100644 --- a/config/application.rb +++ b/config/application.rb @@ -57,6 +57,8 @@ module Scinote config.x.webhooks_enabled = ENV['ENABLE_WEBHOOKS'] == 'true' + config.x.connected_devices_enabled = ENV['CONNECTED_DEVICES_ENABLED'] == 'true' + # Logging config.log_formatter = proc do |severity, datetime, progname, msg| "[#{datetime}] #{severity}: #{msg}\n"