`gare.to_dict()` returns excessive query parameters

[multimeric]

I'm trying to use GAREs + GlobusAuthorizationCodeFlowManager by following these docs. The problem is that GlobusAuthorizationParameters.to_dict() returns parameters even when they're "empty", which the globus auth endpoint rejects.

Here is an example where I've caught GlobusAPIError as e:

>>> e
TransferAPIError('GET', 'https://transfer.api.globus.org/v0.10/operation/endpoint/xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx/ls', 'Bearer', 403, 'PermissionDenied', 'No effective ACL rules on the high-assurance endpoint xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx (xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx)\nAvailable Identity IDs: yyyyyyyy-yyyy-yyyy-yyyy-yyyyyyyyyyyy', 'aaaaaaaaa')
>>> e.raw_json
{'authorization_parameters': {'session_message': 'Session reauthentication required (Globus Transfer)', 'session_required_identities': [...], 'session_required_mfa': False, 'session_required_single_domain': [...]}, 'code': 'PermissionDenied', 'message': 'No effective ACL rules on the high-assurance endpoint xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxx... IDs: yyyyyyyy-yyyy-yyyy-yyyy-yyyyyyyyyyyy', 'request_id': 'bbbbbbbbb', 'resource': '/operation/endpoint/xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx/ls'}
>>> gare = to_gare(e)
>>> gare.authorization_parameters.to_dict()
{'session_message': 'Session reauthentication required (Globus Transfer)', 'session_required_identities': ['yyyyyyyy-yyyy-yyyy-yyyy-yyyyyyyyyyyy'], 'session_required_single_domain': [], 'session_required_mfa': False}
>>> manager.get_authorize_url(query_params=query_params.to_dict())
'https://auth.globus.org/v2/oauth2/authorize?client_id=zzzzzzzz-zzzz-zzzz-zzzz-zzzzzzzzzzzz&redirect_uri=http%3A%2F%2Flocalhost%3A8084%2Fv1%2Fglobus%2Fredirect&scope=urn%3Aglobus%3Aauth%3Ascope%3Atransfer.api.globus.org%3Aall&state=http%3A%2F%2Flocalhost%3A8081%2Frequest%3Fdatasource%3Dglobus_test%26globusAuthenticated%3Dtrue&response_type=code&access_type=offline&session_message=Session+reauthentication+required+%28Globus+Transfer%29&session_required_identities=%5B%27yyyyyyyy-yyyy-yyyy-yyyy-yyyyyyyyyyyy%27%5D&session_required_single_domain=%5B%5D&session_required_mfa=False&prompt=login'

Note how gare.authorization_parameters.to_dict() includes session_required_single_domain and session_required_identities, which can't be used simultaneously.

If a user then browses to that authorize URL, they will get:

Now I recognize that the above docs involve use of a UserApp instead of a GlobusAuthorizationCodeFlowManager like I'm doing here, which works at a lower level. However it would be nice to be able to pull out a valid set of query parameters from GlobusAuthorizationParameters. Even better if it added {"prompt": "login"} for me.

[sirosen]

Hi there! Thanks for reporting this. 👍
Your usage looks correct to me, at a glance.

I think the issue you're seeing derives from that original error you got from the Transfer service. That looks malformed to me, but I also recognize that you've redacted a little bit of info. You're correct that those two parameters (identities and domains) are mutex, but for some reason we're getting an empty list for one of them?

The GlobusAuthorizationParameters object treats those fields as nullable, and dict-ifying it will remove them if they are None. But [] is a non-null value.

If you still have the original full error, could you check if the field was present with a value of []?
I haven't yet fully formed an opinion about whether or not the SDK should special-case falsey non-null values like [] and "", but I'd like to discuss with the team a bit before I jump to make a change.

---

Your usage is something we're trying to support, converting this object to query params. prompt=login is a bit of a special case. The format supports it, but I'll note that I personally don't believe that a service emitting errors should be making decisions about whether or not a fresh login is warranted. That should be the decision of client applications. (So I think that field should never be set by a service, but if it is included we will dutifully pass it along.)

In the general case, deciding whether a new login is desired requires more information than a service could have. In a small number of cases, services could definitively affirm that a new login is needed. So it's tricky to navigate.

In the case of globus-cli, we unconditionally set prompt=login when a login is triggered. In your case, you could do that by modifying the dict you get from the to_dict() call.

[sirosen]

We dug into the history a bit and found that the code which produces these errors service side has had this behavior for at least 4 years. But the Globus CLI has explicit handling for this, which is why for the most part nobody noticed that the newer (on a relative time-scale) SDK code and service logic don't align.

I'm going to propose some explicit handling for [] (rather than all falsey values) and see how the other SDK maintainers like that idea.

[multimeric]

I think the issue you're seeing derives from that original error you got from the Transfer service. That looks malformed to me, but I also recognize that you've redacted a little bit of info. You're correct that those two parameters (identities and domains) are mutex, but for some reason we're getting an empty list for one of them?

I'm fairly sure I'm getting this empty list directly from the API which I think is shown by the e.raw_json output.

prompt=login is a bit of a special case. The format supports it, but I'll note that I personally don't believe that a service emitting errors should be making decisions about whether or not a fresh login is warranted.

I don't quite follow this part. If an operation has failed, and Globus has said "you need to login differently", does that not imply that we need to login again? What other option is there?

But the Globus CLI has explicit handling for this, which is why for the most part nobody noticed that the newer (on a relative time-scale) SDK code and service logic don't align.

It looked like there was some unique behaviour in GlobusApp as well, since it deals directly with GlobusAuthorizationParameters rather than the dict form. My usage is lower level than this.

I'm going to propose some explicit handling for [] (rather than all falsey values) and see how the other SDK maintainers like that idea.

Thanks! I guess you could equally modify the API to to omit this parameter or set it to null instead of []. Whatever has the best compatibility.

[multimeric]

Actually another related issue (which is obvious in hindsight) is that .to_dict() doesn't convert from ["list", "of", "uuids"] to list,of,uuids as it needs to do for query parameters.

So I wonder if a new method which is just GlobusAuthorizationParameters.to_query_params() -> dict[str, str] would be the best way forward.

[sirosen]

Your last comment somewhat overlaps with the fact that when I went to make this fix, there isn't a perfect place for it.

Technically, "session_required_identities": [] is distinct from it being absent. If the job of GlobusAuthorizationParameters is to faithfully represent what we got from the service, converting an empty list to null seems wrong. But converting it when to_dict() is called is also messy and seems wrong.

I need to take some time to figure out what the right change to make would be. I've also spoken with the Transfer maintainers, and they want to fix the empty list coming back anyway.

prompt=login is a bit of a special case. The format supports it, but I'll note that I personally don't believe that a service emitting errors should be making decisions about whether or not a fresh login is warranted.

I don't quite follow this part. If an operation has failed, and Globus has said "you need to login differently", does that not imply that we need to login again? What other option is there?

There are situations in which you could be logged in to the service already, but missing consent for something. If an application sends you through a login flow without prompt specified in that state, you'll go straight to the consent screen. If prompt=login is added, you'll be prompted to select an identity and do a login flow.

When the failure mode is that you need a fresh login for a session, prompt=login generally has no effect anyway, since it's redundant with the other parameters.

prompt=login should be understood as "force a login flow to happen, even if Globus Auth can determine that a login does not appear to be necessary based on the current state".

We use it in the CLI because we've found a common source of surprises and issues is switching between multiple accounts in the webapp and the CLI.

[kurtmckee]

Hello @multimeric,

Thanks for reporting this issue! I believe I've addressed the issue in PR #1430 by updating the .get_authorize_url() methods.

This method was chosen because it simplified multiple call sites and fixed a latent bug in the base Auth client.