User Profile: Adding information on update

[EXTERNAL Idel Martin (TNG Technology Consulting GmbH, IOC/PDL2)]

Hello everybody,

 

As discussed elsewhere we really look forward to be using the new user profile! There is one requirement however which we can’t cover yet:

We need to store a timestamp when a certain attribute is updated. This is for auditing/legal purposes.

 

We brought this up in a different conversation but I wanted to single this out to its own feature:

 

One idea in a different thread was to use the UPDATE_PROFILE event, however this doesn't work well enough for several reasons:

• This requires the events to be configured at the admin UI which is not ideal
• Events are processed asyncronously, so events may get lost on system failure which is not ideal for auditing purposes
• We'd need to have several listeners to also cover admin CLI changes
• If the timestamp is important for other event listeners, this cannot work because event listeners are not executed in a definable order

Instead, we'd like to propose a way to achieve this consistently with as little changes as possible by reactivating an idea that was discussed originally: Update `Processors`.

The API is already rather good: The method `updateInternal` in the `DefaultUserProfile` accepts a list of listeners. If we had an SPI which implemented the Listener interface (or made such a listener available) we could just inject it at this point. So the proposal is to:

• Create an SPI, maybe called `AttributeProcessor` which has a single method `createListener(Attributes attributes)` which returns a `BiConsumer<String, UserModel>` and the `DefaultUserProfileProvider` just collects the SPIs and injects them to the `DefaultUserProfile`. It needs the attribute metadata so that it could work e.g. annotation-based.
• It would be ideal to add a configuration option to the UserProfile editor which allows specification of Processor SPIs at attribute levels (similar to validators), but if you don't like that idea, just having the SPI and injecting all implementations to the UserProfile `updateInternal` would be enough (the SPI could then decide whether the attribute needs further processing e.g. based on some annotation)
• If you agree it is an interesting use-case we could also add a default processor for adding update-Timestamps to the Keycloak codebase similar to default validators. This could be done if we add some configuration option for processors OR if we use it annotation-based.

What do you think about this? I’d be able to contribute a PR for this the annotation-based approach to help the discussion and extend it to the configuration option if we go with that one!

 

Mit freundlichen Grüßen / Best regards

Martin Idel

Project Delivery Berlin 2 (IOC/PDL2)
Bosch.IO GmbH | Ziegelei 7 | 88090 Immenstaad | GERMANY | www.bosch.io
external.M...@bosch.io

Sitz: Berlin, Registergericht: Amtsgericht Charlottenburg; HRB 148411 B
Aufsichtsratsvorsitzender: Dr.-Ing. Thorsten Lücke; Geschäftsführung: Dr. Stefan Ferber, Dr. Aleksandar Mitrovic, Yvonne Reckling

​

[Vlastimil Elias]

Hi,

I see your points why events are not the best for your use case. I think we should still improve events to contain info about old and new values of changed attributes (ideally configurable on the per attribute basis to define which changes to track in the event) as a basic audit system. I can imagine we will use generic mechanism you need to implement this improvement also.

I'd let someone (Pedro?) from the core keycloak team to comment about the SPI addition.

BTW do you see BiConsumer<String, UserModel> as a listener to be good enough? I'd expect that listener gets both old and new values for changed attribute, here I'm even not sure what is in that UserModel (I expect new value is here for given attribute the listener is called for, but no access to the old value).

Have a nice day

Vlastimil

-- 
Vlastimil Elias
He / Him / His
Principal Software Engineer, DXP Application Development
Red Hat

[EXTERNAL Idel Martin (TNG Technology Consulting GmbH, IOC/PDL2)]

Hi Vlastimil,

 

Thanks for your input. I like having a generic mechanism to be configurable on a per-attribute basis. This could be done similar to the validators and if you like, I can adapt my draft https://github.com/keycloak/keycloak/pull/8246 accordingly. Maybe this helps to see how we could accomplish this feature!

 

I agree that it is probably even better if the interface gets the old and new value. For our purpose, the current listener would be good enough, since the listeners are only triggered if the value changed and at that point we’d need to set the timestamp (see the PR). The changes I suggested are minimal and I’m always fine with more functionality!

 

Mit freundlichen Grüßen / Best regards

Martin Idel

Project Delivery Berlin 2 (IOC/PDL2)
Bosch.IO GmbH | Ziegelei 7 | 88090 Immenstaad | GERMANY | www.bosch.io
external.M...@bosch.io

