Skip to content
This repository has been archived by the owner on Jan 19, 2022. It is now read-only.

Add support to allow multiple IAP audience claims #1856

Merged
merged 1 commit into from
Sep 10, 2019

Conversation

acmarcel
Copy link
Contributor

@acmarcel acmarcel commented Sep 5, 2019

Currently autoconfiguration supports a single audience value. With this commit
now is possible to configure multiple allowable IAP audience claims.

Fixes gh-1468

@pivotal-issuemaster
Copy link

@acmarcel Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

Copy link
Contributor

@elefeint elefeint 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!

if (LOGGER.isWarnEnabled()) {
LOGGER.warn(String.format(
"Expected audience %s did not match token audience %s", this.audience, t.getAudience()));
String[] audiences = StringUtils.trimArrayElements(StringUtils.commaDelimitedListToStringArray(this.audience));
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you make audiences an instance variable, and move this logic into afterPropertiesSet()? This way, the audience splitting will only be done once.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it!

"Expected audience %s did not match token audience %s", this.audience, t.getAudience()));
String[] audiences = StringUtils.trimArrayElements(StringUtils.commaDelimitedListToStringArray(this.audience));
for (String audience : audiences) {
if (t.getAudience() != null && t.getAudience().contains(audience)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Now that this is done in a loop, it makes sense to move t.getAudience() != null check to the very beginning of validate() method and return OAuth2TokenValidatorResult.failure(INVALID_AUDIENCE); right away if the audience is missing in token.

@elefeint
Copy link
Contributor

elefeint commented Sep 6, 2019

One more thing, @acmarcel -- could you update the refdoc to indicate support for multiple allowable audiences?

Currently autoconfiguration supports a single audience value. With this commit
now is possible to configure multiple allowable IAP audience claims.

Fixes spring-atticgh-1468
@codecov
Copy link

codecov bot commented Sep 7, 2019

Codecov Report

Merging #1856 into master will increase coverage by 0.01%.
The diff coverage is 77.77%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #1856      +/-   ##
============================================
+ Coverage     72.62%   72.63%   +0.01%     
- Complexity     1833     1834       +1     
============================================
  Files           229      229              
  Lines          6736     6739       +3     
  Branches        695      697       +2     
============================================
+ Hits           4892     4895       +3     
  Misses         1524     1524              
  Partials        320      320
Flag Coverage Δ Complexity Δ
#unittests 72.63% <77.77%> (+0.01%) 1834 <3> (+1) ⬆️
Impacted Files Coverage Δ Complexity Δ
...work/cloud/gcp/security/iap/AudienceValidator.java 84.21% <77.77%> (+2.96%) 6 <3> (+1) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 680ceca...0c32eb8. Read the comment docs.

@pivotal-issuemaster
Copy link

@acmarcel Thank you for signing the Contributor License Agreement!

Copy link
Contributor

@elefeint elefeint 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!

@elefeint elefeint merged commit 6c24daf into spring-attic:master Sep 10, 2019
@flyalico
Copy link

thanks!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Allow multiple allowable IAP audience claims
5 participants