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.

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

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

Merged
merged 1 commit into from
Dec 12, 2024

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 <[email protected]>
@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
13 checks passed
@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 <[email protected]>
@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