Make issued_token_type optional to support OAuth2 Client Credential Flow #466
Conversation
There was a problem hiding this comment.
Hey @flyrain thank you very much for putting this together! I glossed over this discrepancy because PyIceberg was in 'developing' stages. It's exciting how fast this project is maturing!
I left a comment to point out a few other discrepancies in the existing TokenResponse model that I thought would also be great to fix this time around. Let me know what you think!
pyiceberg/catalog/rest.py
Outdated
| @@ -157,7 +157,7 @@ class TokenResponse(IcebergBaseModel): | |||
| access_token: str = Field() | |||
| token_type: str = Field() | |||
| expires_in: int = Field() | |||
| issued_token_type: str = Field() | |||
| issued_token_type: Optional[str] = None | |||
There was a problem hiding this comment.
Could we use this opportunity to align the OAuthTokenResponse exactly with the model specified in the Iceberg Rest Catalog Open API Spec?
- we want to verify within the model that the issued_token_type is one of accepted types, if it is provided
- the current model is missing Optional scope and refresh_token
- also update expires_in to be Optional (expires_in is a recommended property in OAuth and if it wasn't provided in the TokenResponse, it will fail for similar reasons in the existing model)
There was a problem hiding this comment.
That's good ideas. Thanks for the review, @syun64. Fixed in a new commit.
There was a problem hiding this comment.
Looks good from my side. Let's seek approval from @Fokko @danielcweeks to get this merged in 👍
|
Thanks @syun64 for the review. We will need at least an approval from committers. cc @Fokko @danielcweeks |
| expires_in: int = Field() | ||
| issued_token_type: str = Field() | ||
| expires_in: Optional[int] = Field(default=None) | ||
| issued_token_type: Optional[str] = Field(default=None) |
There was a problem hiding this comment.
We could also convert this into a literal, since it is an enum in the spec:
| issued_token_type: Optional[str] = Field(default=None) | |
| issued_token_type: Optional[Literal['urn:ietf:params:oauth:token-type:access_token', 'urn:ietf:params:oauth:token-type:refresh_token', 'urn:ietf:params:oauth:token-type:id_token', 'urn:ietf:params:oauth:token-type:saml1', 'urn:ietf:params:oauth:token-type:saml2', 'urn:ietf:params:oauth:token-type:jwt'] = Field(default=None) |
|
Thanks @Fokko for the review! |
… Flow (apache#466) * Make issued_token_type optional to support OAuth2 Client Credential Flow * Fix the style issue * Resolve comments * Resolve comments --------- Co-authored-by: yufei <[email protected]>
To fix issue(#463). In short, Client Credential Flow's http response doesn't have the field
issued_token_type. Make it optional to avoid validation error like this:cc @danielcweeks @Fokko @syun64 @RussellSpitzer