Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[AGENT-12890] Handle unexpected values for expires_in in OAuth Access response #19371

Merged
merged 6 commits into from
Jan 22, 2025

Conversation

dkirov-dd
Copy link
Contributor

What does this PR do?

Allow for an empty expires_in field in OAuth Access Token response as detailed by RFC 6749.
Handle unexpected type in expires_in field, e.g. string.

Motivation

Review checklist (to be filled by reviewers)

  • Feature or bugfix MUST have appropriate tests (unit, integration, e2e)
  • Add the qa/skip-qa label if the PR doesn't need to be tested during QA.
  • If you need to backport this PR to another branch, you can add the backport/<branch-name> label to the PR and it will automatically open a backport PR once this one is merged

@dkirov-dd dkirov-dd marked this pull request as ready for review January 10, 2025 17:04
@dkirov-dd dkirov-dd requested a review from a team as a code owner January 10, 2025 17:04
Copy link

codecov bot commented Jan 10, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.06%. Comparing base (fd0bf69) to head (330a747).
Report is 49 commits behind head on master.

Additional details and impacted files
Flag Coverage Δ
activemq ?
cassandra ?
hive ?
hivemq ?
hudi ?
ignite ?
jboss_wildfly ?
kafka ?
presto ?
solr ?

Flags with carried forward coverage won't be shown. Click here to find out more.

@dalsania1
Copy link

Correct me if I'm wrong but if a string is returned for the expires_in field, this will still cause a problem. The only difference now is that an exception will be thrown instead, but it still means that this integration won't work if the oauth endpoint that we are calling returns a number like "expires_in": "3599" instead of "expires_in": 3599

@dkirov-dd
Copy link
Contributor Author

Hello @dalsania1,

In the current implementation, if the expires_in value is a string, a TypeError will occur during the addition.
This error is caught by the except which discards it and logs a warning instead.
The resulting behavior is that self._expiration is set to the current timestamp.

The modified unit test ensures that an error is not thrown in the case of a string.

Some logic could be added to handle numeric string values however, thank you for bringing that up!

@dkirov-dd dkirov-dd force-pushed the david.kirov/handle-oauth-empty-expires-in branch from 3614393 to 24375ba Compare January 22, 2025 12:19
@dkirov-dd dkirov-dd force-pushed the david.kirov/handle-oauth-empty-expires-in branch from 24375ba to d765c9e Compare January 22, 2025 12:19
@dkirov-dd dkirov-dd added this pull request to the merge queue Jan 22, 2025
Merged via the queue into master with commit c9ca116 Jan 22, 2025
36 checks passed
@dkirov-dd dkirov-dd deleted the david.kirov/handle-oauth-empty-expires-in branch January 22, 2025 15:20
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