Skip to content

Consolidate SAML/OIDC redirect JavaScript #9985

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
aduth merged 6 commits into from
Jan 30, 2024
Merged

Consolidate SAML/OIDC redirect JavaScript #9985

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
d744af1
Consolidate SAML/OIDC redirect JavaScript
aduth Jan 26, 2024
934eccc
Use classList#replace for no-js replacement
aduth Jan 29, 2024
3594113
Use consistent click_immediate attribute value
aduth Jan 29, 2024
c928e52
Remove unnecessary hidden field for tests
aduth Jan 29, 2024
3900738
Fix helper for OIDC redirect link
aduth Jan 29, 2024
cf1c923
Improve spec helper resilience to HTML tag attributes
aduth Jan 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions app/javascript/packs/click-immediate.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
document
.querySelectorAll<HTMLElement>('[data-click-immediate]')
.forEach((element) => element.click());
5 changes: 0 additions & 5 deletions app/javascript/packs/openid-connect-redirect.ts
View file
Open in desktop

This file was deleted.

4 changes: 0 additions & 4 deletions app/javascript/packs/saml-post.js
View file
Open in desktop

This file was deleted.

2 changes: 1 addition & 1 deletion app/views/layouts/base.html.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
<title><%= title %> | <%= APP_NAME %></title>

<%= javascript_tag(nonce: true) do %>
document.documentElement.className = document.documentElement.className.replace(/\bno-js\b/, 'js');
document.documentElement.classList.replace('no-js', 'js');
<% end %>
<%= preload_link_tag font_url('public-sans/PublicSans-Bold.woff2') %>
<%= preload_link_tag font_url('public-sans/PublicSans-Regular.woff2') %>
Expand Down
11 changes: 7 additions & 4 deletions app/views/openid_connect/shared/redirect_js.html.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,16 @@
<!DOCTYPE html>
<html>
<html class="no-js">
Copy link
Copy Markdown
Contributor Author

@aduth aduth Jan 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It'd be nice to have a base-ier base template that we could extend, to avoid having to bring in these individual elements.

There's probably some other features from the base template we want here too, like the lang attribute, some of the meta tags, etc.

<head>
<meta charset="utf-8" />
<title><%= t('headings.redirecting') %> | <%= APP_NAME %></title>
<%= javascript_tag(nonce: true) do %>
document.documentElement.classList.replace('no-js', 'js');
<% end %>
<%= stylesheet_link_tag 'application', media: 'all' %>
<%= render_stylesheet_once_tags %>
</head>
<body class="tablet:bg-primary-lighter">
<div class="grid-container tablet:padding-y-6">
<div class="grid-container tablet:padding-y-6 no-js">
<div class="grid-row">
<div class="tablet:grid-col-6 tablet:grid-offset-3">
<%= render PageHeadingComponent.new.with_content(t('saml_idp.shared.saml_post_binding.heading')) %>
Expand All @@ -16,10 +19,10 @@
<%= t('saml_idp.shared.saml_post_binding.no_js') %>
</p>

<%= link_to(t('forms.buttons.continue'), @oidc_redirect_uri, class: 'usa-button usa-button--wide usa-button--big', id: 'openid-connect-redirect') %>
<%= link_to(t('forms.buttons.continue'), @oidc_redirect_uri, class: 'usa-button usa-button--wide usa-button--big', data: { click_immediate: '' }) %>
</div>
</div>
</div>
<%= render_javascript_pack_once_tags 'openid-connect-redirect' %>
<%= render_javascript_pack_once_tags 'click-immediate' %>
</body>
</html>
14 changes: 8 additions & 6 deletions app/views/saml_idp/shared/saml_post_binding.html.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,15 +1,17 @@
<!DOCTYPE html>
<html>
<html class="no-js">
<head>
<meta charset="utf-8" />
<title><%= t('headings.redirecting') %> | <%= APP_NAME %></title>
<%= javascript_tag(nonce: true) do %>
document.documentElement.classList.replace('no-js', 'js');
<% end %>
<%= csrf_meta_tags %>
<%= stylesheet_link_tag 'application', media: 'all' %>
<%= render_stylesheet_once_tags %>
<%= render_javascript_pack_once_tags 'saml-post' %>
</head>
<body>
<div class="grid-container tablet:padding-y-6">
<div class="grid-container tablet:padding-y-6 no-js">
<div class="grid-row">
<div class="tablet:grid-col-6 tablet:grid-offset-3">
<%= render PageHeadingComponent.new.with_content(t('.heading')) %>
Expand All @@ -18,16 +20,16 @@
<%= t('.no_js') %>
</p>

<%= form_tag action_url, id: 'saml-post-binding' do %>
<%= hidden_field_tag('csp_uris', csp_uris) if Rails.env.test? %>
<%= simple_form_for('', url: action_url) do |f| %>
<%= hidden_field_tag(type, message) %>
<% if params.key?(:RelayState) %>
<%= hidden_field_tag('RelayState', params[:RelayState]) %>
<% end %>
Comment on lines 25 to 27
Copy link
Copy Markdown
Contributor

@zachmargolis zachmargolis Jan 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could we reuse saml_post_form.html.erb here and pass in :RelayState as form_params?

Copy link
Copy Markdown
Contributor Author

@aduth aduth Jan 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could we reuse saml_post_form.html.erb here and pass in :RelayState as form_params?

Maybe! I can check

Copy link
Copy Markdown
Contributor Author

@aduth aduth Jan 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There was some prior discussion about this approach with a general leaning toward the duplication? Not sure I'd totally agree, but might save this for a separate effort.

https://github.com/18F/identity-idp/pull/5624/files#r753741166

<%= submit_tag t('forms.buttons.submit.default'), class: 'usa-button usa-button--wide usa-button--big' %>
<%= f.submit t('forms.buttons.submit.default'), data: { click_immediate: '' } %>
<% end %>
</div>
</div>
</div>
<%= render_javascript_pack_once_tags 'click-immediate' %>
</body>
</html>
13 changes: 8 additions & 5 deletions app/views/shared/saml_post_form.html.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,16 @@
<!DOCTYPE html>
<html>
<html class="no-js">
<head>
<meta charset="utf-8" />
<%= javascript_tag(nonce: true) do %>
document.documentElement.classList.replace('no-js', 'js');
<% end %>
<%= csrf_meta_tags %>
<%= stylesheet_link_tag 'application', media: 'all' %>
<%= render_stylesheet_once_tags %>
<%= render_javascript_pack_once_tags 'saml-post' %>
</head>
<body>
<div class="grid-container tablet:padding-y-6">
<div class="grid-container tablet:padding-y-6 no-js">
<div class="grid-row">
<div class="tablet:grid-col-6 tablet:grid-offset-3">
<%= render PageHeadingComponent.new.with_content(t('saml_idp.shared.saml_post_binding.heading')) %>
Expand All @@ -17,14 +19,15 @@
<%= t('saml_idp.shared.saml_post_binding.no_js') %>
</p>

<%= form_tag action_url, id: 'saml-post-binding' do %>
<%= simple_form_for('', url: action_url) do |f| %>
<% form_params.each do |key, val| %>
<%= hidden_field_tag(key, val) %>
<% end %>
<%= submit_tag t('forms.buttons.submit.default'), class: 'usa-button usa-button--wide usa-button--big' %>
<%= f.submit t('forms.buttons.submit.default'), data: { click_immediate: '' } %>
<% end %>
</div>
</div>
</div>
<%= render_javascript_pack_once_tags 'click-immediate' %>
</body>
</html>
3 changes: 1 addition & 2 deletions spec/support/oidc_auth_helper.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,8 +120,7 @@ def extract_meta_refresh_url
end

def extract_redirect_url
content = page.find('a#openid-connect-redirect')
content[:href]
page.find_link(t('forms.buttons.continue'))[:href]
end

def oidc_redirect_url
Expand Down
2 changes: 1 addition & 1 deletion spec/support/saml_response_doc.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ def input_id
def raw_xml_response
if @test_type == 'feature'
xml_response
elsif @response.body.include?('<html>')
elsif @response.body.include?('<html')
html_response
else
@response.body
Expand Down
2 changes: 1 addition & 1 deletion tsconfig.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,5 +25,5 @@
"./*.js",
"scripts"
],
"exclude": ["**/fixtures", "**/*.spec.js", "app/javascript/packs/saml-post.js"]
"exclude": ["**/fixtures", "**/*.spec.js"]
}