-
Notifications
You must be signed in to change notification settings - Fork 34
Add SLO and SSO endpoints for Login.gov #1476
Conversation
* Use `SessionsController` * This means users can sign in and out with Login.gov * This is based off of code in https://github.com/18F/identity-sp-rails * Next steps: - protect this route so that only logged-in admins can see it - add whitelisting for certain users who log in with login.gov to be admins - add this config to identity-idp repo
if params.key?(:loa) | ||
request.env['omniauth.strategy'].options[:authn_context] = [ | ||
"http://idmanagement.gov/ns/assurance/loa/#{params[:loa]}", | ||
'http://idmanagement.gov/ns/requested_attributes?ReqAttr=email,phone,first_name,last_name,ssn' |
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.
do you need SSN for this app? Don't ask for it unless you have a business need, because that's PII.
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! didn't even see this. We do not need it. Removing.
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.
Removed at 46cf31f
No SSN is required for this app
…On Thu, Dec 22, 2016 at 1:27 PM, Peter Karman ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In app/controllers/sessions_controller.rb
<#1476 (review)>
:
> +
+ def destroy
+ if params[:SAMLRequest]
+ idp_logout_request
+ elsif params[:SAMLResponse]
+ validate_slo_response
+ else
+ sp_logout_request
+ end
+ end
+
+ def setup
+ if params.key?(:loa)
+ request.env['omniauth.strategy'].options[:authn_context] = [
+ "http://idmanagement.gov/ns/assurance/loa/#{params[:loa]}",
+ 'http://idmanagement.gov/ns/requested_attributes?ReqAttr=email,phone,first_name,last_name,ssn'
do you need SSN for this app? Don't ask for it unless you have a business
need, because that's PII.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#1476 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAGHNa9uUSRw-iIVOOR0WiCD7WD-ypopks5rKutegaJpZM4LUYpS>
.
--
Michael Torres
Product Lead, Micropurchase Platform
18F
(415) 920-3205
|
dd98f8e
to
917a0e8
Compare
**Why**: This is a required step for adding a new service provider * See 18F/micropurchase#1476
def setup | ||
if params.key?(:loa) | ||
request.env['omniauth.strategy'].options[:authn_context] = [ | ||
"http://idmanagement.gov/ns/assurance/loa/#{params[:loa]}", |
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 new to all of this identity stuff. What are these URLs? Is it OK that they are HTTP and not HTTPS?
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, http is fine here. These are identifiers, not actual addressable URLs.
render inline: error_msg | ||
end | ||
|
||
def validate_slo_response |
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.
What does slo
stand for?
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.
Single Logout
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.
this was also very confusing to me at first! while I am generally allergic to acronyms, this one seems pretty broadly accepted in the "single log out" world. do you think we should use full name, @adelevie ?
|
||
delegate :email, :uid, to: :user | ||
|
||
attr_reader :asserted_attributes |
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.
What does asserted_attributes
mean?
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.
@@ -0,0 +1,22 @@ | |||
-----BEGIN CERTIFICATE----- |
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.
What is 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.
this is the x509 cert -- we use a fingerprint of this cert so that the "service provider" (in this case, the identity idp app) knows who is making the request.
the way one SO responder but it:
You can use the public key to verify that the content of the SAML response matches the key - in other words - that response definitely came from someone who has the matching private key to the public key in the message, and the response hasn't been tampered with.
this is the public key. :)
more info here: https://developers.onelogin.com/saml/ruby
also, I may be messing this all up. but this is my working understanding. still learning myself!
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.
not to be too pedantic, but the service provider is micropurchase. The identity idp app is the identity provider.
the cert belongs to the SP (micropurchase) and is registered with the IdP. Likewise, the Idp may register its public cert with the SP. The certs are used as @jessieay indicates: to verify the incoming requests are signed from the org they claim to be.
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, you are right. Sorry to mix up the words and create even more confusion in an already confusing world!
0db3311
to
4967690
Compare
validates :sam_status, presence: true | ||
|
||
enum sam_status: { duns_blank: 0, sam_accepted: 1, sam_rejected: 2, sam_pending: 3 } | ||
|
||
def self.from_omniauth(auth) |
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.
note that once we actually let users sign in with login.gov, this will find an existing user via email rather than find or create. doing it this way for now so it's easier to test, but we can also make it just FIND if people are uncomfortable with this bandaid
validates :github_id, presence: true | ||
validates :github_login, presence: true | ||
validates :github_id, presence: true, if: :not_login_user? | ||
validates :github_login, presence: true, if: :not_login_user? |
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.
these if
validations will be removed 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.
This makes more sense semantically, compared to sessions_controller
**Why**: This is a required step for adding a new service provider * See 18F/micropurchase#1476
**Why**: This is a required step for adding a new service provider * See 18F/micropurchase#1476
**Why**: This is a required step for adding a new service provider * See 18F/micropurchase#1476
**Why**: This is a required step for adding a new service provider * See 18F/micropurchase#1476
admins