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

Fix #24 - Limit the extensibility of classes and methods #26

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

Conversation

OlivierJaquemet
Copy link
Contributor

Apply Guideline 4-5 / EXTEND-5: Limit the extensibility of classes and
methods from the Secure Coding Guidelines for Java SE
https://www.oracle.com/technetwork/java/seccodeguide-139067.html#4-5

(reported by running VisualCodeGrepper
https://github.com/nccgroup/VCG/blob/master/VisualCodeGrepper/modJavaCheck.vb#L318
)

Apply Guideline 4-5 / EXTEND-5: Limit the extensibility of classes and
methods from the Secure Coding Guidelines for Java SE
https://www.oracle.com/technetwork/java/seccodeguide-139067.html#4-5
@samdjstevens
Copy link
Owner

Thanks for the PR - I'm not 100% this is needed/beneficial. Whilst it does improve security and composition should be favoured over inheritence generally, is it being too restrictive by not allowing consumers to override RecoveryCodeGenerator for example?

@OlivierJaquemet
Copy link
Contributor Author

@samdjstevens honestly, I'm not sure either I should have made all classes final.
As stated in the Secure Coding Guidelines, we have two choices "Design classes and methods for inheritance or declare them final"...
So I have absolutely no problem in reverting some of them !!

I may have been a little overzealous :p

Be a little more openminded in applying
Guideline 4-5 / EXTEND-5: Limit the extensibility of classes and
methods from the Secure Coding Guidelines for Java SE
https://www.oracle.com/technetwork/java/seccodeguide-139067.html#4-5

Rule is :
"Design classes and methods for inheritance" or "declare them final"
This commit reverts use of final classes for default implementation
of as it was clearly design for inheritance and should be kept this way
@OlivierJaquemet
Copy link
Contributor Author

PR updated to revert use of final keyword for all classes which could be subclassed.

@samdjstevens
Copy link
Owner

Appreciate the work on this, but because these changes would require a new major version (non backwards compatible API changes) I think I'm going to hold off merging until other breaking changes are introduced into the library. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants