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.

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

Added OpenCHAMI middleware to router.NewRouter() #29

Merged
merged 7 commits into from
Aug 12, 2024
Merged

Conversation

davidallendj
Copy link
Collaborator

Addresses #27 by adding the OpenCHAMI middleware.

@davidallendj davidallendj added enhancement New feature or request need changes labels Aug 12, 2024
@davidallendj
Copy link
Collaborator Author

Seems that the OpenCHAMI JWTAuth used with AuthenticatorWithRequiredClaims is not compatible with the one being used with github.com/OpenCHAMI/jwtauth/v5 and the PR that adds the jwt.NewKeySet and JWKS functionality has not be merged upstream yet (go-chi/jwtauth#85).

@synackd
Copy link
Contributor

synackd commented Aug 12, 2024

Is the functionality in that upstream PR able to be put into the OpenCHAMI jwtauth for now?

@davidallendj
Copy link
Collaborator Author

davidallendj commented Aug 12, 2024

Is the functionality in that upstream PR able to be put into the OpenCHAMI jwtauth for now?

I think the OpenCHAMI jwtauth already has that functionality. I think we would just have to change the import in the OpenCHAMI middleware to point to the OpenCHAMI jwtauth for now and then rebase it with the upstream go-chi changes. Once it finally gets merged, we can change the import back to upstream and then delete the OpenCHAMI one.

@synackd
Copy link
Contributor

synackd commented Aug 12, 2024

OK. Is this ready to be tested?

Copy link
Member

@alexlovelltroy alexlovelltroy left a comment

Choose a reason for hiding this comment

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

LGTM

@davidallendj
Copy link
Collaborator Author

OK. Is this ready to be tested?

Looks like SMD is ready, but we'll have to switch the authenticator to use the OpenCHAMI jwtauth for now.

@synackd
Copy link
Contributor

synackd commented Aug 12, 2024

I'm getting this error when building:

# github.com/OpenCHAMI/smd/v2/cmd/smd
cmd/smd/routers.go:72:61: cannot use s.tokenAuth (variable of type *"github.com/OpenCHAMI/jwtauth/v5".JWTAuth) as *"github.com/go-chi/jwtauth/v5".JWTAuth value in argument to openchami_authenticator.AuthenticatorWithRequiredClaims
cmd/smd/auth.go:13:2: "github.com/lestrrat-go/jwx/jwt" imported and not used
make: *** [Makefile:55: smd] Error 1

@synackd
Copy link
Contributor

synackd commented Aug 12, 2024

Looks like switching the import to use the OpenCHAMI jwtauth will solve this as discussed here.

@synackd
Copy link
Contributor

synackd commented Aug 12, 2024

SMD is now building with those changes.

Copy link
Contributor

@synackd synackd left a comment

Choose a reason for hiding this comment

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

Tested working. LGTM!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request need changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants