eeverman / eeverman/andhow

GroupProxy instances should by internal only

Open
#508 3 comments 0 reactions 0 assignees View on GitHub
API Change Needs More Info
Dominant language
Java
Stars
25
Forks
34
PR merge metrics
No merged PRs in 30d

Description

**Describe the bug**
There are several places application code can get GroupProxies:
* Application code could call new new `PropertyRegistrarLoader().getGroups()` or just invoke the Service Loader it uses directly.
* `AndHowConfiguration` (and thus StdConfig) has a `List getRegisteredGroups()` (Should this be in this class??)
* There are probably other places.

Once application code has a GroupProxy, AndHow allows retrieval of any Property in that Group, effectively providing a backdoor to access values for any Property, even if private.

To resolve this, I propose:
* Public look-up methods for GroupProxy should be disabled. There is no good reason to hand these instances out.
* Manual exports are handled, though perhaps they need a note RE the security implications of `ManualExportAllowed`.

Rejected ideas:
* GroupProxy should not hold a reference to a Group (ie Class) - It looks like the only place that is used is in building error messages, which could also be done using the canonical name of the group. This may cause issues w/ J9 modules anyway, so its probably best to remove.
No: GPs need access to the class they proxy. This has to be held somewhere, so this is the place.
* PropertyRegistrarLoader should thus not attempt to load actual classes or Properties.
No: This is still needed I believe.
* For exports, add an `Exportable` interface that has a default instance method that either reflects to find Property of the current class (its not clear if the security manager considers a default instance method to be part of the runtime class or not, so this may be disallowed), or, have AndHow implement getPropertiesForGroup(Group instance) where the Group is a live class instance. This would mean that `Exportable` should only be applied to empty classes where instances don't make a difference, and that could be enforced. There could be `ExportablePublicOnly` for classes that want to keep secrets and `ExporableAllProperties` for others.
Not sure I understand my own logic here...

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.