Skip to content
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

Jump to bottom

Consolidate SAML/OIDC redirect JavaScript #9985

Merged
merged 6 commits into from
Jan 30, 2024
Merged

Consolidate SAML/OIDC redirect JavaScript #9985

Show file tree
Hide file tree
Changes from 1 commit
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.

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
Member Author

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.className = document.documentElement.className.replace(/\bno-js\b/, '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: '' }) %>
aduth marked this conversation as resolved.
Show resolved Hide resolved
</div>
</div>
</div>
<%= render_javascript_pack_once_tags 'openid-connect-redirect' %>
<%= render_javascript_pack_once_tags 'click-immediate' %>
</body>
</html>
13 changes: 8 additions & 5 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.className = document.documentElement.className.replace(/\bno-js\b/, 'js');
aduth marked this conversation as resolved.
Show resolved Hide resolved
<% 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,17 @@
<%= t('.no_js') %>
</p>

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

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
Member Author

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
Member Author

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: true } %>
<% 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.className = document.documentElement.className.replace(/\bno-js\b/, '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: true } %>
<% end %>
</div>
</div>
</div>
<%= render_javascript_pack_once_tags 'click-immediate' %>
</body>
</html>
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"]
}