7:09 a.m.
Hi,
After KEYCLOAK-3618 is (hopefully) resolved, providers will have an
ability to log admin events with custom ResourceType. However, custom
values won't show in the Events - Admin Events - Filter - Resource
Types dropdown (fortunately, the values can still be typed in
directly).
The values in the dropdown reflect
org.keycloak.services.resources.admin.info.ServerInfoAdminResource::ENU
MS content. How about making it mutable, so that providers could
register provider-specific values? These could be some two methods: one
taking (String, String[]) to add values to existing enum maps, another
one taking (Class... enums) to register completely different enums for
provider's private needs.
What do you think?
Dmitry
7:18 a.m.
I don't like the PRs you've sent at all. It's all very hacky, calling
static methods on other classes is far from elegant. I've rejected both
PRs. It also ends up being completely undocumented so no-one except you
would know about this.
On 17 October 2016 at 21:15, Dmitry Telegin <mitya(a)cargosoft.ru> wrote:
JIRA https://issues.jboss.org/browse/KEYCLOAK-3711
PR https://github.com/keycloak/keycloak/pull/3337
_______________________________________________
keycloak-dev mailing list
keycloak-dev(a)lists.jboss.org
https://lists.jboss.org/mailman/listinfo/keycloak-dev
9:57 p.m.
Hi Stian,
I completely agree on the first point, this deserves a better solution.
But knowing that it would require digging deep into the guts of
KeyCloak, I just didn't dare :)
I would see providers somehow registering their custom enum keys,
either via API call or via callback, and KeyCloak would maintain the
list of custom values per provider. ServerInfoAdminResource then could
return a combined list of enum keys, both built-in and defined by
providers. But I'm afraid that would require changes to API and/or
interfaces. What do you think? How would you approach this problem
yourself?
And BTW, what's wrong with the second PR? The github comment seems a
bit unclear, so could you please elaborate? It has completely different
scope, though deals with the same feature. It's not about exposing
anything via static methods, it's about making ResourceType extensible,
following a well-defined pattern. I was sure that Marek supported it,
and I've implemented it in accordance with his recommendations.
Please forgive my persistence, but I consider this an important
feature. What we love about KeyCloak is its total extensibility. Having
custom entities and realm resources is great, but not being able to log
their events leaves a sensation of incompleteness.
Cheers,Dmitry
I don't like the PRs you've sent at all. It's all very
hacky, calling
static methods on other classes is far from elegant. I've rejected
both PRs. It also ends up being completely undocumented so no-one
except you would know about this.
On 17 October 2016 at 21:15, Dmitry Telegin <mitya(a)cargosoft.ru>
wrote:
> JIRA https://issues.jboss.org/browse/KEYCLOAK-3711
>
> PR https://github.com/keycloak/keycloak/pull/3337
>
> _______________________________________________
>
> keycloak-dev mailing list
>
> keycloak-dev(a)lists.jboss.org
>
> https://lists.jboss.org/mailman/listinfo/keycloak-dev
>
>
6:17 a.m.
On 18 October 2016 at 21:57, Dmitry Telegin <mitya(a)cargosoft.ru> wrote:
Hi Stian,
I completely agree on the first point, this deserves a better solution.
But knowing that it would require digging deep into the guts of KeyCloak, I
just didn't dare :)
I would see providers somehow registering their custom enum keys, either
via API call or via callback, and KeyCloak would maintain the list of
custom values per provider. ServerInfoAdminResource then could return a
combined list of enum keys, both built-in and defined by providers. But I'm
afraid that would require changes to API and/or interfaces. What do you
think? How would you approach this problem yourself?
We have SPIs to make features pluggable so that's the way to go. We'd
probably want a AdminResourceTypeSpi and at the same time refactor the
current enum to use this.
And BTW, what's wrong with the second PR? The github comment seems a bit
unclear, so could you please elaborate? It has completely different scope,
though deals with the same feature. It's not about exposing anything via
static methods, it's about making ResourceType extensible, following a
well-defined pattern. I was sure that Marek supported it, and I've
implemented it in accordance with his recommendations.
Seemed rather hacky to me, but maybe I'm mistaking something. Can you show
an example on how it would actually be used? I don't see how you'd add
additional types and how you'd actually use the resource type not knowing
what type it is. Seems all operations valid on an enum becomes invalid in
this case.
Please forgive my persistence, but I consider this an important feature.
What we love about KeyCloak is its total extensibility. Having custom
entities and realm resources is great, but not being able to log their
events leaves a sensation of incompleteness.
NP, I appreciate and welcome it. However, bear in mind that we can't
accommodate every wish. Adding loads of little things used by only one user
would bloat the code base. At the very least it has to be well designed,
generically usable and documented.
Cheers,
Dmitry
I don't like the PRs you've sent at all. It's all very hacky, calling
static methods on other classes is far from elegant. I've rejected both
PRs. It also ends up being completely undocumented so no-one except you
would know about this.
On 17 October 2016 at 21:15, Dmitry Telegin <mitya(a)cargosoft.ru> wrote:
JIRA https://issues.jboss.org/browse/KEYCLOAK-3711
PR https://github.com/keycloak/keycloak/pull/3337
_______________________________________________
keycloak-dev mailing list
keycloak-dev(a)lists.jboss.org
https://lists.jboss.org/mailman/listinfo/keycloak-dev
10:58 a.m.
Hi Stian,
On 18 October 2016 at 21:57, Dmitry Telegin
<mitya(a)cargosoft.ru>
wrote:
> Hi Stian,
>
> I completely agree on the first point, this deserves a better
> solution. But knowing that it would require digging deep into the
> guts of KeyCloak, I just didn't dare :)
>
> I would see providers somehow registering their custom enum keys,
> either via API call or via callback, and KeyCloak would maintain
> the list of custom values per provider. ServerInfoAdminResource
> then could return a combined list of enum keys, both built-in and
> defined by providers. But I'm afraid that would require changes to
> API and/or interfaces. What do you think? How would you approach
> this problem yourself?
We have SPIs to make features pluggable so that's the way to go. We'd
probably want a AdminResourceTypeSpi and at the same time refactor
the current enum to use this.
I was thinking about SPIs, but to me the ResourceType use case seemed
too minor to deserve its own dedicated SPI. OTOH, it would look quite
natural as a part of more generic Admin Resource SPI. I remember it's
been on the list for a long time, then why not finally getting hands
dirty?
Please take a look at the BeerCloak example I've announced earlier in
the list. There, I've tried to collect all the techniques I've been
using up to the moment to develop ad-hoc admin resources. I think I've
got enough experience and understanding to do the whole Admin Resource
SPI thing (with hypothetical AdminResourceTypeSpi becoming a part of
it, along with roles handling, authorization, event logging etc.)
That's it, I'd like to volunteer to implement Admin Resource SPI, and
to do it right way ("well designed, generically usable and
documented").
Please let me know what do you think. If you greenlight this, I think
we'd start a new thread to discuss the design.
> And BTW, what's wrong with the second PR? The github comment seems
> a bit unclear, so could you please elaborate? It has completely
> different scope, though deals with the same feature. It's not about
> exposing anything via static methods, it's about making
> ResourceType extensible, following a well-defined pattern. I was
> sure that Marek supported it, and I've implemented it in accordance
> with his recommendations.
Seemed rather hacky to me, but maybe I'm mistaking something. Can you
show an example on how it would actually be used? I don't see how
you'd add additional types and how you'd actually use the resource
type not knowing what type it is. Seems all operations valid on an
enum becomes invalid in this case.
See beercloak.BeerResource::create and other methods. There are
commented-out lines that show how it could look like. But I don't think
it's much relevant now, since we may come up with completely different
design with Admin Resource SPI.
Cheers,
Dmitry
10:46 a.m.
Introducing this as part of a admin resource spi would make perfect sense.
One issue with introducing admin resource spi now is that it would have to
be reworked once we rework how permissions are done. We will most likely go
away from a simple role based permissions to utilizing the authorization
services. Further, we simply don't have time to review or discuss a the
admin resource spi at the moment as we're focusing on wrapping up 2.x
series. At this point there is no chance that it would be included in 2.x
in any case.
If you'd like we can revisit this early next year, when we'd be able to
discuss it for 3.x.
On 1 November 2016 at 10:58, Dmitry Telegin <mitya(a)cargosoft.ru> wrote:
Hi Stian,
On 18 October 2016 at 21:57, Dmitry Telegin <mitya(a)cargosoft.ru> wrote:
Hi Stian,
I completely agree on the first point, this deserves a better solution.
But knowing that it would require digging deep into the guts of KeyCloak, I
just didn't dare :)
I would see providers somehow registering their custom enum keys, either
via API call or via callback, and KeyCloak would maintain the list of
custom values per provider. ServerInfoAdminResource then could return a
combined list of enum keys, both built-in and defined by providers. But I'm
afraid that would require changes to API and/or interfaces. What do you
think? How would you approach this problem yourself?
We have SPIs to make features pluggable so that's the way to go. We'd
probably want a AdminResourceTypeSpi and at the same time refactor the
current enum to use this.
I was thinking about SPIs, but to me the ResourceType use case seemed too
minor to deserve its own dedicated SPI. OTOH, it would look quite natural
as a part of more generic Admin Resource SPI. I remember it's been on the
list for a long time, then why not finally getting hands dirty?
Please take a look at the BeerCloak example I've announced earlier in the
list. There, I've tried to collect all the techniques I've been using up to
the moment to develop ad-hoc admin resources. I think I've got enough
experience and understanding to do the whole Admin Resource SPI thing (with
hypothetical AdminResourceTypeSpi becoming a part of it, along with roles
handling, authorization, event logging etc.) That's it, I'd like to
volunteer to implement Admin Resource SPI, and to do it right way ("well
designed, generically usable and documented").
Please let me know what do you think. If you greenlight this, I think we'd
start a new thread to discuss the design.
And BTW, what's wrong with the second PR? The github comment seems a bit
unclear, so could you please elaborate? It has completely different scope,
though deals with the same feature. It's not about exposing anything via
static methods, it's about making ResourceType extensible, following a
well-defined pattern. I was sure that Marek supported it, and I've
implemented it in accordance with his recommendations.
Seemed rather hacky to me, but maybe I'm mistaking something. Can you show
an example on how it would actually be used? I don't see how you'd add
additional types and how you'd actually use the resource type not knowing
what type it is. Seems all operations valid on an enum becomes invalid in
this case.
See beercloak.BeerResource::create and other methods. There are
commented-out lines that show how it could look like. But I don't think
it's much relevant now, since we may come up with completely different
design with Admin Resource SPI.
Cheers,
Dmitry
2731
days inactive
2751
days old
6 comments
2 participants
participants (2)
-
Dmitry Telegin -
Stian Thorgersen