Adding a cache is not trivial as there's quite a few places it needs to be
added. I can't remember exactly where, but you should be able to find it by
simply searching for other caches that we define.
On Fri, 15 Feb 2019 at 03:33, James Campbell <james.p.campbell(a)gmail.com>
wrote:
Thanks, Stian--the more detailed documentation inside the
integration-arquillian folder helped understand the tests better.
I've been able to read through a lot more on the tests, and I thought I
had a picture of how to extend the current tests, but when I tried directly
it seems that there must be configuration elements of the testing
infrastructure itself that I'm not finding. Specifically, the code I added
uses a new Infinispan cache to hold the configs, and I have been able to
successfully test it via a manual build and run of keycloak. However,
running inside the test harness gives me an error that the cache is not
configured. I suspect I just don't know where the relevant XML and/or
DeploymentProcessors are located for the testing framework.
If there's a path to making the work I did useful, I'm game for it, but
I'd need some more hints I'm sure to make it through the proper
implementation of the tests.
James
On Wed, Feb 6, 2019 at 5:30 AM Stian Thorgersen <sthorger(a)redhat.com>
wrote:
>
>
> On Wed, 6 Feb 2019 at 02:27, James Campbell <james.p.campbell(a)gmail.com>
> wrote:
>
>> Inline...
>>
>> On Tue, Feb 5, 2019 at 3:45 AM Stian Thorgersen <sthorger(a)redhat.com>
>> wrote:
>>
>>>
>>>
>>> On Mon, 4 Feb 2019 at 05:14, James Campbell
<james.p.campbell(a)gmail.com>
>>> wrote:
>>>
>>>> All--
>>>>
>>>> Thanks to Stian's recommendations for similar features in the
>>>> codebase, I've developed a proposal for how to implement this
feature, and
>>>> sketched out an initial implementation. As he suggested, I'd like to
get
>>>> feedback before opening up a PR.
>>>>
>>>> 1. We introduce a new subclass of the OIDCIdentityProvider / Factory
>>>> called OIDCDiscoveryIdentityProvider. The new class adds a
discoverConfig
>>>> method that can be used to discover and set all relevant endpoints in a
>>>> configuration except the issuer. Issuer then becomes the sole required
>>>> element of the config. (So the GoogleIdentityProvider, for example, will
>>>> then just call discoverConfig with its hard-coded issuer).
>>>> -> During the parseConfig call, the Factory will raise a
>>>> RuntimeException if the config cannot be obtained, since if the config
can
>>>> never be obtained there is no way to have a valid configuration at all.
>>>> -> During the discoverConfig call, we return the cached value, and
>>>> optionally refresh it based on when we last obtained the configuration.
>>>> -> In the event of a failure in the *refresh* I'm imagining
the
>>>> best behavior is to log a warning but then return the cached config
(last
>>>> known good config).
>>>>
>>>
>>> This should just be added to the OIDCIdentityProvider directly rather
>>> than a subclass. Perhaps we should have an option to configure how
>>> frequently the configuration is reloaded.
>>>
>>
>> Makes sense, thank you.
>>
>>
>>>
>>>> 2. We introduce an SPI and interface called
>>>> OIDCDiscoveryRepresentationProvider / Factory and implementations called
>>>> Infinispan... which obtains OIDCConfigurationRepresentations using the
>>>> /.well-known/openid-configuration endpoint on the issuer.
>>>>
>>>> I have a couple questions:
>>>>
>>>> 1. I began an implementation of this, but the @JsonProperty
>>>> annotations on the existing OIDCCOnfigurationRepresentation class seem
to
>>>> be ignored when I try to have it read from the openid-configuration
>>>> docuement, e.g.:
>>>> - OIDCConfigurationRepresentation rep = SimpleHttp.doGet(issuer +
>>>> "/.well-known/openid-configuration",
>>>> session).asJson(OIDCConfigurationRepresentation.class); // FAILS with
>>>> "Unrecognized field ..." Perhaps a mismatch of the jackson
annotation
>>>> version in the keycloak-model-infinispan submodule?
>>>>
>>>
>>> Is the unknown field actually mapped or is the ignore unknown fields
>>> option enabled? The ignore unknown fields should probably be enabled as
>>> there may be unknown elements in the config.
>>>
>>
>> The unknown field in this case was authorization_endpoint (so yes,
>> mapped, and in code that I did not change). I wondered whether perhaps the
>> issue is that I was doing this from inside the keycloak-model-infinispan
>> module, and a conflicting JSON parser dependency is in play? I did try
>> adding the @JsonIgnoreProperties annotation as well, to no avail. My
>> suspicion is just based on (1) the current class correctly supporting
>> deserialization in existing tests, and (2) a search of common causes for
>> this error (which I've not encountered before).
>>
>
> Is this from unit tests within the keycloak-model-infinispan module or
> test in the Arquillian testsuite? If the latter it should pick up the
> correct serializer. It has to be the fasterxml one otherwise the
> annotations will be ignored.
>
>
>>
>>
>>>
>>>
>>>>
>>>> 2. Assuming the design above makes sense, what is the right way to
>>>> plug this into the testing framework? Is one of the core developers
>>>> interested in tackling that aspect if I push a PR with the above
features
>>>> implemented?
>>>>
>>>
>>> We are very reluctant to receive PRs from the community that do not
>>> include tests. Think about it, would you as a developer like to implement
>>> tests for other developers? We can help give you recommendations on how to
>>> test it though.
>>>
>>>
>>
>> Definitely makes sense; I just didn't see any documentation on the
>> testing framework at all, whereas there is documentation on the core server
>> development components. Happy to either take a look at docs you flag, or to
>> work from any recommendations you have (I certainly wasn't thinking a PR
>> would be ready without tests!). The current testing system is pretty opaque
>> to me.
>>
>
> We have an Arquillian based testsuite that allows us to write functional
> tests with all the benefits of unit tests. The testsuite allows running
> tests from within the IDE (uses Keycloak with an embedded Undertow server)
> and also run them against a proper built Keycloak on top of WildFly from
> the command line.
>
> Some details are here:
>
>
https://github.com/keycloak/keycloak/tree/master/testsuite/integration-ar...
>
> To understand how to write the tests and run them try to run any test
> inside testsuite/integration-arquillian/tests/base/src/test from the IDE.
>
> Identity brokering tests are under
>
testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/broker.
> You should be able to relatively easily extend these tests to include
> testing of OIDC well-known endpoints. We also have support in the testsuite
> to run code directly on the server (embedded or external) to be able to
> probe inside the server if needed. Take a look at
>
https://github.com/keycloak/keycloak/blob/master/testsuite/integration-ar...
> as an example on how to do that.
>
>
>
>>
>>
>>
>>>
>>>> James
>>>>
>>>> On Sun, Jan 27, 2019 at 11:15 AM James Campbell <
>>>> james.p.campbell(a)gmail.com> wrote:
>>>>
>>>>> Got it; I checked that code out and it does look like a pretty
direct
>>>>> model for the refreshing feature. I'll begin working on a PR.
>>>>>
>>>>> On Mon, Jan 21, 2019 at 3:25 AM Stian Thorgersen
<sthorger(a)redhat.com>
>>>>> wrote:
>>>>>
>>>>>> JWKs used by identity brokering and client authentication.
>>>>>>
>>>>>> On Sat, 19 Jan 2019 at 23:29, James Campbell <
>>>>>> james.p.campbell(a)gmail.com> wrote:
>>>>>>
>>>>>>> Stian--
>>>>>>>
>>>>>>> Thanks for confirming. Which keys are you referring
to--I'll take a
>>>>>>> look at that in the code and try to follow that pattern
closely.
>>>>>>>
>>>>>>> James
>>>>>>>
>>>>>>> On Wed, Jan 16, 2019 at 1:52 AM Stian Thorgersen <
>>>>>>> sthorger(a)redhat.com> wrote:
>>>>>>>
>>>>>>>> It would be good to get these changes included. We do
something
>>>>>>>> similar for the keys today and they are cached in an
Infinispan local
>>>>>>>> cache. Could do something similar for the OIDC
discovery/config.
>>>>>>>>
>>>>>>>> On Tue, 15 Jan 2019 at 17:44, James Campbell <
>>>>>>>> james.p.campbell(a)gmail.com> wrote:
>>>>>>>>
>>>>>>>>> Tomas--
>>>>>>>>>
>>>>>>>>> Thanks--it certainly seems close, you're right!
It looks, however
>>>>>>>>> like an
>>>>>>>>> OIDC provider still uses a static configuration even
though it
>>>>>>>>> loads it
>>>>>>>>> from the discovery URL--that is, once it's loaded
at
>>>>>>>>> configuraiton time, it
>>>>>>>>> doesn't discover new changes, and there isn't
an option to
>>>>>>>>> refresh/store
>>>>>>>>> that discovery endpoint outside of configuration.
>>>>>>>>>
>>>>>>>>> It's not clear to me how important that feature
is--on one hand,
>>>>>>>>> it seems
>>>>>>>>> unlikely that we should expect frequent changes; on
the other, in
>>>>>>>>> the short
>>>>>>>>> time since I started exploring this setup, I have
encountered
>>>>>>>>> three changes
>>>>>>>>> in google's OIDC endpoints between what was
hard-coded into
>>>>>>>>> keycloak, what
>>>>>>>>> is in their documentation, and what their current
discovery
>>>>>>>>> endpoint
>>>>>>>>> provides. (And, the google docs specifically suggest
refreshing
>>>>>>>>> from the
>>>>>>>>> endpoint periodically).
>>>>>>>>>
>>>>>>>>> Thoughts?
>>>>>>>>>
>>>>>>>>> James
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On Tue, Jan 15, 2019 at 10:31 AM Tomas Kyjovsky <
>>>>>>>>> tkyjovsk(a)redhat.com> wrote:
>>>>>>>>>
>>>>>>>>> > Please disregard my previous message.
>>>>>>>>> >
>>>>>>>>> > Looking at the docs [1] and the Admin Console UI
this should be
>>>>>>>>> already
>>>>>>>>> > possible with the OIDC identity provider.
>>>>>>>>> > When creating a OIDC identity provider in the
Admin Console
>>>>>>>>> there is an
>>>>>>>>> > option at the bottom of the page to import OIDC
configuration
>>>>>>>>> metadata from
>>>>>>>>> > URL.
>>>>>>>>> >
>>>>>>>>> > Does this cover your use case?
>>>>>>>>> >
>>>>>>>>> >
>>>>>>>>> > Regards,
>>>>>>>>> > Tomas
>>>>>>>>> >
>>>>>>>>> > [1]
>>>>>>>>> >
>>>>>>>>>
https://www.keycloak.org/docs/latest/server_admin/index.html#openid-conne...
>>>>>>>>> >
>>>>>>>>> >
>>>>>>>>> > ----- Original Message -----
>>>>>>>>> > > Hello James,
>>>>>>>>> > >
>>>>>>>>> > > See my 2 cents inline..
>>>>>>>>> > >
>>>>>>>>> > > ----- Original Message -----
>>>>>>>>> > > > All--
>>>>>>>>> > > >
>>>>>>>>> > > >
>>>>>>>>> > > >
>>>>>>>>> > > > After observing that the Google Social
Identity Provider in
>>>>>>>>> Keycloak
>>>>>>>>> > was
>>>>>>>>> > > > using a deprecated userprofile
endpoint [
>>>>>>>>> > > > <
>>>>>>>>>
https://issues.jboss.org/projects/KEYCLOAK/issues/KEYCLOAK-9179>
>>>>>>>>> > > > Keycloak-9179, <
>>>>>>>>>
https://issues.jboss.org/browse/KEYCLOAK-9169>
>>>>>>>>> > > > Keycloak-9169], I wanted to propose
the creation of an
>>>>>>>>> IdentityProvider
>>>>>>>>> > > > that
>>>>>>>>> > > > will use the OIDC Discovery mechanism
to dynamically build
>>>>>>>>> a config [
>>>>>>>>> > > >
<
https://issues.jboss.org/browse/KEYCLOAK-9194>
>>>>>>>>> Keycloak-9194].
>>>>>>>>> > > >
>>>>>>>>> > > >
>>>>>>>>> > > >
>>>>>>>>> > > > I see a few decision points along the
way that I wanted to
>>>>>>>>> ask about
>>>>>>>>> > before
>>>>>>>>> > > > an implementation, since I'm very
new to keycloak and just
>>>>>>>>> starting to
>>>>>>>>> > > > understand the codebase. In
particular, I wondered if this
>>>>>>>>> group could
>>>>>>>>> > > > share
>>>>>>>>> > > > insight into these couple issues so I
can make a more
>>>>>>>>> informed design:
>>>>>>>>> > > >
>>>>>>>>> > > >
>>>>>>>>> > > >
>>>>>>>>> > > > 1. It looks to me like the actual
IdentityProviders are
>>>>>>>>> instantiated
>>>>>>>>> > > > just
>>>>>>>>> > > > as they're being used, but that
the models are persisted in
>>>>>>>>> the
>>>>>>>>> > RealmModel.
>>>>>>>>> > > > It's not clear to me where the
separation of concerns is
>>>>>>>>> supposed to be
>>>>>>>>> > > > between the IdentityProvider and the
>>>>>>>>> IdentityProviderModel-in
>>>>>>>>> > particular
>>>>>>>>> > > > since the GoogeIdentityProvider, say,
immediately sets
>>>>>>>>> endpoints in its
>>>>>>>>> > > > constructor. Normatively, where
*should* social identity
>>>>>>>>> providers'
>>>>>>>>> > model
>>>>>>>>> > > > configuration be set (and, e.g., where
are the models first
>>>>>>>>> added to
>>>>>>>>> > the
>>>>>>>>> > > > RealmModel)?
>>>>>>>>> > >
>>>>>>>>> > > Provider classes are being instantiated per
transaction by
>>>>>>>>> their
>>>>>>>>> > > corresponding ProviderFactories and then
left to be
>>>>>>>>> garbage-collected
>>>>>>>>> > after
>>>>>>>>> > > Provider.close() is called.
>>>>>>>>> > > The Provider class is given its
configuration
>>>>>>>>> (IdentityProviderModel in
>>>>>>>>> > this
>>>>>>>>> > > case) by its factory which I believe loads
it from cache/jpa
>>>>>>>>> layer. Any
>>>>>>>>> > > class extending AbstractIdentityProvider
should then be able
>>>>>>>>> to access
>>>>>>>>> > its
>>>>>>>>> > > config via getConfig() method but I
don't think it will be
>>>>>>>>> able to
>>>>>>>>> > > update/persist it back. The provider
configuration/model
>>>>>>>>> itself is
>>>>>>>>> > managed
>>>>>>>>> > > by the IdentityProviderResource (REST
endpoint accessible via
>>>>>>>>> REST or
>>>>>>>>> > admin
>>>>>>>>> > > console UI) in the keycloak/services module
so I think the
>>>>>>>>> > > auto-configuration logic would have to be
placed somewhere
>>>>>>>>> there.
>>>>>>>>> > >
>>>>>>>>> > > >
>>>>>>>>> > > >
>>>>>>>>> > > >
>>>>>>>>> > > > 2. I see that there is logic to
parse OIDC Discovery
>>>>>>>>> configuration
>>>>>>>>> > as
>>>>>>>>> > > > part of configuring Keycloak itself as
an OIDC provider /
>>>>>>>>> implementer
>>>>>>>>> > of
>>>>>>>>> > > > OIDC protocol (including building and
parsing the
>>>>>>>>> .well-known config
>>>>>>>>> > > > elements), but that logic seems not to
be used in any
>>>>>>>>> setting
>>>>>>>>> > currently as
>>>>>>>>> > > > a
>>>>>>>>> > > > client. Should I plan to reuse, say,
the
>>>>>>>>> > OIDCConfigurationRepresentation
>>>>>>>>> > > > and
>>>>>>>>> > > > OIDCWellKnownProvider classes for
their logic in handling
>>>>>>>>> such configs?
>>>>>>>>> > > >
>>>>>>>>> > > >
>>>>>>>>> > > >
>>>>>>>>> > > >
>>>>>>>>> > > >
>>>>>>>>> > > > Currently, I'm imagining something
along the lines of
>>>>>>>>> extending
>>>>>>>>> > > > OIDCIdentityProvider with a new
>>>>>>>>> OIDCDiscoveryIdentityProvider that
>>>>>>>>> > adds a
>>>>>>>>> > > > discoverConfig method which can be
used by an implementing
>>>>>>>>> class (such
>>>>>>>>> > as
>>>>>>>>> > > > GoogleIdentityProvider) to discover
and cache endpoints
>>>>>>>>> such that they
>>>>>>>>> > are
>>>>>>>>> > > > not hard-coded into the implementing
class. Each
>>>>>>>>> implementing class
>>>>>>>>> > would
>>>>>>>>> > > > then have a public static final
DISCOVERY_URL that it
>>>>>>>>> passes to
>>>>>>>>> > > > discoverConfig.
>>>>>>>>> > > >
>>>>>>>>> > > >
>>>>>>>>> > > >
>>>>>>>>> > > > My main hangup, as suggested above, is
that to implement
>>>>>>>>> the caching, I
>>>>>>>>> > > > want
>>>>>>>>> > > > to ensure that the model configuration
is stored/set in the
>>>>>>>>> right
>>>>>>>>> > place.
>>>>>>>>> > > >
>>>>>>>>> > > >
>>>>>>>>> > > >
>>>>>>>>> > > > Thanks for bearing with me as I come
up to speed on this!
>>>>>>>>> > > >
>>>>>>>>> > > >
>>>>>>>>> > > >
>>>>>>>>> > > > James
>>>>>>>>> > > >
>>>>>>>>> > > >
_______________________________________________
>>>>>>>>> > > > keycloak-dev mailing list
>>>>>>>>> > > > keycloak-dev(a)lists.jboss.org
>>>>>>>>> > > >
https://lists.jboss.org/mailman/listinfo/keycloak-dev
>>>>>>>>> > > >
>>>>>>>>> > >
>>>>>>>>> > >
>>>>>>>>> > > Regards,
>>>>>>>>> > > Tomas
>>>>>>>>> > >
>>>>>>>>> >
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> --
>>>>>>>>> -... . / - .-. ..- . .-.-.- / -... . / .--. .-. . ...
. -. -
>>>>>>>>> .-.-.- / -...
>>>>>>>>> . / ..-. .. -
>>>>>>>>> 07:d0:2d:0b:d6:9d:27:c6:a7:c1:ec:e3:b1:65:f4:70
>>>>>>>>> _______________________________________________
>>>>>>>>> keycloak-dev mailing list
>>>>>>>>> keycloak-dev(a)lists.jboss.org
>>>>>>>>>
https://lists.jboss.org/mailman/listinfo/keycloak-dev
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>> --
>>>>>>> -... . / - .-. ..- . .-.-.- / -... . / .--. .-. . ... . -. -
.-.-.-
>>>>>>> / -... . / ..-. .. -
>>>>>>> 07:d0:2d:0b:d6:9d:27:c6:a7:c1:ec:e3:b1:65:f4:70
>>>>>>>
>>>>>>
>>>>>
>>>>> --
>>>>> -... . / - .-. ..- . .-.-.- / -... . / .--. .-. . ... . -. - .-.-.-
/
>>>>> -... . / ..-. .. -
>>>>> 07:d0:2d:0b:d6:9d:27:c6:a7:c1:ec:e3:b1:65:f4:70
>>>>>
>>>>
>>>>
>>>> --
>>>> -... . / - .-. ..- . .-.-.- / -... . / .--. .-. . ... . -. - .-.-.- /
>>>> -... . / ..-. .. -
>>>> 07:d0:2d:0b:d6:9d:27:c6:a7:c1:ec:e3:b1:65:f4:70
>>>>
>>>
>>
>> --
>> -... . / - .-. ..- . .-.-.- / -... . / .--. .-. . ... . -. - .-.-.- /
>> -... . / ..-. .. -
>> 07:d0:2d:0b:d6:9d:27:c6:a7:c1:ec:e3:b1:65:f4:70
>>
>