Refactor authentication code #2027

zaralouis-sf · 2022-03-07T23:37:29Z

Description

See this doc for more context. Refactor civiform authentication code to make it more extensible for the future. This refactor replaces the various auth client annotations with an AdminAuthClient and ApplicantAuthClient. Those clients implement IndirectClient. We bind those clients to their corresponding client providers in SecurityModule. Currently, the AdminAuthClient is always bound to the AdOidcClientProvider, but in the future this can be easily modified as we onboard more authentication IDPs. The ApplicantAuthClient is bound based on the auth.admin_idp field found in the config.

bion · 2022-03-09T01:54:36Z

Before this merges, it'd be good to build and push it to Seattle staging to test that IDCS and ADFS integration are still working. @gwendolyngoetz would you be up for trying that?

Steps are:

checkout this branch
build a production image with the staging tag:

docker build -f prod.Dockerfile \
  -t public.ecr.aws/t1q6b4h2/universal-application-tool:latest \
  --cache-from civiform/civiform:latest \
  --build-arg BUILDKIT_INLINE_CACHE=1 \
  .

tell ecs to deploy:

REGION=us-west-2

ECSSERVICE=$(aws cloudformation describe-stacks \
    --region ${REGION} \
    --stack-name civiform-staging |
      jq -r '.Stacks[0].Outputs[] | select(.OutputKey == "ECSService") | .OutputValue')

ECSCLUSTER=$(aws cloudformation describe-stacks \
    --region ${REGION} \
    --stack-name civiform-staging |
      jq -r '.Stacks[0].Outputs[] | select(.OutputKey == "ECSCluster") | .OutputValue')

aws ecs update-service \
  --region=${REGION} \
  --cluster "$ECSCLUSTER" \
  --service "$ECSSERVICE" \
  --force-new-deployment

wait for it to finish

aws ecs wait services-stable --region=${REGION} --cluster "$ECSCLUSTER" --services "$ECSSERVICE"

Once it's deployed, trying logging in as an applicant and as an admin using ADFS and IDCS (not the debug buttons). This would go a long way toward ensuring this change doesn't cause auth issues for Seattle that would require reverting it.

gwendolyngoetz · 2022-03-09T04:00:16Z

@bion Don't I need to do a docker push on the locally built container?

bion · 2022-03-09T08:22:32Z

@gwendolyngoetz ah yep, after building it docker push public.ecr.aws/t1q6b4h2/universal-application-tool:latest

gwendolyngoetz · 2022-03-09T19:15:19Z

I followed the steps, adding the docker push between steps 2 and 3. I am able to log in via both ADFS and IDCS still. Nice work team!

zaralouis-sf · 2022-03-09T19:30:54Z

Thank you so much for your help @gwendolyngoetz!! This is a much needed sanity check for me

shubha-rajan · 2022-03-09T21:26:01Z

universal-application-tool-0.0.1/app/modules/SecurityModule.java

-  protected GuestClient guestClient(ProfileFactory profileFactory) {
-    return new GuestClient(profileFactory);
+    // Default to these options
+    String applicantAuthClient = "adfs";


question (blocking): shouldn't the applicant auth client default to IDCS?

Ah yes thank you for catching that!

shubha-rajan · 2022-03-09T21:27:15Z

universal-application-tool-0.0.1/conf/routes

+GET     /adminLogin                  controllers.LoginController.adminLogin(request: Request)
+GET     /applicantLogin              controllers.LoginController.applicantLogin(request: Request, redirectTo: java.util.Optional[String])


praise: these routes look so much cleaner, I love it!!!

shubha-rajan · 2022-03-09T21:34:10Z

universal-application-tool-0.0.1/app/modules/SecurityModule.java

 import org.pac4j.play.CallbackController;
 import org.pac4j.play.LogoutController;
 import org.pac4j.play.http.PlayHttpActionAdapter;
 import org.pac4j.play.store.PlayCookieSessionStore;
 import org.pac4j.play.store.ShiroAesDataEncrypter;
-import org.pac4j.saml.client.SAML2Client;
-import org.pac4j.saml.config.SAML2Configuration;
+import org.springframework.lang.Nullable;


question (blocking): did you mean to switch from using javax.annotation.Nullable to org.springframework.lang.Nullable?

No I definitely did not thank you!

shubha-rajan · 2022-03-09T21:35:52Z

universal-application-tool-0.0.1/app/auth/oidc/IdcsOidcProvider.java

+      ProfileFactory profileFactory,
+      Provider<UserRepository> applicantRepositoryProvider) {
+    this.configuration = checkNotNull(configuration);
+    this.baseUrl = configuration.getString("base_url");


question (blocking): are we validating that base_url is set anywhere?

I realized we actually don't, anywhere in the code base. How do you think I should handle it-- should we throw an exception in these provider classes if it's not set? Would it make more sense to throw an exception from somewhere else? I actually think we have a default setting for it in application.conf so maybe we don't need to check it? Curious your thoughts

It's defaulted to localhost:9000 right? my worry here is that someone deploying to a staging or production instance might forget to set this value, and then the authentication won't work because the callback is wrong. @bion @shanemc-goog what are your thoughts on this?

Given this is pre-existing code that's being moved, I'd say it's a Tech Debt task, so file a bug I guess. Do you see any new concerns Shubha, or a clear idea of how to handle it better?

I don't understand the base url or its setting concerns enough to say what's correct.

shubha-rajan · 2022-03-09T21:40:21Z

universal-application-tool-0.0.1/app/auth/saml/LoginRadiusSamlProvider.java

+  private final String baseUrl;
+
+  @Inject
+  public LoginRadiusSamlProvider(


praise: great work writing these provider classes! they look so clean and it's nice to have the logic specific to each IDP separated out into its own file.

universal-application-tool-0.0.1/app/auth/AdminAuthClient.java

universal-application-tool-0.0.1/app/auth/ApplicantAuthClient.java

universal-application-tool-0.0.1/app/auth/oidc/AdOidcProvider.java

shanemc-goog · 2022-03-10T17:49:56Z

universal-application-tool-0.0.1/app/controllers/LoginController.java

    }
-    if (Strings.isNullOrEmpty(registerUrl)) {
-      return badRequest("Registration is not enabled.");
+    // This register behavior is specific to IDCS. Because this is only being called when we know


suggestion: This comment seems to be a verbose reading of the "if(X) { return; Y} return Z;" behavior. It doesn't seem to provide anything else that I can't read from the code. Is there something deeper to call out here? EG Why are we doing something different for IDCS etc

I'm not super clear on why we only need it for IDCS, I just know that we don't for LoginRadius. I think it's just how different IDPs work

Suggestion:

restructure/rephrase this similar to the following to return early in the common case, and draw out the exception more.

boolean isIDCS = idp.equals(AuthIdentityProviderName.IDCS_APPLICANT.toString());

if(!isIDCS) {
return login(request, applicantClient);
}

// IDCS requires a special registration flow.
.......

This might also indicate that the IDCS code blob should be a method that is called here to compartmentalize things.

I restructured and tagged you in the comment, let me know what you think

universal-application-tool-0.0.1/app/modules/SecurityModule.java

shanemc-goog · 2022-03-10T18:09:28Z

universal-application-tool-0.0.1/app/modules/SecurityModule.java

+            .toProvider(LoginRadiusSamlProvider.class);
+        break;
+      case IDCS_APPLICANT:
+      default:


Similar to the other comment, I don't know that we should have a default, all options should be equal I think, regardless of the existing state.

Are you concerned that some of these values won't be set in the existing Seattle deployment?

Yes, I feel a lot better if the code defaults to the existing prod options because I'd like to remove the possibility of breaking prod as much as possible. This is what we did for file upload as well, and it felt a lot safer to me.

shanemc-goog · 2022-03-10T18:22:38Z

universal-application-tool-0.0.1/conf/routes

@@ -139,11 +139,9 @@ POST    /callback/:client_name       controllers.CallbackController.callback(req

 # Log into application
 GET     /loginForm                   controllers.HomeController.loginForm(request: Request, message: java.util.Optional[String])
-GET     /idcsLogin                   controllers.LoginController.idcsLoginWithRedirect(request: Request, redirectTo: java.util.Optional[String])


We need to be careful with removing existing routes, often times things like this need to be maintained for a release cycle as through the release different versions of the server may be available, as well if a route is effectively loaded into a users browser before a release, they may try to access it after. That seems like a concern here correct?

This isn't something we have a good plan on afaik.

Yes, that is definitely a concern. Should I leave the former routes in this file for now and then have a TODO to clean them up in the future?

Yeah, if that works with your PR leaving them and cleaning up later is safest.

So, looking at it now, I am realizing that in order to leave the routes, I'll also have to leave the deprecated code in LoginController. Is that something I should do? Wondering if we should discuss this specific code in our next daily sync.

I would have to leave all the methods in LoginController that are referenced by the old routes for the code not to break. Sorry, not sure if my above comment made total sense.

Is it possible to update the existing specific routes to the new controllers and have it still work?

zaralouis-sf · 2022-03-11T00:03:06Z

universal-application-tool-0.0.1/app/modules/SecurityModule.java

+    try {
+      applicantAuthClient = configuration.getString("auth.applicant_idp");
+    } catch (ConfigurationException ignore) {
+      // Default to IDCS.


Just fyi, I had to bring this comment back because ErrorProne was complaining that "this exception should not be ignored" when I had a completely empty catch block

Ah, looks like this is in internal/old exception that the public tool doesn't use.

google/error-prone#1654

shanemc-goog · 2022-03-14T14:59:52Z

universal-application-tool-0.0.1/app/auth/AdminAuthClient.java

@@ -6,7 +6,7 @@

 /**
 * AdminAuthClient is the annotation for the auth client responsible for admin authentication. This
- * client will implement IndirectClient.
+ * client must implement IndirectClient -> {@link org.pac4j.core.client.IndirectClient}.


In javadoc you don't need "IndirectClient -> " It'll link the term in the javadoc viewer. Though TBF I don't know where that is in GH. :)

Sorry I am a javadoc novice I guess! How does this look?

shanemc-goog · 2022-03-14T15:04:35Z

universal-application-tool-0.0.1/app/auth/saml/LoginRadiusSamlProvider.java

+              configuration.getString("login_radius.api_key"),
+              configuration.getString("login_radius.saml_app_name"));
+      return Optional.of(metadataResourceUrl);
+    } catch (IllegalFormatException | NullPointerException e) {


getString returns two other exceptions wrt to the value being in an unexpected state, we need those also right?

https://lightbend.github.io/config/latest/api/com/typesafe/config/Config.html#getString-java.lang.String-

Yes good catch! Should be fixed.

universal-application-tool-0.0.1/app/auth/saml/LoginRadiusSamlProvider.java

shanemc-goog · 2022-03-14T15:15:16Z

universal-application-tool-0.0.1/app/modules/SecurityModule.java

+    try {
+      applicantAuthClient = configuration.getString("auth.applicant_idp");
+    } catch (ConfigurationException ignore) {
+      // Default to IDCS.


Ah, looks like this is in internal/old exception that the public tool doesn't use.

google/error-prone#1654

shanemc-goog · 2022-03-14T15:16:02Z

universal-application-tool-0.0.1/app/controllers/LoginController.java

    }
-    if (Strings.isNullOrEmpty(registerUrl)) {
-      return badRequest("Registration is not enabled.");
+    // This register behavior is specific to IDCS. Because this is only being called when we know


Suggestion:

restructure/rephrase this similar to the following to return early in the common case, and draw out the exception more.

boolean isIDCS = idp.equals(AuthIdentityProviderName.IDCS_APPLICANT.toString());

if(!isIDCS) {
return login(request, applicantClient);
}

// IDCS requires a special registration flow.
.......

This might also indicate that the IDCS code blob should be a method that is called here to compartmentalize things.

zaralouis-sf · 2022-03-18T18:04:28Z

universal-application-tool-0.0.1/app/controllers/LoginController.java

-    return login(request, loginRadiusClient)
-        .addingToSession(request, REDIRECT_TO_SESSION_KEY, redirectTo.get());
+
+    return idcsRegister(request);


@shanemc-goog do these changes look like what you had in mind?

Per the team discussion I believe you'll have the existing url routes internally use your new controller.

Should be fixed, let me know what you think

shanemc-goog · 2022-03-22T22:26:34Z

universal-application-tool-0.0.1/app/controllers/LegacyRoutesController.java

@@ -11,14 +13,14 @@
 public class LegacyRoutesController {

  public Result idcsLoginWithRedirect(Http.Request request, Optional<String> redirectTo) {


Should the request be used in the body? I see it's dropped here. Is the intent to always redirect to the new handler rather than handle the old route through new refactored flow?

If I understand correctly, I think the redirect method used automatically passes in the request context. Here's the docs on it: https://www.playframework.com/documentation/2.8.x/JavaRouting#Reverse-routing

Yes Shubha that was my understanding as well! Thanks for the docs! I can add them as a comment if you think it would be helpful Shane?

Great! thanks yall!

zaralouis-sf force-pushed the auth-branch branch 2 times, most recently from 0180778 to 6009ccb Compare March 8, 2022 22:59

zaralouis-sf requested review from shubha-rajan and shanemc-goog March 9, 2022 00:10

zaralouis-sf marked this pull request as ready for review March 9, 2022 01:32

zaralouis-sf requested review from gwendolyngoetz and goldblatt March 9, 2022 19:45

shubha-rajan suggested changes Mar 9, 2022

View reviewed changes

zaralouis-sf force-pushed the auth-branch branch from 6009ccb to f50d86b Compare March 9, 2022 22:25

zaralouis-sf requested a review from shubha-rajan March 9, 2022 22:32

shanemc-goog suggested changes Mar 10, 2022

View reviewed changes

zaralouis-sf commented Mar 11, 2022

View reviewed changes

zaralouis-sf requested a review from shanemc-goog March 11, 2022 00:03

shanemc-goog suggested changes Mar 14, 2022

View reviewed changes

zaralouis-sf force-pushed the auth-branch branch from 08c6a00 to e9ba70d Compare March 18, 2022 18:00

zaralouis-sf commented Mar 18, 2022

View reviewed changes

zaralouis-sf requested a review from shanemc-goog March 18, 2022 18:05

zaralouis-sf added 8 commits March 22, 2022 09:21

Begin refactoring authentication code

8823d19

Refactoring auth code

ad591ea

formatting fixes

4e9da49

fix security module

9df693a

Fix broken browser test

a715e11

Try to fix injection

8b5a51b

Removing unnecessary annotations

093bae1

Change bound client type

475b102

zaralouis-sf added 12 commits March 22, 2022 09:21

Revrese changes

cd7161c

Fix one line

393fb42

Add nullable annotation

d1e2446

Default config options

d7ea60b

universal-application-tool-0.0.1/app/controllers/LoginController.java

9ab3473

Formatting

57d1901

Add null checks

5c6b52a

Responding to PR comments

faf128c

Responding to PR comments

9a6c4b1

Respond to more PR comments

478cdb5

Re add comment

f3457f9

Responding to more PR comments

704187b

zaralouis-sf force-pushed the auth-branch branch from e9ba70d to 704187b Compare March 22, 2022 16:22

zaralouis-sf added 2 commits March 22, 2022 10:06

Add legacy routes handler

10e68e1

Update LegacyRoutesController

34ff231

shanemc-goog reviewed Mar 22, 2022

View reviewed changes

shubha-rajan approved these changes Mar 24, 2022

View reviewed changes

shanemc-goog approved these changes Mar 25, 2022

View reviewed changes

zaralouis-sf merged commit 30cb93d into main Mar 25, 2022

zaralouis-sf deleted the auth-branch branch March 25, 2022 16:58

		GET /adminLogin controllers.LoginController.adminLogin(request: Request)
		GET /applicantLogin controllers.LoginController.applicantLogin(request: Request, redirectTo: java.util.Optional[String])

		@@ -11,14 +13,14 @@
		public class LegacyRoutesController {

		public Result idcsLoginWithRedirect(Http.Request request, Optional<String> redirectTo) {

Refactor authentication code #2027

Refactor authentication code #2027

Conversation

zaralouis-sf commented Mar 7, 2022 • edited

Description

bion commented Mar 9, 2022 • edited

gwendolyngoetz commented Mar 9, 2022

bion commented Mar 9, 2022

gwendolyngoetz commented Mar 9, 2022 • edited

zaralouis-sf commented Mar 9, 2022

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

Choose a reason for hiding this comment

shanemc-goog Mar 14, 2022 • edited

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

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

shanemc-goog Mar 14, 2022 • edited

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

zaralouis-sf commented Mar 7, 2022 •

edited

bion commented Mar 9, 2022 •

edited

gwendolyngoetz commented Mar 9, 2022 •

edited

shanemc-goog Mar 14, 2022 •

edited

shanemc-goog Mar 14, 2022 •

edited