Ensure client_id/client_secret patterns are not enforced as before #3276
Conversation
dimas-b
left a comment
dimas-b
left a comment
There was a problem hiding this comment.
Thanks for your contribution, @rmannibucau ! Please see my comment about not performing these checks. If you agree, the other comments can be ignored.
| */ | ||
| public abstract class PolarisConfiguration<T> { | ||
|
|
||
| private static final Logger LOGGER = LoggerFactory.getLogger(PolarisConfiguration.class); |
There was a problem hiding this comment.
nit: if you find "dead code", it's generally preferable to cleanup in a dedicated PR and avoid mixing cleanup with feature changes :)
| private void validateClientId(String clientId) { | ||
| if (!clientId.matches("^[0-9a-f]{16}$")) { | ||
| if (!clientId.matches( | ||
| realmConfig.getConfig(FeatureConfiguration.CREDENTIAL_RESET_CLIENT_ID_PATTERN))) { |
There was a problem hiding this comment.
we should validate the value of CREDENTIAL_RESET_CLIENT_ID_PATTERN on startup to avoid deferred RegEx syntax errors (it's an admin mistake, not the API client's mistake)
There was a problem hiding this comment.
Hmm, it will be per realm so no sure it can be done at startup properly, do you have a code pointer in mind? would bootstrap realm fulfill your expectation?
default is known valid - same as before, it is hardcoded
There was a problem hiding this comment.
There was a problem hiding this comment.
hmm, not sure i'm a fan of this one, feature is quite specific and we would make the realm down for it? 🤔
let's drop the validation for now then
| private void validateClientSecret(String clientSecret) { | ||
| if (!clientSecret.matches("^[0-9a-f]{32}$")) { | ||
| if (!clientSecret.matches( | ||
| realmConfig.getConfig(FeatureConfiguration.CREDENTIAL_RESET_CLIENT_SECRET_PATTERN))) { |
There was a problem hiding this comment.
If validating client ID/secret format is becoming a nuisance to users, from my POV this check can just be removed completely. I do not think Polaris code relies on these values following a particular format.
If backward compatibility is a concern, I'd rather add a simple boolean flag to disable these checks (defaulting to enabled). WDYT?
There was a problem hiding this comment.
I'm fine dropping the feature (the two validateClientX methods) too
There was a problem hiding this comment.
Let's try removing the checks completely and get some more reviews. If people raise concerns we can add an on/off flag then.
rmannibucau
force-pushed
the
dev/make-client-id-secret-pattern-configurable
branch
from
548e086 to
cd0bd7c
Compare
| } | ||
| } | ||
|
|
||
| private PolarisServiceImpl noAdminResetCredentialPolarisService() { |
There was a problem hiding this comment.
Is this method still used?
rmannibucau
force-pushed
the
dev/make-client-id-secret-pattern-configurable
branch
from
cd0bd7c to
b842b06
Compare
rmannibucau
force-pushed
the
dev/make-client-id-secret-pattern-configurable
branch
from
b842b06 to
8dfbac6
Compare
dimas-b
left a comment
dimas-b
left a comment
There was a problem hiding this comment.
Code change LGTM 👍 The PR title probably needs to be adjusted now 🙂
github-project-automation
bot
moved this from PRs In Progress
to Ready to merge
in Basic Kanban Board
rmannibucau
changed the title
3 participants
client_id/client_secret patterns are validated when calling reset endpoint but the pattern is hardcoded which can be too rigid.
this PR just enables to configure it.
Checklist
CHANGELOG.md(if needed)site/content/in-dev/unreleased(if needed)