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.

Sign up for GitHub

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

Jump to bottom

Updates for #6911 and #6918 #1131

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Updates for #6911 and #6918 #1131

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
fef018e
fix(api): Use consistent anonymous principal
jvz Jan 3, 2024
08ec1ef
refactor(core): Use kork-security abstractions
jvz Jan 3, 2024
026ad25
Merge branch 'master' into issue-6918
jvz Jan 4, 2024
df29277
refactor(api): Extract common anonymous auth
jvz Jan 8, 2024
683d0fa
Merge branch 'master' into issue-6918
jvz Jan 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
9 changes: 5 additions & 4 deletions ...n/java/com/netflix/spinnaker/fiat/shared/AuthenticatedRequestAuthenticationConverter.java
View file
Open in desktop
Expand Up @@ -17,11 +17,12 @@
package com.netflix.spinnaker.fiat.shared;

import com.netflix.spinnaker.security.AuthenticatedRequest;
import com.netflix.spinnaker.security.SpinnakerAuthorities;
import com.netflix.spinnaker.security.SpinnakerUsers;
import java.util.List;
import javax.servlet.http.HttpServletRequest;
import org.springframework.security.authentication.AnonymousAuthenticationToken;
import org.springframework.security.core.Authentication;
import org.springframework.security.core.authority.AuthorityUtils;
import org.springframework.security.web.authentication.AuthenticationConverter;
import org.springframework.security.web.authentication.preauth.PreAuthenticatedAuthenticationToken;

Expand All @@ -41,8 +42,8 @@ public Authentication convert(HttpServletRequest request) {
.orElseGet(
() ->
new AnonymousAuthenticationToken(
"anonymous",
"anonymous",
AuthorityUtils.createAuthorityList("ROLE_ANONYMOUS")));
SpinnakerUsers.ANONYMOUS,
Copy link
Contributor

Choose a reason for hiding this comment

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

notes for reviewers -- the new code looks like it does exactly the same thing as the old code, so this is "just" a refactor.

SpinnakerUsers.ANONYMOUS,
List.of(SpinnakerAuthorities.ANONYMOUS_AUTHORITY)));
}
}
3 changes: 3 additions & 0 deletions fiat-api/src/main/java/com/netflix/spinnaker/fiat/shared/FiatAuthenticationConfig.java
View file
Open in desktop
Expand Up @@ -28,6 +28,7 @@
import com.netflix.spinnaker.kork.web.exceptions.ExceptionMessageDecorator;
import com.netflix.spinnaker.okhttp.SpinnakerRequestInterceptor;
import com.netflix.spinnaker.retrofit.Slf4jRetrofitLogger;
import com.netflix.spinnaker.security.SpinnakerUsers;
import lombok.Setter;
import lombok.val;
import okhttp3.OkHttpClient;
Expand Down Expand Up @@ -143,6 +144,8 @@ protected void configure(HttpSecurity http) throws Exception {
.exceptionHandling()
.and()
.anonymous()
// match the same anonymous userid as expected elsewhere
Copy link
Contributor

Choose a reason for hiding this comment

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

How is this different from .anonymous() above? Can you write a test that demonstrates this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The default settings in this DSL set the principal to the string anonymousUser. The DSL is tricky to test since it's mostly a DSL for setting up beans.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So using the updated Spring Security DSL, this whole method would look more like the following:

      http.servletApi(Customizer.withDefaults())
          .exceptionHandling(Customizer.withDefaults())
          .anonymous(anonymous -> anonymous.principal(SpinnakerUsers.ANONYMOUS))
          .addFilterBefore(
              new FiatAuthenticationFilter(fiatStatus, authenticationConverter),
              AnonymousAuthenticationFilter.class);

Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't see before. .principal(SpinnakerUsers.ANONYMOUS) modifies how .anonymous() works...it's not another mechanism in addition to it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll figure out some sort of test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Turns out testing this is futile. I've extracted some constants to make this more consistent.

.principal(SpinnakerUsers.ANONYMOUS)
.and()
.addFilterBefore(
new FiatAuthenticationFilter(fiatStatus, authenticationConverter),
Expand Down
5 changes: 4 additions & 1 deletion fiat-api/src/main/java/com/netflix/spinnaker/fiat/shared/FiatAuthenticationConverter.java
View file
Open in desktop
Expand Up @@ -19,6 +19,7 @@
import com.netflix.spinnaker.fiat.model.SpinnakerAuthorities;
import com.netflix.spinnaker.fiat.model.UserPermission;
import com.netflix.spinnaker.kork.common.Header;
import com.netflix.spinnaker.security.SpinnakerUsers;
import java.util.List;
import javax.servlet.http.HttpServletRequest;
import lombok.RequiredArgsConstructor;
Expand Down Expand Up @@ -47,6 +48,8 @@ public Authentication convert(HttpServletRequest request) {
}
}
return new AnonymousAuthenticationToken(
"anonymous", "anonymous", List.of(SpinnakerAuthorities.ANONYMOUS_AUTHORITY));
SpinnakerUsers.ANONYMOUS,
SpinnakerUsers.ANONYMOUS,
List.of(SpinnakerAuthorities.ANONYMOUS_AUTHORITY));
}
}
2 changes: 1 addition & 1 deletion fiat-core/fiat-core.gradle
View file
Open in desktop
@@ -1,6 +1,6 @@
dependencies {

api "org.springframework.security:spring-security-core"
api "io.spinnaker.kork:kork-security:$korkVersion"

implementation "com.fasterxml.jackson.core:jackson-annotations"
implementation "com.google.code.findbugs:jsr305"
Expand Down
19 changes: 3 additions & 16 deletions fiat-core/src/main/java/com/netflix/spinnaker/fiat/model/SpinnakerAuthorities.java
View file
Open in desktop
Expand Up @@ -20,25 +20,12 @@
import org.springframework.security.core.authority.SimpleGrantedAuthority;

/**
* Constants and utilities for working with Spring Security GrantedAuthority objects specific to
* Spinnaker and Fiat. Spinnaker-specific roles such as admin and account manager are represented
* here as granted authorities.
* Migrated to {@link com.netflix.spinnaker.security.SpinnakerAuthorities} in {@code kork-security}.
* This is left for backward compatibility.
*/
public class SpinnakerAuthorities {
public static final String ADMIN = "SPINNAKER_ADMIN";
/** Granted authority for Spinnaker administrators. */
public static final GrantedAuthority ADMIN_AUTHORITY = new SimpleGrantedAuthority(ADMIN);

public class SpinnakerAuthorities extends com.netflix.spinnaker.security.SpinnakerAuthorities {
public static final String ACCOUNT_MANAGER = "SPINNAKER_ACCOUNT_MANAGER";
/** Granted authority for Spinnaker account managers. */
public static final GrantedAuthority ACCOUNT_MANAGER_AUTHORITY =
new SimpleGrantedAuthority(ACCOUNT_MANAGER);

/** Granted authority for anonymous users. */
public static final GrantedAuthority ANONYMOUS_AUTHORITY = forRoleName("ANONYMOUS");

/** Creates a granted authority corresponding to the provided name of a role. */
public static GrantedAuthority forRoleName(String role) {
return new SimpleGrantedAuthority(String.format("ROLE_%s", role));
}
}