Skip to content

Handle incoming OpenID RISC Events (LG-3119)#3967

Merged
zachmargolis merged 17 commits intomasterfrom
margolis-incoming-risc-events
Jul 23, 2020
Merged

Handle incoming OpenID RISC Events (LG-3119)#3967
zachmargolis merged 17 commits intomasterfrom
margolis-incoming-risc-events

Conversation

@zachmargolis
Copy link
Contributor

This is a first PR to support the new RISC (Risk and Incident Sharing and Coordination) framework outlined by the OpenID folks: https://openid.net/specs/openid-risc-profile-1_0-ID1.html

There were a bunch of docs I jumped between to pull this together, I tried to link out to docs where I figured things might be unclear

This first stab creates an endpoint where issuers can submit events to us, and events that are validated get written to our DB

Open future questions:

  • Is the DB a good place to store these? If so we probably need a job to purge old events, etc etc
  • How we are going to act on these, good topic for a future PR

There are a few places the specs were "TODO" or unclear, so I'll drop comments where I took some guesses

@zachmargolis zachmargolis requested review from achapm, adunkman, aduth, jmhooper, mitchellhenke and stevegsa and removed request for adunkman July 22, 2020 19:19
Comment on lines +14 to +19
delivery: [
{
delivery_method: DELIVERY_METHOD_PUSH,
url: api_security_events_url,
},
]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding a delivery clause here was something I kind of made up...

The spec allows for dynamic registration of various endpoints, and the responses there contain sections like this: https://openid.net/specs/openid-risc-profile-1_0-ID1.html#rfc.section.4.1.2.1

The section on HTTP Push (which we support) describes these parts, so I just figured they seemed like a good thing to add because the HTTP Push Doc doesn't seem to include anything about discovery, just how the endpoint should act

t.string :issuer
t.timestamps
end
add_index :security_events, ["jti"], name: "index_security_events_on_jti"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this was a guess that we might want it in the future, in case we ever want to find an event that somebody gave us?

Using t.references gives us an index on user_id so that access pattern is already taken care of

module Risc
class ConfigurationController < ApplicationController
def index
render json: RiscConfigurationPresenter.new.configuration
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I added the OpenID discovery endpoint, I added caching headers because I'm pretty sure some specification required it....but I can't find it and didn't see any caching direction on the specs for this. I left out caching, but it's a one-liner so happy to add it if anybody feels strongly

Copy link
Contributor

@mitchellhenke mitchellhenke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only one question from me, looks good 🙂

Copy link
Contributor

@stevegsa stevegsa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A+!!!!

Copy link
Contributor

@jmhooper jmhooper left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nicely done

@zachmargolis zachmargolis merged commit 90b049e into master Jul 23, 2020
@zachmargolis zachmargolis deleted the margolis-incoming-risc-events branch July 23, 2020 18:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants