Make sure the base url is set when calling authenticate_request #27
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This is a fix for the issue where the base url was not being set when calling authenticate_request. This caused an error later in the kiota-authentication-oauth-ruby library. The request_info is passed to the access_token_provider and request_info.uri is used to build the request. This uri is missing the base url. https://github.com/microsoft/kiota-abstractions-ruby/blob/7367a156276923eddd22e52335adbba88b1bf71e/lib/microsoft_kiota_abstractions/authentication/base_bearer_token_authentication_provider.rb#L21 Further down the line, the uri is parsed and an error is raised if scheme is not https. Because the base url is missing, the URI is only parsed as a relative path and thus always fails the scheme check. https://github.com/microsoft/kiota-authentication-oauth-ruby/blob/69ebeb6705b1d621a9943a15d634e9a33c77d23e/lib/microsoft_kiota_authentication_oauth/oauth_access_token_provider.rb#L54
louim
requested review from
andrueastman,
baywet,
zengin,
MIchaelMainer,
peombwa,
calebkiage,
Ndiritu and
rkodev
as code owners
baywet
previously approved these changes
Feb 14, 2024
There was a problem hiding this comment.
Thank you so much for the contribution!
To ensure a swift publication of the patch, can you please:
- bump the patch version https://github.com/microsoft/kiota-http-ruby/blob/main/lib%2Fmicrosoft_kiota_faraday%2Fversion.rb
- add an entry in the changelog (tomorrow's date)
I'll let @rkodev handle the release (you just need to create a tag on main like for the go repos) as I'm out this week.
|
|
@rkodev I pushed commit containing the version bump/changelog. @baywet you said to bump the |
andrueastman
approved these changes
Feb 15, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a fix for the issue where the base url was not being set when calling authenticate_request.It caused an error later in the
microsoft_kiota_authentication_oauthgem. The crux of the problem goes like that:request_infois passed to theaccess_token_providerandrequest_info.uriis used to build the request here. Thisuriis missing the base url.schemeis nothttps. Because the base url is missing, the URI is only parsed as a relative path and thus always fails the scheme check.The fix is to always make sure we set the
baseurlbefore calling action where the url might be templated. The fix is inspired from the Csharp version of the class.I tried adding some kind of test, but there almost nothing already in place and to test my small fix, I would have to either mock or instantiate a dozen of classes which some are not even in the same gem 😢.