Skip to content

[fix] SDKS-5074 Introduce InvalidUriException in mfa-commons for typed OATH URI parse errors#210

Open
rodrigoareis wants to merge 2 commits into
developfrom
SDKS-5074
Open

[fix] SDKS-5074 Introduce InvalidUriException in mfa-commons for typed OATH URI parse errors#210
rodrigoareis wants to merge 2 commits into
developfrom
SDKS-5074

Conversation

@rodrigoareis

@rodrigoareis rodrigoareis commented Jun 12, 2026

Copy link
Copy Markdown
Contributor

JIRA Ticket

SDKS-5074 "[Android] Add dedicated InvalidUriException to mfa-commons for OATH URI parsing errors"

Description

Introduce a dedicated, typed InvalidUriException in mfa:commons so callers — notably the React Native OATH bridge (OathErrorMapper.kt) — can distinguish OATH URI structural/parsing failures from general argument-validation failures without sniffing e.message for the substring "uri". The new exception extends the module's MfaException family. URI-structure throw sites in OathUriParser.parse() (invalid scheme, invalid OATH type, missing required secret, the catch-all wrapper) and the shared UriParser.parseLabelComponents issuer/label-mismatch check are reclassified to throw InvalidUriException, while genuine value-validation errors (invalid digits, period, counter) remain IllegalArgumentException. This reaches parity with the iOS OathError.invalidUri case. The exception is placed in commons (not oath) so the parallel PushUriParser can adopt it later with no further module changes.

Phases completed:

  • Phase 1 — Add the InvalidUriException type to mfa-commons: Added InvalidUriException : MfaException to MfaException.kt
  • Phase 2 — Reclassify URI-structure throw sites in the shared base parser: UriParser.parseLabelComponents issuer/label mismatch → InvalidUriException
  • Phase 3 — Reclassify URI-structure throw sites in OathUriParser: scheme, type, missing-secret, catch-all → InvalidUriException; value-validation stays IllegalArgumentException
  • Phase 4 — Tests for typed exception + full module build: 6 new tests (4 typed-exception, 2 regression); full mfa:oath + mfa:commons build

Note: The downstream React Native bridge (OathErrorMapper.kt, RN repo) must be updated separately to catch (e: InvalidUriException) and remove the e.message string-sniffing heuristic. Coordinate that PR alongside this SDK release.

Summary by CodeRabbit

  • New Features

    • Introduced a dedicated InvalidUriException for structurally invalid/unparseable MFA URIs.
  • Bug Fixes

    • OATH and Push URI parsers now throw InvalidUriException (instead of generic exceptions) for structural/validation issues like scheme/type/secret requirements and issuer mismatches.
  • Documentation

    • Updated changelog and KDoc to reflect the new exception behavior.
  • Tests

    • Expanded OATH and Push negative-case coverage to assert InvalidUriException is raised for URI structural problems, while parameter validation errors remain IllegalArgumentException.
  • Chores

    • Added .sdk/ to .gitignore.

…d OATH URI parse errors

Introduce InvalidUriException (extends MfaException) in mfa:commons so callers —
notably the React Native OATH bridge — can distinguish OATH URI structural/parsing
failures from general argument-validation failures without sniffing e.message for
the substring "uri". URI-structure throw sites in OathUriParser.parse() (invalid
scheme, invalid OATH type, missing required secret, catch-all wrapper) and
UriParser.parseLabelComponents (issuer/label mismatch) are reclassified to throw
InvalidUriException. Value-validation errors (digits, period, counter) remain
IllegalArgumentException. Reaches parity with iOS OathError.invalidUri.

Phases:
- phase 1: Add InvalidUriException type to mfa-commons
- phase 2: Reclassify UriParser issuer/label-mismatch throw to InvalidUriException
- phase 3: Reclassify OathUriParser URI-structure throws to InvalidUriException
- phase 4: Tests for typed InvalidUriException + full module build
- reviewer fix: copyright year + test message assertions

CHANGELOG: entry added for SDKS-5074.

Refs: SDKS-5074
@coderabbitai

coderabbitai Bot commented Jun 12, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

This PR introduces InvalidUriException as a standardized exception type for structural URI parsing failures across the MFA framework. The new exception replaces generic IllegalArgumentException in both OATH and Push URI parsers, along with the commons UriParser. Credential APIs are updated to document the new exception type, comprehensive tests validate exception typing, and the changelog records the change.

Changes

Exception Type Standardization for URI Parsing

Layer / File(s) Summary
Exception type definition
mfa/commons/src/main/kotlin/com/pingidentity/mfa/commons/exception/MfaException.kt
New InvalidUriException class extends MfaException with optional message and cause, providing a reusable type for structural URI parsing failures.
Parser implementation updates
mfa/commons/src/main/kotlin/com/pingidentity/mfa/commons/UriParser.kt, mfa/oath/src/main/kotlin/com/pingidentity/mfa/oath/OathUriParser.kt, mfa/push/src/main/kotlin/com/pingidentity/mfa/push/PushUriParser.kt
Commons, OATH, and Push parsers import and throw InvalidUriException for scheme, type, secret, and issuer/label validation failures. KDoc @throws declarations updated, and exception handling rethrows known exception types unchanged while wrapping others as InvalidUriException.
Credential API documentation
mfa/oath/src/main/kotlin/com/pingidentity/mfa/oath/OathCredential.kt, mfa/push/src/main/kotlin/com/pingidentity/mfa/push/PushCredential.kt
OathCredential.fromUri and PushCredential.fromUri KDocs updated to document InvalidUriException for structural failures and IllegalArgumentException for parameter validation failures. PushCredential.fromJson KDoc also documents SerializationException.
Test coverage for exception types
mfa/oath/src/test/kotlin/com/pingidentity/mfa/oath/OathCredentialTest.kt, mfa/oath/src/test/kotlin/com/pingidentity/mfa/oath/OathUriParserTest.kt, mfa/push/src/test/kotlin/com/pingidentity/mfa/push/PushUriParserTest.kt
Tests validate that structural failures throw InvalidUriException (catchable as MfaException), while parameter validation errors throw IllegalArgumentException specifically. Coverage spans invalid schemes, types, missing secrets, issuer/label mismatches, and issuer parameter mismatches.
Configuration and changelog
CHANGELOG.md, mfa/commons/src/main/kotlin/com/pingidentity/mfa/commons/UriParser.kt, .gitignore
CHANGELOG documents the structural exception type change [SDKS-5074], copyright year updated in UriParser, and .sdk/ directory added to .gitignore.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Poem

🐰 A rabbit hops through parsing streams,
With exceptions typed in structured dreams,
No more generic errors sprawl—
InvalidUri catches all! 🎯
From schema checks to secret strings,
Type safety brings such joyful things!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 42.11% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and specifically describes the main change: introducing InvalidUriException in mfa-commons for OATH URI parsing errors.
Description check ✅ Passed The description covers the JIRA ticket link and provides a comprehensive explanation of the change, phases completed, and implementation details.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch SDKS-5074

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@codecov

codecov Bot commented Jun 12, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 33.33333% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 44.42%. Comparing base (2d7fa5d) to head (cff6d4f).

Files with missing lines Patch % Lines
...pingidentity/mfa/commons/exception/MfaException.kt 0.00% 4 Missing ⚠️
.../kotlin/com/pingidentity/mfa/push/PushUriParser.kt 0.00% 1 Missing and 1 partial ⚠️
...n/kotlin/com/pingidentity/mfa/commons/UriParser.kt 0.00% 1 Missing ⚠️
.../kotlin/com/pingidentity/mfa/oath/OathUriParser.kt 80.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@              Coverage Diff               @@
##             develop     #210       +/-   ##
==============================================
+ Coverage      26.11%   44.42%   +18.31%     
- Complexity      1016     1393      +377     
==============================================
  Files            308      315        +7     
  Lines           9374     9627      +253     
  Branches        1408     1474       +66     
==============================================
+ Hits            2448     4277     +1829     
+ Misses          6646     4788     -1858     
- Partials         280      562      +282     
Flag Coverage Δ
integration-tests 28.66% <33.33%> (?)
unit-tests 26.10% <0.00%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@tsdamas

tsdamas commented Jun 15, 2026

Copy link
Copy Markdown

Thanks for pushing this improvement so quickly. Two minor things:

  • parseLabelComponents now throws InvalidUriException on an issuer/label mismatch, but PushUriParser's catch-block at line 147 only passes through IllegalArgumentException. The InvalidUriException misses that check, falls to the else branch, and gets re-wrapped as IllegalArgumentException, which lose the typed exception. Any caller catching InvalidUriException through the Push path will never see it.
  • Stale KDoc on OathCredential.fromUri: Still say IllegalArgumentException if the URI is invalid. Worth adding InvalidUriException for structural failures and narrowing the IllegalArgumentException description to value-validation only.

@coderabbitai coderabbitai Bot left a comment

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.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
mfa/push/src/test/kotlin/com/pingidentity/mfa/push/PushUriParserTest.kt (1)

2-2: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Update the modified-file copyright year range.

Line 2 still uses a single year (2025), but this file is modified in 2026 and should use a range (2025-2026) per repository convention.

Based on learnings, modified Kotlin/Java files in this repository should use a creation-year-to-current-year range in the copyright header.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@mfa/push/src/test/kotlin/com/pingidentity/mfa/push/PushUriParserTest.kt` at
line 2, Update the copyright year on line 2 of PushUriParserTest.kt from the
single year format "2025" to the year range format "2025-2026" to follow the
repository convention for modified files. Change the copyright header to use the
creation year and current year as a range.

Source: Learnings

mfa/push/src/main/kotlin/com/pingidentity/mfa/push/PushUriParser.kt (1)

48-51: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Use InvalidUriException for scheme mismatch to keep structural failures typed.

At Line 50, scheme mismatch still throws IllegalArgumentException, which conflicts with the updated contract/KDoc (Lines 40-41) that structural URI failures are InvalidUriException. This leaks the wrong type to callers.

Suggested fix
             val scheme = parsedUri.scheme?.lowercase() ?: ""
             if (scheme != PUSHAUTH_SCHEME && scheme != MFAUTH_SCHEME) {
-                throw IllegalArgumentException("Invalid URI scheme: $scheme, expected: $PUSHAUTH_SCHEME or $MFAUTH_SCHEME")
+                throw InvalidUriException("Invalid URI scheme: $scheme, expected: $PUSHAUTH_SCHEME or $MFAUTH_SCHEME")
             }
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@mfa/push/src/main/kotlin/com/pingidentity/mfa/push/PushUriParser.kt` around
lines 48 - 51, The scheme validation in PushUriParser.kt is throwing
IllegalArgumentException when the URI scheme does not match PUSHAUTH_SCHEME or
MFAUTH_SCHEME, but the contract documented in the KDoc specifies that structural
URI failures should throw InvalidUriException. Replace the
IllegalArgumentException with InvalidUriException at the scheme mismatch check
to maintain consistent exception typing for structural URI validation failures.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@mfa/push/src/main/kotlin/com/pingidentity/mfa/push/PushUriParser.kt`:
- Around line 48-51: The scheme validation in PushUriParser.kt is throwing
IllegalArgumentException when the URI scheme does not match PUSHAUTH_SCHEME or
MFAUTH_SCHEME, but the contract documented in the KDoc specifies that structural
URI failures should throw InvalidUriException. Replace the
IllegalArgumentException with InvalidUriException at the scheme mismatch check
to maintain consistent exception typing for structural URI validation failures.

In `@mfa/push/src/test/kotlin/com/pingidentity/mfa/push/PushUriParserTest.kt`:
- Line 2: Update the copyright year on line 2 of PushUriParserTest.kt from the
single year format "2025" to the year range format "2025-2026" to follow the
repository convention for modified files. Change the copyright header to use the
creation year and current year as a range.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: b93e61ea-ef2c-43a0-b209-d874e0fe13aa

📥 Commits

Reviewing files that changed from the base of the PR and between 77d9228 and cff6d4f.

📒 Files selected for processing (5)
  • CHANGELOG.md
  • mfa/oath/src/main/kotlin/com/pingidentity/mfa/oath/OathCredential.kt
  • mfa/push/src/main/kotlin/com/pingidentity/mfa/push/PushCredential.kt
  • mfa/push/src/main/kotlin/com/pingidentity/mfa/push/PushUriParser.kt
  • mfa/push/src/test/kotlin/com/pingidentity/mfa/push/PushUriParserTest.kt
✅ Files skipped from review due to trivial changes (3)
  • CHANGELOG.md
  • mfa/push/src/main/kotlin/com/pingidentity/mfa/push/PushCredential.kt
  • mfa/oath/src/main/kotlin/com/pingidentity/mfa/oath/OathCredential.kt

@vibhorgoswami vibhorgoswami left a comment

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.

LGTM

@tsdamas tsdamas left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

LGTM

@spetrov spetrov left a comment

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.

LGTM!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

4 participants