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

Authentication header xsd validations #5275

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
+75 −34
Open

Authentication header xsd validations #5275

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
7c319aa
Authentication header xsd validations
embarnard Dec 23, 2024
4c8ed99
Merge remote-tracking branch 'origin/main' into FYST-1304-auth-header
embarnard Dec 23, 2024
a9c96b5
Fix issue where wasn't recognizing more than one question with follow…
embarnard Dec 23, 2024
06690c3
protext against XSS
embarnard Dec 24, 2024
d04d5a2
Add test for device id and add comment for follow-up js code
embarnard Dec 24, 2024
49b5283
Merge remote-tracking branch 'origin/main' into FYST-1304-auth-header
embarnard Dec 24, 2024
57cf3e3
Merge branch 'refs/heads/main' into FYST-1304-auth-header
mpidcock Jan 6, 2025
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: 1 addition & 2 deletions app/forms/state_file/state_id_concern.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,7 @@ module StateIdConcern
validates :id_type, presence: true
validates :id_number,
presence: true,
# Seems to just be for New York
# alphanumeric: true, length: {is: 9},
mpidcock marked this conversation as resolved.
Show resolved Hide resolved
length: { in: 1..40 },
if: -> { id_type != "no_id" }
end
end
Expand Down
16 changes: 10 additions & 6 deletions app/javascript/lib/honeycrisp.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,13 +103,17 @@ var followUpQuestion = (function () {

// set initial state of follow-ups based on the page
$(this).find('input').each(function (index, input) {
if ($(this).attr('data-follow-up') != null) {
if ($(this).is(':checked')) {
$($(this).attr('data-follow-up')).find('input, select').attr('disabled', false);
$($(this).attr('data-follow-up')).show();
var $input = $(this);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The question-follow-up pattern only assumes that there is one option with the show follow-up option.
So if two or more radio buttons have data-follow-up="#id-details-fields", the final iteration might be one that is not checked—so it will hide #id-details-fields. This change makes it so that we will load the follow-up if any of the options are checked, not just the last one.

Copy link
Member

Choose a reason for hiding this comment

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

I know that @mrotondo and @anisharamnani worked with this code for the Idaho grocery credit - wondering if one of them could give a closer review

Copy link
Contributor

Choose a reason for hiding this comment

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

@mrotondo and i paired on reviewing this.

@embarnard, oof good catch on this bug! this is quite hairy code, so thank you for updating it.

do you mind adding a comment at line 115 just to explain that two questions can refer to the same followup? since this is so tricky, a comment would help.

for future notice, @mrotondo and i spoke about moving this logic to the update command, but that doesn't need to be included in this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

definitely! Thank you for reviewing it so thoroughly

var followUpSelector = $input.attr('data-follow-up');
if (followUpSelector && /^[a-zA-Z0-9_\-#\.]+$/.test(followUpSelector)) {
if ($input.is(':checked')) {
$(followUpSelector).find('input, select').attr('disabled', false);
Fixed Show fixed Hide fixed
$(followUpSelector).show();
Fixed Show fixed Hide fixed
} else {
$($(this).attr('data-follow-up')).find('input, select').attr('disabled', true);
$($(this).attr('data-follow-up')).hide();
if ($('[data-follow-up="' + followUpSelector + '"]:checked').length === 0) {
$(followUpSelector).find('input, select').attr('disabled', true);
Fixed Show fixed Hide fixed
$(followUpSelector).hide();
Fixed Show fixed Hide fixed
}
}
}
});
Expand Down
2 changes: 1 addition & 1 deletion app/lib/submission_builder/authentication_header.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ def document
xml.IPv6AddressTxt device_info&.ip_address if device_info&.ip_address&.ipv6?
end
xml.IPTs datetime_type(device_info&.updated_at)
xml.DeviceId device_info&.device_id || 'AB' * 20
xml.DeviceId /^[A-F0-9]{40}$/.match?(device_info&.device_id) ? device_info&.device_id : 'AB' * 20
Copy link
Contributor

Choose a reason for hiding this comment

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

can we write a test for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@anisharamnani done! thanks for adding that, it actually helped me catch that I hadn't added it to the initial device info block so glad you pointed that out

xml.DeviceTypeCd 'Browser-based'
end
xml.TotActiveTimePrepSubmissionTs state_file_total_preparation_submission_minutes
Expand Down
36 changes: 17 additions & 19 deletions app/views/state_file/questions/primary_state_id/_state_id.html.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,22 +3,20 @@
<%= hidden_field_tag "return_to_review", params[:return_to_review] %>
<% end %>
<div class="question-with-follow-up spacing-below-25">
<div class="question-with-follow-up__question spacing-below-15">
<div class="white-group">
<div class="form-question spacing-below-15">
<strong><%= t("state_file.questions.primary_state_id.state_id.id_type_question.label") %></strong>
</div>
<%=
f.cfa_radio_set(
:id_type,
collection: [
{ value: :driver_license, label: t("state_file.questions.primary_state_id.state_id.id_type_question.drivers_license"), input_html: { "data-follow-up": "#id-details-fields" } },
{ value: :dmv_bmv, label: (options[:dmv_bmv_label] || t("state_file.questions.primary_state_id.state_id.id_type_question.dmv")), input_html: { "data-follow-up": "#id-details-fields" } },
{ value: :no_id, label: (options[:no_id_label] || t("state_file.questions.primary_state_id.state_id.id_type_question.no_id")) },
]
)
%>
<div class="question-with-follow-up__question spacing-below-15 white-group">
<div class="form-question spacing-below-15">
<strong><%= t("state_file.questions.primary_state_id.state_id.id_type_question.label") %></strong>
</div>
<%=
f.cfa_radio_set(
:id_type,
collection: [
{ value: :driver_license, label: t("state_file.questions.primary_state_id.state_id.id_type_question.drivers_license"), input_html: { "data-follow-up": "#id-details-fields" } },
{ value: :dmv_bmv, label: (options[:dmv_bmv_label] || t("state_file.questions.primary_state_id.state_id.id_type_question.dmv")), input_html: { "data-follow-up": "#id-details-fields" } },
{ value: :no_id, label: (options[:no_id_label] || t("state_file.questions.primary_state_id.state_id.id_type_question.no_id")) },
]
)
%>
</div>

<div class="question-with-follow-up__follow-up" id="id-details-fields">
Expand All @@ -45,7 +43,7 @@
<div onchange="$('#first_3_doc_num_container')[event.target.value == 'NY' ? 'show' : 'hide']()">
<%= f.cfa_select(:state, t("state_file.questions.primary_state_id.state_id.id_details.issue_state"), States.name_value_pairs, include_blank: true) %>
</div>
<div id="first_3_doc_num_container" style="display: <%=options[:include_first_three_doc_num] ? 'block' : 'none'%>">
<div id="first_3_doc_num_container" style="display: <%= options[:include_first_three_doc_num] ? 'block' : 'none' %>">
<%= f.cfa_input_field(:first_three_doc_num, t("state_file.questions.primary_state_id.state_id.id_details.first_three_doc_num_html"), classes: ["form-width--short"]) %>
</div>
<% if options[:info_link] %>
Expand All @@ -65,7 +63,7 @@
document.addEventListener("DOMContentLoaded", function () {
// Function to clear selections in fields_to_clear
function clearFieldsToClear() {
$('#fields_to_clear select').each(function() {
$('#fields_to_clear select').each(function () {
$(this).val('');
});
}
Expand All @@ -76,14 +74,14 @@
}

// Attach a change event listener to the selects within fields_to_clear
$('#fields_to_clear select').change(function() {
$('#fields_to_clear select').change(function () {
if ($('#checkbox input').is(':checked')) {
clearFieldsToClear();
}
});

// Attach a change event listener to the checkbox as well
$('#checkbox input').change(function() {
$('#checkbox input').change(function () {
if (this.checked) {
clearFieldsToClear();
}
Expand Down
Loading