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

Add an interceptor to support AuthenticationManagerResolver #1034

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

Add an interceptor to support AuthenticationManagerResolver #1034

wants to merge 14 commits into from

Conversation

mehrabisajad
Copy link

This proposal introduces the AuthenticationManagerResolver<GrpcServerRequest> to Spring's gRPC authentication framework. This provides a flexible mechanism for implementing dynamic authentication based on your specific needs.

The Problem:

  • Traditional Spring gRPC authentication relies on fixed configuration of authentication providers, limiting flexibility.
  • Changing authentication mechanisms requires code modifications and redeployments.

The Solution:

  • Introduce AuthenticationManagerResolver:
    • Allows dynamic selection of the AuthenticationManager based on the gRPC request.
    • Enables fine-grained control over authentication based on request headers, payload, or other criteria.

@ST-DDT
Copy link
Collaborator

ST-DDT commented Jan 22, 2024

Nice suggestion, I'm currently quite busy so I might be slow to respond.

ST-DDT
ST-DDT reviewed Jan 25, 2024
View reviewed changes
Copy link
Collaborator

@ST-DDT ST-DDT left a comment

Choose a reason for hiding this comment

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

Please add a test that checks whether this works as expected.

...net/devh/boot/grpc/server/security/interceptors/AbstractAuthenticatingServerInterceptor.java Outdated Show resolved Hide resolved
...net/devh/boot/grpc/server/security/interceptors/AbstractAuthenticatingServerInterceptor.java Outdated Show resolved Hide resolved
docs/en/server/security.md Outdated Show resolved Hide resolved
.../net/devh/boot/grpc/server/security/interceptors/DefaultAuthenticatingServerInterceptor.java Outdated Show resolved Hide resolved
.../net/devh/boot/grpc/server/security/interceptors/DefaultAuthenticatingServerInterceptor.java Outdated Show resolved Hide resolved
...nfigure/src/main/java/net/devh/boot/grpc/server/security/interceptors/GrpcServerRequest.java Outdated Show resolved Hide resolved
...h/boot/grpc/server/security/interceptors/ManagerResolverAuthenticatingServerInterceptor.java Outdated Show resolved Hide resolved
...h/boot/grpc/server/security/interceptors/ManagerResolverAuthenticatingServerInterceptor.java Outdated Show resolved Hide resolved
ST-DDT
ST-DDT reviewed Feb 3, 2024
View reviewed changes
...h/boot/grpc/server/autoconfigure/GrpcServerSecurityWithManagerResolverAutoConfiguration.java Outdated Show resolved Hide resolved
...net/devh/boot/grpc/server/security/interceptors/AbstractAuthenticatingServerInterceptor.java Outdated Show resolved Hide resolved
...net/devh/boot/grpc/server/security/interceptors/AbstractAuthenticatingServerInterceptor.java Outdated Show resolved Hide resolved
...h/boot/grpc/server/autoconfigure/GrpcServerSecurityWithManagerResolverAutoConfiguration.java Outdated Show resolved Hide resolved
...nfigure/src/main/java/net/devh/boot/grpc/server/security/interceptors/GrpcServerRequest.java Outdated Show resolved Hide resolved
...ava/net/devh/boot/grpc/test/config/WithBasicAuthAndManagerResolverSecurityConfiguration.java Outdated Show resolved Hide resolved
@ST-DDT
Copy link
Collaborator

ST-DDT commented Feb 11, 2024

We moved some files. Could you please fix the merge conflicts?

@mehrabisajad
Merge branch 'master' into support-AuthenticationManagerResolver
18bd3e2 
# Conflicts:
#	grpc-server-spring-boot-starter/src/main/java/net/devh/boot/grpc/server/security/interceptors/AbstractAuthenticatingServerInterceptor.java
#	grpc-server-spring-boot-starter/src/main/java/net/devh/boot/grpc/server/security/interceptors/GrpcServerRequest.java
#	grpc-server-spring-boot-starter/src/main/java/net/devh/boot/grpc/server/security/interceptors/ManagerResolverAuthenticatingServerInterceptor.java
@mehrabisajad
Merge branch 'master' into support-AuthenticationManagerResolver
a5904ab
@mehrabisajad
Copy link
Author

Great, I've fixed the merge conflicts that arose due to the file movements. I've carefully reviewed the changes and believe everything is now in sync.

I've pushed the changes to the branch. Could you please take a look at the latest version and let me know if you have any further questions or require additional modifications?

ST-DDT
ST-DDT previously approved these changes Apr 1, 2024
View reviewed changes
Copy link
Collaborator

@ST-DDT ST-DDT left a comment

Choose a reason for hiding this comment

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

Implementation wise looks good to me, but I have no time to check the security/config side effects.

...h/boot/grpc/server/security/interceptors/ManagerResolverAuthenticatingServerInterceptor.java Outdated Show resolved Hide resolved
@ST-DDT
Update grpc-server-spring-boot-starter/src/main/java/net/devh/boot/gr…
4a754c2 
…pc/server/security/interceptors/ManagerResolverAuthenticatingServerInterceptor.java
ST-DDT
ST-DDT previously approved these changes Apr 13, 2024
View reviewed changes
Copy link
Collaborator

@ST-DDT ST-DDT left a comment

Choose a reason for hiding this comment

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

I haven't tested this myself and I'm unsure about the change in condition regarding when the security config is enabled.

Other than that, it looks good to me.

...h/boot/grpc/server/security/interceptors/ManagerResolverAuthenticatingServerInterceptor.java Outdated Show resolved Hide resolved
.../net/devh/boot/grpc/server/security/interceptors/DefaultAuthenticatingServerInterceptor.java Outdated Show resolved Hide resolved
@mehrabisajad
Copy link
Author

I haven't tested this myself and I'm unsure about the change in condition regarding when the security config is enabled.

Other than that, it looks good to me.

If on of the interceptors condition met, then it will be registered. On the other hand if both conditions met then the first one will be registered according to the priority.

According to existing GrpcServerSecurityAutoConfiguration Conditions, if GrpcAuthenticationReader is not registered, I think it will probably encounter error. So all the configuration depends on GrpcAuthenticationReader.

If you have any other situation to consider, please let me know.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ST-DDT ST-DDT ST-DDT approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@mehrabisajad @ST-DDT