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

Algorithm enum needs to be moved from JwtSigner impls #7

Closed
aanganes opened this issue Mar 22, 2012 · 10 comments
Closed

Algorithm enum needs to be moved from JwtSigner impls #7

aanganes opened this issue Mar 22, 2012 · 10 comments
Assignees

Comments

@aanganes
Copy link
Contributor

The Algorithm enum is defined in all of the JwtSigner implementations. It should be moved to a centralized Enum class.

@ghost ghost assigned jricher Mar 22, 2012
@nemonik
Copy link
Contributor

nemonik commented Mar 23, 2012

Each is relevant to the specific implementation, e.g., HMACSHA256 has no relevance to a RSA Signer.

@nemonik
Copy link
Contributor

nemonik commented Mar 23, 2012

So doesn't belong in an interface.

@jricher
Copy link
Member

jricher commented Mar 23, 2012

No, these values are defined by the JWS specification in a central list and they need to be used as common keys to select signers. They need to be coalesced into a central Enum class.

@nemonik
Copy link
Contributor

nemonik commented Mar 23, 2012

We should discuss out of band.

@aanganes
Copy link
Contributor Author

The issue is that there are several places in the code where (in future) we will need to have services that can get a JwtSigner by algorithm name. In order for that to work smoothly, the algorithm name should be accessible from the interface, not from the implementations. That way I can say "JwtSigner currentSigner = signerService.getSignerByAlgorithm("HMACSHA256")". It would also be useful to be able to retrieve the algorithm name from a JwtSigner whose implementation is unknown.

For right now we are just using a default signer (the RSA one), but further on we will need to be able to retrieve a signer corresponding to the Client's registered preferred algorithm.

@nemonik
Copy link
Contributor

nemonik commented Mar 23, 2012

We should discuss offline as the spec permits multiple instances of a specific signer algorithm as i read it

@jricher
Copy link
Member

jricher commented Mar 23, 2012

Yes it does, and this doesn't restrict that capability.

@aanganes
Copy link
Contributor Author

We still need to be able to choose a signer based on the Client's preferred algorithm. signerService.getSignerByAlgorithm() can return a list, and we can have some rule for determining which one to use if we have several. But either way, we need that capability.

@nemonik
Copy link
Contributor

nemonik commented Mar 29, 2012

I recognized some needed refactoring an code clean up across the signers responding to issue #5 as I learned more about creating Spring beans writing the client Authentication Filter. My advice would be to the keep the Algorithm enums where they are as they are specific to the individual signers in the purpose that they serve: They map the Spec defined names to the Java Standard cryptographic algorithm names for each respective signer. Each of the signers use the ENUM in their respective afterPropertiesSet-method to instantiate an instance of the algorithm by which the respective class will sign and verify signatures. I do believe you both are correct an enum needs to be in the interface, though. The afterPropertiesSet interface supports an exception being thrown for implementations of this method, and an exception will be thrown if an spec algorithm name is submitted not matching one supported by the signer.

@jricher jricher closed this as completed Apr 2, 2012
@jricher
Copy link
Member

jricher commented Apr 2, 2012

Refactored to JwsAlgorithm enum

@jricher jricher reopened this Apr 2, 2012
@jricher jricher closed this as completed Apr 3, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants