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

fix: External credentials URL validations #2235

Merged
merged 2 commits into from
Oct 18, 2022

Conversation

amanda-tarafa
Copy link
Contributor

No description provided.

@amanda-tarafa amanda-tarafa requested a review from jskeet October 17, 2022 16:39
@amanda-tarafa amanda-tarafa requested a review from a team as a code owner October 17, 2022 16:39
@amanda-tarafa
Copy link
Contributor Author

FYI @lsirac

Copy link
Collaborator

@jskeet jskeet left a comment

Choose a reason for hiding this comment

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

Generally fine, with a few nits.

[InlineData("https://iamcredentials-xyz.p1.googleapis.com")]
[InlineData("https://iamcredentials-xyz.p.foo.com")]
[InlineData("https://iamcredentials-xyz.p.foo.googleapis.com")]
public void ValidateImpersonationUrl_InvaliddUrl(string impersonationUrl) =>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit - here and below - Invalidd => Invalid

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

using System.Threading.Tasks;
using Xunit;

namespace Google.Apis.Auth.Tests.OAuth2
{
public abstract class ExternalAccountCredentialTestsBase
public class ExternalAccountCredentialTestsBase
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm... does this mean that these tests end up being run multiple times, one for the base class and one for each derived class? Maybe these tests should be in a separate class instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh yes you are right. Moving to a new class.

private static readonly IEnumerable<Regex> ValidTokenUrlHostRegexes = new List<Regex>
{
// sts.xyz123.googleapis.com or exactly sts.googleapis.com
new Regex("^sts\\.([^\\.]+\\.)?googleapis\\.com$", RegexOptions.Compiled | RegexOptions.IgnoreCase),
Copy link
Collaborator

Choose a reason for hiding this comment

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

I assume you're not using verbatim string literals so that we can just copy/paste from Java?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, it was just left over from copy pasting.

Copy link
Contributor Author

@amanda-tarafa amanda-tarafa left a comment

Choose a reason for hiding this comment

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

All comments addressed.

[InlineData("https://iamcredentials-xyz.p1.googleapis.com")]
[InlineData("https://iamcredentials-xyz.p.foo.com")]
[InlineData("https://iamcredentials-xyz.p.foo.googleapis.com")]
public void ValidateImpersonationUrl_InvaliddUrl(string impersonationUrl) =>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

using System.Threading.Tasks;
using Xunit;

namespace Google.Apis.Auth.Tests.OAuth2
{
public abstract class ExternalAccountCredentialTestsBase
public class ExternalAccountCredentialTestsBase
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh yes you are right. Moving to a new class.

private static readonly IEnumerable<Regex> ValidTokenUrlHostRegexes = new List<Regex>
{
// sts.xyz123.googleapis.com or exactly sts.googleapis.com
new Regex("^sts\\.([^\\.]+\\.)?googleapis\\.com$", RegexOptions.Compiled | RegexOptions.IgnoreCase),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, it was just left over from copy pasting.

@amanda-tarafa amanda-tarafa merged commit ed1aa54 into googleapis:main Oct 18, 2022
@amanda-tarafa amanda-tarafa deleted the wif-url-validations branch October 18, 2022 08:53
amanda-tarafa added a commit to amanda-tarafa/google-api-dotnet-client that referenced this pull request Oct 18, 2022
Features
- googleapis#2228 Supports Quota Project environment variable for ADC.
- googleapis#2219 Removes obsolete OOB code.
   BREAKING CHANGE: We have removed OOB support from the library because the feature is not supported by Google OAuth as of October 3rd 2022. See the [official announcement](https://developers.googleblog.com/2022/02/making-oauth-flows-safer.html#disallowed-oob) for more information.
- googleapis#2216 Log errors when trying to reach Compute's metadata server.
- googleapis#2177, googleapis#2197, googleapis#2200, googleapis#2206, googleapis#2215, googleapis#2235 Support for Workload Identity Federation and Worforce Identity Federation.
- googleapis#2103 Compute credentials support explicit scopes.
- googleapis#2096 Improves deserialization error handling.
amanda-tarafa added a commit that referenced this pull request Oct 18, 2022
Features
- #2228 Supports Quota Project environment variable for ADC.
- #2219 Removes obsolete OOB code.
   BREAKING CHANGE: We have removed OOB support from the library because the feature is not supported by Google OAuth as of October 3rd 2022. See the [official announcement](https://developers.googleblog.com/2022/02/making-oauth-flows-safer.html#disallowed-oob) for more information.
- #2216 Log errors when trying to reach Compute's metadata server.
- #2177, #2197, #2200, #2206, #2215, #2235 Support for Workload Identity Federation and Worforce Identity Federation.
- #2103 Compute credentials support explicit scopes.
- #2096 Improves deserialization error handling.
amanda-tarafa added a commit to amanda-tarafa/google-api-dotnet-client that referenced this pull request Nov 30, 2022
And support version: 1.58.0-beta02 => 1.58.0

Bug fixes:

- googleapis#2273 Don't attempt to use an empty/null media type in batch responses
- googleapis#2251 Which fixes a typo on then name of one of the AWS Credential environment variables.
- googleapis#2264 Which adds a missing line break in AWS Credential canonical request.

Features
- googleapis#2269 Support per-request max retry configuration.
- googleapis#2228 Supports Quota Project environment variable for ADC.
- googleapis#2219 Removes obsolete OOB code. BREAKING CHANGE: We have removed OOB support from the library because the feature is not supported by Google OAuth as of October 3rd 2022. See the [official announcement](https://developers.googleblog.com/2022/02/making-oauth-flows-safer.html#disallowed-oob) for more information.
- googleapis#2216 Log errors when trying to reach Compute's metadata server.
- googleapis#2177, googleapis#2197, googleapis#2200, googleapis#2206, googleapis#2215, googleapis#2235 Support for Workload Identity Federation and Worforce Identity Federation.
- googleapis#2103 Compute credentials support explicit scopes.
- googleapis#2096 Improves deserialization error handling.
amanda-tarafa added a commit to amanda-tarafa/google-api-dotnet-client that referenced this pull request Nov 30, 2022
And support version: 1.58.0-beta02 => 1.58.0

Dependencies:

- Client libraries no longer target netstandard1.0 or net40.

Bug fixes:

- googleapis#2273 Don't attempt to use an empty/null media type in batch responses
- googleapis#2251 Which fixes a typo on then name of one of the AWS Credential environment variables.
- googleapis#2264 Which adds a missing line break in AWS Credential canonical request.

Features
- googleapis#2269 Support per-request max retry configuration.
- googleapis#2228 Supports Quota Project environment variable for ADC.
- googleapis#2219 Removes obsolete OOB code. BREAKING CHANGE: We have removed OOB support from the library because the feature is not supported by Google OAuth as of October 3rd 2022. See the [official announcement](https://developers.googleblog.com/2022/02/making-oauth-flows-safer.html#disallowed-oob) for more information.
- googleapis#2216 Log errors when trying to reach Compute's metadata server.
- googleapis#2177, googleapis#2197, googleapis#2200, googleapis#2206, googleapis#2215, googleapis#2235 Support for Workload Identity Federation and Worforce Identity Federation.
- googleapis#2103 Compute credentials support explicit scopes.
- googleapis#2096 Improves deserialization error handling.
amanda-tarafa added a commit that referenced this pull request Nov 30, 2022
And support version: 1.58.0-beta02 => 1.58.0

Dependencies:

- Client libraries no longer target netstandard1.0 or net40.

Bug fixes:

- #2273 Don't attempt to use an empty/null media type in batch responses
- #2251 Which fixes a typo on then name of one of the AWS Credential environment variables.
- #2264 Which adds a missing line break in AWS Credential canonical request.

Features
- #2269 Support per-request max retry configuration.
- #2228 Supports Quota Project environment variable for ADC.
- #2219 Removes obsolete OOB code. BREAKING CHANGE: We have removed OOB support from the library because the feature is not supported by Google OAuth as of October 3rd 2022. See the [official announcement](https://developers.googleblog.com/2022/02/making-oauth-flows-safer.html#disallowed-oob) for more information.
- #2216 Log errors when trying to reach Compute's metadata server.
- #2177, #2197, #2200, #2206, #2215, #2235 Support for Workload Identity Federation and Worforce Identity Federation.
- #2103 Compute credentials support explicit scopes.
- #2096 Improves deserialization error handling.
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.

2 participants