spring-projects / spring-security

Spring Security
http://spring.io/projects/spring-security
Apache License 2.0
8.75k stars 5.88k forks source link

Consider adding dependency convergence detection #13990

Open sjohnr opened 12 months ago

sjohnr commented 12 months ago

We should consider adding dependency convergence detection to our build to prevent issues like gh-13843. For example, the following dependencies must agree on the version of com.nimbusds:nimbus-jose-jwt.

See comment and related issue for more information.

marcusdacoregio commented 11 months ago

@sjohnr if there aren't more dependencies that have to be checked, this might have been resolved via https://github.com/spring-projects/spring-security/issues/14047. I'll close this but we can reopen if needed.

sjohnr commented 11 months ago

@marcusdacoregio based on this comment, I think the idea was to add general purpose detection when convergence issues arise. But I don't have too much experience with that. gh-14047 definitely solves the most immediate problem, so I'll let others weigh in on whether more general detection is still needed. @ThomasKasene

ThomasKasene commented 11 months ago

I'll weigh in my own experience - make of it what you will 😄

My team maintains a large number of Spring Boot applications which all use Spring Security for OAuth 2.0 stuff. When Boot 3.1.1 (I think) was released, Dependabot gave us a bunch of PRs to upgrade, but all the builds broke because we use the maven-enforcer-plugin to detect dependency convergence errors. The culprit was, of course, the fact that spring-security-oauth2-jose and oauth2-oidc-sdk depended on different versions of nimbus-jose-jwt.

We could have overridden the versions of the offending dependency in all the apps, and that would've been the "correct" thing to do. Doing so would've meant having to look out for changes/fixes in Spring Security to this issue (like the one provided through #13843). In the meantime, we'd have to manage any Dependabot PRs about nimbus-jose-jwt in all the apps, which would possibly all be invalid anyway because both Spring Security and oauth2-oidc-sdk use very old versions of nimbus-jose-jwt.

In the end, a general lack of bandwidth forced us to jump to plan B, which was simply to not upgrade to Spring Boot 3.1.1 at all. The fix didn't arrive until 3.1.5, so our adoption rate crashed down to zero in the meantime.

So yeah, a dependency convergence error on this exact artifact has happened before, and perhaps the Nimbus stuff is particularly prone to it because their release cycles are so out of sync. But the idea was to get something in place to prevent these kinds of issues from happening at all in the future. 😃

marcusdacoregio commented 11 months ago

Thanks, @ThomasKasene. I'll keep this open and label it as ideal for contribution because I think it can be worth adding that to prevent such problems. It would be great if someone could investigate what would be needed to apply the plugin that you mentioned.

andreilisa commented 10 months ago

Hello @marcusdacoregio, can I take this one ?

marcusdacoregio commented 10 months ago

Hello, @andreilisa. Yes, the issue is yours.

I'd start by adding the plugin to the build and check what dependencies should be aligned to have a successful compilation. Additionally, we might even be able to remove this if the plugin can detect such a setup.

andreilisa commented 10 months ago

Yes got it, I will ask here if I have additional questions.

andreilisa commented 10 months ago

Hey @marcusdacoregio, after stating to ExcludeDependencies.

enforce {
    rule(enforcer.rules.DependencyConvergence)
    rule(enforcer.rules.ExcludeDependencies) { r ->
        r.exclude("org.slf4j:slf4j-api:1.7.26")
        r.exclude("org.slf4j:slf4j-api:1.7.25")
        r.exclude("com.puppycrawl.tools:checkstyle:9.3")
        r.exclude("com.puppycrawl.tools:checkstyle:8.33")
        r.exclude("net.bytebuddy:byte-buddy:1.12.21")
        r.exclude("org.junit.jupiter:junit-jupiter-api:5.10.0")
//      r.exclude("commons-collections:commons-collections:3.2.1")
    }
}

I get next one error message and I can`t understand where is the problem:

Could not resolve all files for configuration ':spring-security-config:apiDependenciesMetadataCopy'.
> Could not find org.springframework:spring-aop:.
  Required by:
      project :spring-security-config
      project :spring-security-config > project :spring-security-core

But if I comment // r.exclude("org.junit.jupiter:junit-jupiter-api:5.10.0") I will get next one message:

Could not resolve all dependencies for configuration ':spring-security-acl:testRuntimeClasspathCopy'.
> Conflict found for the following module:
    - org.junit.jupiter:junit-jupiter-api between versions 5.10.1 and 5.10.0

Can you have a look at dependency_convergence_detection ?

marcusdacoregio commented 10 months ago

Hi, @andreilisa.

It seems that the plugin performs the check in every configuration available, where, ideally, I think it should only check the compileClasspath in our case.

andreilisa commented 10 months ago

@marcusdacoregio, Related PR 14256 ?