Make JWT brokers injectable #570
Conversation
dimas-b
requested review from
jbonofre,
ashvina,
RussellSpitzer,
snazy,
vvcephei,
takidau,
jackye1995,
flyrain,
eric-maynard,
collado-mike,
ebyhr and
adutra
as code owners
| private String file; | ||
| private String secret; | ||
|
|
||
| public String getType() { |
There was a problem hiding this comment.
I suspect you need some @JsonSetter annotations here for Jackson? (#561)
There was a problem hiding this comment.
Or a @JsonCreator constructor?
There was a problem hiding this comment.
"Need" in what sense? It works "as is" as the receiver of the DW YAML config data.
There was a problem hiding this comment.
Added @JsonProperty for clarity.
|
|
||
| @Override | ||
| public TokenBroker apply(RealmContext realmContext) { | ||
| if (file == null && secret == null) { | ||
| String secret = config.secret(); |
There was a problem hiding this comment.
I guess this check can be moved to the constructor.
There was a problem hiding this comment.
True, but I did not want to alter behaviour is this PR. Let's me know and I will move the check.
There was a problem hiding this comment.
I'm for checking in the constructor, FWIW
There was a problem hiding this comment.
refactored (and rebased)
| config.maxTokenGenerationInSeconds()); | ||
| } | ||
|
|
||
| public interface Config { |
There was a problem hiding this comment.
Would it be better / simpler to have one config interface for all token broker factories?
There was a problem hiding this comment.
Updated to a unified config interface.
| private String file; | ||
| private String secret; | ||
|
|
||
| public String getType() { |
There was a problem hiding this comment.
Or a @JsonCreator constructor?
| } | ||
|
|
||
| public interface Config { | ||
| int maxTokenGenerationInSeconds(); |
There was a problem hiding this comment.
| int maxTokenGenerationInSeconds(); | |
| Duration maxTokenGeneration(); |
(but I don't know if DW has support for java time)
There was a problem hiding this comment.
I did not want to introduce functional changes in this PR. It's just a refactoring for injectability.
dimas-b
force-pushed
the
injectable-jwt-brokers
branch
from
826d70a to
29bbfb3
Compare
* Detach `TokenBrokerFactory` sub-classes from Jackson-based config parsing logic. * Add a new unified `TokenBrokerConfig` under DW to hold JWT config data. * Resolve JWT factories through HK2 named service lookup, using the type name from `TokenBrokerConfig` * Add dedicated class `NoneTokenBrokerFactory` for the default do-nothing token broker implementation. * Yaml config (`polaris-server.yml`) is not affected
dimas-b
force-pushed
the
injectable-jwt-brokers
branch
from
29bbfb3 to
b4ea9e3
Compare
dimas-b
force-pushed
the
injectable-jwt-brokers
branch
from
b4ea9e3 to
39891f5
Compare
Detach
TokenBrokerFactorysub-classes from Jackson-based config parsing logic.Add a new unified
TokenBrokerConfigunder DW to hold JWT config data.Resolve JWT factories through HK2 named service lookup, using the type name from
TokenBrokerConfigAdd dedicated class
NoneTokenBrokerFactoryfor the default do-nothing token broker implementation.Yaml config (
polaris-server.yml) is not affected