Add 'unauthorizedRedirectUrl' usable for pac4j module #3293
Conversation
…jInitialFlowSetupAction with InitialFlowSetupAction
| /** | ||
| * Protected casProperties. | ||
| */ | ||
| protected CasConfigurationProperties casProperties; |
There was a problem hiding this comment.
Remove all protected instance. Remove their javadocs. Only mark those as protected that are needed by your changes.
There was a problem hiding this comment.
For what I see, they all need to be protected because those variables will be accessed by configureWebflowContextForService and the constructor
There was a problem hiding this comment.
Isn't configureWebflowContextForService owned by the same class that houses all the properties? If so, you do not need a field to be marked as protected only to then access it from a method inside the same class that has the field.
| @@ -96,7 +124,12 @@ private void configureWebflowContextForService(final RequestContext context) { | |||
| WebUtils.putService(context, service); | |||
| } | |||
|
|
|||
| private void configureWebflowContext(final RequestContext context) { | |||
| /** | |||
| * configureWebflowContext. | |||
There was a problem hiding this comment.
You need to explain this better.
There was a problem hiding this comment.
I will try do my best
| * | ||
| * @param context a request context | ||
| */ | ||
| protected void configureCookieGenerators(final RequestContext context) { |
There was a problem hiding this comment.
You need to explain this better.
There was a problem hiding this comment.
I don't know what it does... Do you have a suggestion ?
There was a problem hiding this comment.
Ensures cookie paths are properly set for the generators when the flow is initialized.
|
|
||
| /** | ||
| * Get ServicesManager. | ||
| * |
| final TicketGrantingTicket tgt = this.centralAuthenticationService.createTicketGrantingTicket(authenticationResult); | ||
| WebUtils.putTicketGrantingTicketInScopes(context, tgt); | ||
| } catch (final PrincipalException e) { | ||
| LOGGER.warn("PrincipalException caught on service [{}]", e.getMessage()); |
| * @author Francis Le Coq | ||
| * @since 5.2 | ||
| */ | ||
| public class Pac4jInitialFlowSetupAction extends InitialFlowSetupAction { |
There was a problem hiding this comment.
Because on Pac4j you dont need configureCookieGenerators(context); configureWebflowContext(context); to be executed
There was a problem hiding this comment.
In that case, you need to simply add a flag that displays that behavior, rather than adding a whole new class and action and accompanying beans.
There was a problem hiding this comment.
Furthermore, as far as I can tell the configuration of cookie generators only sets paths for the generators. Therefore, is there a reason that operation is removed? does it cause a failure or break something? If not, you should simply let it be and remove this class altogether.
| @@ -49,6 +50,8 @@ protected void doInitialize() { | |||
| createClientActionActionState(flow); | |||
| createStopWebflowViewState(flow); | |||
| createSaml2ClientLogoutAction(); | |||
|
|
|||
| createAuthnFailureAction(flow); | |||
| setStartState(flow, actionState); | ||
| } | ||
|
|
||
| private void createAuthnFailureAction(final Flow flow){ | ||
| final ActionState actionState = createActionState(flow, "pac4jFailure", |
There was a problem hiding this comment.
Use a constant for the failure state id.
|
I updated for the requested changes The master branch will need more work because changes have been made and some files have been deleted and created elsewhere or renamed. Git does not seem to know how to deal with those so I will need to adapt the code to the new master changes. |
| final TicketGrantingTicket tgt = this.centralAuthenticationService.createTicketGrantingTicket(authenticationResult); | ||
| WebUtils.putTicketGrantingTicketInScopes(context, tgt); | ||
| } catch (final PrincipalException e) { | ||
| LOGGER.warn("Principal attributes have not been validated on service [{}]", e.getMessage()); |
There was a problem hiding this comment.
Possibly should change to:
LOGGER.warn(e.getMessage(), e);
The error message that is indicated above does not make that much sense. Principal attributes cannot be validated in any way by CAS.
| private final CookieRetrievingCookieGenerator warnCookieGenerator; | ||
| private final CookieRetrievingCookieGenerator ticketGrantingTicketCookieGenerator; | ||
| private final List<ArgumentExtractor> argumentExtractors; | ||
| private CasConfigurationProperties casProperties; |
There was a problem hiding this comment.
Any reason these variables are removed to not be final?
|
This patch has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
|
This patch has been automatically closed because it has not had recent activity. If you wish to resume work, please re-open the pull request and continue as usual. Thank you for your contributions. |
Re-opening PR from #3138