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

Refactor StrongboxAuthenticationFilter extend Spring's AuthenticationFilter interface and related re-workings #1866

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Refactor StrongboxAuthenticationFilter extend Spring's AuthenticationFilter interface and related re-workings #1866

wants to merge 6 commits into from

Conversation

soapdogg
Copy link

@soapdogg soapdogg commented Sep 30, 2020 •
edited by carlspring

Pull Request Description

This pull request makes progress on #1552

This satisfies the following

  • Make org.carlspring.strongbox.security.authentication.StrongboxAuthenticationFilter extend the org.springframework.security.web.authentication.AuthenticationFilter
  • Re-work AuthenticationSupplier implementations to AuthenticationConverter implementations
  • Re-work AuthenticationSuppliers implementation to DelegatingAuthenticationConverter (it should delegate to first AuthenticationConverter that will return not null Authentication object)
  • Check that new StrongboxAuthenticationFilter implementation consistent with logic that we currently have
  • Provide unit tests that check different authentication scenarios with few mocked AuthenticationConverter implementations

Acceptance Test

  • Building the code with mvn clean install -Dintegration.tests still works.
  • Running mvn spring-boot:run in the strongbox-web-core still starts up the application correctly.
  • Building the code and running the strongbox-distribution from a zip or tar.gz still works.
  • The tests in the strongbox-web-integration-tests still run properly.

Questions

  • Does this pull request break backward compatibility?

    • Yes
    • No

  • Does this pull request require other pull requests to be merged first?

    • Yes, please see #...
    • No

  • Does this require an update of the documentation?

    • Yes, please see strongbox/strongbox-docs#{PR_NUMBER}
    • No

@carlspring carlspring changed the title Refactor StrongboxAuthenticationFilter extend Spring's Authentication… Refactor StrongboxAuthenticationFilter extend Spring's AuthenticationFilter interface and related re-workings Sep 30, 2020
steve-todorov
steve-todorov reviewed Oct 1, 2020
View reviewed changes
...a/org/carlspring/strongbox/security/authentication/strategy/BasicAuthenticationStrategy.java
@@ -20,28 +20,23 @@
*/
@Component
@Order(4)
class BasicAuthenticationSupplier implements AuthenticationSupplier
class BasicAuthenticationStrategy implements AuthenticationStrategy
Copy link
Member

Choose a reason for hiding this comment

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

+1 for the naming! :)

Copy link
Member

Choose a reason for hiding this comment

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

not sure that *AuthenticationStrategy is good naming here, because this interface do not authenticate by itself, it just supplies the Authentication object based on HttpServletRequest.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I read it too fast before bed. You're right.

sbespalov
sbespalov suggested changes Oct 1, 2020
View reviewed changes
Copy link
Member

@sbespalov sbespalov left a comment

Choose a reason for hiding this comment

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

@soapdogg thanks for the PR. It's seems that task requirements was not really clear for you so the main point was not to use custom StrongboxAuthenticationFilter implementation and make it use original AuthenticationFilter instead. Would you be able to do it as it was assumed to be done? Also if you have any question please ask.

README.md
@@ -1,4 +1,6 @@
# ![strongbox-logo][strongbox-logo]
<p align="center" width="100%">
Copy link
Member

Choose a reason for hiding this comment

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

@soapdogg these changes seems not related with the PR. Where does it come from?

Copy link
Member

Choose a reason for hiding this comment

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

He probably merged from upstream where I applied them.

Copy link
Author

Choose a reason for hiding this comment

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

Yes, that is my mistake. I can make another pull request with just my changes.

Copy link
Member

Choose a reason for hiding this comment

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

we can stay with this PR, just force push new commits here without merge commits

...ain/java/org/carlspring/strongbox/security/authentication/StrongboxAuthenticationFilter.java
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import org.springframework.security.authentication.AuthenticationManager;
import org.springframework.security.core.Authentication;
import org.springframework.security.core.context.SecurityContextHolder;
import org.springframework.web.filter.OncePerRequestFilter;
import org.springframework.security.web.authentication.AuthenticationFilter;

/**
* @author Przemyslaw Fusik
*/
public class StrongboxAuthenticationFilter
Copy link
Member

Choose a reason for hiding this comment

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

@soapdogg you shouldn't override doFilterInternal method here. The point of this task was to use original AuthenticationFilter implementation to work same way as it was before with StrongboxAuthenticationFilter.

Copy link
Author

Choose a reason for hiding this comment

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

Make org.carlspring.strongbox.security.authentication.StrongboxAuthenticationFilter extend the org.springframework.security.web.authentication.AuthenticationFilter is in the issue description.

It seems like what you are saying an this is contradicting each other. Which implementation is preferred?

Copy link
Member

Choose a reason for hiding this comment

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

I mean that you can extend but shouldn't just override everything, obviously it doesn't make sense.
sorry if it was not clear :)

...a/org/carlspring/strongbox/security/authentication/strategy/BasicAuthenticationStrategy.java
@@ -20,28 +20,23 @@
*/
@Component
@Order(4)
class BasicAuthenticationSupplier implements AuthenticationSupplier
class BasicAuthenticationStrategy implements AuthenticationStrategy
Copy link
Member

Choose a reason for hiding this comment

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

not sure that *AuthenticationStrategy is good naming here, because this interface do not authenticate by itself, it just supplies the Authentication object based on HttpServletRequest.

...java/org/carlspring/strongbox/security/authentication/StrongboxAuthenticationFilterTest.java
import javax.servlet.http.HttpServletResponse;
import java.io.IOException;

class StrongboxAuthenticationFilterTest
Copy link
Member

Choose a reason for hiding this comment

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

why not public class?

Copy link
Author

Choose a reason for hiding this comment

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

habit from using junit jupiter in other projects. I can add public

Copy link
Member

Choose a reason for hiding this comment

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

I think it should be public

@carlspring
Copy link
Member

@soapdogg ,

How are things going with this pull request? Is it clear for you what still needs to be done?

@soapdogg
Copy link
Author

@carlspring

Hi,

Not necessarily -- it does seem like some of the comments on the pull request are contradicting to what is in the task description so it's unclear what direction needs to be taken with this.

Feel free to unassign the task from me if someone else wants to take a look.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@carlspring carlspring carlspring left review comments

@steve-todorov steve-todorov steve-todorov left review comments

@sbespalov sbespalov sbespalov requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@soapdogg @carlspring @steve-todorov @sbespalov