KEYCLOAK-18276 consent screen enhancement by cgeorgilakis · Pull Request #8092 · keycloak/keycloak

cgeorgilakis · 2021-05-26T06:43:57Z

This PR is needed in order to being able to configure client attributes. PRs code are independent.

cgeorgilakis · 2021-10-14T09:04:34Z

Keycloak documentation PR : https://github.com/keycloak/keycloak-documentation/pull/1270/files
Merged with related PR into this PR.
You could see previous discussion in the merged PR.

Our goal was to do no database changes. However, for logo_uri some SAML clients use data:image and the logo_uri length is over 4000 characters. Current database length for VALUE field of CLIENT_ATTRIBUTES is insufficient.
We believe that we should create a new field in ClientEntity for logo_uri with text format or make VALUE field clob. What do you believe?

pedroigor · 2021-10-20T13:17:22Z

@cgeorgilakis I don't think we can change the column to a CLOB. I remember @hmlnarik being against that a long time ago.

A separate field sounds too much for me.

Aren't these URLs too big? Any chance you can try to have them shorter and fit the size?

pedroigor

@cgeorgilakis In general it is LGTM, mainly the OIDC related changes. We also need some feedback from @ssilvert (new console changes) and @hmlnarik (SAML).

One thing I'm wondering is whether it makes sense to add this for SAML too? Or at least include now the OIDC related changes (which are backed by specs) and deal with SAML in a subsequent PR, if necessary?

Another thing, is there anything else we can do to avoid XSS/CSRF when rendering those URLs? For instance, bad admins should be able to exploit sites vulnerable to CSRF (e.g.: in images). Not sure if we need to worry about that or if there is anything else we can do help to mitigate this.

pedroigor · 2021-10-20T13:22:13Z

Does make sense to enforce realm settings for external requests using

keycloak/common/src/main/java/org/keycloak/common/util/UriUtils.java

Line 92 in 1b8369c

public static void checkUrl(SslRequired sslRequired, String url, String name) throws IllegalArgumentException{

?

I did not know this method.
However, some SAML clients have value starting with data:image/png ( accepted according to SAML specifications).
It is the same value that have length over 4000 characters.

pedroigor · 2021-10-20T13:28:15Z

@mposolda I think this one is a nice improvement to consent. Are you able to check if anything else is missing?

cgeorgilakis · 2021-10-21T11:53:01Z

@cgeorgilakis I don't think we can change the column to a CLOB. I remember @hmlnarik being against that a long time ago.

A separate field sounds too much for me.

Aren't these URLs too big? Any chance you can try to have them shorter and fit the size?

I agree with @hmlnarik, because Keycloak use CLIENT_ATTRIBUTES for many client attributes.

These is configuration in some existing SAML client( in edugain f.e.) . I can not change them.

Examples url can be found in edugain metadata. md:SPSSODescriptor is for SAML clients. For example see SP with entityId = https://ssotest.sifulan.my/shibboleth.

If this is a blocking issue for accepting PR for you, could you accept it without any db change and propose a solution for our case ( maybe other Keycloak users)?

cgeorgilakis · 2021-10-21T12:45:36Z

@cgeorgilakis In general it is LGTM, mainly the OIDC related changes. We also need some feedback from @ssilvert (new console changes) and @hmlnarik (SAML).

One thing I'm wondering is whether it makes sense to add this for SAML too? Or at least include now the OIDC related changes (which are backed by specs) and deal with SAML in a subsequent PR, if necessary?

Our main use case and goal is OIDC clients. However, SAML clients exist also and changes is strain forward to be added. The only problem is related to SAML logoUri ( see my other comments).

Another thing, is there anything else we can do to avoid XSS/CSRF when rendering those URLs? For instance, bad admins should be able to exploit sites vulnerable to CSRF (e.g.: in images). Not sure if we need to worry about that or if there is anything else we can do help to mitigate this.

I do not know about it. How do you deal with it in other client URIs? I use same check method except from logoUri.

cgeorgilakis · 2021-10-21T12:51:47Z

About missing/tests, my problem is that the changes is in consent (ui). How do you test other consent components?

pedroigor · 2021-10-21T13:06:29Z

@cgeorgilakis W.r.t. SAML, not so straightforward if compared to the OIDC code you had to change :) Plus the tests we also need to add to SAML.

Another point I have on this one is that consent is probably more related to OIDC? I mean, that is what our community is most interested about?

W.r.t. to tests, here is an example

keycloak/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/admin/ConsentsTest.java

Line 73 in a7fd1bc

public class ConsentsTest extends AbstractKeycloakTest {

.

cgeorgilakis · 2021-10-22T11:23:05Z

@cgeorgilakis W.r.t. SAML, not so straightforward if compared to the OIDC code you had to change :) Plus the tests we also need to add to SAML.

Another point I have on this one is that consent is probably more related to OIDC? I mean, that is what our community is most interested about?

Sure. If you ensist, I can seperate two PRs. Keep this one for OIDC and make another for SAML after this has been approved.

I have also added tests for OIDC. Thanks.

pedroigor

@cgeorgilakis very minor comments. Other than that, LGTM.

I do think it makes sense to initially add support for OIDC. But I'm not sure what others think.

@hmlnarik @stianst @mposolda

pedroigor · 2021-11-03T13:44:56Z

Does it make sense to use the "config wrapper" to fetch those new properties rather than using their names directly (considering you have created constants for them)?

pedroigor · 2021-11-03T13:45:56Z

Does it make more sense to use constants from the "config wrapper"?

I have created constants variables to ClientScopeModel. If we move to only OIDC PR, we could move them to OIDCConfigAttributes. I prefer to keep them in ClientScopeModel for SAML future extension.

mposolda

@cgeorgilakis Thanks for this PR! And sorry for late review. I've added one minor comment, but in general, it is great work.

mposolda · 2021-11-16T16:03:21Z

I am not sure why these constants are defined on the ClientScopeModel? Existing constants on ClientScopeModel are the configuration options of client scope. However these constants logoUri, policyUri, tosUri are not config options of clientScopeModel. It seems the better place for define them might be ClientModel class or "org.keycloak.models.Constants"

You are right. I move this constants to ClientModel.

mposolda

@cgeorgilakis Thanks!

This was referenced May 26, 2021

KEYCLOAK-18268 logo_uri, policy_uri and tos_uri ( client attributes) #8082

Closed

Show additional SAML & OpenID Connect client metadata in consent screen eosc-kc/keycloak#87

Open

cgeorgilakis changed the title ~~KEYCLOAK-18276 content screen enhancement~~ KEYCLOAK-18276 consent screen enhancement Aug 4, 2021

cgeorgilakis force-pushed the 87-consent-screen branch from bc32a79 to 0527703 Compare August 4, 2021 14:06

cgeorgilakis force-pushed the 87-consent-screen branch 3 times, most recently from f9056b8 to 01d476e Compare October 14, 2021 08:01

pedroigor reviewed Oct 20, 2021

View reviewed changes

pedroigor added area/oidc Indicates an issue on OIDC area kind/enhancement Categorizes a PR related to an enhancement area/testsuite Indicates an issue on the Testsuite area missing/tests and removed area/testsuite Indicates an issue on the Testsuite area labels Oct 20, 2021

cgeorgilakis requested a review from pedroigor November 3, 2021 13:37

pedroigor reviewed Nov 3, 2021

View reviewed changes

stianst requested a review from mposolda November 3, 2021 14:37

stianst mentioned this pull request Nov 10, 2021

KEYCLOAK-18276 client content screen enhancement keycloak/keycloak-documentation#1270

Merged

stianst assigned mposolda Nov 10, 2021

mposolda requested changes Nov 16, 2021

View reviewed changes

KEYCLOAK-18276 client content screen enhancement

334ac04

cgeorgilakis force-pushed the 87-consent-screen branch from 363374b to 334ac04 Compare November 17, 2021 07:51

cgeorgilakis requested a review from pedroigor November 17, 2021 09:02

cgeorgilakis requested a review from mposolda November 17, 2021 09:02

mposolda approved these changes Nov 18, 2021

View reviewed changes

mposolda merged commit 63c9845 into keycloak:main Nov 18, 2021

Conversation

cgeorgilakis commented May 26, 2021

Uh oh!

cgeorgilakis commented Oct 14, 2021

Uh oh!

pedroigor commented Oct 20, 2021

Uh oh!

pedroigor left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pedroigor Oct 20, 2021

Choose a reason for hiding this comment

Uh oh!

cgeorgilakis Oct 21, 2021

Choose a reason for hiding this comment

Uh oh!

pedroigor commented Oct 20, 2021

Uh oh!

cgeorgilakis commented Oct 21, 2021

Uh oh!

cgeorgilakis commented Oct 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cgeorgilakis commented Oct 21, 2021

Uh oh!

pedroigor commented Oct 21, 2021

Uh oh!

cgeorgilakis commented Oct 22, 2021

Uh oh!

pedroigor left a comment

Choose a reason for hiding this comment

Uh oh!

pedroigor Nov 3, 2021

Choose a reason for hiding this comment

Uh oh!

pedroigor Nov 3, 2021

Choose a reason for hiding this comment

Uh oh!

cgeorgilakis Nov 3, 2021

Choose a reason for hiding this comment

Uh oh!

mposolda left a comment

Choose a reason for hiding this comment

Uh oh!

mposolda Nov 16, 2021

Choose a reason for hiding this comment

Uh oh!

cgeorgilakis Nov 17, 2021

Choose a reason for hiding this comment

Uh oh!

mposolda left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

pedroigor left a comment •

edited

Loading

cgeorgilakis commented Oct 21, 2021 •

edited

Loading