Output Subject Claim as Identity in Logging interceptor #946
Conversation
mrzzy
requested review from
davidheryanto,
khorshuheng,
pyalex,
woop and
zhilingc
as code owners
auth/src/main/java/feast/auth/interceptors/GrpcMessageInterceptor.java
Outdated
Show resolved
Hide resolved
|
/test test-end-to-end-redis-cluster |
|
/test test-end-to-end-auth |
3 similar comments
|
/test test-end-to-end-auth |
|
/test test-end-to-end-auth |
|
/test test-end-to-end-auth |
|
/test test-end-to-end |
mrzzy
added
compat/breaking
| @@ -54,6 +52,8 @@ | |||
| private String provider; | |||
|
|
|||
| // K/V options to initialize the provider with | |||
| // Key for Subject Claim option which sets the name of the subject claim field in tokens. | |||
There was a problem hiding this comment.
Should the comment above this one be below at the options?
|
/retest |
|
/lgtm |
|
/retest |
|
/test test-end-to-end |
| // When security is enabled, should only allow unauthenticated access to actuator and | ||
| // metrics endpoints. | ||
| .antMatchers("/") | ||
| // .antMatchers("/actuator/**", "/metrics/**") |
There was a problem hiding this comment.
Does this still have to be so permissive?
There was a problem hiding this comment.
Opted for disabling Spring Security autoconfiguration beans instead of hardcoding endpoints to disable in WebSecurityConfig.
| @@ -731,6 +814,9 @@ | |||
| <groupId>org.apache.maven.plugins</groupId> | |||
| <artifactId>maven-javadoc-plugin</artifactId> | |||
| <version>3.1.1</version> | |||
| <configuration> | |||
| <doclint>all</doclint> | |||
There was a problem hiding this comment.
Originally doclint is enabled via the maven-compiler-plugin option here. This enables doclint from maven-javadoc-plugin so that we can disable doclint for classpath of the OpenAPI client which fails doclint.
There was a problem hiding this comment.
But there is an exclusion. Why did this work in the past but not any more?
There was a problem hiding this comment.
I'm am not entirely sure myself.
The effect of this move is that doclint moves from the compile maven lifecycle to the test maven lifecycle.
Doclint still works and flags issues with the docs.
…tead of just id For the 'google' authentication provider this means that the email would be output as identity instead of just a user id
* This is done as subjectClaim would be used even when only authentication is enabled. * For HttpAuthorizationProvider, which requires both authentication and authorization options, the options maps are merged together.
…pecified (ie in Job Controller).
…Interceptor optinal
…ding dummy WebSecurityConfig As GrpcMessageInterceptor is moved feast-auth package to prevent a circular dependency, jobcontroller has to import feast-auth to get GrpcMessageInterceptor. feast-auth also imports spring security, which automatically requires WebSecurityConfigurerAdapter to be present in the spring application context. Commit provides a dummy WebSecurityConfigurerAdapter required to statisfy spring.
… in JobController
…bean instead of WebSecurityConfig
|
/test @feast-ci-bot |
|
/test test-end-to-end-redis-cluster |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mrzzy, woop The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/lgtm |
What this PR does / why we need it:
subjectClaimoption fromfeast.authorization.optionstofeast.authentication.optionssubjectClaimoptions is used byGrpcMessageInterceptoreven when only authentication is enabled.GrpcMessageInterceptorto output subject claim as identity.googleauthentication provider, this would output the users email as identity.feast.authentication.options.subjectClaim.feast-authmodule intofeast-commonmodule to prevent a circular dependency.Which issue(s) this PR fixes:
Fixes #
Does this PR introduce a user-facing change?: