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

Add remaining methods from ExpressionUrlAuthorizationConfigurer to AuthorizeHttpRequestsConfigurer #11360

Closed
5 tasks done
Tracked by #9290
jzheaux opened this issue Jun 10, 2022 · 12 comments
Closed
5 tasks done
Tracked by #9290
Assignees
Labels
in: core An issue in spring-security-core type: enhancement A general enhancement
Milestone

Comments

@jzheaux
Copy link
Contributor

jzheaux commented Jun 10, 2022

  • Add rememberMe to AuthorizeHttpRequestsConfigurer
  • Add fullyAuthenticated to AuthorizeHttpRequestsConfigurer
  • Add anonymous to AuthorizeHttpRequestsConfigurer
  • Add permitAll to AuthorizeHttpRequestsConfigurer
  • Add denyAll to AuthorizeHttpRequestsConfigurer

Note that each of these may require some enhancement to AuthenticatedAuthorizationManager or AuthorityAuthorizationManager

@jzheaux jzheaux added status: waiting-for-triage An issue we've not yet triaged type: enhancement A general enhancement in: core An issue in spring-security-core status: ideal-for-contribution An issue that we actively are looking for someone to help us with and removed status: waiting-for-triage An issue we've not yet triaged labels Jun 10, 2022
@andresbermeoq
Copy link

Hello @jzheaux.... maybe I can work in this issue?

@jzheaux
Copy link
Contributor Author

jzheaux commented Jun 16, 2022

Please do, @andresbermeoq. I've assigned it to you.

@jzheaux jzheaux removed the status: ideal-for-contribution An issue that we actively are looking for someone to help us with label Jun 16, 2022
@jzheaux jzheaux added this to the 5.8.0-M1 milestone Jun 16, 2022
@andresbermeoq andresbermeoq removed their assignment Jun 16, 2022
@andresbermeoq
Copy link

@jzheaux one question maybe... you can explicate to me about the enhancement for AuthenticatedAuthorizationManager and AuthorityAuthorizationManager

@jzheaux
Copy link
Contributor Author

jzheaux commented Jun 21, 2022

Hi, @andresbermeoq, each change will look something like this:

public AuthorizationManagerRequestMatcherRegistry rememberMe() {
    AuthorizationManager<RequestAuthorizationContext> manager = // ... construct
    return access(manager);
}

More generally, each of the bullet points above is a convenience method that needs to be added to AuthorizeHttpRequestsConfigurer.AuthorizedUrl. These methods delegate to AuthorizeHttpRequestsConfigurer.AuthorizedUrl#access(AuthorizationManager).

you can explicate to me about the enhancement for AuthenticatedAuthorizationManager and AuthorityAuthorizationManager

AuthenticatedAuthorizationManager has a static method called authenticated, which returns an AuthorizationManager that authorizes if the user is authenticated. To this class, you could add three other static methods: fullyAuthenticated, rememberMe, and anonymous. Each of those would return an instance of this manager that passes when the user fits in that corresponding circumstance. You can look at SecurityExpressionRoot to see how isRememberMe, isAnonymous, and isFullyAuthenticated are computed.

In that case, the code above would evolve to:

public AuthorizationManagerRequestMatcherRegistry rememberMe() {
    AuthorizationManager<RequestAuthorizationContext> manager = AuthorityAuthorizationManager.rememberMe();
    return access(manager);
}

Regarding AuthorityAuthorizationManager, you could add static permitAll and denyAll methods. That said, I think these may be simple enough for a lambda, like so:

public AuthorizationManagerRequestMatcherRegistry denyAll() {
    AuthorizationManager<RequestAuthorizationContext> manager = (auth, context) -> new AuthorizationDecision(false);
    return access(manager);
}

or more simply:

public AuthorizationManagerRequestMatcherRegistry denyAll() {
    return access((auth, context) -> new AuthorizationDecision(false));
}

@jzheaux
Copy link
Contributor Author

jzheaux commented Jun 24, 2022

Also, @andresbermeoq, given that this will go to 5.8, will you please base your changes off of the 5.8.x branch?

@andresbermeoq
Copy link

@jzheaux I try to implement the methods in AuthorizeHttpRequestsConfigurer in #11443, but I'm a little lost with the methods inside the AuthenticatedAuthorizationManager.
Thanks for your time.

@evgeniycheban
Copy link
Contributor

@jzheaux I wonder if we could simply use WebExpressionAuthorizationManager for rememberMe, fullyAuthenticated, anonymous.

@jzheaux
Copy link
Contributor Author

jzheaux commented Jun 27, 2022

@evgeniycheban, do you mean not add DSL methods for rememberMe, etc. at all? The reason for adding them was we received community feedback that these would greatly simplify migration, so I think we should try adding them.

Or maybe you are saying that the DSL rememberMe method could use WebExpressionAuthorizationManager? I agree that we could start there. That said, I think we'd eventually want to have a SpeL-less solution; allowing SpEL to interpret the string "rememberMe" doesn't add very much value since if the application wants to customize how "rememberMe" is interpreted, they can simply provide a custom AuthenticationManager implementation instead of a custom ExpressionHandler.

@andresbermeoq, do you want to try going this route? If you do, that's fine. I'm also sensing that having this be your very first ticket might be a bit of a stretch and I wonder if you would be interested in trying a simpler ticket first? Let me know either way; it you choose the latter route, I'm happy to suggest a ticket or two.

@evgeniycheban
Copy link
Contributor

@jzheaux Yes, I mean the DSL rememberMe could use WebExpressionAuthorizationManager, but of course we could implement it without using SpEL.

@evgeniycheban
Copy link
Contributor

If @andresbermeoq would like to try a simpler ticket, I'd like to take this one.

@andresbermeoq
Copy link

@jzheaux yes, it's my first ticket…. You could help me with the suggestion for newbies, I'd be happy. @evgeniycheban no problem, you can try, I would like to know the branch in which you plan to work to see how you fix it.

@jzheaux
Copy link
Contributor Author

jzheaux commented Jul 7, 2022

Sounds good, @andresbermeoq and @evgeniycheban.

@andresbermeoq, I didn't see any first-timers-only tickets up right now, but there is a related documentation ticket that would be a good place to start. You can also periodically check the first-timers-only search to find tickets targeting folks just getting started.

evgeniycheban added a commit to evgeniycheban/spring-security that referenced this issue Jul 14, 2022
…thorizeHttpRequestsConfigurer

- Added fullyAuthenticated
- Added rememberMe
- Added anonymous

Closes spring-projectsgh-11360
jzheaux pushed a commit that referenced this issue Jul 14, 2022
…thorizeHttpRequestsConfigurer

- Added fullyAuthenticated
- Added rememberMe
- Added anonymous

Closes gh-11360
jzheaux added a commit that referenced this issue Jul 14, 2022
jzheaux added a commit that referenced this issue Jul 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment