Skip to content

move phishing check#48495

Open
edewit wants to merge 3 commits intokeycloak:mainfrom
edewit:phishing-check
Open

move phishing check#48495
edewit wants to merge 3 commits intokeycloak:mainfrom
edewit:phishing-check

Conversation

@edewit
Copy link
Copy Markdown
Contributor

@edewit edewit commented Apr 27, 2026

fixes: #48010
Signed-off-by: Erik Jan de Wit [email protected]

@vmuzikar
Copy link
Copy Markdown
Contributor

Spotless is failing.

@mabartos
Copy link
Copy Markdown
Contributor

@edewit JFYI - you should rebase this PR as there's no NewClientService anymore.

@shawkins
Copy link
Copy Markdown
Contributor

@edewit JFYI - you should rebase this PR as there's no NewClientService anymore.

I think it still exists unfortunately - it just needs to now be deleted.

@mabartos
Copy link
Copy Markdown
Contributor

mabartos commented Apr 27, 2026

Oops.. it supposed to be done here:

I thought it was already done. Ok

@vmuzikar
Copy link
Copy Markdown
Contributor

I thought it was already done.

Me too... I was even pointing that out there in some comment but then I missed it somehow. :D

@vmuzikar
Copy link
Copy Markdown
Contributor

@edewit Tests seem to be failing.

fixes: keycloak#48010
Signed-off-by: Erik Jan de Wit <[email protected]>
Signed-off-by: Erik Jan de Wit <[email protected]>
@GET
@Override
public BaseClientRepresentation getClient() {
enforceAntiPhishingIfClientMissing();
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The intent of the issue was to move the check in DefaultClientsApi prior to creating DefaultClientApi - that aligns with admin api v1. We should not worry yet about the case of hitting a 403 on a PUT that performs a create as we'll eventually have direction from #47804 to address that.

This was just to be a simplification over currently having the permission check in the service layer. If / when we want to promote more general usage of the service layer and we have resolution on #47804 we could move the phishing check back.

@Path("{id}")
@Override
public ClientApi client(@PathParam("id") String clientId) {
if (!HttpMethod.PUT.equals(session.getContext().getHttpRequest().getHttpMethod())) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No check for PUT is needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Move the phishing check to the REST layer

4 participants