Add 'unauthorizedRedirectUrl' usable for pac4j module #3138

cifren · 2018-01-15T13:23:13Z

P4j had a crash when trying to use the service option 'unauthorizedRedirectUrl'.

I had to :

manage execption into DelegatedClientAuthenticationAction.java in order to be able to catch an exception event in the webflow
add a new webflow transition catching this event
add a new action handling the new exception and redirect to 'unauthorizedRedirectUrl'
add a 'initialFlowSetupP4jAction', the goal is to setup variables when entering the 'P4jFailure' action. The default initLoginForm action is not working because of the exit of the webflow when communicating with tiers application.

How to test it :

add a new 'cas.authn.pac4j.oidc[0]' client
setup a new access strategy for your service :

 "accessStrategy": {
      "@class" : "org.apereo.cas.services.DefaultRegisteredServiceAccessStrategy",
      "unauthorizedRedirectUrl" : "https://google.com",
      "requiredAttributes" : {
        "@class" : "java.util.HashMap",
        // fake parameter forcing CAS to redirect
        "uids" : [ "java.util.HashSet", [ ".*" ] ]
      }
   }

What should happen :

On OIDC validation, CAS should redirect to google if it doesn't find the 'uids' when logging

What is happening :

CAS crash and display an issue that it doesn't find webflow action to apply on clientLogin webflow action
However works fine on form default login

Where to find the example code used in this modifications :

org.apereo.cas.web.flow.configurer.DefaultWebflowConfigurer
org.apereo.cas.web.flow.GenerateServiceTicketAction

mmoayyed · 2018-01-15T17:28:00Z

Thanks very much for the change. A number of comments:

There is no such thing called P4j. It's Pac4j. Please be consistent.
The initial action that is added seems like it's entirely duplicated code from elsewhere, right?
Please ensure the same patch is posted to the master branch as well.

cifren · 2018-01-16T12:40:24Z

Sorry for the typo on P4j...
There is an initial action for the class initialFlowSetupP4jAction.java, it is initialFlowSetupAction.java, I didn't know if it was working, it needed only the context values. I could improve it and extend the initialAction instead of copy a part of it. Let me know what you think about it.
I will do this when it is done with this part

mmoayyed · 2018-01-16T14:21:42Z

...src/main/java/org/apereo/cas/support/pac4j/web/flow/DelegatedClientAuthenticationAction.java

+                try {
+                    final TicketGrantingTicket tgt = this.centralAuthenticationService.createTicketGrantingTicket(authenticationResult);
+                    WebUtils.putTicketGrantingTicketInScopes(context, tgt);
+                } catch (final PrincipalException e) {


This probably should be catching Exception. Right?

I catch only the exception I need in that case and I redirect it to the proper page, if I catch more, I am not sure the page will represent really the exception. As you wish, tell me and I'll make it.

Cool. Thanks for the note. This is fine as is.

mmoayyed · 2018-01-16T14:22:01Z

...src/main/java/org/apereo/cas/support/pac4j/web/flow/DelegatedClientAuthenticationAction.java

+                    final TicketGrantingTicket tgt = this.centralAuthenticationService.createTicketGrantingTicket(authenticationResult);
+                    WebUtils.putTicketGrantingTicketInScopes(context, tgt);
+                } catch (final PrincipalException e) {
+                    LOGGER.warn("Could not grant service ticket [{}]. Routing to [{}]", e.getMessage(), CasWebflowConstants.TRANSITION_ID_AUTHENTICATION_FAILURE);


The log message here is incorrect. You're not granting service tickets here.

I kind of copy that part, but the message from "RegisteredServiceAccessStrategyUtils.ensurePrincipalAccessIsAllowedForService" is 'LOGGER.warn("Cannot grant access to service [{}] because it is not authorized for use by [{}].", service.getId(), principalId);', It was pretty close for me which comes from the initial error.

I will remove and add a simpler one.

mmoayyed · 2018-01-16T14:22:41Z

...rver-support-pac4j-webflow/src/main/java/org/apereo/cas/web/flow/Pac4jWebflowConfigurer.java

+        final ActionState actionState = createActionState(flow, "P4jFailure",
+                createEvaluateAction(CasWebflowConstants.ACTION_ID_AUTHENTICATION_EXCEPTION_HANDLER));
+
+        actionState.getEntryActionList().add(createEvaluateAction("initialFlowSetupP4jAction"));


The action name must be changed too.

mmoayyed · 2018-01-16T14:22:49Z

...rver-support-pac4j-webflow/src/main/java/org/apereo/cas/web/flow/Pac4jWebflowConfigurer.java

        setStartState(flow, actionState);
    }

