Skip to content

changelog: Bug Fixes, Document Authentication, Fix exception when DoS…#12299

Merged
Mawar2 merged 3 commits intomainfrom
mwarren-LG-16143-fix-dos-error-handling
Jul 1, 2025
Merged

changelog: Bug Fixes, Document Authentication, Fix exception when DoS…#12299
Mawar2 merged 3 commits intomainfrom
mwarren-LG-16143-fix-dos-error-handling

Conversation

@Mawar2
Copy link
Copy Markdown
Contributor

@Mawar2 Mawar2 commented Jun 27, 2025

🎫 Ticket

Link to the relevant ticket:
LG-16143

🛠 Summary of changes

Fixed an issue where the DoS MRZ API client was throwing exceptions when receiving error responses in a simple string format. The client was originally built expecting errors to be nested objects like {"error": {"code": "...", "message": "..."}}, but we discovered the API sometimes returns errors as simple strings like {"error": "Invalid Client"} or {"error": "Authentication denied."}.

Updated the error handling to support both formats, preventing crashes and ensuring error messages are properly logged for debugging

📜 Testing Plan

  • Added test coverage for both error formats in spec/services/doc_auth/dos/request_spec.rb
  • bundle exec rspec spec/services/doc_auth/dos/request_spec.rb
  • Added specific test cases for the reported error strings in spec/services/doc_auth/dos/requests/mrz_request_spec.rb
  • bundle exec rspec spec/services/doc_auth/dos/requests/mrz_request_spec.rb

@Mawar2 Mawar2 self-assigned this Jun 27, 2025
Copy link
Copy Markdown
Contributor

@solipet solipet left a comment

Choose a reason for hiding this comment

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

A couple of non-blocking suggestions on the specs, but LGTM!

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This spec seems redundant with the spec above. Maybe have 'Authentication denied.' be the error for the simple string case?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good catch! I'll merge this into one test case.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Again since we're not actually looking at the string returned, I think it's fair to have just one spec, maybe 'when the request fails with a simple string error'

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I would update these tests to be a similar structure as the tests you added to spec/services/doc_auth/dos/request_spec.rb

context 'when the request fails' do
  context 'when the response error is a hash' do
  end

  context 'when the response error is a string' do
  end

  context 'when the response error is empty' do
  end
end

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Makes sense, I'll update these tests to match that structure. Thanks!

Copy link
Copy Markdown
Contributor

@AShukla-GSA AShukla-GSA left a comment

Choose a reason for hiding this comment

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

LGTM

@Mawar2 Mawar2 force-pushed the mwarren-LG-16143-fix-dos-error-handling branch from 37eb3d7 to 3bba89b Compare July 1, 2025 16:13
@Mawar2 Mawar2 merged commit e75de5f into main Jul 1, 2025
1 check passed
@Mawar2 Mawar2 deleted the mwarren-LG-16143-fix-dos-error-handling branch July 1, 2025 16:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants