DRIVERS-2836 OIDC: Clarify that TOKEN_RESOURCE must be url-encoded#1569
Conversation
|
Tested with mongodb/mongo-python-driver#1616 |
Co-authored-by: Maxim Katcharov <maxim.katcharov@mongodb.com>
| The URI of the target resource. If `TOKEN_RESOURCE` is provided and `ENVIRONMENT` is not one of | ||
| `["azure", "gcp"]` or `TOKEN_RESOURCE` is not provided and `ENVIRONMENT` is one of `["azure", "gcp"]`, the driver | ||
| MUST raise an error. | ||
| MUST raise an error. Drivers MUST ensure that `TOKEN_RESOURCE` is url-encoded, such as by using a regex for special |
There was a problem hiding this comment.
I would prefer to have plain value here, because if some driver decide to use Azure/GCP/other SDK to pass the value - most likely SDK will do encoding on it's end. The fact that the value is being used as a url query parameter - this is implementation details and might be vary (if some other provider will use POST and pass the value in request body).
There was a problem hiding this comment.
But then in the Azure and GCP section we have to require driver to encode the value as it being used as a query parameter.
| "mechanism": "MONGODB-OIDC", | ||
| "mechanism_properties": { | ||
| "ENVIRONMENT": "azure", | ||
| "TOKEN_RESOURCE": "a%24b" |
There was a problem hiding this comment.
No, because it is azure and we would have encoded it.
|
@sanych-sun @katcharov I've updated the description and tests based on feedback |
|
Updated tests in mongodb/mongo-python-driver#1620 |
sanych-sun
left a comment
There was a problem hiding this comment.
The latest revision looks good to me. Will implement the changes and verify if tests are working as expected for CSHARP Driver. If everything works well - will LGTM it later today.
| ENVIRONMENT: azure | ||
| TOKEN_RESOURCE: 'mongodb://test-cluster' | ||
| - description: should accept an un-encoded TOKEN_RESOURCE (MONGODB-OIDC) | ||
| uri: mongodb://user@localhost/?authMechanism=MONGODB-OIDC&authMechanismProperties=ENVIRONMENT:azure,TOKEN_RESOURCE:mongodb://test-cluster |
There was a problem hiding this comment.
Is this Uri valid? Should we have : in the mongodb://test-cluster encoded as %3A? Having more then one value separator confuses our parser. And it's kind of ambiguous:
TOKEN_RESOURCE:mongodb://test-cluster
should it be parsed as TOKEN_RESOURCE=mongodb://test-cluster or TOKEN_RESOURCE:mongodb=//test-cluster
There was a problem hiding this comment.
This usage is against the advice we give to users in the spec, but I think we should optimistically decode this (like this in Java), rather than failing. Although it is indeed ambiguous, I think we will never put a : in the name of any property (precisely because it is a separator).
There was a problem hiding this comment.
Yep, can do the same, just want to double check if we see this as expected behavior.
alcaeus
left a comment
There was a problem hiding this comment.
One minor note, LGTM other than that.
Co-authored-by: Andreas Braun <git@alcaeus.org>
Co-authored-by: Maxim Katcharov <maxim.katcharov@mongodb.com>
katcharov
left a comment
There was a problem hiding this comment.
LGTM, with one comment
Co-authored-by: Maxim Katcharov <maxim.katcharov@mongodb.com>
Please complete the following before merging:
clusters, and serverless).