+    private void createAuthnFailureAction(final Flow flow){
+        final ActionState actionState = createActionState(flow, "P4jFailure",


The action id must be changed too.

mmoayyed · 2018-01-16T14:23:01Z

...rver-support-pac4j-webflow/src/main/java/org/apereo/cas/web/flow/Pac4jWebflowConfigurer.java

@@ -66,9 +70,23 @@ private void createClientActionActionState(final Flow flow) {
        actionState.getTransitionSet().add(createTransition(CasWebflowConstants.TRANSITION_ID_ERROR, getStartState(flow).getId()));
        actionState.getTransitionSet().add(createTransition(DelegatedClientAuthenticationAction.STOP,
                DelegatedClientAuthenticationAction.STOP_WEBFLOW));
+        createTransitionForState(actionState, CasWebflowConstants.TRANSITION_ID_AUTHENTICATION_FAILURE, "P4jFailure");


The failure event id must be changed too.

mmoayyed · 2018-01-16T14:24:35Z

I could improve it and extend the initialAction instead of copy a part of it. Let me know what you think about it.

Sounds like a better idea to me. Override bits that you need, if any, while delegating much of the work to the InitialAction class.

Thanks for your help.

cifren · 2018-01-17T13:48:20Z

I tried to override, but private method gives a little less flexibility. It means it didn't change that much the class except we know where it comes from. :)

…jInitialFlowSetupAction with InitialFlowSetupAction

mmoayyed · 2018-01-17T15:22:41Z

I tried to override, but private method gives a little less flexibility. It means it didn't change that much the class except we know where it comes from. :)

No. It simply means you need to take the method off of private. Otherwise, there'd be no point in extending the class.

cifren · 2018-01-18T09:36:46Z

Changes have been applied

mmoayyed · 2018-01-19T04:52:38Z

Looks great. Thank you. Please post the same changeset for the master branch

cifren · 2018-01-30T13:01:17Z

I made the changes on master #3172

mmoayyed · 2018-01-31T08:10:47Z

:support:cas-server-support-pac4j-core-clients:javadoc/home/travis/build/apereo/cas/support/cas-server-support-pac4j-webflow/src/main/java/org/apereo/cas/web/flow/config/Pac4jWebflowConfiguration.java:6: error: package org.apereo.cas.util does not exist
import org.apereo.cas.util.CollectionUtils;
                          ^
/home/travis/build/apereo/cas/support/cas-server-support-pac4j-webflow/src/main/java/org/apereo/cas/web/flow/config/Pac4jWebflowConfiguration.java:12: error: cannot find symbol
import org.apereo.cas.web.support.CookieRetrievingCookieGenerator;
                                 ^
  symbol:   class CookieRetrievingCookieGenerator
  location: package org.apereo.cas.web.support
/home/travis/build/apereo/cas/support/cas-server-support-pac4j-webflow/src/main/java/org/apereo/cas/web/flow/config/Pac4jWebflowConfiguration.java:68: error: cannot find symbol
    private CookieRetrievingCookieGenerator ticketGrantingTicketCookieGenerator;
            ^
  symbol:   class CookieRetrievingCookieGenerator
  location: class Pac4jWebflowConfiguration
/home/travis/build/apereo/cas/support/cas-server-support-pac4j-webflow/src/main/java/org/apereo/cas/web/flow/config/Pac4jWebflowConfiguration.java:72: error: cannot find symbol
    private CookieRetrievingCookieGenerator warnCookieGenerator;
            ^
  symbol:   class CookieRetrievingCookieGenerator
  location: class Pac4jWebflowConfiguration
/home/travis/build/apereo/cas/support/cas-server-support-pac4j-webflow/src/main/java/org/apereo/cas/web/flow/Pac4jInitialFlowSetupAction.java:11: error: cannot find symbol
import org.apereo.cas.web.flow.InitialFlowSetupAction;
                              ^
  symbol:   class InitialFlowSetupAction
  location: package org.apereo.cas.web.flow
/home/travis/build/apereo/cas/support/cas-server-support-pac4j-webflow/src/main/java/org/apereo/cas/web/flow/Pac4jInitialFlowSetupAction.java:13: error: cannot find symbol
import org.apereo.cas.web.support.CookieRetrievingCookieGenerator;
                                 ^
  symbol:   class CookieRetrievingCookieGenerator
  location: package org.apereo.cas.web.support
/home/travis/build/apereo/cas/support/cas-server-support-pac4j-webflow/src/main/java/org/apereo/cas/web/flow/Pac4jInitialFlowSetupAction.java:30: error: cannot find symbol
public class Pac4jInitialFlowSetupAction extends InitialFlowSetupAction {
                                                 ^
  symbol: class InitialFlowSetupAction
/home/travis/build/apereo/cas/support/cas-server-support-pac4j-webflow/src/main/java/org/apereo/cas/web/flow/Pac4jInitialFlowSetupAction.java:37: error: cannot find symbol
                                  final CookieRetrievingCookieGenerator ticketGrantingTicketCookieGenerator,
                                        ^
  symbol:   class CookieRetrievingCookieGenerator
  location: class Pac4jInitialFlowSetupAction
/home/travis/build/apereo/cas/support/cas-server-support-pac4j-webflow/src/main/java/org/apereo/cas/web/flow/Pac4jInitialFlowSetupAction.java:38: error: cannot find symbol
                                  final CookieRetrievingCookieGenerator warnCookieGenerator,
                                        ^

:support:cas-server-support-pac4j-webflow:compileJava FAILED
  symbol:   class CookieRetrievingCookieGenerator
  location: class Pac4jInitialFlowSetupAction
9 errors
:autoLintGradle

leleuj · 2018-03-29T13:37:54Z

@cifren what's the status of this PR (and the one for the master)? I guess you just miss some dependencies to make the build pass. Did you test it?

cifren · 2018-03-30T00:05:28Z

I didn't take the time to do it and I went traveling... I'll take care of it in the next days

leleuj · 2018-03-30T08:11:28Z

Excellent! Keep me posted if you need some additional help with testing.

cifren · 2018-03-30T11:39:57Z

Yes tell me how to test it ? How should I do ?

On my part, I use the gradle overlay, I test all my files by putting them into src. After that I have copied one by one those files into the repository apereo/cas, for me everything was good. I suppose that is not the way I should process... Must I generate a war from that modified apereo/cas and copy it over the overlay ?

leleuj · 2018-03-30T14:07:35Z

It depends a bit on the changes: for limited changes, I generally copy/paste the CAS classes in my overlay to change them and test things quickly. Then, I copy the changes into the CAS sources. It seems it's what you do.

Or for more global changes, I make the changes in the CAS source code, rebuild and install only the necessary modules, rebuild my overlay (based on the related SNAPSHOT) and test the changes.

cifren · 2018-03-31T08:37:59Z

If I only copy the files how can I test like @mmoayyed does ?

How would you "rebuild and install only the necessary modules" in CAS project ?

cifren · 2018-04-01T03:58:46Z

Ok never mind the last question, I cloned CAS project and I ran "./gradlew -q :support:cas-server-support-pac4j-authentication:build" and gave me the jar

Is it working now ?

leleuj · 2018-04-02T08:40:50Z

By "rebuild and install", I mean "./gradlew clean build install -p module", with "-p" to just build a module and not the whole project which takes too much time.

So here is the process for big changes (like this one):

rebuild the CAS source, only the necessary modules
rebuild the CAS overlay pointing to the SNAPSHOT source version
run the CAS overlay and test.

Is it clear now?

cifren · 2018-04-03T00:34:41Z

Yes thank you very much for this explaination, the solution I found kind of did the same. I could not make the overlay work with my solution, but it should be ok, the compilation worked.

mmoayyed · 2018-04-03T09:49:46Z

Also see https://apereo.github.io/2018/04/05/cas-codebase-feature-build/

leleuj · 2018-04-04T12:41:42Z

Your test should go further than just "the compilation worked", but I guess you test the change in some CAS overlay, don't you?

cifren · 2018-05-02T07:30:42Z

Sorry, it took me a while to get time to work on it.

I made it perfect (as much as I could)

I ran the checkstyle, javadoc and created a jar that I sent to an overlay and all is working fine !!

Is it good on your part ?

leleuj · 2018-05-02T14:39:11Z

This looks good to me.

@mmoayyed is this ok for you or do you want me to do an additional test?

@cifren are the changes for the master branch ready as well?

mmoayyed · 2018-05-02T14:40:49Z

re-open the pull request please, merge with 5.2.x and get the build to pass. repeat the same exact exercise for master. Happy to merge then. (We should try to merge both at the same time to avoid feature imbalance)

mmoayyed · 2018-05-02T14:56:42Z

@leleuj speaking of which btw, a couple of oauth-related tests are failing in 5.2.x as a result of the last change done to scope redirect urls to codes, etc. Would you mind taking a look, and fixing those?

leleuj · 2018-05-02T16:48:46Z

@mmoayyed Sure, I'll do that on Friday. I'll certainly cut the pac4j v3 release as well, I know you were expecting it very soon...

mmoayyed · 2018-05-02T16:51:10Z

Excellent. We have been banging on RC1 for a while with no issues yet and have several projects running on CAS 5.3 RC4 which includes that as well. Thanks for the notice. Look forward to the final release.

I might selfishly ask that you cut an RC2 first so that we can test any upcoming changes and then align the final release of pac4j with CAS 5.3. But, of course, do whatever makes better sense.

cifren · 2018-05-02T22:14:46Z

How to re-open the pull request ? Should I create a new one ?

I did a git rebase 5.2.x is it good ?

leleuj · 2018-05-03T08:33:51Z

@mmoayyed I have no issue to cut a RC2. We have had a few changes on SAML, Twitter... I can release the RC2 tomorrow: is this ok for you?

@cifren you should have a "Reopen and comment" button at the bottom of the page...

mmoayyed · 2018-05-03T08:35:50Z

Yes, that is perfect. Thank you.

cifren · 2018-05-03T09:24:33Z

I dont have the "Reopen and comment" button, I am not sure why

mmoayyed · 2018-05-03T09:27:17Z

You may want to start a new pull request. It looks the underlying branch was either force-pushed or recreated somehow.

cifren · 2018-05-03T09:28:14Z

See #3293

Add 'unauthorizedRedirectUrl' usable for p4j module

db8dbcb

cifren changed the title ~~Add 'unauthorizedRedirectUrl' usable for p4j module~~ Add 'unauthorizedRedirectUrl' usable for pac4j module Jan 16, 2018

Apply consistence on naming Pac4j

026aecf

mmoayyed requested changes Jan 16, 2018

View reviewed changes

mmoayyed added the Pending label Jan 16, 2018

mmoayyed added this to the 5.2.2 milestone Jan 16, 2018

mmoayyed added Authorization & Access Strategy Delegated Authentication Pac4J Bug labels Jan 16, 2018

Changed the message log + P4j by Pac4j for consistency + extends Pac4…

180bca5

…jInitialFlowSetupAction with InitialFlowSetupAction

take the method off of private in InitialFlowSetupAction

622d48d

cifren mentioned this pull request Jan 19, 2018

Add 'unauthorizedRedirectUrl' usable for pac4j module #3148

Closed

mmoayyed modified the milestones: 5.2.2, 5.2.3 Jan 26, 2018

cifren mentioned this pull request Jan 30, 2018

Add 'unauthorizedRedirectUrl' usable for pac4j module for master branch #3172

Closed

mmoayyed removed this from the 5.2.3 milestone Mar 3, 2018

Add depedencies for pac4j-webflow

97d85c6

mmoayyed added Pending and removed Pending labels Apr 11, 2018

stale bot closed this Apr 18, 2018

cifren mentioned this pull request May 3, 2018

Add 'unauthorizedRedirectUrl' usable for pac4j module #3293

Closed

Add 'unauthorizedRedirectUrl' usable for pac4j module #3138

Add 'unauthorizedRedirectUrl' usable for pac4j module #3138

Conversation

cifren commented Jan 15, 2018

mmoayyed commented Jan 15, 2018

cifren commented Jan 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmoayyed commented Jan 16, 2018

cifren commented Jan 17, 2018

mmoayyed commented Jan 17, 2018

cifren commented Jan 18, 2018

mmoayyed commented Jan 19, 2018

cifren commented Jan 30, 2018

mmoayyed commented Jan 31, 2018

leleuj commented Mar 29, 2018

cifren commented Mar 30, 2018

leleuj commented Mar 30, 2018

cifren commented Mar 30, 2018

leleuj commented Mar 30, 2018

cifren commented Mar 31, 2018

cifren commented Apr 1, 2018

leleuj commented Apr 2, 2018

cifren commented Apr 3, 2018 • edited

mmoayyed commented Apr 3, 2018

leleuj commented Apr 4, 2018

cifren commented May 2, 2018 • edited

leleuj commented May 2, 2018

mmoayyed commented May 2, 2018

mmoayyed commented May 2, 2018

leleuj commented May 2, 2018

mmoayyed commented May 2, 2018

cifren commented May 2, 2018

leleuj commented May 3, 2018

mmoayyed commented May 3, 2018

cifren commented May 3, 2018

mmoayyed commented May 3, 2018

cifren commented May 3, 2018

cifren commented Apr 3, 2018 •

edited

cifren commented May 2, 2018 •

edited