diff --git a/app/controllers/admin/oauth_applications_controller.rb b/app/controllers/admin/oauth_applications_controller.rb index 7add45e..5bc0ad8 100644 --- a/app/controllers/admin/oauth_applications_controller.rb +++ b/app/controllers/admin/oauth_applications_controller.rb @@ -1,8 +1,8 @@ # frozen_string_literal: true class Admin::OauthApplicationsController < Admin::BaseController - before_action :set_application, only: [:show, :edit, :update, :destroy, :regenerate_secret, :access_policy] - before_action :require_superadmin, only: [:access_policy] + before_action :set_application, only: [:show, :edit, :update, :destroy, :regenerate_secret, :access_policy, :code_of_conduct] + before_action :require_superadmin, only: [:access_policy, :code_of_conduct] def index @applications = Doorkeeper::Application.order(created_at: :desc) @@ -88,6 +88,16 @@ def access_policy notice: policy == "restricted" ? "Only users with an access grant can use this app now." : "Everyone can use this app now." end + # Opts an app out of the code-of-conduct requirement, or back in (see + # AppAccess). Superadmin only: this decides who can sign in to the app. + def code_of_conduct + required = ActiveModel::Type::Boolean.new.cast(params.require(:required)) + @application.update!(requires_code_of_conduct: required) + RevokeLostAppAccessJob.perform_later(application_id: @application.id) if required + redirect_to admin_oauth_application_path(@application), + notice: required ? "This app now requires the Code of Conduct." : "This app no longer requires the Code of Conduct." + end + private def require_superadmin diff --git a/app/controllers/admin/users_controller.rb b/app/controllers/admin/users_controller.rb index f0edb37..c891662 100644 --- a/app/controllers/admin/users_controller.rb +++ b/app/controllers/admin/users_controller.rb @@ -1,12 +1,12 @@ # frozen_string_literal: true class Admin::UsersController < Admin::BaseController - before_action :set_user, only: [:show, :edit, :update, :destroy, :impersonate, :regen_pid, :invite_to_slack] + before_action :set_user, only: [:show, :edit, :update, :destroy, :impersonate, :regen_pid, :invite_to_slack, :code_of_conduct_exemption] before_action :ensure_can_view_user, only: [:show] before_action :ensure_can_manage_user, only: [:edit, :update, :destroy, :regen_pid] before_action :ensure_can_impersonate, only: [:impersonate] before_action :ensure_not_already_impersonating, only: [:impersonate] - before_action :require_superadmin, only: [:regen_pid] + before_action :require_superadmin, only: [:regen_pid, :code_of_conduct_exemption] skip_before_action :authenticate_user!, only: [:stop_impersonating] skip_before_action :require_admin, only: [:stop_impersonating] helper_method :admin_permissions_for @@ -133,6 +133,17 @@ def invite_to_slack redirect_to admin_user_path(@user), notice: "#{verb} #{@user.email} to Slack (queued)." end + # Lets one user use apps without accepting the code of conduct, or takes + # that away (see AppAccess). Superadmin only: this decides who can sign in to + # apps. + def code_of_conduct_exemption + exempt = ActiveModel::Type::Boolean.new.cast(params.require(:exempt)) + @user.update!(code_of_conduct_exempt: exempt) + RevokeLostAppAccessJob.perform_later(user_id: @user.id) unless exempt + redirect_to admin_user_path(@user), + notice: exempt ? "#{@user.full_name} can use apps without accepting the Code of Conduct." : "#{@user.full_name} must accept the Code of Conduct to use apps." + end + private def ensure_can_impersonate diff --git a/app/controllers/oauth/authorizations_controller.rb b/app/controllers/oauth/authorizations_controller.rb index b8ff619..b85e844 100644 --- a/app/controllers/oauth/authorizations_controller.rb +++ b/app/controllers/oauth/authorizations_controller.rb @@ -27,7 +27,8 @@ module Oauth # screen and the consent POST, so a previously approved app can't skip it. # The user sees a Weave page instead of a redirect to the client with # `error=access_denied`: the client can't grant access, and Weave can say who - # to ask. + # to ask. A user who only lacks the code of conduct is sent to accept it, and + # comes back here afterwards (see SlackOnboardingController). class AuthorizationsController < Doorkeeper::AuthorizationsController include OauthClientRedirectOrigins @@ -45,13 +46,21 @@ def require_app_access return unless pre_auth.authorizable? application = pre_auth.client.application - return if AppAccess.permitted?(current_resource_owner, application) + decision = AppAccess.explain(current_resource_owner, application) + return if decision.permitted? + + if decision.reason == :code_of_conduct && request.get? + session[:code_of_conduct_return_to] = request.fullpath + redirect_to slack_onboarding_path, alert: "Please accept the Code of Conduct to continue to #{application.name}." + return + end # Ahoy drops requests it takes for bots, so the log line is the record # that is always kept. Rails.logger.info("OAuth access denied: user=#{current_resource_owner.p_id} application=#{application.uid}") ahoy.track "OAuth access denied", application_uid: application.uid, user_id: current_resource_owner.id @application = application + @decision = decision render "doorkeeper/authorizations/access_denied", status: :forbidden end diff --git a/app/controllers/slack_onboarding_controller.rb b/app/controllers/slack_onboarding_controller.rb index b05d6cf..4a85a84 100644 --- a/app/controllers/slack_onboarding_controller.rb +++ b/app/controllers/slack_onboarding_controller.rb @@ -66,6 +66,13 @@ def accept_code_of_conduct return end + # Sent here from an app's sign-in (see Oauth::AuthorizationsController). + return_to = session.delete(:code_of_conduct_return_to).to_s + if return_to.start_with?("/oauth/authorize") + redirect_to return_to + return + end + notice = if current_user.slack_member? "Thanks for accepting the Code of Conduct." else diff --git a/app/models/user.rb b/app/models/user.rb index 315065a..9c70302 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -8,6 +8,7 @@ # id :bigint not null, primary key # acknowledged_over_13_at :datetime # birthday :date +# code_of_conduct_exempt :boolean default(FALSE), not null # confirmation_sent_at :datetime # confirmation_token :string # email :string not null @@ -190,8 +191,10 @@ def self.generate_secure_password # Full Slack members who have not accepted the code of conduct. They joined # before the code-of-conduct flow, so nothing has asked them yet (see - # CodeOfConductRequestJob). - scope :code_of_conduct_pending, -> { slack_member.where(slack_coc_accepted_at: nil).where.not(slack_id: nil) } + # CodeOfConductRequestJob). Exempt users are left out. + scope :code_of_conduct_pending, lambda { + slack_member.where(slack_coc_accepted_at: nil, code_of_conduct_exempt: false).where.not(slack_id: nil) + } # Names the Slack import saves when a Slack profile has no name. PLACEHOLDER_NAMES = %w[NOTSET Unknown User].freeze diff --git a/app/serializers/user_serializer.rb b/app/serializers/user_serializer.rb index bcef621..7565eb8 100644 --- a/app/serializers/user_serializer.rb +++ b/app/serializers/user_serializer.rb @@ -8,6 +8,7 @@ # id :bigint not null, primary key # acknowledged_over_13_at :datetime # birthday :date +# code_of_conduct_exempt :boolean default(FALSE), not null # confirmation_sent_at :datetime # confirmation_token :string # email :string not null diff --git a/app/services/app_access.rb b/app/services/app_access.rb index 1c3e593..e9c104e 100644 --- a/app/services/app_access.rb +++ b/app/services/app_access.rb @@ -9,6 +9,10 @@ # # There is no admin bypass: an admin needs a grant like anyone else. # +# A user who has not accepted the code of conduct may use no app, open or +# restricted. An admin can opt out an app (requires_code_of_conduct) or a user +# (code_of_conduct_exempt). +# # Every place that hands out or honors a user's token asks this module: # the authorize endpoint, the token endpoint (code exchange and refresh), # introspection, and userinfo. Checking at consent alone is not enough, @@ -24,6 +28,7 @@ def to_s when :group then "Member of #{groups.map(&:name).to_sentence}" when :role then "Holds the #{roles.map(&:name).to_sentence} role" when :no_user then "No user" + when :code_of_conduct then "Has not accepted the Code of Conduct" else "No grant for this user or their groups" end end @@ -33,6 +38,7 @@ class << self def permitted?(user, application) = explain(user, application).permitted? def explain(user, application) + return decision(false, :code_of_conduct) if code_of_conduct_missing?(user, application) return decision(true, :open) unless restricted?(application) return decision(false, :no_user) if user.nil? @@ -50,6 +56,11 @@ def explain(user, application) def restricted?(application) = application.access_policy != "everyone" + def code_of_conduct_missing?(user, application) + user.present? && application.requires_code_of_conduct && !user.code_of_conduct_exempt && + user.slack_coc_accepted_at.nil? + end + # Groups linked to an app by an access grant or a role assignment. These # are the only groups the app may see (the `groups` claim, the directory). def linked_groups(application) diff --git a/app/views/admin/oauth_applications/index.html.erb b/app/views/admin/oauth_applications/index.html.erb index 3343817..2c56756 100644 --- a/app/views/admin/oauth_applications/index.html.erb +++ b/app/views/admin/oauth_applications/index.html.erb @@ -42,6 +42,9 @@ <% if AppAccess.restricted?(application) %> Restricted <% end %> + <% unless application.requires_code_of_conduct %> + CoC optional + <% end %> <% if application.scopes.present? %> diff --git a/app/views/admin/oauth_applications/show.html.erb b/app/views/admin/oauth_applications/show.html.erb index bed0f45..5d339c9 100644 --- a/app/views/admin/oauth_applications/show.html.erb +++ b/app/views/admin/oauth_applications/show.html.erb @@ -169,6 +169,27 @@

No grants yet. Nobody can use this app.

<% end %> +
+

+ <% if @application.requires_code_of_conduct %> + Users must accept the Code of Conduct before they can use this app. + <% else %> + This app is opted out: users who have not accepted the Code of Conduct can use it. + <% end %> +

+ <% if current_user.superadmin? %> + <% if @application.requires_code_of_conduct %> + <%= button_to "Opt out of the Code of Conduct", code_of_conduct_admin_oauth_application_path(@application), + method: :patch, params: { required: false }, class: "btn-stitch-ghost", + data: { turbo_confirm: "Let users who have not accepted the Code of Conduct use #{@application.name}?" } %> + <% else %> + <%= button_to "Require the Code of Conduct", code_of_conduct_admin_oauth_application_path(@application), + method: :patch, params: { required: true }, class: "btn-stitch-ghost", + data: { turbo_confirm: "Users who have not accepted the Code of Conduct will lose access to #{@application.name}. Continue?" } %> + <% end %> + <% end %> +
+ <% if current_user.superadmin? %>
<%= form_with scope: :access_grant, url: admin_oauth_application_access_grants_path(@application), local: true, diff --git a/app/views/admin/users/show.html.erb b/app/views/admin/users/show.html.erb index 6a1e7d6..97744f5 100644 --- a/app/views/admin/users/show.html.erb +++ b/app/views/admin/users/show.html.erb @@ -210,6 +210,24 @@

