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
Create trigger for verifying cloud.gov origin #153
Conversation
Triggers FTW
SET ( origin, externalId ) = ( 'cloud.gov', username ) | ||
WHERE "f_isValidEmail"( username ) AND | ||
origin = 'uaa' AND | ||
verified = false AND |
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.
Shouldn't this be:
verified = true AND
created::date != passwd_lastmodified::date
That would indicate that the user has accepted the invite after those two conditions, yeah?
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
This should be good for now, but I have some local changes I'd like to integrate for the |
EOT | ||
psql_adm -d "${db}" -c << EOT | ||
CREATE TRIGGER enforce_cloud_gov_idp_origin_trigger | ||
AFTER INSERT OR UPDATE ON users |
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.
Can this be done just on UPDATE? since UAA inserts the user when it invites (and we never want to do anything then), and then presumably does an UPDATE to set their password / verified flag which is when we want to catch this?
All the Heredocs here are wrong. Or at least they don't work locally. I had been testing this within the DB client. 😞 |
Chips away at cloud-gov/product#255 |
) | ||
EOT | ||
psql_adm -d "${db}" <<-EOT | ||
ALTER TABLE IF EXISTS totp_seed |
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 for this table not to exist at this point?
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 any more but sure. Also, it's certainly safer to make it all happen within a single transaction.
' LANGUAGE sql | ||
EOT | ||
psql_adm -d "${db}" <<-EOT | ||
CREATE OR REPLACE FUNCTION "f_enforceCloudGovOrigin"( text ) RETURNS TRIGGER AS $$ |
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.
Your call on this, since I don't know the UAA internals, but some inline docs might be useful here. For example, it's not obvious to me why we need the email check, or what exactly the date comparison does.
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.
thanks, added a comment above the psql_adm
command.
) | ||
EOT | ||
psql_adm -d "${db}" <<-EOT | ||
ALTER TABLE IF EXISTS totp_seed |
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.
Another idempotency question--do we need to drop the constraint if it exists and re-add? I think we do, but haven't verified.
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 good call.
Thanks to @cnelson's advice on using Triggers, we can now verify that whenever we update a user in the DB we'll enforce that cloud.gov users are properly set up.
I verified that the
verified
is set to false and thatcreated
andpasswd_lastmodified
match by inviting a test user. I think this might be all we need. But I'd appreciate extra eyes on this.cc: @LinuxBozo