-
Notifications
You must be signed in to change notification settings - Fork 56
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
Refactor authentication code #2027
Conversation
0180778
to
6009ccb
Compare
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:
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. |
@bion Don't I need to do a docker push on the locally built container? |
@gwendolyngoetz ah yep, after building it |
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! |
Thank you so much for your help @gwendolyngoetz!! This is a much needed sanity check for me |
protected GuestClient guestClient(ProfileFactory profileFactory) { | ||
return new GuestClient(profileFactory); | ||
// Default to these options | ||
String applicantAuthClient = "adfs"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question (blocking): shouldn't the applicant auth client default to IDCS?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yes thank you for catching that!
GET /adminLogin controllers.LoginController.adminLogin(request: Request) | ||
GET /applicantLogin controllers.LoginController.applicantLogin(request: Request, redirectTo: java.util.Optional[String]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
praise: these routes look so much cleaner, I love it!!!
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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question (blocking): did you mean to switch from using javax.annotation.Nullable
to org.springframework.lang.Nullable
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No I definitely did not thank you!
ProfileFactory profileFactory, | ||
Provider<UserRepository> applicantRepositoryProvider) { | ||
this.configuration = checkNotNull(configuration); | ||
this.baseUrl = configuration.getString("base_url"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question (blocking): are we validating that base_url
is set anywhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
private final String baseUrl; | ||
|
||
@Inject | ||
public LoginRadiusSamlProvider( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
6009ccb
to
f50d86b
Compare
universal-application-tool-0.0.1/app/auth/ApplicantAuthClient.java
Outdated
Show resolved
Hide resolved
universal-application-tool-0.0.1/app/auth/oidc/AdOidcProvider.java
Outdated
Show resolved
Hide resolved
universal-application-tool-0.0.1/app/auth/oidc/AdOidcProvider.java
Outdated
Show resolved
Hide resolved
} | ||
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I restructured and tagged you in the comment, let me know what you think
universal-application-tool-0.0.1/app/modules/SecurityModule.java
Outdated
Show resolved
Hide resolved
universal-application-tool-0.0.1/app/modules/SecurityModule.java
Outdated
Show resolved
Hide resolved
.toProvider(LoginRadiusSamlProvider.class); | ||
break; | ||
case IDCS_APPLICANT: | ||
default: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -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]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, if that works with your PR leaving them and cleaning up later is safest.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to update the existing specific routes to the new controllers and have it still work?
try { | ||
applicantAuthClient = configuration.getString("auth.applicant_idp"); | ||
} catch (ConfigurationException ignore) { | ||
// Default to IDCS. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, looks like this is in internal/old exception that the public tool doesn't use.
@@ -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}. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I am a javadoc novice I guess! How does this look?
configuration.getString("login_radius.api_key"), | ||
configuration.getString("login_radius.saml_app_name")); | ||
return Optional.of(metadataResourceUrl); | ||
} catch (IllegalFormatException | NullPointerException e) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
getString returns two other exceptions wrt to the value being in an unexpected state, we need those also right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes good catch! Should be fixed.
universal-application-tool-0.0.1/app/auth/saml/LoginRadiusSamlProvider.java
Outdated
Show resolved
Hide resolved
try { | ||
applicantAuthClient = configuration.getString("auth.applicant_idp"); | ||
} catch (ConfigurationException ignore) { | ||
// Default to IDCS. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, looks like this is in internal/old exception that the public tool doesn't use.
} | ||
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
08c6a00
to
e9ba70d
Compare
return login(request, loginRadiusClient) | ||
.addingToSession(request, REDIRECT_TO_SESSION_KEY, redirectTo.get()); | ||
|
||
return idcsRegister(request); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@shanemc-goog do these changes look like what you had in mind?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Per the team discussion I believe you'll have the existing url routes internally use your new controller.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be fixed, let me know what you think
e9ba70d
to
704187b
Compare
@@ -11,14 +13,14 @@ | |||
public class LegacyRoutesController { | |||
|
|||
public Result idcsLoginWithRedirect(Http.Request request, Optional<String> redirectTo) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great! thanks yall!
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.