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.

Sign up for GitHub

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

Jump to bottom

feat: Introduced component-based security configuration for Spring #14303

Merged
merged 13 commits into from
Aug 16, 2022
Merged

feat: Introduced component-based security configuration for Spring #14303

merged 13 commits into from
Aug 16, 2022

Conversation

MarcinVaadin
Copy link
Member

@MarcinVaadin MarcinVaadin commented Aug 9, 2022 •
edited

Description

Introduced component-based security configuration for Spring

Fixes #13910

Type of change

  • Bugfix
  • Feature

Checklist

  • I have read the contribution guide: https://vaadin.com/docs/latest/guide/contributing/overview/
  • I have added a description following the guideline.
  • The issue is created in the corresponding repository and I have referenced it.
  • I have added tests to ensure my change is effective and works as intended. - Spring security configuration already covered by test-spring-security-flow tests.
  • New and existing tests are passing locally with my change.
  • I have performed self-review and corrected misspellings.

Additional for Feature type of change

  • Enhancement / new feature was discussed in a corresponding GitHub issue and Acceptance Criteria were created.

@MarcinVaadin MarcinVaadin linked an issue Aug 9, 2022 that may be closed by this pull request
Support for the component-based security configuration #13910
Closed
@project-bot project-bot bot added this to Iteration Reviews in OLD Vaadin Flow ongoing work (Vaadin 10+) Aug 9, 2022
@github-actions
Copy link

github-actions bot commented Aug 9, 2022 •
edited

Unit Test Results

   917 files  ±  0     917 suites  ±0   54m 58s ⏱️ + 5m 12s
6 008 tests ±  0  5 955 ✔️ +  1  53 💤 ±0  0 ❌ ±0 
6 221 runs  +13  6 161 ✔️ +14  60 💤 ±0  0 ❌ ±0 

Results for commit e12b0b3. ± Comparison against base commit b350740.

♻️ This comment has been updated with latest results.

@MarcinVaadin MarcinVaadin marked this pull request as ready for review August 10, 2022 08:47
@vaadin-bot vaadin-bot added +0.1.0 and removed +1.0.0 labels Aug 11, 2022
caalador
caalador previously approved these changes Aug 12, 2022
View reviewed changes
Copy link
Contributor

@caalador caalador left a comment

Choose a reason for hiding this comment

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

This also fixes first WARN mentioned in #13868
The pattern='/images/*.png' is in the starter application so should be fixed there after this is merged. Also stareters should be updated to use the new way.

@MarcinVaadin
feat: moved getDefaultWebSecurityIgnoreMatcher from WebSecurityCustom…
275fdda 
…izer to SecurityFilterChain

Changed approach of registering public resources (from ignoring to permiAll).

Fixes: #13868
@MarcinVaadin MarcinVaadin mentioned this pull request Aug 12, 2022
Closed
@MarcinVaadin
Merge remote-tracking branch 'origin/feat/13910-support-for-the-compo…
f2b17ad 
…nent-based-security-configuration' into feat/13910-support-for-the-component-based-security-configuration
@MarcinVaadin
Copy link
Member Author

This also fixes first WARN mentioned in #13868 The pattern='/images/*.png' is in the starter application so should be fixed there after this is merged. Also stareters should be updated to use the new way.

Changed approach of registering public resources (from ignoring to permitAll) to fix mentioned issues.

@vaadin-bot vaadin-bot added +1.0.0 and removed +0.1.0 labels Aug 12, 2022
caalador
caalador previously approved these changes Aug 12, 2022
View reviewed changes
mcollovati
mcollovati reviewed Aug 12, 2022
View reviewed changes
Copy link
Collaborator

@mcollovati mcollovati left a comment

Choose a reason for hiding this comment

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

Just a random thought: with previous implementation, it was common to override configure(HttpSecurity) invoking super and then apply custom configuration.
What about preserving that method to ease migration?
New implementation will provide SecurityFilterChain bean, and developers just need to override the protected configure(HttpSecurity) method.
Migration would just be changing base class

vaadin-spring/src/main/java/com/vaadin/flow/spring/security/VaadinWebSecurity.java Outdated Show resolved Hide resolved
@mcollovati
Copy link
Collaborator

Same thought about the old configure(WebSecurity web)
Should we maybe expose a WebSecurityCustomizer bean with logic previously from configure(WebSecurity web)?
If we adopt the same pattern, the migration would be likely just change the super class

@MarcinVaadin
Copy link
Member Author

Same thought about the old configure(WebSecurity web) Should we maybe expose a WebSecurityCustomizer bean with logic previously from configure(WebSecurity web)? If we adopt the same pattern, the migration would be likely just change the super class

The functionality of configure(WebSecurity web) has been moved into filterChain(HttpSecurity http) and TBH I don't see a point leaving empty bean.

Yes, it should be described in migration docs.

@MarcinVaadin @mcollovati
Update vaadin-spring/src/main/java/com/vaadin/flow/spring/security/Va…
fdae699 
…adinWebSecurity.java

Co-authored-by: Marco Collovati <marco@vaadin.com>
@mshabarov
Copy link
Contributor

After this feature applied, we also need to update the following article to not mention the deprecated adapter, but describe a new approach https://vaadin.com/docs/latest/security/enabling-security

mshabarov
mshabarov previously approved these changes Aug 16, 2022
View reviewed changes
Copy link
Contributor

@mshabarov mshabarov left a comment

Choose a reason for hiding this comment

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

Missing javadocs, otherwise LGTM: old configure methods are there for backwards compatibility and a new Spring classes for filter chain and web customisation are also available through dedicated methods as a beans.

@sonarcloud
Copy link

sonarcloud bot commented Aug 16, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 11 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@mshabarov mshabarov merged commit 8b59f70 into master Aug 16, 2022
OLD Vaadin Flow ongoing work (Vaadin 10+) automation moved this from Iteration Reviews to Done - pending release Aug 16, 2022
@mshabarov mshabarov deleted the feat/13910-support-for-the-component-based-security-configuration branch August 16, 2022 13:29
@Artur- Artur- mentioned this pull request Aug 16, 2022
Closed
@vaadin-bot
Copy link
Collaborator

This ticket/PR has been released with Vaadin 23.2.0.beta2 and is also targeting the upcoming stable 23.2.0 version.

@mshabarov mshabarov mentioned this pull request Aug 18, 2022
Closed
@knoobie knoobie mentioned this pull request Aug 25, 2022
Closed
@vursen vursen mentioned this pull request Oct 24, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mcollovati mcollovati mcollovati approved these changes

@mshabarov mshabarov mshabarov approved these changes

@caalador caalador caalador left review comments

Assignees
No one assigned
Labels
prerelease version for Vaadin 23.2.0 Released with Vaadin 23.2.0.beta2 +1.0.0
Projects
OLD Vaadin Flow ongoing work (Vaadin ...
  
Done - pending release
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support for the component-based security configuration
5 participants
@MarcinVaadin @mcollovati @mshabarov @vaadin-bot @caalador