Handle invalid OIDC requests before persisting by zachmargolis · Pull Request #1494 · 18F/identity-idp

zachmargolis · 2017-06-19T15:07:35Z

Why: Saving invalid forms to the database causes errors

monfresh · 2017-06-19T15:41:12Z

app/controllers/openid_connect/authorization_controller.rb

Rename this to validate_oidc_request, to make it consistent with validate_saml_request? The OIDC request needs to be validated before store_request can be called, so I think this more accurately describes what's happening.

Updated in 5608e6d1c622cc432d7bf2405ba29b1223756b9e to validate_authorize_form

monfresh · 2017-06-19T15:42:48Z

app/controllers/openid_connect/authorization_controller.rb

Create a new analytics event for this, to separate the validation from the actual authorization? That way, we can differentiate between users who made the request, hence triggering the validation, but did not actually sign in.

To me, it's the same event, they attempted and had a bad form so we can't complete the request.

monfresh · 2017-06-19T15:56:18Z

app/forms/openid_connect_authorize_form.rb

Rename to validate? Also, since we are now validating earlier in the process, what do you think about simplifying #submit to this:

link_identity_to_client_id(user, rails_session_id) FormResponse.new(success: true, errors: {}, extra: extra_analytics_attributes)

I wouldn't trust a method like that, because we'd already have to know ahead of time before submitting the request was valid, I think it puts too much responsibility on the caller.

Can I push the change that I'm thinking of? I verified that all tests pass with my proposed changes. Maybe that will better explain what I have in mind. Basically, I don't think we need to be validating the request twice. By validating earlier in the process, we know that once it reaches the index action, it is valid. If it is not valid, it will never reach the index action.

This means we don't need a new check_submit method. We only need to modify the existing submit method to only validate and return the FormResponse, and link the identity in the index action once the user has signed in.

This is consistent with what we do with SAML: First we validate the SAML request, and if it's valid, then we proceed to the auth action, and once the user is fully authenticated, we link the identity: https://github.com/18F/identity-idp/blob/master/app/controllers/saml_idp_controller.rb#L41-L50.

This also reminds me we need to update the OIDC controller (in a separate PR) to match the latest changes in SAML for the scenario where a user needs to finish verifying their profile (when they are waiting to confirm their mailing address for example). Eventually, it would be great if we could abstract out these controllers so we don't have to remember to keep them in sync.

monfresh · 2017-06-19T15:58:32Z

app/controllers/openid_connect/authorization_controller.rb

Remove this line, so that the request is always validated? For example, what if the user is already signed in, but then makes a bad OIDC request? Should we add a test for that scenario to make sure it doesn't blow up?

There are already existing specs for the case where a user is logged in but provides a bad request

The problem I was trying to fix was behavior and analytics specifically for logged out users (because our processes for persisting data and redirecting was too fragile) so I wanted to skip that entirely if we know ahead of time that the requests will fail.

I ended up removing it in 5608e6d1c622cc432d7bf2405ba29b1223756b9e

monfresh · 2017-06-19T16:09:28Z

spec/controllers/openid_connect/authorization_controller_spec.rb

What is the reason for redirecting in this scenario? Shouldn't all invalid requests be treated the same? Also, what is the user experience here (in general, not this particular test)? If bad requests are (usually) the fault of the SP, does it make sense to display an error page to the user for something they did not cause? Does the error page have a helpful message explaining what happened and what they can do?

Some requests are "good enough" that we can redirect and provide an error to the client, so that they can handle the error (good enough in that the client_id and redirect_uri match each other)

We display errors when the requests are not good enough to redirect

monfresh · 2017-06-20T13:40:41Z

app/controllers/openid_connect/authorization_controller.rb

Should this line come before the return on line 76? Otherwise, it will only track the failure scenario.

The success scenario will be tracked when it goes through the #index method

monfresh · 2017-06-20T20:13:49Z

Here are my changes: ce0e9e7

Let me know what you think.

Bonus points: It fixes the ABC complexity Rubocop offense in index.

zachmargolis

@monfresh thanks for the changes! Now I see what you are talking about, and it definitely makes sense, I really like the extracted link_identity_to_client_id as a way to simplify submit/check_submit.

Thanks! 💯

zachmargolis · 2017-06-21T13:37:49Z

spec/forms/openid_connect_authorize_form_spec.rb

this should have no redirect_uri key at all right? not just a nil value?

It should have a key. The redirect_uri method didn't change. The value depends on success. If it's true (i.e the request was valid), then the redirect_uri value is the return value of success_redirect_uri (which is nil in this case because the identity hasn't been linked yet), otherwise of error_redirect_uri.

The redirect_uri key is what is used by the controller to determine whether to redirect or render the error page.

Ah right! I forgot about that part, thanks!

zachmargolis · 2017-06-21T13:38:40Z

spec/forms/openid_connect_authorize_form_spec.rb

nit: two spaces after not instead of 1

Thanks. Will fix.

zachmargolis · 2017-06-21T13:39:40Z

app/forms/openid_connect_authorize_form.rb

What do you think of calling this link_identity_to_client or link_identity_to_service_provider?

I like the latter. I'll rename.

Ooh or actually the identity itself is a link between a user and service provider.

So maybe: link_user_to_service_provider ?

I think what we have now works. It's the Identity model that's being created/updated, and we call a class called IdentityLinker.

Sounds good to me!

zachmargolis · 2017-06-21T14:28:19Z

(I can't approve this PR, or merge it without overriding first?) I'll squash my commits together and then go ahead and approve + merge if you're happy with the changes?

**Why**: Saving invalid forms to the database causes errors

monfresh · 2017-06-21T17:47:07Z

I approved the PR and addressed your feedback here: f12b051

monfresh

Good to go!

zachmargolis · 2017-06-21T18:19:50Z

ok! thanks!! I'll squash your last commits together and then merge this

**Why**: To avoid validating the request twice. By validating earlier in the process (with `validate_authorize_form`), we know that once it reaches the `index` action, it is valid. If it is not valid, it will never reach the `index` action. This means we don't need to validate the request both in the form's `check_submit` and `submit` methods. We can remove the `check_submit` method and modify the existing `submit` method to only validate and return the FormResponse, and link the identity in the `index` action once the user has signed in. This is consistent with what we do with SAML: First we validate the SAML request, and if it's valid, then we proceed to the auth action, and once the user is fully authenticated, we link the identity. Bonus points: This fixes the ABC complexity offense of the `index` method.

zachmargolis requested a review from monfresh June 19, 2017 15:07

zachmargolis self-assigned this Jun 19, 2017

zachmargolis requested a review from amoose June 19, 2017 15:08

zachmargolis added the status - ready for review label Jun 19, 2017

monfresh reviewed Jun 19, 2017

View reviewed changes

monfresh reviewed Jun 20, 2017

View reviewed changes

monfresh force-pushed the margolis-openid-params-bugs branch 2 times, most recently from de01621 to ce0e9e7 Compare June 20, 2017 20:12

zachmargolis commented Jun 21, 2017

View reviewed changes

zachmargolis force-pushed the margolis-openid-params-bugs branch from ce0e9e7 to a1a4079 Compare June 21, 2017 14:29

Handle invalid OIDC requests before persisting

7442716

**Why**: Saving invalid forms to the database causes errors

monfresh approved these changes Jun 21, 2017

View reviewed changes

zachmargolis force-pushed the margolis-openid-params-bugs branch from f12b051 to ccbbd8e Compare June 21, 2017 18:20

zachmargolis merged commit 68e307a into master Jun 21, 2017

zachmargolis deleted the margolis-openid-params-bugs branch June 21, 2017 18:43

Conversation

zachmargolis commented Jun 19, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

monfresh commented Jun 20, 2017

Uh oh!

zachmargolis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zachmargolis commented Jun 21, 2017

Uh oh!

monfresh commented Jun 21, 2017

Uh oh!

monfresh left a comment

Choose a reason for hiding this comment

Uh oh!

zachmargolis commented Jun 21, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants