Skip to content

Add a dedicated error class for OAuth response parsing errors#596

Merged
hadley merged 1 commit intomainfrom
oauth-parse-error-class
Dec 12, 2024
Merged

Add a dedicated error class for OAuth response parsing errors#596
hadley merged 1 commit intomainfrom
oauth-parse-error-class

Conversation

@atheriel
Copy link
Collaborator

Not all OAuth providers in the wild return spec-compliant responses in the case of an error. A prominent example is Facebook, but I've seen issues with Snowflake OAuth's token exchange flow, too. Posit Connect is not well-behaved here, either.

I've also seen weird issues in the past like authentication failues resulting in redirects to human-readable HTML pages rather than proper OAuth2 error response JSON.

All this to say: I think it can be really useful to have access to the original response object when trying to diagnose an issue.

So this commit adds a dedicated httr2_oauth_parse error class and embeds the response object in it. This allows consumers to write code that handles these errors with a lot more context:

tryCatch(
  some_oauth_using_code(),
  httr2_oauth_parse = function(cnd) {
    cli::cli_abort(
      c(
        "Unexpected failure during an OAuth flow.",
        i = "Status code: {resp_status(cnd$resp)}",
        i = "Response content type: {resp_content_type(cnd$resp)}"
      ),
      parent = cnd
    )
  }
)

Unit tests are included.

Not all OAuth providers in the wild return spec-compliant responses in
the case of an error. A prominent example is Facebook [0], but I've seen
issues with Snowflake OAuth's token exchange flow, too.

I've also seen weird issues in the past like authentication failues
resulting in redirects to human-readable HTML pages rather than proper
OAuth2 error response JSON.

All this to say: I think it can be really useful to have access to the
original response object when trying to diagnose an issue.

So this commit adds a dedicated `httr2_oauth_parse` error class and
embeds the response object in it. This allows consumers to write code
that handles these errors with a lot more context:

    tryCatch(
      some_oauth_using_code(),
      httr2_oauth_parse = function(cnd) {
        cli::cli_abort(
          c(
            "Unexpected failure during an OAuth flow.",
            i = "Status code: {resp_status(cnd$resp)}",
            i = "Response content type: {resp_content_type(cnd$resp)}"
          ),
          parent = cnd
        )
      }
    )

Unit tests are included.

[0]: https://pilcrowonpaper.com/blog/dear-oauth-providers/

Signed-off-by: Aaron Jacobs <aaron.jacobs@posit.co>
@atheriel
Copy link
Collaborator Author

Right now these errors look more like the following:

! Failed to parse response from `client$token_url` OAuth url.
* Did not contain `access_token`, `device_code`, or `error` field.

or

! Unexpected content type "text/plain".
* Expecting type "application/json" or suffix "json".

@hadley hadley merged commit 0ea4ec9 into main Dec 12, 2024
@hadley
Copy link
Member

hadley commented Dec 12, 2024

Thanks!

@atheriel atheriel deleted the oauth-parse-error-class branch December 12, 2024 20:40
atheriel added a commit to posit-dev/connectcreds that referenced this pull request Dec 12, 2024
Picks up on the special error class introduced in r-lib/httr2#596.

This substantially improves the error message when there are
misconfigurations on the Connect side.

This commit does not update the snapshots so that tests continue to pass
with the CRAN version of httr2.

Signed-off-by: Aaron Jacobs <aaron.jacobs@posit.co>
@jennybc
Copy link
Member

jennybc commented Dec 12, 2024

... redirects to human-readable HTML pages rather than proper OAuth2 error response JSON

cough, google

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.

3 participants