Skip to content

KEYCLOAK-3244: Required Action "Configure Totp" should be "Configure OTP"#2991

Merged
stianst merged 2 commits intokeycloak:masterfrom
ssilvert:RHSSO-296
Jul 7, 2016
Merged

KEYCLOAK-3244: Required Action "Configure Totp" should be "Configure OTP"#2991
stianst merged 2 commits intokeycloak:masterfrom
ssilvert:RHSSO-296

Conversation

@ssilvert
Copy link
Contributor

@ssilvert ssilvert commented Jul 1, 2016

Implemented the "trivial" fix as indicated on the JIRA. However, there are a few other things to think about.

First, the displayed value actually comes from totp.setName() in DefaultRequiredActions. The value is stored in the database. So for upgraded servers, it will always say "Configure TOTP". That is, unless we want to change it as part of an upgrade script.

Second, the code uses the term "totp" all over the place. I assume that sometimes this is appropriate, but most of the time we really just mean "otp"?

@ssilvert ssilvert changed the title RHSSO-296: Required Action "Configure Totp" should be "Configure OTP" KEYCLOAK-3244: Required Action "Configure Totp" should be "Configure OTP" Jul 1, 2016
@stianst
Copy link
Contributor

stianst commented Jul 4, 2016

#1 We should add migration of existing realms and change the name if alias, name and providerId matches DefaultRequiredActions (this should be done in MigrationModelManager with a MigrateTo2_1_0 class)

#2 Let's just keep it as totp for now. We'll probably need to refactor this in the future anyways to make it less hard-coded on otp as two-factor mechanism.

@stianst stianst self-assigned this Jul 4, 2016
@ssilvert
Copy link
Contributor Author

ssilvert commented Jul 5, 2016

OK. I'll implement #1 very soon.

@stianst
Copy link
Contributor

stianst commented Jul 6, 2016

Looks like this is ready now?

@ssilvert
Copy link
Contributor Author

ssilvert commented Jul 6, 2016

Yes, ready to go.

@stianst stianst merged commit 78fbf45 into keycloak:master Jul 7, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants