Repository navigation
Conversation
|
No worries, I just closed it as it's kinda hard to reset the review policy, so creating a new PR just works around that. |
|
@wistefan Thanks for the PR. It looks OK, but I am pretty sure that you will have some test failures. As you can see in the https://github.com/keycloak/keycloak/actions/runs/7555072770/job/20572008260 , there is failure in |
|
@mposolda @thomasdarimont @jonkoops Hello, I will also review the PR but maybe in the next week (22 Jan-). |
74650a2 to
4b93d31
Compare
6e0f5ee to
f369a68
Compare
There was a problem hiding this comment.
Not sure why the UI team was requested as a reviewer, doesn't look like anything was touched in our domain.
There was a problem hiding this comment.
Just a question: how this CLOCK is used?
There was a problem hiding this comment.
Clock is handed to the concrete signing services, they will need access to time. I could remove them if you prefer and readd once the concrete signing services are added.
There was a problem hiding this comment.
Thank you for the clarification.
There was a problem hiding this comment.
Thank you. I confirmed the revised codes.
There was a problem hiding this comment.
Small suggestion: Keycloak uses org.keycloak.common.util.Time for accessing the clock, which also allows to provide a global offset (for tests). However org.keycloak.common.util.Time uses System time and not necessarily UTC, so using Clock.systemUTC() directly might be fine, too. I wonder if it would make sense to have a Time.currentTimeUTC() method that also supports the offset.
There was a problem hiding this comment.
Hi, thanks for mentioning that. Is there also a mechanism to mock time with that class?
There was a problem hiding this comment.
Yes, +1 for adding something like Time.currentTimeUTC() (if using UTC is really needed). We want only one utility for handling "time" in Keycloak, not multiple different utilities :-)
There was a problem hiding this comment.
AFAIK, I did not see the use of "var" in Keycloak codebase.
@mposolda using "var" is OK?
There was a problem hiding this comment.
Not sure... Probably using standard string is slightly better.
a36c099 to
95f1546
Compare
There was a problem hiding this comment.
Is it possible to make the feature EXPERIMENTAL instead of PREVIEW ? For the inspiration, you can take a look at this PR, which introduces some experimental feature what are all the various things needed to make tests working #26389 .
Signed-off-by: Stefan Wiedemann <wistefan@googlemail.com>
Signed-off-by: Stefan Wiedemann <wistefan@googlemail.com>
Signed-off-by: Stefan Wiedemann <wistefan@googlemail.com>
Signed-off-by: Stefan Wiedemann <wistefan@googlemail.com>
remove wildcard imports Signed-off-by: Stefan Wiedemann <wistefan@googlemail.com>
Signed-off-by: Stefan Wiedemann <wistefan@googlemail.com>
Signed-off-by: Stefan Wiedemann <wistefan@googlemail.com>
bb67c4d to
3219b68
Compare
Signed-off-by: Stefan Wiedemann <wistefan@googlemail.com>
Signed-off-by: Stefan Wiedemann <wistefan@googlemail.com>
Signed-off-by: Stefan Wiedemann <wistefan@googlemail.com>
|
@wistefan @tnorimat @thomasdarimont Thanks everyone for the review and participation on this PR! |
…5937 (keycloak#26263) * implement oid4vci service interfaces Signed-off-by: Stefan Wiedemann <wistefan@googlemail.com> * add oid4vc to the disabled features test Signed-off-by: Stefan Wiedemann <wistefan@googlemail.com> * fix test and add doc Signed-off-by: Stefan Wiedemann <wistefan@googlemail.com> * add the new preview feature Signed-off-by: Stefan Wiedemann <wistefan@googlemail.com> * add class-level doc remove wildcard imports Signed-off-by: Stefan Wiedemann <wistefan@googlemail.com> * add license headers Signed-off-by: Stefan Wiedemann <wistefan@googlemail.com> * fix year Signed-off-by: Stefan Wiedemann <wistefan@googlemail.com> * fix teste Signed-off-by: Stefan Wiedemann <wistefan@googlemail.com> * two additional test fixes Signed-off-by: Stefan Wiedemann <wistefan@googlemail.com> * make the feature experimental Signed-off-by: Stefan Wiedemann <wistefan@googlemail.com> * remove clock Signed-off-by: Stefan Wiedemann <wistefan@googlemail.com> * remove usage of var Signed-off-by: Stefan Wiedemann <wistefan@googlemail.com> * fix tests Signed-off-by: Stefan Wiedemann <wistefan@googlemail.com> --------- Signed-off-by: Stefan Wiedemann <wistefan@googlemail.com> Signed-off-by: ShefeeqPM <86718986+ShefeeqPM@users.noreply.github.com>
The PR adds implementations for SPI and Provider factories and interfaces to prepare the implementation of the #25936 feature.
Closes #25937
@jonkoops @mposolda Sorry for creating such a mess on #25947 , this one should now only include the relevant changes.