ylqjgm
/
canvas-lms
mirror of https://github.com/instructure/canvas-lms.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

Remove feature flag for lti advantage 

Fixes PLAT-4698

Test Plan:
-verify lti 1.3 and lti advantage flag is gone
-verify that all lti 1.3 features are still available/functional

Change-Id: I8be9e02fb2d32a92098bde43099ba0198e8d3329
Reviewed-on: https://gerrit.instructure.com/204381
Reviewed-by: Tucker Mcknight <tmcknight@instructure.com>
QA-Review: Tucker Mcknight <tmcknight@instructure.com>
Product-Review: Jesse Poulos <jpoulos@instructure.com>
Tested-by: Jenkins
This commit is contained in:
Drake Harper 2019-08-08 09:26:50 -06:00
parent a51cca64a5
commit d766d16326
25 changed files with 19 additions and 167 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

1
app/controllers/accounts_controller.rb
View File

@ -1005,7 +1005,6 @@ class AccountsController < ApplicationController
EXTERNAL_TOOLS_CREATE_URL: url_for(controller: :external_tools, action: :create, account_id: @context.id),
TOOL_CONFIGURATION_SHOW_URL: account_show_tool_configuration_url(account_id: @context.id, developer_key_id: ':developer_key_id'),
MEMBERSHIP_SERVICE_FEATURE_FLAG_ENABLED: @account.root_account.feature_enabled?(:membership_service_for_lti_tools),
LTI_13_TOOLS_FEATURE_FLAG_ENABLED: @account.root_account.feature_enabled?(:lti_1_3),
CONTEXT_BASE_URL: "/accounts/#{@context.id}",
MASKED_APP_CENTER_ACCESS_TOKEN: @account.settings[:app_center_access_token].try(:[], 0...5),
NEW_FEATURES_UI: @account.root_account.feature_enabled?(:new_features_ui),

1
app/controllers/courses_controller.rb
View File

@ -1293,7 +1293,6 @@ class CoursesController < ApplicationController
EXTERNAL_TOOLS_CREATE_URL: url_for(controller: :external_tools, action: :create, course_id: @context.id),
TOOL_CONFIGURATION_SHOW_URL: course_show_tool_configuration_url(course_id: @context.id, developer_key_id: ':developer_key_id'),
MEMBERSHIP_SERVICE_FEATURE_FLAG_ENABLED: @context.root_account.feature_enabled?(:membership_service_for_lti_tools),
LTI_13_TOOLS_FEATURE_FLAG_ENABLED: @context.root_account.feature_enabled?(:lti_1_3),
CONTEXT_BASE_URL: "/courses/#{@context.id}",
PUBLISHING_ENABLED: @publishing_enabled,
COURSE_IMAGES_ENABLED: @context.feature_enabled?(:course_card_images),

6
app/controllers/developer_keys_controller.rb
View File

@ -30,7 +30,6 @@ class DeveloperKeysController < ApplicationController
js_env(
accountEndpoint: api_v1_account_developer_keys_path(@context),
enableTestClusterChecks: DeveloperKey.test_cluster_checks_enabled?,
LTI_1_3_ENABLED: lti_1_3_enabled?,
validLtiScopes: TokenScopes::LTI_SCOPES,
validLtiPlacements: Lti::ResourcePlacement::PLACEMENTS
)
@ -86,11 +85,6 @@ class DeveloperKeysController < ApplicationController
private
def lti_1_3_enabled?
return true if @context == Account.site_admin
@context.root_account.feature_enabled?(:lti_1_3)
end
def index_scope
scope = if params[:inherited].present?
return DeveloperKey.none if @context.site_admin?

8
app/controllers/lti/ims/concerns/lti_services.rb
View File

@ -105,17 +105,9 @@ module Lti::Ims::Concerns
before_action(
:verify_access_token,
:verify_developer_key,
:verify_1_3_enabled,
:verify_access_scope
)
def verify_1_3_enabled
owner_account = developer_key.owner_account
return if owner_account.feature_enabled?(:lti_1_3) ||
(owner_account.site_admin? && owner_account.feature_allowed?(:lti_1_3))
render_error("LTI 1.3/Advantage features not enabled", :unauthorized)
end
def verify_access_token
if access_token.blank?
render_error("Missing access token", :unauthorized)

18
app/jsx/developer_keys/NewKeyTrigger.js
View File

@ -37,8 +37,6 @@ export default class DeveloperKeyModalTrigger extends React.Component {
this.props.store.dispatch(this.props.actions.developerKeysModalOpen('lti'))
}
isLti13Enabled = ENV.LTI_1_3_ENABLED
developerKeyMenuItem(title, onClick) {
return (
<MenuItem onClick={onClick} type="button">
@ -59,7 +57,6 @@ export default class DeveloperKeyModalTrigger extends React.Component {
return (
<Button
variant="primary"
onClick={this.isLti13Enabled ? () => {} : this.showCreateDeveloperKey}
buttonRef={this.props.setAddKeyButtonRef}
icon={IconPlusLine}
>
@ -70,15 +67,12 @@ export default class DeveloperKeyModalTrigger extends React.Component {
}
developerKeyTrigger() {
if (this.isLti13Enabled) {
return (
<Menu placement="bottom" trigger={this.triggerButton()} shouldHideOnSelect>
{this.developerKeyMenuItem(I18n.t('API Key'), this.showCreateDeveloperKey)}
{this.developerKeyMenuItem(I18n.t('LTI Key'), this.showCreateLtiKey)}
</Menu>
)
}
return this.triggerButton()
return (
<Menu placement="bottom" trigger={this.triggerButton()} shouldHideOnSelect>
{this.developerKeyMenuItem(I18n.t('API Key'), this.showCreateDeveloperKey)}
{this.developerKeyMenuItem(I18n.t('LTI Key'), this.showCreateLtiKey)}
</Menu>
)
}
render() {

4
app/jsx/developer_keys/__tests__/NewKeyTrigger.test.js
View File

@ -34,10 +34,6 @@ let wrapper = 'empty wrapper'
const menuContentsNode = () => wrapper.find('Portal').instance().DOMNode
beforeEach(() => {
window.ENV = {
LTI_1_3_ENABLED: true
}
wrapper = mount(
<DeveloperKeyModalTrigger store={store} actions={actions} setAddKeyButtonRef={() => {}} />
)

1
app/jsx/external_apps/components/AddExternalToolButton.js
View File

@ -256,7 +256,6 @@ export default class AddExternalToolButton extends React.Component {
handleSubmit={this.createTool}
hideComponent={this.state.isLti2}
membershipServiceFeatureFlagEnabled={window.ENV.MEMBERSHIP_SERVICE_FEATURE_FLAG_ENABLED}
lti13Enabled={ENV.LTI_13_TOOLS_FEATURE_FLAG_ENABLED}
>
<button type="button" className="Button" onClick={this.closeModal}>
{I18n.t('Cancel')}

3
app/jsx/external_apps/components/ConfigurationForm.js
View File

@ -35,13 +35,11 @@ export default class ConfigurationForm extends React.Component {
showConfigurationSelector: PropTypes.bool,
hideComponent: PropTypes.bool,
membershipServiceFeatureFlagEnabled: PropTypes.bool,
lti13Enabled: PropTypes.bool,
children: PropTypes.node
}
static defaultProps = {
showConfigurationSelector: true,
lti13Enabled: false
}
constructor(props, context) {
@ -226,7 +224,6 @@ export default class ConfigurationForm extends React.Component {
ref="configurationTypeSelector"
handleChange={this.handleSwitchConfigurationType}
configurationType={this.props.configurationType}
lti13Enabled={this.props.lti13Enabled}
/>
)
}

11
app/jsx/external_apps/components/ConfigurationTypeSelector.js
View File

@ -25,7 +25,6 @@ export default class ConfigurationTypeSelector extends React.Component {
static propTypes = {
handleChange: PropTypes.func.isRequired,
configurationType: PropTypes.string.isRequired,
lti13Enabled: PropTypes.bool.isRequired
}
componentDidMount() {
@ -35,14 +34,6 @@ export default class ConfigurationTypeSelector extends React.Component {
}
}
ltiAdvantageOption() {
if (this.props.lti13Enabled) {
return(
<option value="byClientId">{I18n.t('By Client ID')}</option>
)
}
}
render() {
return (
<div className="ConfigurationsTypeSelector">
@ -59,7 +50,7 @@ export default class ConfigurationTypeSelector extends React.Component {
<option value="manual">{I18n.t('Manual Entry')}</option>
<option value="url">{I18n.t('By URL')}</option>
<option value="xml">{I18n.t('Paste XML')}</option>
{this.ltiAdvantageOption()}
<option value="byClientId">{I18n.t('By Client ID')}</option>
<option value="lti2">{I18n.t('By LTI 2 Registration URL')}</option>
</select>
</label>

1
app/jsx/external_apps/components/__tests__/AppFilters.test.js
View File

@ -107,7 +107,6 @@ const defaultApps = () => [
let wrapper = 'empty wrapper'
beforeEach(() => {
window.ENV = {LTI_13_TOOLS_FEATURE_FLAG_ENABLED: true}
store.setState({apps: defaultApps()})
})

4
app/models/context_external_tool.rb
View File

@ -100,10 +100,6 @@ class ContextExternalTool < ActiveRecord::Base
settings['content_migration'].key?('import_start_url')
end
def lti_1_3_enabled?
use_1_3? && context.root_account.feature_enabled?(:lti_1_3)
end
def extension_setting(type, property = nil)
val = calclulate_extension_setting(type, property)
if val && property == :icon_url

7
config/feature_flags/00_standard.yml
View File

@ -393,13 +393,6 @@ non_scoring_rubrics:
description: If enabled, the option will be presented to have non-scoring rubrics.
applies_to: RootAccount
root_opt_in: true
lti_1_3:
state: allowed
display_name: LTI 1.3 and LTI Advantage
description: If enabled, access to LTI 1.3 and LTI Advantage will be enabled.
applies_to: RootAccount
root_opt_in: true
beta: true
assignments_2:
state: hidden
display_name: Assignments 2

1
lib/lti/messages/jwt_message.rb
View File

@ -147,7 +147,6 @@ module Lti::Messages
def include_names_and_roles_service_claims?
include_claims?(:names_and_roles_service) &&
(@context.is_a?(Course) || @context.is_a?(Group)) &&
@tool.lti_1_3_enabled? &&
@tool.developer_key&.scopes&.include?(TokenScopes::LTI_NRPS_V2_SCOPE)
end

1
lib/lti/messages/resource_link_request.rb
View File

@ -95,7 +95,6 @@ module Lti::Messages
def include_assignment_and_grade_service_claims?
include_claims?(:assignment_and_grade_service) &&
(@context.is_a?(Course) || @context.is_a?(Group)) &&
@tool.lti_1_3_enabled? &&
(@tool.developer_key.scopes & TokenScopes::LTI_AGS_SCOPES).present?
end

10
spec/controllers/developer_keys_controller_spec.rb
View File

@ -59,11 +59,6 @@ describe DeveloperKeysController do
end
end
it 'Should set LTI_1_3_ENABLED to true' do
get 'index', params: { account_id: Account.site_admin.id }
expect(assigns.dig(:js_env, :LTI_1_3_ENABLED)).to eq true
end
it 'should return the list of developer keys' do
dk = DeveloperKey.create!
get 'index', params: { account_id: Account.site_admin.id }, format: :json
@ -305,11 +300,6 @@ describe DeveloperKeysController do
allow_any_instance_of(Account).to receive(:feature_enabled?).and_return(false)
end
it 'Should set LTI_1_3_ENABLED to false' do
get 'index', params: {account_id: sub_account.id}
expect(assigns.dig(:js_env, :LTI_1_3_ENABLED)).to be_falsey
end
it 'responds with not found if the account is a sub account' do
allow(controller).to receive(:require_context_with_permission).and_return nil
get 'index', params: {account_id: sub_account.id}

2
spec/controllers/external_tools_controller_spec.rb
View File

@ -675,7 +675,7 @@ describe ExternalToolsController do
end
before do
lti_1_3_tool.context.root_account.enable_feature!(:lti_1_3)
lti_1_3_tool
user_session(@teacher)
end

24
spec/controllers/lti/ims/concerns/advantage_services_shared_context.rb
View File

@ -20,7 +20,7 @@ require File.expand_path(File.dirname(__FILE__) + '/../../../../spec_helper')
shared_context 'advantage services context' do
let_once(:root_account) do
enable_1_3(Account.default)
Account.default
end
let_once(:developer_key) do
dk = DeveloperKey.create!(account: root_account)
@ -105,26 +105,4 @@ shared_context 'advantage services context' do
reject { |s| scopes_to_remove.include? s }.
join(' ')
end
def enable_1_3(enableable)
if enableable.is_a?(ContextExternalTool)
enableable.use_1_3 = true
elsif enableable.is_a?(Account)
enableable.enable_feature!(:lti_1_3)
else raise "LTI 1.3/Advantage features not relevant for #{enableable.class}"
end
enableable.save!
enableable
end
def disable_1_3(enableable)
if enableable.is_a?(ContextExternalTool)
enableable.use_1_3 = false
elsif enableable.is_a?(Account)
enableable.disable_feature!(:lti_1_3)
else raise "LTI 1.3/Advantage features not relevant for #{enableable.class}"
end
enableable.save!
enableable
end
end

1
spec/controllers/lti/ims/concerns/gradebook_services_spec.rb
View File

@ -30,7 +30,6 @@ module Lti
:verify_developer_key,
:verify_tool,
:verify_active_in_account,
:verify_1_3_enabled,
:verify_access_scope
)

15
spec/controllers/lti/ims/concerns/lti_services_shared_examples.rb
View File

@ -93,7 +93,6 @@ shared_examples_for "lti services" do
let(:before_send_request) do
-> do
developer_key.update!(account: nil)
Account.site_admin.allow_feature!(:lti_1_3)
end
end
@ -255,20 +254,6 @@ shared_examples_for "lti services" do
end
end
context 'with disabled LTI 1.3/Advantage account-level features' do
# Would also work to just override :root_account, but let's have all the setup run w/ 1.3 enabled in case
# that has any side-effects, _then_ suddenly disable features before a LTI Advantage call arrives... as if a
# customer had a change of heart after initially turning on LTI/Advantage 1.3 features.
let(:before_send_request) { -> { disable_1_3(root_account) } }
it_behaves_like 'mime_type check'
it 'returns 401 unauthorized and complains about disabled LTI 1.3/Advantage features' do
expect(response).to have_http_status :unauthorized
expect(json).to be_lti_advantage_error_response_body('unauthorized', 'LTI 1.3/Advantage features not enabled')
end
end
it_behaves_like 'extra developer key and account tool check'
it_behaves_like 'extra developer key and course tool check'
end

10
spec/javascripts/jsx/developer_keys/AppSpec.js
View File

@ -306,8 +306,10 @@ test('displays the developer key on click of show key button', () => {
<DeveloperKeysApp {...props} />
)
wrapper.find('Button').at(1).simulate('click')
ok(wrapper.find('Popover').first().html().includes("Hide Key"))
btn = wrapper.find('table button').first()
ok(btn.html().includes('Show Key'))
btn.simulate('click')
ok(btn.html().includes('Hide Key'))
wrapper.unmount()
})
@ -336,10 +338,6 @@ test('renders the spinner', () => {
})
test('opens the key selection menu when the create button is clicked', () => {
window.ENV = {
LTI_1_3_ENABLED: true
}
const applicationState = {
listDeveloperKeyScopes,
createOrEditDeveloperKey: {},

4
spec/lib/lti/messages/jwt_message_spec.rb
View File

@ -360,10 +360,6 @@ describe Lti::Messages::JwtMessage do
end
let(:lti_advantage_service_claim) { raise 'Set in example' }
before(:each) do
course.root_account.enable_feature!(:lti_1_3)
course.root_account.save!
end
end
shared_context 'with lti advantage group context' do

4
spec/lib/lti/messages/resource_link_request_spec.rb
View File

@ -23,10 +23,6 @@ describe Lti::Messages::ResourceLinkRequest do
let(:tool_override) { nil }
before(:each) do
course.root_account.enable_feature!(:lti_1_3)
course.root_account.save!
end
# rubocop:enable RSpec/ScatteredLet
shared_examples 'disabled rlid claim group check' do

43
spec/models/context_external_tool_spec.rb
View File

@ -1610,49 +1610,6 @@ describe ContextExternalTool do
end
end
describe '#lti_1_3_enabled?' do
context 'when the tool is not configured for LTI 1.3 but the account is' do
let(:tool) do
@root_account.enable_feature!(:lti_1_3)
@root_account.save!
@root_account.context_external_tools.new(:name => "bob", :consumer_key => "bob", :shared_secret => "bob", :domain => "www.example.com")
end
it 'returns false' do
expect(tool).to_not be_lti_1_3_enabled
end
end
context 'when the account is not configured for LTI 1.3 but the tool is' do
let(:tool) do
tool = @root_account.context_external_tools.new(:name => "bob", :consumer_key => "bob", :shared_secret => "bob", :domain => "www.example.com")
tool.use_1_3 = true
tool.save!
tool
end
it 'returns false' do
expect(tool).to_not be_lti_1_3_enabled
end
end
context 'when both the tool and account are configured for LTI 1.3' do
let(:tool) do
@root_account.enable_feature!(:lti_1_3)
@root_account.save!
tool = @root_account.context_external_tools.new(:name => "bob", :consumer_key => "bob", :shared_secret => "bob", :domain => "www.example.com")
tool.settings['use_1_3'] = true
tool.save!
tool
end
it 'returns true' do
expect(tool).to be_lti_1_3_enabled
end
end
end
describe 'editor_button_json' do
let(:tool) { @root_account.context_external_tools.new(name: "editor thing", domain: "www.example.com") }

5
spec/selenium/admin/account_admin_developer_keys_rewrite_spec.rb
View File

@ -49,6 +49,7 @@ describe 'Developer Keys' do
get "/accounts/#{Account.default.id}/developer_keys"
find_button("Developer Key").click
find_button("API Key").click
f("input[name='developer_key[name]']").send_keys("Cool Tool")
f("input[name='developer_key[email]']").send_keys("admin@example.com")
f("textarea[name='developer_key[redirect_uris]']").send_keys("http://example.com")
@ -281,6 +282,7 @@ describe 'Developer Keys' do
it "does not have enforce scopes toggle activated on initial dev key creation" do
get "/accounts/#{Account.default.id}/developer_keys"
find_button("Developer Key").click
find_button("API Key").click
expect(f("span[data-automation='enforce_scopes']")).to contain_css("svg[name='IconX']")
expect(f("form")).to contain_jqcss("h2:contains('When scope enforcement is disabled, tokens have access to all endpoints available to the authorizing user.')")
end
@ -365,6 +367,7 @@ describe 'Developer Keys' do
it "keeps all endpoints read only checkbox checked after save" do
get "/accounts/#{Account.default.id}/developer_keys"
find_button("Developer Key").click
find_button("API Key").click
click_enforce_scopes
click_select_all_readonly_checkbox
find_button("Save Key").click
@ -390,6 +393,7 @@ describe 'Developer Keys' do
it "displays flash alert if scopes aren't selected when enforce scopes toggled" do
get "/accounts/#{Account.default.id}/developer_keys"
find_button("Developer Key").click
find_button("API Key").click
wait_for_ajaximations
click_enforce_scopes
wait_for_ajaximations
@ -408,6 +412,7 @@ describe 'Developer Keys' do
driver.navigate.back
wait_for_dev_key_modal_to_close
find_button("Developer Key").click
find_button("API Key").click
expect(f("input[name='developer_key[name]']").attribute('value')).not_to be_present
end

1
spec/selenium/helpers/developer_keys_rewrite_common.rb
View File

@ -55,6 +55,7 @@ module DeveloperKeysRewriteCommon
def expand_scope_group_by_filter(scope_group, context_id)
get "/accounts/#{context_id}/developer_keys"
find_button("Developer Key").click
find_button("API Key").click
click_enforce_scopes
filter_scopes_by_name(scope_group)
fj("[data-automation='toggle-scope-group'] span:contains('#{scope_group}')").click