Sitz: Berlin, Registergericht: Amtsgericht Charlottenburg; HRB 148411 B
Aufsichtsratsvorsitzender: Dr.-Ing. Thorsten Lücke; Geschäftsführung: Dr. Stefan Ferber, Dr. Aleksandar Mitrovic, Yvonne Reckling

​

[Pedro Igor Craveiro e Silva]

Hi,

For me, looks like we can avoid having another SPI and leverage the existing SPI. One of the main characteristics is of the User Profile SPI is that the core behavior is provided by base classes so you should be able to easily extend it for this purpose.

For that, you should be able to create your own `UserProfileProvider` and extend the behavior using your own version of `UserProfile`.

Isn't it simpler without the burden of adding another SPI into the picture?

[Pedro Igor Craveiro e Silva]

It should also be easier to add support for old and current values by providing specific hooks prior to setting the attribute.

[Hynek Mlnarik]

+1

--
You received this message because you are subscribed to the Google Groups "Keycloak Dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to keycloak-dev...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/keycloak-dev/CA%2B3s2iQKQFWDg5kQ9Oveb89WDgdEcdrfDWtMPc%3DN3V10UJz9Tw%40mail.gmail.com.

[EXTERNAL Idel Martin (TNG Technology Consulting GmbH, IOC/PDL2)]

Hi,

 

Adding a user profile SPI with this feature requires subclassing the SPI classes but also the UserProfile itself, so the overhead is definitely higher than having a dedicated SPI similar to validators. However, I can understand that you want to limit the numbers of SPIs unless the use case is heavily requested.

 

I still think that it makes sense to support it because it will probably be a much required use-case to track user consent via attributes – and then it would be nice to store change timestamps alongside them to be able to get both the attribute value and when it changed in one API-call instead of having to leave through events. A similar idea, the “converter” of attributes is still present in the original UserProfile design document.

 

If we don’t add a new SPI, I’d like to discuss simplifying leveraging the existing SPI + classes. For that matter I updated https://github.com/keycloak/keycloak/pull/8246 to make a few changes to the DefaultUserProfile and the AbstractUserProfileProvider: The PR simply introduces the “global” listeners to the DefaultUserProfile and provides a constructor to add them which is unused. Subclassing the DeclarativeUserProfileProvider and overriding the createUserProfile method would then be sufficient to support the scenario I outlined in my opening post. Without this change, one has to also provide a custom UserProfile implementation which would be a 99% copy of the existing DefaultUserProfile (or partially delegates to the default user profile).

 

It might be even better to change the listener interface for the “global listeners” by also handing in the current value and updated value, but that would require the introduction of a new class, so I went with the minimal changes for now.

 

What do you think about this suggestion?

 

Mit freundlichen Grüßen / Best regards

Martin Idel

Project Delivery Berlin 2 (IOC/PDL2)
Bosch.IO GmbH | Ziegelei 7 | 88090 Immenstaad | GERMANY | www.bosch.io
external.M...@bosch.io

Sitz: Berlin, Registergericht: Amtsgericht Charlottenburg; HRB 148411 B
Aufsichtsratsvorsitzender: Dr.-Ing. Thorsten Lücke; Geschäftsführung: Dr. Stefan Ferber, Dr. Aleksandar Mitrovic, Yvonne Reckling

​

[Pedro Igor Craveiro e Silva]

Thanks for your comprehension.

Your changes LGTM, but I would rather go for subclassing both the provider and userprofile classes. Both were designed to make it easier to add custom behavior without redundant code. And potentially provide specific hooks for specific operations.

The reason is that it should give you much more flexibility by overriding the methods you need (e.g.: update) to add your custom listeners.

We can also mimic the SPI you are looking for by introducing a specific interface for dealing with events like update, create, and validation events (for both attributes the user profile itself). We would still leverage the existing SPI but avoid customizing the user profile.

If that makes sense for you, I can write something and we can evaluate it.

[Vlastimil Elias]

BTW we just found nice internal use case for this attribute change listener in UserProfile SPI. It is logic to reset emailVerified flag to false when email address is changed by user.

Currently the logic to reset it is spread around client code calling UserProfile SPI, I created PR [1] to centralize it inside of the UserProfile Provider (also adding integration tests to cover it). As Pedro noticed, it will be nice to move this logic into listener instead of hardcoding it into update method of the provider.

Cheers

Vl.

[1] https://github.com/keycloak/keycloak/pull/8271

[EXTERNAL Idel Martin (TNG Technology Consulting GmbH, IOC/PDL2)]

Hi Pedro,

 

I agree, if you really want to change the implementation of the user profile, the architecture has a good way to allow this.

 

The question is what to do when you basically want to keep the UserProfile as is but just tweak it at a few points. You mention an interface for dealing with events like update, create and validation events. I don’t think I completely understand, but this seems to be exactly what I’m aiming at, so I would be delighted to see more!

 

 

Just to make it a bit clearer where I’m coming from:

 

For instance, given the current implementation, to achieve the behavior I want, I need to

 

-       Subclass the DeclarativeUserProfileProvider and implement the four create methods

-       Add my own UserProfile.

 

There are two ways to achieve this:

-       Either subclass the DefaultUserProfile and reimplement the `update` method, so basically do something like this:

 

public class MyCustomUserProfile extends DefaultUserProfile {

 

        public List< BiConsumer<String, UserModel>> globalListeners;

        public MyCustomUserProfile(…) {

        }

 

    @Override

    public void update(boolean removeAttributes, BiConsumer<String, UserModel>... changeListener) {

        if (!validated) {

            validate();

        }

 

        updateInternal(user, removeAttributes, changeListener);

    }

 

    private UserModel updateInternal(UserModel user, boolean removeAttributes, BiConsumer<String, UserModel>... changeListener) {

        if (user == null) {

            throw new RuntimeException("No user model provided for persisting changes");

        }

 

        try {

            for (Map.Entry<String, List<String>> attribute : attributes.attributeSet()) {

                String name = attribute.getKey();

 

                if (attributes.isReadOnly(name)) {

                    continue;

                }

 

                List<String> currentValue = user.getAttributeStream(name).filter(Objects::nonNull).collect(Collectors.toList());

                List<String> updatedValue = attribute.getValue().stream().filter(Objects::nonNull).collect(Collectors.toList());

 

                if (currentValue.size() != updatedValue.size() || !currentValue.containsAll(updatedValue)) {

                    user.setAttribute(name, updatedValue);

                    for (BiConsumer<String, UserModel> listener : changeListener) {

                        listener.accept(name, user);

                    }

                    for (BiConsumer<String, UserModel> myGlobalListener : globalListeners {

                        listener.accept(name, user)

                    }

                }

            }

 

            // this is a workaround for supporting contexts where the decision to whether attributes should be removed depends on

            // specific aspect. For instance, old account should never remove attributes, the admin rest api should only remove if

            // the attribute map was sent.

            if (removeAttributes) {

                Set<String> attrsToRemove = new HashSet<>(user.getAttributes().keySet());

 

                attrsToRemove.removeAll(attributes.nameSet());

 

                for (String attr : attrsToRemove) {

                    if (this.attributes.isReadOnly(attr)) {

                        continue;

                    }

                    user.removeAttribute(attr);

                }

            }

        } catch (ModelException me) {

            // some client code relies on this exception to react to exceptions from the storage

            throw me;

        } catch (Exception cause) {

            throw new RuntimeException("Unexpected error when persisting user profile", cause);

        }

 

        return user;

    }

}

 

Which is okay, although none of this is actually my code – just necessary “boilerplate”, which can subtly break on update

-       Or, I use the DefaultUserProfile as a delegate, which means I have to implement the whole interface, which is a bit of boilerplate.

 

Both implementations I fear could easily break on updates. Adding a bit of usage convenience in the DefaultUserProfile would mean I only have to subclass the UserProvider, which eliminates this kind of errors without – I think – adding real overhead for Keycloak and the dev-team.

However, I do see that this is a tradeoff: Having a simple way (as is) to completely override the behavior makes it possible to implement every use-case. Adding convenience for some use-cases means that the most important ones may be missed which is why I’m only focusing on the specific use-case of doing something on attribute update which we already have several users who need this scenario, so I believe it will be a case that’ll come up again!

 

Thanks for the quick replies!

 

Mit freundlichen Grüßen / Best regards

Martin Idel

Project Delivery Berlin 2 (IOC/PDL2)
Bosch.IO GmbH | Ziegelei 7 | 88090 Immenstaad | GERMANY | www.bosch.io
external.M...@bosch.io

Sitz: Berlin, Registergericht: Amtsgericht Charlottenburg; HRB 148411 B
Aufsichtsratsvorsitzender: Dr.-Ing. Thorsten Lücke; Geschäftsführung: Dr. Stefan Ferber, Dr. Aleksandar Mitrovic, Yvonne Reckling

​

[Pedro Igor Craveiro e Silva]

Yeah, I think the events might help too.

But based on what I previously said, your custom code would be:

@Override
public void update(boolean removeAttributes, BiConsumer<String, UserModel>... listeners) {

    List<BiConsumer<String, UserModel>> allListeners = new ArrayList();

    if (listeners != null && listeners.length > 0) {
        allListeners.addAll(Arrays.asList(listeners));
    }

    // add your own listeners here
    super.update(removeAttributes, allListeners.toArray(new BiConsumer[allListeners.size()]));
}

This is how we initially thought on how to extend the provider for custom behavior when updating/validating/creating. But there is some room for improvement (like providing a well-defined interface specifically for this or perhaps leveraging existing support for events within the factory).

If the code above works for you, I would just use it for now though. If your listeners can be defined in a constant the code should be even simpler.

[Stian Thorgersen]

I haven't had time to read through this whole thread, but my 2 cents around this is that it's not something the user profile should be handling. Rather, this is something that is in the scope of a "user event listener" capability that has been discussed a few times.

To view this discussion on the web visit https://groups.google.com/d/msgid/keycloak-dev/CA%2B3s2iR7e2tCZmYb9PrMa0JUPvEUiUeXMfEdx%2B8ES35xWRvo4g%40mail.gmail.com.

[EXTERNAL Idel Martin (TNG Technology Consulting GmbH, IOC/PDL2)]

Hi Stian, Hi Pedro,

 

Thanks for your input. We’ll see how far we get with this and maybe reopen the discussion sometime farther in the future.

 

Mit freundlichen Grüßen / Best regards

Martin Idel

Project Delivery Berlin 2 (IOC/PDL2)
Bosch.IO GmbH | Ziegelei 7 | 88090 Immenstaad | GERMANY | www.bosch.io
external.M...@bosch.io

Sitz: Berlin, Registergericht: Amtsgericht Charlottenburg; HRB 148411 B
Aufsichtsratsvorsitzender: Dr.-Ing. Thorsten Lücke; Geschäftsführung: Dr. Stefan Ferber, Dr. Aleksandar Mitrovic, Yvonne Reckling

​

[Vlastimil Elias]

Hi,

related to this thread, I created PR https://github.com/keycloak/keycloak/pull/8389 which improves few things from this area:

• UserProfile.update() listener is no more BiConsumer, but new regular AttributeChangeListener which also gets oldValue of the attribute in the callback, so it is much easier and clean to write listeners. It is used for next improvements.
  
• new annotation auditChangeIntoEvent can be used for attribute in UserProfile configuration. If set then attribute change is audited into UPDATE_PROFILE event details (email, first and last name change is always audited as before without need to configure it)
  
• UPDATE_PROFILE event details, auditing attributes change, are now consistent for all places the event is fired from (old and new account console, profile update from required actions in login flow). Previously events was inconsistent, mainly new account console didn't wrote any info about attribute changes.
• UPDATE_PROFILE event also contains detail called "context" identifying where the change was performed (it contains name of the UserProfileContext used for given user profile update)
• tests for UPDATE_PROFILE event are improved to better cover and verify current functionality (all places this event is fired from)
  

I believe this PR implements basic configurable user profile attribute change auditing mechanism into Keycloak, which may be useful/enough for many cases. It also improves UserProfile SPI for easier extensibility where more sophisticated auditing mechanism is necessary.

Any comments or proposals are welcome

Vlastimil

On 07. 07. 21 15:35, EXTERNAL Idel Martin (TNG Technology Consulting GmbH, IOC/PDL2) wrote:

-- 
Vlastimil Elias
He / Him / His
Principal Software Engineer, DXP Application Development
Red Hat

[Matysiak Joerg (IOC/PAU1)]

Hi Vlastimil,

 

thanks for these changes, they helped a lot. Based on this I was able to implement a custom user profile provider which handles our use case by adding an additional AttributeChangeListener to the user profile.

 

Only thing was a little bit ugly is, that I had to create an own implementation of the DeclarativeUserProfileModel and override the method “configureUserProfile()” in the custom profile provider.
Since this copy-pasted code is likely to break in upcoming versions, I created a tiny PR to overcome this.

 

Please have a look here: https://github.com/keycloak/keycloak/pull/8513

 

With this PR merged it becomes quite lean to implement a custom user provider:

 

Just derive from DeclarativeUserProfileProvider and override the constructors, getId() and the create() methods.

 

 

Mit freundlichen Grüßen / Best regards

Jörg Matysiak
IOC/PAU1
​

--

You received this message because you are subscribed to the Google Groups "Keycloak Dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to keycloak-dev...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/keycloak-dev/948a1a94-da16-84d9-c585-5996b23e4e3c%40redhat.com.