With this change we are allowing editing any secret (even for view only users that cannot submit). I think that this was initially read-only because the idea is only allow the generate button. This way the passwords are secure/complex. Maybe we should maintain it read-only initially and allow editing when changed to visible (only for admins that can edit). Not sure about this. I thought about showing a message when toggle to visible, commenting about complexity and so on and so forth... But maybe it's too much.

But I would at least present it read-only and just enable editing when toggled to visible if the admin has permissions for edit (if not it remains read-only).

Change the documentation accordingly if you think my proposal is better.

@edewit @tsaarni WDYT? Any other idea?

I think that this was initially read-only because the idea is only allow the generate button. This way the passwords are secure/complex.

Likely true, though the client secrets were possible to change via REST API anyways.

But I would at least present it read-only and just enable editing when toggled to visible if the admin has permissions for edit (if not it remains read-only).

Thanks! I had missed the readOnly={!isManager}. Regarding being editable only after toggling visibility, I suppose <PasswordInput> component remains editable by design even when password is not revealed?

I was thinking about something similar to rmartinc@d59fa46. I mean, by default is read-only, and only when revealed is also editable. But I'm not sure if I'm just overthinking and complicating the UI. We can go even farther and can show a warning or similar before make it editable with this idea.

@edewit @mposolda WDYT? Do we always allow modifying the secret or just when revealing it? I was trying to make the generate button the easiest path except if you really want to type the secret (because you want a vault or any other reason).

I think it's fine to leave it like this that when you want to edit it you can only do that when it's viewable, otherwise we would have to have 2 fields to confirm the new password. One thing to note is instead of using <Control it would be better to use

            <PasswordControl
              name="secret"
              isDisabled={!isManager}
            />

Thanks @edewit! I think this is using directly the PasswordInput instead of the PasswordControl because there are more elements in the row (the copy and regenerate buttons and more things for the client secret rotation). And you need to change the PasswordControl to add everything inside it.

Then @tsaarni, if Erik thinks that it is OK like it is now, just change the the read-only for disabled (it's the same and I think that it works like the rest of fields) and that's all:

            <Controller
              name="secret"
              control={form.control}
              render={({ field }) => (
                <PasswordInput id={id} {...field} isDisabled={!isManager} />
              )}
            />

Thanks! I've changed from readOnly to isDisabled.

Minor: Should not this method use the same like ClientIdAndSecretAuthenticator.getAdapterConfiguration to actually obtain secret for adapters?

Yep! I missed this one. It should also use the vault to get the secret.

I only planned to add vault support for the client secret authenticator but I now tested the JWT client secret authenticator as well and it's not working with vault references. Not sure what's needed to implement vault support for that as well.

OK, but then this should be at least managed as follow-up. The JWTClientSecretAuthentication is just an authenticator that uses the secret as the HMAC to sign... We would need also tests for this. @mposolda WDYT?

Ah, ok. It would be good to update that one as well for consistency. It seems this authenticator uses key, which is created from client-secret and secret is obtained in ClientMacSignatureVerifierContext (not 100% sure it is this class. You can likely doublecheck).

@mposolda @rmartinc I've now added support for JWT client secret authenticator + test case.