`token` arg is not validated sufficiently

[elipousson]

Describe the bug

This is arguably an enhancement (and may need to be handled within {arcgisutils}) but I definitely experienced it as a bug so figured I'd start here.

• arcgisutils::set_auth_token() requires that token is an object of class httr2_token but could easily take a string instead
• arcgislayers::arc_open() (and arc_select() and arc_read() etc.) requires that token is a string not a httr2_token object but could easily take a httr2_token instead
• arc_token() doesn't take any parameters. It returns a token from a preset environmental variable name but could easily take a token parameter that supports basic input checking or allow the use of multiple environmental variable names for different accounts.

To Reproduce

Initially, I assumed I could pass the output of arcgisutils::auth_code() to arc_open() and was completely confused by the resulting error:

arcgislayers::arc_open(
  "https://services1.arcgis.com/43Lm3JYE3nM91DAF/arcgis/rest/services/Floodplain_Dissolved/FeatureServer",
  token = httr2::oauth_token("") # Error is the same if you use the output from arcgisutils::auth_code()
)
#> Error in `httr2::req_body_form()`:
#> ! All elements of `...` must be either an atomic vector or NULL.

^{Created on 2024-01-23 with reprex v2.1.0}

Expected behavior

At a minimum, I expect arc_open() to return an informative error message explaining that token is the wrong type.

Ideally, I expect token to support both string inputs and httr2_token class inputs since I can't see any meaningful performance or technical reasons that wouldn't be straightforward to implement. One option could be to still limit the input token for arc_open() to strings only but allow arc_token() to take a token parameter that is converted to a string (reusing the code from arcgisutils::set_auth_token()) as a (hopefully) more intuitive alternate workflow.

[JosiahParry]

There's more than one issue here. For set_auth_token() not accepting strings, see https://github.com/R-ArcGIS/arcgisutils/issues/11

The other point is about validation of the token and permitting polymorphism in the argument.

I agree that this is inconsistent and could be improved. Only one type of object should be supported for clarity and type safety. I think httr2_token is the best choice. If that's the choice a number of changes have to take place:

• set_auth_token() should set the token in package environment not an environment variable since it is required it be stored in plain text and does not support complex types
• arc_token() should fetch the token from the package environment
• a token validation function such as arcgisutils::validate_or_refresh_token() should be used in every function that executes a request which is quite extensive. This function should be modified to not accept a string. I think including polymorphic arguments just causes more bugs than necessary.
  • In arcgislayers:
    • arc_select(), arc_open(), add_item(), arc_raster(), add_features(), arc_read(), create_feature_server(), delete_features(), get_all_layers(), get_layer(), get_layers(), get_layer_estimates(), publish_item(), refresh_layer(), publish_layer(), truncate_layer(), update_features()
  • In arcgisutils:
    • fetch_layer_metadata() and count_features(), though I think they could be restructured a bit. they're both useful but assume creation with arcgislayers which makes me think they may be best exposed there?

[elipousson]

Hmmm. If the token moves from the environment variable to the package environment, I do think some caching features for the token would still be essential.

I will typically re-start a session several times in a single hour of analytic or programming work to ensure the full script is still reproducible from the start. A single token would still be valid throughout that time period—but if I need to re-authenticate in the browser every time I restart that would be a huge PITA.

I'm swamped today but I'll revisit the information you shared here and in our discussion about error handling with {httr2} and respond in more detail later this week.

[JosiahParry]

I think we should specifically focus on caching the (refresh token at least first) but probably move that convo to https://github.com/R-ArcGIS/arcgisutils/issues/10