App Access

+
+

Code of Conduct

+

+ <% if @user.slack_coc_accepted_at.present? %> + Accepted <%= @user.slack_coc_accepted_at.strftime("%b %-d, %Y") %>. + <% elsif @user.code_of_conduct_exempt %> + Not accepted. This user is exempt, so they can use apps anyway. + <% else %> + Not accepted. This user can only use apps that are opted out of the Code of Conduct. + <% end %> +

+ <% if current_user.superadmin? && @user.slack_coc_accepted_at.nil? %> + <%= button_to (@user.code_of_conduct_exempt ? "Remove the exemption" : "Exempt from the Code of Conduct"), + code_of_conduct_exemption_admin_user_path(@user), + method: :patch, params: { exempt: !@user.code_of_conduct_exempt }, class: "btn-stitch-ghost mt-2", + data: { turbo_confirm: @user.code_of_conduct_exempt ? "#{@user.full_name} will lose access to apps until they accept. Continue?" : "Let #{@user.full_name} use apps without accepting the Code of Conduct?" } %> + <% end %> +
<% restricted_apps = Doorkeeper::Application.where.not(access_policy: "everyone").order(:name) %> <% if restricted_apps.any? %> -

Apps not listed are open to everyone who can sign in.

+

Apps not listed are open to everyone who can sign in and meets the Code of Conduct requirement.

<% else %>

Every app is open to everyone who can sign in.

<% end %> diff --git a/app/views/doorkeeper/authorizations/access_denied.html.erb b/app/views/doorkeeper/authorizations/access_denied.html.erb index 4b9dc2e..4a056d5 100644 --- a/app/views/doorkeeper/authorizations/access_denied.html.erb +++ b/app/views/doorkeeper/authorizations/access_denied.html.erb @@ -14,13 +14,23 @@

-

- Access to this app is limited to some people and groups. If you need it, ask a Weave admin to give you access. -

+ <% if @decision&.reason == :code_of_conduct %> +

+ Patchwork Labs apps are for members who have accepted the Code of Conduct. +

-
- <%= link_to "Go to your Weave account", root_path, class: "btn-stitch-ghost" %> -
+
+ <%= link_to "Accept the Code of Conduct", slack_onboarding_path, class: "btn-stitch" %> +
+ <% else %> +

+ Access to this app is limited to some people and groups. If you need it, ask a Weave admin to give you access. +

+ +
+ <%= link_to "Go to your Weave account", root_path, class: "btn-stitch-ghost" %> +
+ <% end %>
diff --git a/config/routes.rb b/config/routes.rb index 8d6674e..bd21da3 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -77,6 +77,7 @@ # impersonate_admin_user POST /admin/users/:id/impersonate(.:format) admin/users#impersonate # regen_pid_admin_user PATCH /admin/users/:id/regen_pid(.:format) admin/users#regen_pid # invite_to_slack_admin_user POST /admin/users/:id/invite_to_slack(.:format) admin/users#invite_to_slack +# code_of_conduct_exemption_admin_user PATCH /admin/users/:id/code_of_conduct_exemption(.:format) admin/users#code_of_conduct_exemption # admin_users GET /admin/users(.:format) admin/users#index # POST /admin/users(.:format) admin/users#create # new_admin_user GET /admin/users/new(.:format) admin/users#new @@ -130,6 +131,7 @@ # DELETE /admin/services/:id(.:format) admin/services#destroy # regenerate_secret_admin_oauth_application PATCH /admin/oauth_applications/:id/regenerate_secret(.:format) admin/oauth_applications#regenerate_secret # access_policy_admin_oauth_application PATCH /admin/oauth_applications/:id/access_policy(.:format) admin/oauth_applications#access_policy +# code_of_conduct_admin_oauth_application PATCH /admin/oauth_applications/:id/code_of_conduct(.:format) admin/oauth_applications#code_of_conduct # admin_oauth_application_access_grants POST /admin/oauth_applications/:oauth_application_id/access_grants(.:format) admin/oauth_application_access_grants#create # admin_oauth_application_access_grant DELETE /admin/oauth_applications/:oauth_application_id/access_grants/:id(.:format) admin/oauth_application_access_grants#destroy # admin_oauth_application_role_assignments POST /admin/oauth_applications/:oauth_application_id/roles/:role_id/assignments(.:format) admin/oauth_application_role_assignments#create @@ -442,6 +444,7 @@ post :impersonate patch :regen_pid post :invite_to_slack + patch :code_of_conduct_exemption end end @@ -477,6 +480,7 @@ member do patch :regenerate_secret patch :access_policy + patch :code_of_conduct end resources :access_grants, controller: "oauth_application_access_grants", only: [:create, :destroy] diff --git a/db/migrate/20261009130000_add_requires_code_of_conduct_to_oauth_applications.rb b/db/migrate/20261009130000_add_requires_code_of_conduct_to_oauth_applications.rb new file mode 100644 index 0000000..6c9d4dc --- /dev/null +++ b/db/migrate/20261009130000_add_requires_code_of_conduct_to_oauth_applications.rb @@ -0,0 +1,9 @@ +# frozen_string_literal: true + +class AddRequiresCodeOfConductToOauthApplications < ActiveRecord::Migration[8.1] + def change + # Every app requires the code of conduct unless an admin opts it out (see + # AppAccess). + add_column :oauth_applications, :requires_code_of_conduct, :boolean, default: true, null: false + end +end diff --git a/db/migrate/20261009140000_add_code_of_conduct_exempt_to_users.rb b/db/migrate/20261009140000_add_code_of_conduct_exempt_to_users.rb new file mode 100644 index 0000000..727f770 --- /dev/null +++ b/db/migrate/20261009140000_add_code_of_conduct_exempt_to_users.rb @@ -0,0 +1,9 @@ +# frozen_string_literal: true + +class AddCodeOfConductExemptToUsers < ActiveRecord::Migration[8.1] + def change + # An admin can let one user use apps without accepting the code of conduct + # (see AppAccess). Exempt users are not asked to accept or demoted. + add_column :users, :code_of_conduct_exempt, :boolean, default: false, null: false + end +end diff --git a/db/schema.rb b/db/schema.rb index eb76105..3f3e977 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -1,6 +1,5 @@ # frozen_string_literal: true - # This file is auto-generated from the current state of the database. Instead # of editing this file, please use the migrations feature of Active Record to # incrementally modify your database, and then regenerate this schema definition. @@ -13,7 +12,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[8.1].define(version: 2026_10_09_120000) do +ActiveRecord::Schema[8.1].define(version: 2026_10_09_140000) do # These are extensions that must be enabled in order to support this database enable_extension "fuzzystrmatch" enable_extension "pg_catalog.plpgsql" @@ -414,6 +413,7 @@ t.datetime "created_at", null: false t.string "name", null: false t.text "redirect_uri", null: false + t.boolean "requires_code_of_conduct", default: true, null: false t.string "scopes", default: "", null: false t.string "secret", null: false t.string "uid", null: false @@ -566,6 +566,7 @@ create_table "users", force: :cascade do |t| t.datetime "acknowledged_over_13_at" t.date "birthday" + t.boolean "code_of_conduct_exempt", default: false, null: false t.datetime "confirmation_sent_at" t.string "confirmation_token" t.datetime "created_at", null: false diff --git a/spec/factories/users.rb b/spec/factories/users.rb index 3a2cdd9..297d1bb 100644 --- a/spec/factories/users.rb +++ b/spec/factories/users.rb @@ -8,6 +8,7 @@ # id :bigint not null, primary key # acknowledged_over_13_at :datetime # birthday :date +# code_of_conduct_exempt :boolean default(FALSE), not null # confirmation_sent_at :datetime # confirmation_token :string # email :string not null @@ -100,6 +101,12 @@ email_confirmed_at { nil } end + # Every OAuth app requires the code of conduct unless it is opted out (see + # AppAccess). + trait :accepted_code_of_conduct do + slack_coc_accepted_at { 1.day.ago } + end + trait :with_magic_link do transient do magic_link_token { SecureRandom.urlsafe_base64(32) } diff --git a/spec/jobs/revoke_lost_app_access_job_spec.rb b/spec/jobs/revoke_lost_app_access_job_spec.rb index 80d0e18..2a8bc6b 100644 --- a/spec/jobs/revoke_lost_app_access_job_spec.rb +++ b/spec/jobs/revoke_lost_app_access_job_spec.rb @@ -3,7 +3,7 @@ require "rails_helper" RSpec.describe RevokeLostAppAccessJob do - let(:user) { create(:user, :verified) } + let(:user) { create(:user, :verified, :accepted_code_of_conduct) } let(:group) { create(:group) } let(:application) do Doorkeeper::Application.create!(name: "Wiki", redirect_uri: "https://wiki.example.com/cb", access_policy: "restricted") @@ -44,7 +44,7 @@ def code_for(app) end it "checks every user of an app when given the app" do - other = create(:user, :verified) + other = create(:user, :verified, :accepted_code_of_conduct) ApplicationAccessGrant.create!(application: application, grantee: other) kept = token_for(application, other) lost = token_for(application) diff --git a/spec/models/user_slack_membership_spec.rb b/spec/models/user_slack_membership_spec.rb index 5a73454..6e0095f 100644 --- a/spec/models/user_slack_membership_spec.rb +++ b/spec/models/user_slack_membership_spec.rb @@ -90,6 +90,7 @@ legacy = create(:user, slack_id: "U1", slack_membership: "member") create(:user, slack_id: "U2", slack_membership: "member", slack_coc_accepted_at: 1.day.ago) create(:user, slack_id: "U3") + create(:user, slack_id: "U4", slack_membership: "member", code_of_conduct_exempt: true) create(:user) expect(described_class.code_of_conduct_pending).to contain_exactly(legacy) diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 4207f45..298cec2 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -8,6 +8,7 @@ # id :bigint not null, primary key # acknowledged_over_13_at :datetime # birthday :date +# code_of_conduct_exempt :boolean default(FALSE), not null # confirmation_sent_at :datetime # confirmation_token :string # email :string not null diff --git a/spec/requests/admin_oauth_application_access_spec.rb b/spec/requests/admin_oauth_application_access_spec.rb index 764e5df..84dbdc8 100644 --- a/spec/requests/admin_oauth_application_access_spec.rb +++ b/spec/requests/admin_oauth_application_access_spec.rb @@ -7,7 +7,7 @@ RSpec.describe "Admin OAuth application access", type: :request do let(:admin) { create(:user, :admin, :verified) } let(:superadmin) { create(:user, :superadmin, :verified) } - let(:member) { create(:user, :verified) } + let(:member) { create(:user, :verified, :accepted_code_of_conduct) } let(:group) { create(:group, name: "Engineering") } let(:application) { Doorkeeper::Application.create!(name: "Wiki", redirect_uri: "https://wiki.example.com/cb") } @@ -115,4 +115,46 @@ expect(response.body).to include("Wiki", "Member of Engineering", "Payroll", "No grant for this user or their groups") end end + + describe "the code-of-conduct requirement" do + it "lets a superadmin opt an app out and back in" do + sign_in_via_magic_link(superadmin) + + patch code_of_conduct_admin_oauth_application_path(application), params: { required: false } + expect(application.reload.requires_code_of_conduct).to be(false) + + expect { patch code_of_conduct_admin_oauth_application_path(application), params: { required: true } } + .to have_enqueued_job(RevokeLostAppAccessJob).with(application_id: application.id) + expect(application.reload.requires_code_of_conduct).to be(true) + end + + it "does not let a plain admin change it" do + sign_in_via_magic_link(admin) + + patch code_of_conduct_admin_oauth_application_path(application), params: { required: false } + + expect(application.reload.requires_code_of_conduct).to be(true) + end + end + + describe "the code-of-conduct exemption for one user" do + it "lets a superadmin exempt a user and take the exemption away" do + sign_in_via_magic_link(superadmin) + + patch code_of_conduct_exemption_admin_user_path(member), params: { exempt: true } + expect(member.reload.code_of_conduct_exempt).to be(true) + + expect { patch code_of_conduct_exemption_admin_user_path(member), params: { exempt: false } } + .to have_enqueued_job(RevokeLostAppAccessJob).with(user_id: member.id) + expect(member.reload.code_of_conduct_exempt).to be(false) + end + + it "does not let a plain admin change it" do + sign_in_via_magic_link(admin) + + patch code_of_conduct_exemption_admin_user_path(member), params: { exempt: true } + + expect(member.reload.code_of_conduct_exempt).to be(false) + end + end end diff --git a/spec/requests/api/directory_spec.rb b/spec/requests/api/directory_spec.rb index 55e51e7..2e9ef19 100644 --- a/spec/requests/api/directory_spec.rb +++ b/spec/requests/api/directory_spec.rb @@ -9,7 +9,7 @@ Doorkeeper::Application.create!(name: "Krater", redirect_uri: "https://krater.example.com/cb", scopes: "openid profile groups roles directory", access_policy: "restricted") end - let(:reviewer) { create(:user, :verified, slack_id: "U0REVIEW") } + let(:reviewer) { create(:user, :verified, :accepted_code_of_conduct, slack_id: "U0REVIEW") } let(:reviewers) { create(:group, name: "Krater Reviewers") } let(:role) { ApplicationRole.create!(application: application, key: "reviewer", name: "Reviewer") } @@ -51,7 +51,7 @@ def directory_get(path, token: app_token, params: {}) end it "is not found for a user the app may not serve" do - directory_get(api_v1_directory_user_path(create(:user, :verified).p_id)) + directory_get(api_v1_directory_user_path(create(:user, :verified, :accepted_code_of_conduct).p_id)) expect(response).to have_http_status(:not_found) end @@ -72,7 +72,7 @@ def directory_get(path, token: app_token, params: {}) describe "GET /api/v1/directory/users" do it "lists users who hold a role, directly or through a group" do - direct = create(:user, :verified) + direct = create(:user, :verified, :accepted_code_of_conduct) ApplicationRoleAssignment.create!(role: role, assignee: direct) body = directory_get(api_v1_directory_users_path, params: { role: "reviewer" }) diff --git a/spec/requests/oauth/account_state_spec.rb b/spec/requests/oauth/account_state_spec.rb index 0329877..64051a4 100644 --- a/spec/requests/oauth/account_state_spec.rb +++ b/spec/requests/oauth/account_state_spec.rb @@ -9,7 +9,7 @@ # deactivated user with a browser cookie could still complete an OAuth sign-in # to every client, and their refresh tokens kept minting access tokens forever. RSpec.describe "OAuth access for accounts that can no longer sign in", type: :request do - let(:user) { create(:user, :verified) } + let(:user) { create(:user, :verified, :accepted_code_of_conduct) } let(:redirect_uri) { "https://client.example.com/callback" } let(:code_verifier) { SecureRandom.urlsafe_base64(64) } diff --git a/spec/requests/oauth/app_access_spec.rb b/spec/requests/oauth/app_access_spec.rb index b7d09fe..9be1b88 100644 --- a/spec/requests/oauth/app_access_spec.rb +++ b/spec/requests/oauth/app_access_spec.rb @@ -7,7 +7,7 @@ # tokens, userinfo and introspection working after access is removed, so # every endpoint that hands out or honors a user's token checks too. RSpec.describe "OAuth access to restricted apps", type: :request do - let(:user) { create(:user, :verified) } + let(:user) { create(:user, :verified, :accepted_code_of_conduct) } let(:group) { create(:group) } let(:redirect_uri) { "https://client.example.com/callback" } let(:code_verifier) { SecureRandom.urlsafe_base64(64) } @@ -89,6 +89,53 @@ def remove_access = Group::Membership.where(user: user).delete_all expect(response.body).to include("Authorization required") end + describe "the code-of-conduct requirement" do + before { user.update!(slack_coc_accepted_at: nil) } + + it "sends a user who has not accepted to accept, then back to the app" do + user.update!(slack_id: "U1", slack_membership: "member") + sign_in_via_magic_link(user) + + get oauth_authorization_path, params: authorization_params + + expect(response).to redirect_to(slack_onboarding_path) + authorize_path = URI.parse(request.url).request_uri + + post accept_code_of_conduct_slack_onboarding_path, params: { accept: "1" } + + expect(response).to redirect_to(authorize_path) + follow_redirect! + expect(response.body).to include("Authorization required") + end + + it "refuses the consent POST" do + sign_in_via_magic_link(user) + + post oauth_authorization_path, params: authorization_params + + expect(response).to have_http_status(:forbidden) + expect(response.body).to include("Accept the Code of Conduct") + end + + it "lets the user into an app that is opted out" do + application.update!(requires_code_of_conduct: false) + sign_in_via_magic_link(user) + + get oauth_authorization_path, params: authorization_params + + expect(response).to have_http_status(:ok) + end + + it "stops a refresh token once acceptance is required" do + user.update!(slack_coc_accepted_at: 1.day.ago) + tokens = obtain_tokens + user.update_columns(slack_coc_accepted_at: nil) # rubocop:disable Rails/SkipsModelValidations + + expect(refresh(tokens["refresh_token"])["error"]).to be_present + expect(userinfo_status(tokens["access_token"])).to eq(401) + end + end + describe "the authorization endpoint" do before { restrict! } diff --git a/spec/requests/oauth/authorization_consent_spec.rb b/spec/requests/oauth/authorization_consent_spec.rb index 683624d..01b26bf 100644 --- a/spec/requests/oauth/authorization_consent_spec.rb +++ b/spec/requests/oauth/authorization_consent_spec.rb @@ -7,7 +7,7 @@ # consent forms round-trip every pre-auth parameter — losing the OIDC nonce # here would silently break clients that validate it in the id_token. RSpec.describe "OAuth authorization consent screen", type: :request do - let(:user) { create(:user, :verified) } + let(:user) { create(:user, :verified, :accepted_code_of_conduct) } let(:application) do Doorkeeper::Application.create!( diff --git a/spec/requests/oauth/login_screen_spec.rb b/spec/requests/oauth/login_screen_spec.rb index ebf8c29..2fc907a 100644 --- a/spec/requests/oauth/login_screen_spec.rb +++ b/spec/requests/oauth/login_screen_spec.rb @@ -14,7 +14,7 @@ # itself is remembered, that return lands on the dashboard and the client waits # for a callback that never arrives. RSpec.describe "OAuth login screen", type: :request do - let(:user) { create(:user, :verified) } + let(:user) { create(:user, :verified, :accepted_code_of_conduct) } let(:application) do Doorkeeper::Application.create!( @@ -105,7 +105,7 @@ def authorization_target # the authorize request goes through instead of stalling behind a second # email the client would never wait for. it "carries an unconfirmed account through to consent" do - unconfirmed = create(:user, :unverified) + unconfirmed = create(:user, :unverified, :accepted_code_of_conduct) get oauth_authorization_path, params: authorization_params sign_in_via_magic_link(unconfirmed) diff --git a/spec/requests/oauth/openid_connect_spec.rb b/spec/requests/oauth/openid_connect_spec.rb index 25a326f..250ca8d 100644 --- a/spec/requests/oauth/openid_connect_spec.rb +++ b/spec/requests/oauth/openid_connect_spec.rb @@ -8,7 +8,7 @@ # suite exists to prevent was precisely a gap between what we claimed to do and # what we actually did. RSpec.describe "OpenID Connect", type: :request do - let(:user) { create(:user, :verified, phone_number: "+18025550123") } + let(:user) { create(:user, :verified, :accepted_code_of_conduct, phone_number: "+18025550123") } # Public (non-confidential) client, so `force_pkce` applies and the token # exchange is authenticated by the code_verifier rather than a secret. diff --git a/spec/requests/slack_membership_flow_spec.rb b/spec/requests/slack_membership_flow_spec.rb index 105c852..f726a8f 100644 --- a/spec/requests/slack_membership_flow_spec.rb +++ b/spec/requests/slack_membership_flow_spec.rb @@ -18,9 +18,12 @@ ) end + # Opted out of the code-of-conduct requirement, so the claims can be read + # before the user accepts (see AppAccess). let(:application) do Doorkeeper::Application.create!( - name: "Krater", redirect_uri: "https://krater.example.com/callback", scopes: "openid profile slack" + name: "Krater", redirect_uri: "https://krater.example.com/callback", scopes: "openid profile slack", + requires_code_of_conduct: false ) end @@ -73,7 +76,7 @@ def slack_claims(user) end it "makes an existing account a member when the Slack sync finds it as a full member" do - user = create(:user, :verified, email: "known@example.com", slack_id: "U0KNOWN") + user = create(:user, :verified, :accepted_code_of_conduct, email: "known@example.com", slack_id: "U0KNOWN") service = SlackService.allocate allow(service).to receive_messages(configured?: true, list_members: [ { "id" => "U0KNOWN", "updated" => 1.year.ago.to_i, "profile" => { "email" => "known@example.com" } } diff --git a/spec/serializers/user_serializer_spec.rb b/spec/serializers/user_serializer_spec.rb index 3fd6b56..4d13374 100644 --- a/spec/serializers/user_serializer_spec.rb +++ b/spec/serializers/user_serializer_spec.rb @@ -8,6 +8,7 @@ # id :bigint not null, primary key # acknowledged_over_13_at :datetime # birthday :date +# code_of_conduct_exempt :boolean default(FALSE), not null # confirmation_sent_at :datetime # confirmation_token :string # email :string not null diff --git a/spec/services/app_access_spec.rb b/spec/services/app_access_spec.rb index f51f91f..6e9fda2 100644 --- a/spec/services/app_access_spec.rb +++ b/spec/services/app_access_spec.rb @@ -3,7 +3,7 @@ require "rails_helper" RSpec.describe AppAccess do - let(:user) { create(:user, :verified) } + let(:user) { create(:user, :verified, :accepted_code_of_conduct) } let(:application) do Doorkeeper::Application.create!(name: "Client", redirect_uri: "https://client.example.com/cb", access_policy: "restricted") end @@ -145,4 +145,61 @@ def token_for(owner) = Doorkeeper::AccessToken.create!(application: application, expect(described_class.token_usable?(token_for(user))).to be(true) end end + + describe "the code-of-conduct requirement" do + let(:user) { create(:user, :verified, slack_coc_accepted_at: nil) } + + before { application.update!(access_policy: "everyone") } + + it "refuses a user who has not accepted, even for an open app" do + decision = described_class.explain(user, application) + + expect(decision).not_to be_permitted + expect(decision.to_s).to eq("Has not accepted the Code of Conduct") + end + + it "refuses a user who has not accepted, even with a grant" do + application.update!(access_policy: "restricted") + grant(user) + + expect(described_class.permitted?(user, application)).to be(false) + end + + it "lets a user who accepted use the app" do + user.update!(slack_coc_accepted_at: 1.day.ago) + + expect(described_class.permitted?(user, application)).to be(true) + end + + it "lets anyone use an app that is opted out" do + application.update!(requires_code_of_conduct: false) + + expect(described_class.permitted?(user, application)).to be(true) + end + + it "lets an exempt user use any app" do + user.update!(code_of_conduct_exempt: true) + + expect(described_class.permitted?(user, application)).to be(true) + end + + it "still applies the access policy to an exempt user" do + user.update!(code_of_conduct_exempt: true) + application.update!(access_policy: "restricted") + + expect(described_class.permitted?(user, application)).to be(false) + end + + it "still applies the access policy to an app that is opted out" do + application.update!(requires_code_of_conduct: false, access_policy: "restricted") + + expect(described_class.permitted?(user, application)).to be(false) + end + + it "stops the user's tokens" do + token = Doorkeeper::AccessToken.create!(application: application, resource_owner_id: user.id) + + expect(described_class.token_usable?(token)).to be(false) + end + end end diff --git a/test/fixtures/users.yml b/test/fixtures/users.yml index 4f65253..61376e8 100644 --- a/test/fixtures/users.yml +++ b/test/fixtures/users.yml @@ -12,6 +12,7 @@ # id :bigint not null, primary key # acknowledged_over_13_at :datetime # birthday :date +# code_of_conduct_exempt :boolean default(FALSE), not null # confirmation_sent_at :datetime # confirmation_token :string # email :string not null diff --git a/test/models/user_test.rb b/test/models/user_test.rb index e06733a..e974bc0 100644 --- a/test/models/user_test.rb +++ b/test/models/user_test.rb @@ -8,6 +8,7 @@ # id :bigint not null, primary key # acknowledged_over_13_at :datetime # birthday :date +# code_of_conduct_exempt :boolean default(FALSE), not null # confirmation_sent_at :datetime # confirmation_token :string # email :string not null