Skip to content

Add support for RP-Initiated logout #32

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

Merged
merged 11 commits into from
Sep 7, 2022
Merged

Add support for RP-Initiated logout #32

merged 11 commits into from
Sep 7, 2022

Conversation

tamasmak
Copy link
Contributor

@tamasmak tamasmak commented Sep 2, 2022

Description

  • Add logout method and its implementaion to the VaadinAuthContext to handle the single sign-off
  • Add logout redirect url to the properties
  • Modify the logout button in the Profile view

Fixes #24

Missing:

  • Logout redirect url handling
  • Finalize documentation
  • Tests

Type of change

  • Bugfix
  • Feature

@CLAassistant
Copy link

CLAassistant commented Sep 2, 2022

CLA assistant check
All committers have signed the CLA.

@heruan heruan changed the title Add single sing-off logout Add support for RP-Initiated logout Sep 5, 2022
@heruan heruan self-assigned this Sep 5, 2022
@heruan heruan marked this pull request as draft September 5, 2022 12:17
@heruan
Copy link
Member

heruan commented Sep 5, 2022

There was some code shared with another branch which led to conflicts when merging with main, I'm converting this to draft until they're fixed.

@heruan heruan marked this pull request as ready for review September 5, 2022 13:42
@heruan
Copy link
Member

heruan commented Sep 5, 2022

The PR is now ready for review. On top of the previous commits, it now has:

  • A default implementation of VaadinAuthContext::logout which takes care of logout handlers
  • Auto-configuration for logout and exception handling
  • Inline comments to explain each step of auto-configuration
  • JavaDoc on new methods and classes
  • Tests to ensure logout handlers are set and executed

@knoobie
Copy link

knoobie commented Sep 5, 2022

Inline comments to explain each step of auto-configuration

Is it planned to let this module stay open source? Or is it planned to change it later to closed source like done with the addition of the legacy components? I can totally understand the reasoning behind the license / payed model. But I personally think that security related code should stay open source to find and report security problems instead of forcing people to decompile it to find bugs or security holes. This comment comes from the fact that inline comments are used, which in turn are missing if close source is used and the source files aren't distributed.

@heruan
Copy link
Member

heruan commented Sep 7, 2022

Is it planned to let this module stay open source? Or is it planned to change it later to closed source like done with the addition of the legacy components?

Hey @knoobie, thanks for the question! Current plan is to keep the sources open, although using the add-on or parts of the code will require a commercial license.

@knoobie
Copy link

knoobie commented Sep 7, 2022

the add-on or parts of the code will require a commercial license

That is totally valid! Thanks for the feedback, really appreciated!

Copy link
Contributor

@DiegoCardoso DiegoCardoso left a comment

Choose a reason for hiding this comment

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

Initial comments.

Copy link
Contributor

@DiegoCardoso DiegoCardoso left a comment

Choose a reason for hiding this comment

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

The changes look good to me.

I was able to configure and see that the user's session is correctly removed when I was using it with Keycloak.
With Okta, I wasn't able to make it work, but because it fails with a CORS error when trying to reach the provider's API. I tried to configure it there, but not successfully. Judging by the format of the request, I believe it should work when properly configured.

@heruan heruan merged commit cde9c44 into main Sep 7, 2022
@heruan heruan deleted the feat/auth-logout branch September 7, 2022 21:49
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.

Single sign-off: RP-Initiated logout
5 participants