Skip to content

Conversation

@flyrain
Copy link
Contributor

@flyrain flyrain commented Feb 23, 2024

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:

ValidationError: 1 validation error for TokenResponse
issued_token_type
  Field required [type=missing, input_value={'access_token': 'eyJhbGc... 'token_type': 'bearer'},
input_type=dict]

cc @danielcweeks @Fokko @syun64 @RussellSpitzer

Copy link
Collaborator

@sungwy sungwy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

token_type: str = Field()
expires_in: int = Field()
issued_token_type: str = Field()
issued_token_type: Optional[str] = None
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

@flyrain flyrain Feb 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's good ideas. Thanks for the review, @syun64. Fixed in a new commit.

Copy link
Collaborator

@sungwy sungwy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good from my side. Let's seek approval from @Fokko @danielcweeks to get this merged in 👍

@flyrain
Copy link
Contributor Author

flyrain commented Feb 27, 2024

Thanks @syun64 for the review. We will need at least an approval from committers. cc @Fokko @danielcweeks

Copy link
Contributor

@Fokko Fokko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @flyrain for fixing this 👍 I wasn't aware of this discrepancy!

Thanks @syun64 for the great review 🙌

expires_in: int = Field()
issued_token_type: str = Field()
expires_in: Optional[int] = Field(default=None)
issued_token_type: Optional[str] = Field(default=None)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could also convert this into a literal, since it is an enum in the spec:

Suggested change
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)

@Fokko Fokko merged commit 9a5bb07 into apache:main Feb 29, 2024
@flyrain
Copy link
Contributor Author

flyrain commented Feb 29, 2024

Thanks @Fokko for the review!

himadripal pushed a commit to himadripal/iceberg-python that referenced this pull request Mar 1, 2024
… 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]>
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