feat: add support for migrating totp #197
Conversation
|
Hi! Sorry it's taking me so long to look at this one. I'll try to take a closer look around this weekend. In the meantime, could you document how this TOTP migration works in the |
|
Sorry, I just read your message. Let me now if you need more documentation. |
|
...And so the weekend came and went. Sorry for the delay, again. Could you add the information you provided in the PR description to the README.md:
It could even be get its own subheader for TOTP migration. I'm not well-versed in TOTP, but I assume both of these are important. I'll merge this MR regardless, but, if you have some time, it would be cool if you could reformat the code, as there's currently no auto-formatter configured for this project. Some examples:
Last, but not least, a suggestion. This line of code makes me think that the "devices must have a base32 encoded secret and be based on SHA1" requirement could be removed by making the otpModel configurable via the UI: I understand if you don't feel like doing it, but at least for future reference, am I correct in that assumption? |
|
Thank you. |
|
Good question! Would you like to do it as part of this MR, or should I merge this in? |
|
I added the configurability now. If your fine with it and the documentation you could merge it. |
README.md
Outdated
| "totps": [ | ||
| { | ||
| "name": "string", | ||
| "secret": "string" |
There was a problem hiding this comment.
Some of the properties are missing here.
README.md
Outdated
| "encoding": "BASE32" | ||
| } | ||
| ``` | ||
| name should be the name of the totp device, while secret is the secret, that could be encoded in "BASE32" or as the utf8 bytes. |
There was a problem hiding this comment.
Using backticks for the properties would increase readability. E.g.,:
"name should be the name of the totp device (...)".
| totp.setName(expectedValue); | ||
| assertEquals(expectedValue, totp.getName()); | ||
| } | ||
|
|
README.md
Outdated
| "encoding": "BASE32" | ||
| } | ||
| ``` | ||
| name should be the name of the totp device, while secret is the secret, that could be encoded in "BASE32" or as the utf8 bytes. |
There was a problem hiding this comment.
Grammar nitpick: the utf8 bytes →UTF-8 bytes (removed the, uppercased and kebab-cased UTF-8).
Ditto about the utf8 bytes below.
README.md
Outdated
| "encoding": "BASE32" | ||
| } | ||
| ``` | ||
| name should be the name of the totp device, while secret is the secret, that could be encoded in "BASE32" or as the utf8 bytes. |
There was a problem hiding this comment.
I'm not sure if I understand it correctly, but wouldn't UTF-8 plaintext be easier to understand than UTF-8 bytes?
|
Great! I added a few nitpicks (mostly about documentation) and I see that Sonar found something, as well. After that's addressed, I'll gladly merge it in :) |
|
I think i fixed everything |
|
|
Great, thanks for the contribution! |
This PR adds the support for a totps attribute in LegacyUser, that allows to migrate multiple Totp devices, that have a base32 encoded secret and are based on SHA1