KEYCLOAK-17622 Terms and Conditions - periodically ask for reacceptance by laskasn · Pull Request #7895 · keycloak/keycloak

laskasn · 2021-03-30T12:03:17Z

Resolves issue:
https://issues.redhat.com/browse/KEYCLOAK-17622

stianst

Firstly, I'm sorry for the long time to respond to this PR.

There's a few issues I see with the approach taken in this PR:

T&C are periodically invalidated, which means a user could have to approve terms and conditions again a few minutes after they've already done so.
Background/timer task to periodically expire T&C is fairly expensive and not needed really. It would be better to simple check during authentication time if a given users acceptance has expired or not.
The approach to providing configuration for the T&C is not ideal, and should rather be configuration on the required action itself, instead at the realm level. This makes it quite a bit more tricky to implement this change as a first step would be to support configuration options for required actions themselves.

Rather than having a time that invalidates terms and conditions periodically for all users I'd rather like to see an max-age on terms and conditions approval, and then check that when users authenticate.

laskasn · 2022-08-26T09:24:26Z

Firstly, I'm sorry for the long time to respond to this PR.

There's a few issues I see with the approach taken in this PR:

T&C are periodically invalidated, which means a user could have to approve terms and conditions again a few minutes after they've already done so.

Background/timer task to periodically expire T&C is fairly expensive and not needed really. It would be better to simple check during authentication time if a given users acceptance has expired or not.

The approach to providing configuration for the T&C is not ideal, and should rather be configuration on the required action itself, instead at the realm level. This makes it quite a bit more tricky to implement this change as a first step would be to support configuration options for required actions themselves.

Rather than having a time that invalidates terms and conditions periodically for all users I'd rather like to see an max-age on terms and conditions approval, and then check that when users authenticate.

First of all, thanks @stianst for evaluating this PR.

Let me answer on your points, one by one (i am struggling a bit to remember what i have done many months before :P )

Regarding the first one, the T&C are not just periodically invalidated. What really happens is that they are periodically checked and if an expiry has occured, then they get invalidated :)

The last time (timestamp) user has accepted the task is stored in the user attributes (by the requiredActionModel.getProviderId() field name). The task checks for all users
if(epochSecondsNow > expiryOffset + userLastAcceptTime)
and if true, it resets the relevant required action.
If the user accepts at some time the required action, then the userLastAcceptTime will be updated and the (epochSecondsNow > expiryOffset + userLastAcceptTime) of the next task will be false till the expiryOffset has elapsed. (So no invalidation will occur)
About the second point, the background task runs every 900 sec (15 mins) which i think is quite "sparce" to add a large processing burden on a running instance (moreover, it's asynchronous).
Of cource, this depends on the number of users in a realm, and the user list might grow really large.
What you propose is a nice idea though, and i will try to add the check during the authentication (hopefully, it will not add any delay to the authentication hotcode path), instead of having a periodic task.
About the third point, why not have both? We could have a default-global, and an override (if an override is specified for the user, it is evaluated instead of the realm's default).

@stianst Please, check also the PR #7841 which is simpler - whenever an admin changes the T&C, he/she might wish to require all users to reaccept the new one!!!

stianst · 2022-08-26T11:29:03Z

With regard to performance this is going to be pretty horrible. Let's say you have 1000+ realms you're fetching and looping over all those realms every 15 minutes. Now let's say you have 100+ million users in one realm, you're again fetching and looping over all those users every 15 minutes.

stianst · 2022-08-26T11:31:26Z

With regard to how it is configured it really isn't suitable to hard-code the config for a specific action like you're doing in the admin console. Nor is it really ideal to use realm config for this purpose. This results in code that is hard for us to maintain, so I'm afraid we really can't accept this approach.

laskasn · 2022-08-26T12:00:06Z

With regard to how it is configured it really isn't suitable to hard-code the config for a specific action like you're doing in the admin console. Nor is it really ideal to use realm config for this purpose. This results in code that is hard for us to maintain, so I'm afraid we really can't accept this approach.

How would you envisage the configuration of the "max-age on terms and conditions approval" you mention? We could change the way the whole thing is defined.

laskasn · 2022-08-26T12:03:07Z

stianst · 2022-08-26T12:13:24Z

With regard to how it is configured it really isn't suitable to hard-code the config for a specific action like you're doing in the admin console. Nor is it really ideal to use realm config for this purpose. This results in code that is hard for us to maintain, so I'm afraid we really can't accept this approach.

How would you envisage the configuration of the "max-age on terms and conditions approval" you mention? We could change the way the whole thing is defined.

It's a non-trivial thing I'm afraid as it would require updating required actions to be configurable like authenticators are. @mposolda wdyt?

mposolda · 2022-08-29T13:05:55Z

With regard to how it is configured it really isn't suitable to hard-code the config for a specific action like you're doing in the admin console. Nor is it really ideal to use realm config for this purpose. This results in code that is hard for us to maintain, so I'm afraid we really can't accept this approach.

How would you envisage the configuration of the "max-age on terms and conditions approval" you mention? We could change the way the whole thing is defined.

It's a non-trivial thing I'm afraid as it would require updating required actions to be configurable like authenticators are. @mposolda wdyt?

Yes, we don't have configuration at the level of required actions, we have just the configuration at the level of authenticators in the authentication flow.

And agree that adding configuration for the required actions would be non-trivial thing. I am not sure if we should rather do it as part of the "Authentication policies" effort instead of introducing "required action configuration" now?

stianst · 2022-08-29T14:58:03Z

@mposolda with auth policies in mind, perhaps we would just need to allow this PR to configure through realm attributes for now, with an plan to refactor it down the line?

mposolda · 2022-08-30T12:40:45Z

@mposolda with auth policies in mind, perhaps we would just need to allow this PR to configure through realm attributes for now, with an plan to refactor it down the line?

@stianst +1

stianst · 2022-09-02T08:50:25Z

@laskasn are you still interested in working on this issue? For it to reach a mergeable state we'd need:

To resolve performance issues move te check of terms and conditions expirations to authentication time, instead of using a background
Revert changes to the old admin console and send a PR to https://github.com/keycloak/keycloak-ui, we may want to reconsider how it is configured in the admin-ui. Perhaps it should be a separate section that is visible when the t&c required action is enabled. @ssilvert wdyt?
We'd probably want to update docs as well to mention this capability

laskasn · 2022-09-02T10:13:22Z

@stianst
YES ! i have completed the reform!

Now it's done during the authentication of the user (i admit your idea was very good)
Reformed/Created 2 tests το cover the basic "execution" paths.
Rebased on main branch.

I am updating the PR!

ssilvert · 2022-09-06T19:44:12Z

@laskasn are you still interested in working on this issue? For it to reach a mergeable state we'd need:

Revert changes to the old admin console and send a PR to https://github.com/keycloak/keycloak-ui, we may want to reconsider how it is configured in the admin-ui. Perhaps it should be a separate section that is visible when the t&c required action is enabled. @ssilvert wdyt?

We'd probably want to update docs as well to mention this capability

@stianst @mposolda
Maybe I missed something but it does seem to me that this belongs in authentication policies. Is there some reason we can't put the UI for it in auth policies now so we don't have to refactor later?

@laskasn Once this is settled and an API is place, open an issue at https://github.com/keycloak/keycloak-ui/issues. The UI team will work with you to get this done quickly.

laskasn · 2022-10-07T10:00:25Z

@laskasn are you still interested in working on this issue? For it to reach a mergeable state we'd need:

Revert changes to the old admin console and send a PR to https://github.com/keycloak/keycloak-ui, we may want to reconsider how it is configured in the admin-ui. Perhaps it should be a separate section that is visible when the t&c required action is enabled. @ssilvert wdyt?

We'd probably want to update docs as well to mention this capability

@stianst @mposolda Maybe I missed something but it does seem to me that this belongs in authentication policies. Is there some reason we can't put the UI for it in auth policies now so we don't have to refactor later?

@laskasn Once this is settled and an API is place, open an issue at https://github.com/keycloak/keycloak-ui/issues. The UI team will work with you to get this done quickly.

OK, i removed the UI part, and i am posting an issue on the keycloak-ui team to see how this is going to be implemented in the new UI.

Please, note that it looked like this on the old admin console! It was getting enabled only if the "terms and conditions" flag (leftmost one) was "true".

laskasn · 2022-10-07T10:45:53Z

I am thining about the removal you asked @ssilvert of the UI part on the old admin console. Why do you prefer to remove it and not let it there (since it's a deprecated/fallback UI) and just add also the required stuff on the new admin console?
I believe the addition on the old admin console would allow people using the old console to have access to this feature, plus the js and the html would help the guys from the new UI admin console to understand what we are trying to do. What do you think @stianst @mposolda ?

xianli123 · 2023-08-07T07:31:46Z

I looked through all the comments. I'm curious if we decided to add editing capabilities to T&C. In addition, should the version control be added? Once we have the final decision, I can come up with some UX solutions for the requirements.

mposolda · 2023-08-07T16:05:20Z

So if I understand correctly, there are really 2 separate use-cases:

Ask users for re-acceptance always when terms&conditions are changed
Ask users for re-acceptance every 12 months (even if nothing is changed in terms & conditions).

@cgeorgilakis Could you confirm that (1) is not sufficient for you and you also need to cover use-case (2) ? For instance, if you can live with (1), it means that administrator will need to update the terms & conditions required action every 12 months to enforce that users will re-approve 10000 them during their next authentication. This is not entirely same like periodic re-acceptance (as also users who accepted yesterday will be asked again today for re-acceptance due version change), but maybe it is sufficient compromise?

mposolda · 2023-08-07T16:17:11Z

One generic question: Are the use-cases for "periodic re-acceptance" or "versioning" specific only to Terms & Conditions? Or do we want them to be applied also for other required actions?

I can imagine periodic updates for more actions (Like Make sure user is required to update his profile every 12 months or Make sure user is required to update his TOTP every 12 months etc), however things like "versioning" with the use-case like (Make sure all users are required to update their profile now) likely does not makes sense to every action? Just trying to figure if we should introduce "configuration options" in the RequiredActionProvider and make sure that admin console UI will render just the configuration options, which are applicable for the particular required-action implementation?

ssilvert · 2023-08-07T18:09:35Z

One generic question: Are the use-cases for "periodic re-acceptance" or "versioning" specific only to Terms & Conditions? Or do we want them to be applied also for other required actions?

with the use-case like (Make sure all users are required to update their profile now) likely does not makes sense to every action?

Apply required action to all users immediately could make sense for most required actions. It would be a simple change to the UI in the Authentication section.

xianli123 · 2023-08-08T07:22:42Z

@mposolda @ssilvert Thanks. If I understand correctly, we need to allow administrators to configure T&C and periodically make the general users accept new or old T&C at the UI level. Is that right?

mposolda · 2023-08-08T08:58:49Z

I guess it makes sense to plan the UI if we agree on what we want to achieve? I am still not 100% sure if it is clear TBH. Few use-cases come to me, which we might agree on:

Use-case Ask all users for re-acceptance terms & conditions during their next login (aka versioning). Do we want to support this? AFAIK answer is yes
Use-case Apply required action XY to all users during their next login. More generic version of above, which is not specific only to "terms & conditions", but pretty much to all required actions. @ssilvert mentioned it can makes sense for most required actions. I agree with that, however I think that required actions makes sense only to some "applicable users", not to all users of the realm. For instance "Ask all users to update their TOTP" - this would makes sense only to users with TOTP, likely not to all users in the realm? Similarly "Verify email" applicable only to users with the email, which may not be all realm users etc.
Use-case Ask all users to re-accept terms & conditions every 12 hours . Do we want to support this? IMO yes as "versioning" (or "Asking all users to re-acceptance") is not exactly same use-case as periodic re-accept
Periodic re-acceptance for all required actions (not specific only to terms & conditions). IMO makes sense to have this capability for all required actions, but again, usually just for "Applicable" users. So for instance: Ask users to update their TOTP every 6 months, but just those users with TOTP already set. For Update Password, we already have password policy Expire Password, which effectively does this, so no need to have this also through required action IMO.

I guess that for Milestone 1, we can do this only for Terms & Conditions and then at next stage extend to other required actions? Maybe in tab Authentication -> Required actions, we can have "Config" action to allow configure particular required actions? Similarly like we have in the Authentication -> Flows for individual authentication executions?

ssilvert · 2023-08-08T10:57:43Z

I guess it makes sense to plan the UI if we agree on what we want to achieve? I am still not 100% sure if it is clear TBH. Few use-cases come to me, which we might agree on:

Use-case Ask all users for re-acceptance terms & conditions during their next login (aka versioning). Do we want to support this? AFAIK answer is yes

Use-case Apply required action XY to all users during their next login. More generic version of above, which is not specific only to "terms & conditions", but pretty much to all required actions. @ssilvert mentioned it can makes sense for most required actions. I agree with that, however I think that required actions makes sense only to some "applicable users", not to all users of the realm. For instance "Ask all users to update their TOTP" - this would makes sense only to users with TOTP, likely not to all users in the realm? Similarly "Verify email" applicable only to users with the email, which may not be all realm users etc.

Use-case Ask all users to re-accept terms & conditions every 12 hours . Do we want to support this? IMO yes as "versioning" (or "Asking all users to re-acceptance") is not exactly same use-case as periodic re-accept

Periodic re-acceptance for all required actions (not specific only to terms & conditions). IMO makes sense to have this capability for all required actions, but again, usually just for "Applicable" users. So for instance: Ask users to update their TOTP every 6 months, but just those users with TOTP already set. For Update Password, we already have password policy Expire Password, which effectively does this, so no need to have this also through required action IMO.

I guess that for Milestone 1, we can do this only for Terms & Conditions and then at next stage extend to other required actions? Maybe in tab Authentication -> Required actions, we can have "Config" action to allow configure particular required actions? Similarly like we have in the Authentication -> Flows for individual authentication executions?

I think this is very good @mposolda. I agree with all of it.

The only part that might be a little tricky is figuring out who an "applicable user" is. I guess you would need some sort of callback where you pass the user and ask the required action to decide if the user is "applicable". It's doable though.

pedroigor · 2023-08-08T12:16:13Z

@mposolda Sounds like a good plan.

However, I agree with @cgeorgilakis that we should let administrators indicate whether a required action should run for users. I'm wondering how the "version property" in a resource bundle/theme would work considering that themes are also deployed as JARs.

Looking at the T&C required action, a timestamp is already set for the user. What we need now is to allow administrators to set a global timestamp to compare and check if the action should run again for a particular user (similar to not-before when expiring sessions at the realm level, I think). For now, we could store the global config as a realm attribute ... Or just think about enabling a component model to required actions.

xianli123 · 2023-08-09T06:57:40Z

@mposolda @ssilvert @pedroigor I will come up with some designs based on the discussions and decisions. Once I have some deliverables, I will share them with you.

xianli123 · 2023-08-10T08:37:05Z

Hi @mposolda @pedroigor , if the admin can configure the T&C, what attributes should be included in the T&C configurations? Does it just include the Reset interval including some number input fileds and time units, like the image shows below?

pedroigor · 2023-08-10T20:37:34Z

For now, and based on the discussions, I would say the Reset interval (not sure if the name is the best, perhaps Expires in or something) is enough for now and allows defining the validity period for the T&C acceptance.

However, in case you want to force users to accept the T&C when it changes, we need something else. Something along the lines suggested by Stian like a version. As I'm not sure how to deal with a version property, we could have an option to set the "not-before" timestamp. This timestamp will basically invalidate any previous T&C accepted by the user before it and force the user to re-accept the T&C. You don't necessarily need to provide a date/time but just a button that sets this "not-before" timestamp based on the current time. We probably need to show this information to administrators too so that they know the last timestamp set.

Makes sense?

I see you are also working with the old administration console ... Ideally, you should consider developing on top of the new admin console. I think they have specific components there that can be re-used from authentication flows to configure required actions.

xianli123 · 2023-08-11T03:05:01Z

@pedroigor Thanks.

What's the deadline for your plan? @laskasn @mposolda @pedroigor @stianst

pedroigor · 2023-08-11T14:25:20Z

@xianli123 Not sure. As this is touching the UI (in addition to storing new data to realm/actions) we probably need more discussions to make sure we agree about the solution.

I see a lot of benefit on this proposal so it would be nice to have it done before 23.

ssilvert · 2023-08-11T19:36:51Z

Hi @mposolda @pedroigor , if the admin can configure the T&C, what attributes should be included in the T&C configurations? Does it just include the Reset interval including some number input fileds and time units, like the image shows below?

@xianli123 I believe this needs a way to select that you want no reset interval. Maybe some sort of enable/disable.

xianli123 · 2023-08-14T07:58:38Z

As this is touching the UI (in addition to storing new data to realm/actions) we probably need more discussions to make sure we agree about the solution.

Thanks @pedroigor Who can help us review the solution and make the final decisions? Is it necessary to have a meeting to discuss the solution?

xianli123 · 2023-08-14T08:00:18Z

I believe this needs a way to select that you want no reset interval. Maybe some sort of enable/disable.

@ssilvert Yes. I agree. That's also a use case.

xianli123 · 2023-08-16T04:43:32Z

Can users upload or create a term and condition in the UI? The Keycloak documentation mentioned that Edit the terms.ftl file in the base login theme. So I wonder if it's possible to allow the admin to edit the content of the terms and conditions in the UI when the admin can configure the Terms and conditions. @pedroigor @mposolda

ssilvert · 2023-08-17T12:51:34Z

@xianli123 Yes, it is possible to upload the text. If you are using our version of ftl files like terms.ftl and register-commons.ftl, you can override the text in those files if you re-define these messages:

termsTitle=Terms and Conditions
termsText=<p>Terms and conditions to be defined</p>
termsPlainText=Terms and conditions to be defined.
termsAcceptanceRequired=You must agree to our terms and conditions.
acceptTerms=I agree to the terms and conditions

So, an administrator can go to Realm settings --> Localization and define these from the UI.

BTW, I'm not sure where all those messages are used.

xianli123 · 2023-08-21T09:40:24Z

@ssilvert I think the message will display on the login page if the admin enables the "Terms and conditions".

ssilvert · 2023-08-21T12:59:50Z

@ssilvert I think the message will display on the login page if the admin enables the "Terms and conditions".

Yeah. Some of those messages are easy to track down. Others take a little more digging to find out what they are for.

cgeorgilakis · 2023-08-22T07:23:51Z

So if I understand correctly, there are really 2 separate use-cases:

Ask users for re-acceptance always when terms&conditions are changed

Ask users for re-acceptance every 12 months (even if nothing is changed in terms & conditions).

@cgeorgilakis Could you confirm that (1) is not sufficient for you and you also need to cover use-case (2) ? For instance, if you can live with (1), it means that administrator will need to update the terms & conditions required action every 12 months to enforce that users will re-approve them during their next authentication. This is not entirely same like periodic re-acceptance (as also users who accepted yesterday will be asked again today for re-acceptance due version change), but maybe it is sufficient compromise?

@mposolda yes we want both uses cases. This issue is for use case 1 and this for use case 2. What you describe after can not work in a real environment. Although now we want these only for T&C, I agree that it would be nice to have them for all Required Actions.

@xianli123 the previous icon was for reset button in old admin console. @ssilvert has proposed for new admin console:

I suspect we would add a kabab menu to trigger a popup for required actions that have config settings.

Finally, T&C can be changed with custom theme ( changing terms.ftl). We use this.

xianli123 · 2023-08-23T07:51:30Z

@cgeorgilakis What is the aim of the Reset icon, to refresh the T&C or reset the intervals？

cgeorgilakis · 2023-08-23T10:18:40Z

@cgeorgilakis What is the aim of the Reset icon, to refresh the T&C or reset the intervals？

Refresh T&C. This is from our PR. We could decide the appropriate name.

ssilvert · 2023-08-23T18:10:37Z

@cgeorgilakis What is the aim of the Reset icon, to refresh the T&C or reset the intervals？

Refresh T&C. This is from our PR. We could decide the appropriate name.

So does it mean, "Make all users perform the action on next login."?

xianli123 · 2023-08-24T07:17:47Z

@cgeorgilakis Ok. I created a conceptual design as shown in the screen below. I put the configurations on this separate page. Users can configure the T&C in this page. Does it make sense?

cgeorgilakis · 2023-08-24T09:37:42Z

@cgeorgilakis Ok. I created a conceptual design as shown in the screen below. I put the configurations on this separate page. Users can configure the T&C in this page. Does it make sense?

Yes. It looks good.

jonkoops · 2024-01-10T12:43:42Z

I am closing this PR as it has already been out there for some time and seems to be stuck in eternal review. We have this tracked under #19275, so we can continue further discussion there.

laskasn mentioned this pull request Mar 30, 2021

Support AUP periodic renewal eosc-kc/keycloak#49

Open

stianst requested changes Aug 24, 2022

View reviewed changes

stianst self-assigned this Aug 24, 2022

laskasn force-pushed the 49_only branch from 31d0868 to 6ead6fb Compare September 2, 2022 10:18

laskasn requested a review from stianst September 2, 2022 10:19

laskasn marked this pull request as draft September 2, 2022 10:24

laskasn force-pushed the 49_only branch from 6ead6fb to ab9521a Compare September 2, 2022 11:14

laskasn marked this pull request as ready for review September 2, 2022 11:14

laskasn force-pushed the 49_only branch from ab9521a to 48dfe3f Compare September 2, 2022 11:34

laskasn marked this pull request as draft September 2, 2022 14:17

laskasn force-pushed the 49_only branch from 48dfe3f to 7c732f6 Compare September 2, 2022 14:48

laskasn marked this pull request as ready for review September 2, 2022 14:49

laskasn force-pushed the 49_only branch 2 times, most recently from af97872 to e15a412 Compare October 7, 2022 09:49

laskasn mentioned this pull request Mar 15, 2023

Add the reset TaC user action field in the new Admin Console #17739

Closed

laskasn force-pushed the 49_only branch from e15a412 to 8a18e1e Compare October 7, 2022 12:15

jonkoops closed this Jan 10, 2024

xianli123 mentioned this pull request Jan 12, 2024

Allow the admin to request existing users to reaccept the new Terms & Conditions #26124

Open

jonkoops mentioned this pull request Jan 12, 2024

Terms and conditions - ask periodically to reaccept #19275

Open

Conversation

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants