move phishing check#48495
Conversation
|
Spotless is failing. |
|
@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. |
|
Oops.. it supposed to be done here: I thought it was already done. Ok |
Me too... I was even pointing that out there in some comment but then I missed it somehow. :D |
|
@edewit Tests seem to be failing. |
| @GET | ||
| @Override | ||
| public BaseClientRepresentation getClient() { | ||
| enforceAntiPhishingIfClientMissing(); |
There was a problem hiding this comment.
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())) { |
There was a problem hiding this comment.
No check for PUT is needed.
3fa4711 to
bf7fdf1
Compare
shawkins
left a comment
There was a problem hiding this comment.
We no longer need the changes to DefaultClientApi, but that's not a blocker.
| @Path("{id}") | ||
| @Override | ||
| public ClientApi client(@PathParam("id") String clientId) { | ||
| enforceAntiPhishingIfClientMissing(clientId); |
There was a problem hiding this comment.
This enforces the check also for PUT which would mean PUT can't create without list permissions, right? That's not correct, AFAICT.
There was a problem hiding this comment.
Right, it will get resolved later by #47804. It's not ideal but I agree we don't need to create workarounds for that, especially when v1 has the same issue.
LGTM then.
fixes: keycloak#48010 Signed-off-by: Erik Jan de Wit <erikjan.dewit@gmail.com>
Signed-off-by: Steven Hawkins <shawkins@redhat.com>
fixes: #48010
Signed-off-by: Erik Jan de Wit erikjan.dewit@gmail.com