Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable TOTP Token Enrollment #17

Merged
merged 10 commits into from
Jan 25, 2022

Conversation

SoarinFerret
Copy link
Contributor

Hello! I am looking for feedback on a feature I have started working on for this ADFS provider. I would like to roll out privacyIDEA as the 2FA provider for an ADFS instance, however a large number of users do not have any TOTP codes enrolled in the system. In order to ease the onboarding process, I thought it would be useful to enable token enrollment with ADFS, similar to how it works with the Keycloak provider.

To accomplish this, I added a new registry key enable_enrollment which is by default disabled. When enabled, it uses the username & password specified by the trigger_challenge functionality to make two API calls:

  • Check if the user has any existing tokens (/token/). If not, then
  • Enroll a new TOTP token (/token/init)

After the token is created, the user is displayed the following page:
image

The page is only shown once after initializing, if the user fails to register the token correctly, they are just presented with a screen to input the OTP. This could be fixed by adding a check if the user was able to successfully connect on the first attempt, and if not, delete the old token and create a new one. But the keycloak-provider did not do this from what I could tell.

Also, currently I have not added the enable_enrollment setting into the installer yet.

One point of feedback I am specifically looking for is the text on the ADFS page. The organization has some specific wants when it comes to which authenticator apps they would like to use - I thought maybe another registry key that would allow the organization to list the applications (and maybe include links?) they would like to promote internally could be useful.

@nilsbehlen
Copy link
Member

Hi,
thanks for your contribution.
It would be great to see our own app (privacyIDEA Authenticator) on that list ;)

In the future there will be a feature in the server that handles token enrollment via challenge-response so that only the server can decide if a token should be enrolled if there is none yet. This will also solve the problem that the token is enrolled as soon as you do /token/init without actually checking if the user scanned the QR (and entered the first OTP). However, that feature is not implemented yet.

The organization has some specific wants when it comes to which authenticator apps they would like to use - I thought maybe another registry key that would allow the organization to list the applications (and maybe include links?) they would like to promote internally could be useful.

I would not spend too much effort for this since the enrollment feature will be reworked in the (hopefully near) future so that the server can dictate this kind of information for all plugins uniformly. But if you need that now you can add it anyway and most parts of your implementation could be reused later too.

Also, currently I have not added the enable_enrollment setting into the installer yet.
It is fine if there is no checkbox in the installer. But i think it would be good if the installer at least writes the entry so that people do not have to check the documentation for the name of the entry.

Copy link
Member

@nilsbehlen nilsbehlen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just very minor things. If you dont understand what i mean with my comments let me know and will push my suggestion

privacyIDEAADFSProvider/AdapterPresentationForm.cs Outdated Show resolved Hide resolved
privacyIDEAADFSProvider/AdapterPresentationForm.cs Outdated Show resolved Hide resolved
privacyIDEAADFSProvider/AdapterPresentationForm.cs Outdated Show resolved Hide resolved
{
htmlTemplate = htmlTemplate.Replace("#ENROLLMENT#", EnrollmentText);
htmlTemplate = htmlTemplate.Replace("#ENROLLVAL#", EnrollmentUrl);
htmlTemplate = htmlTemplate.Replace("#ENROLLIMG#", EnrollmentImg);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

htmlTemplate = htmlTemplate.Replace("#ENROLLIMG#", "data:," + EnrollmentImg);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I understand what this one does - isn't the data text already provided when pulling the data from the API? I originally had the img set to data:, because it was the equivalent of an empty image. In earlier revisions it was always shown, but now that it is hidden it makes sense for it to be empty upon creation.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, i thought it was required to prepend the "data:,", nevermind this comment then

privacyIDEAADFSProvider/PrivacyIDEA.cs Outdated Show resolved Hide resolved
privacyIDEAADFSProvider/PrivacyIDEA.cs Outdated Show resolved Hide resolved
@SoarinFerret
Copy link
Contributor Author

I have made the requested changes, with the exception of the one I commented on. I also have a working branch of storing the authenticator applications in a REG_MULTI_SZ that I can include as part of the PR if you would like.

image

@SoarinFerret SoarinFerret changed the title WIP: Enable TOTP Token Enrollment - feedback wanted WIP: Enable TOTP Token Enrollment Jan 24, 2022
@nilsbehlen
Copy link
Member

I'm happy with this now. If you have nothing more to add, we can merge.

@SoarinFerret SoarinFerret changed the title WIP: Enable TOTP Token Enrollment Enable TOTP Token Enrollment Jan 25, 2022
@SoarinFerret
Copy link
Contributor Author

I don't have anything more - happy for it to be merged. Thanks for your help!

@nilsbehlen
Copy link
Member

thanks for making this pr!

@nilsbehlen nilsbehlen merged commit d8b0a04 into privacyidea:main Jan 25, 2022
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