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

GUACAMOLE-1289: Refactor Duo and authentication flow to instead leverage support for updating/replacing credentials. #980

Merged
merged 9 commits into from
Apr 29, 2024

Conversation

mike-jumper
Copy link
Contributor

@mike-jumper mike-jumper commented Apr 25, 2024

This change builds off and refactors the changes from #966 to instead allow extensions to update (or even replace) the Credentials received by a user prior to authentication.

In the context of Duo, this allows the Duo extension to internally cache and restore the credentials that were originally received prior to redirecting the user to the Duo service, allowing authentication to continue where it left off (and allowing the ${GUAC_USERNAME} and ${GUAC_PASSWORD} tokens to continue to work as intended).

These changes also switch the duo-auth-timeout property from seconds (with a default of 30 seconds) to minutes (with a default of 5 minutes). This is in line with the timeout used for SAML and with the AuthenticationSessionManager class (which only enforces timeouts within roughly one minute).

If acceptable, this is intended to supersede #973 and incorporates the same changes updating/removing old dependencies.

@mike-jumper
Copy link
Contributor Author

I'll see if I can throw some backward compatibility in here, too...

@mike-jumper
Copy link
Contributor Author

It's looking like backward compatibility isn't really possible. Users will simply need to update their Duo config or use an older version of the extension.

@mike-jumper mike-jumper marked this pull request as ready for review April 26, 2024 01:16
@mike-jumper mike-jumper marked this pull request as draft April 26, 2024 01:18
@mike-jumper mike-jumper marked this pull request as ready for review April 26, 2024 07:46
@mike-jumper
Copy link
Contributor Author

mike-jumper commented Apr 26, 2024

Tested things out and seems to work well!

Copy link
Contributor

@necouchman necouchman left a comment

Choose a reason for hiding this comment

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

Looks good to me. @jmuehlner or @aleitner, any comments/concerns on this one?

@jmuehlner
Copy link
Contributor

Looks good to me. @jmuehlner or @aleitner, any comments/concerns on this one?

I haven't had time to dig into it yet, but I trust you if you think it's good.

@jmuehlner
Copy link
Contributor

Looks good to me. @jmuehlner or @aleitner, any comments/concerns on this one?

I haven't had time to dig into it yet, but I trust you if you think it's good.

Having a look now.

@jmuehlner
Copy link
Contributor

Looks good to me. @jmuehlner or @aleitner, any comments/concerns on this one?

I haven't had time to dig into it yet, but I trust you if you think it's good.

Having a look now.

LGTM. Probably worth @aleitner having a look too, as he's the New Duo Guy.

@necouchman
Copy link
Contributor

Thanks @jmuehlner. I'll hold off on the merge for a day or two and let @aleitner have a chance to have a look.

@aleitner
Copy link
Contributor

I love this. Much simpler and solves the problem my open pr was meant to solve.

@necouchman necouchman merged commit 73f05ec into apache:main Apr 29, 2024
1 check passed
@mike-jumper mike-jumper deleted the simplify-auth-resume branch April 29, 2024 16:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants