Handle signin permission during oauth #560

h-lame · 2017-08-15T14:46:10Z

For: https://trello.com/c/c4bgclDp/195-implement-rfc-78-make-signon-handle-signin-permission-during-oauth

Implements: https://github.com/alphagov/govuk-rfcs/blob/8cbb2a0de86de02f54ae37b245e79b46ad62cb6a/rfc-078-re-architect-signin-permissions-in-signon.md

This is a work-in-progress that I'm pushing up to get some extra eyes on it earlier rather than later. In particular, I'm interested in any thoughts people have on the 2nd commit Add signin permission requirement to doorkeeper_authorize! as I'm not 100% sure it's a required change. I would definitely be grateful for any advice from people who know a bunch of stuff about oauth.

Todo

Signon MUST check for 'signin' permission during the oauth handshake with an application and reject users that do not have this permission. (commits 1 and 2 in this PR)
Signon MUST allow a super admin to grant permissions to all users in one go. (commits 3-9 in this PR)
Signon SHOULD allow super admins to mark permissions for an application as 'default' meaning it is added to all new users. (commits 10-13 - mostly repurposed from [DISCUSS] Add signin on support to all users #553)
gds-sso SHOULD deprecate require_signin_permission!. (Deprecate require_signin_permission! controller method gds-sso#119)
applications using signon SHOULD be audited to make sure they will continue to function after these changes are made - see https://docs.google.com/spreadsheets/d/1ypHdDHaHJZELkdQC3xQprMxJpkPCLTfnJPsEtPhhENI/edit#gid=0
applications using signon SHOULD be audited to collect the set of default permissions that should be created and bulk granted to all existing users.
~~omniauth-gds SHOULD deprecate whatever it does with signin permissions~~ omniauth-gds doesn't do anything wrt signin permissions, each app that uses it does this manually

Some of these will be completed by doing work in other repos, I'll link to the relevant PRs when they're done.

This will be easiest reviewed commit-by-commit as it's a big change, however, github has messed up the commit order. To review in the correct order do them in this sequence (it's the git graph order, not the date order that github shows):

1656e10 Require signin permission to complete oauth authorisation
9c1c760 Add signin permission requirement to doorkeeper_authorize!
3ab1b9f Add name to supported_permissions factory
718077d Introduce BulkGrantPermissionSet model and job
55f7246 Rename status_message helper to batch_invite_status_message
cdf24b8 Add processed and total user counts to bulk grant permission set
f63e48d Restrict bulk grant permission set to admins and superadmins
03e56b1 Add UI to allow bulk granting permissions
03eda87 Represent the bulk grant form if there are errors
9c40ce1 Add default flag to supported permissions
6dc5b58 Remove unsupported permissions check from rake users:create
4f5f0aa Extract internals of rake users:create to an object in lib
04ef719 New users are granted default permissions
30ce041 Remove special case for support app on dashboard
f4d92b7 Record an EventLog when bulk granting permissions

Screenshots

UI for granting permissions to all users

UI for watching that bulk grant being processed

These take "inspiration" (e.g. are copy+paste jobs pretty much) from the existing UI for batch inviting a CSV file of users and granting them permissions.

h-lame · 2017-08-21T09:15:31Z

Removed WIP tag as this is now feature complete and ready for review. Gonna work on the outstanding audit tasks separately. There may be additional commits to add seed data based on those audits, but it's unlikely they'd change the rest of the code in the PR.

Also, it'd be nice to see a test, or an addition to an existing test to explain why this functionality exists.

boffbowsh · 2017-08-31T14:49:34Z

This all looks reasonable to me, but I've only had time to skim it. I don't see any obvious security holes. It'd still be good to get a review from someone else. Perhaps a willing volunteer could pair with you on a review?

tijmenb · 2017-09-01T16:03:56Z

app/models/bulk_grant_permission_set.rb

+  def perform(_options = {})
+    self.update_column(:total_users, User.count)
+    User.find_each do |user|
+      supported_permissions.each do |permission|


Could we audit-log the permission changes here like we do for manual updates?

See #541:

signon/app/services/user_update.rb

Lines 34 to 40 in 6208df3

EventLog.record_event(

user,

EventLog::PERMISSIONS_ADDED,

initiator: current_user,

application_id: application_id,

trailing_message: "(#{permissions.map(&:name).join(', ')})",

)

oh, yes, absolutely. I'll do a quick check to make sure that we're always adding event logs for all the places where we create users and grant permissions as I suspect this won't be the only place where we've slipped through the net.

fofr · 2017-11-06T12:35:38Z

Are there some screenshots of any new UI?

When an application makes an authorisation request on behalf of a user that user must have the `signin` permission granted to them for the request to be completed successfully. If they don't the user will be redirected to a "sorry, you don't have permission" page on signon. This new page is based on the error page from the gds-sso gem that serves the same purpose. We have to configure doorkeeper to use a custom authorizations controller to achieve this. The `resource_owner_authenticator` block where we check if the user needs 2 step verification or to change their password or other user based reasons to interrupt the login flow would appear to be the pefect place to do the `signin` permission check, unfortunately however this block is run before doorkeeper has extracted the application from the requset so we don't have access to it. The `skip_authorization` block is yielded the user and application (inside the client), but this is only run for the auth/new path and is not expected to have side-effects like rendering or redirecting. Instead we use our own controller and add a branch for permission check. The test provided for the custom controller is a port of the test in doorkeeper for the default authorizations controller, translated into minitest and given some extra config to run in our suite insetead of the doorkeeper one. We've also added test cases to cover our permissions functionality. We'll need to be careful to make sure that when we update doorkeeper we port any changes to the default authorisations controller or test to our versions. If a hook becomes available it would be a good idea to use that if possible.

h-lame · 2017-11-06T15:54:50Z

@fofr - good point. Added some to the PR description.

Davidslv · 2017-11-06T16:06:43Z

This should probably be tested on integration and staging before merging

h-lame

Resurrecting some comments and responses between @Davidslv and I that were lost when I did a rebase.

h-lame · 2017-11-06T16:24:34Z

app/controllers/signin_required_authorizations_controller.rb

+    if pre_auth.authorizable?
+      if skip_authorization? || matching_token?
+        if user_has_signin_permission_to_application?
+          auth = authorization.authorize


Lost comment due to rebase:

@Davidslv:

you might want to call super here instead of repeating code.

@h-lame:

If I do that then it'll run these lines of code again:

if pre_auth.authorizable? if skip_authorization? || matching_token?

And I don't know if those are "cheap" or have no side-effects, so while it's annoying to duplicate the code, I'd rather keep this as-is.

h-lame · 2017-11-06T16:26:10Z

app/policies/bulk_grant_permission_set_policy.rb

+
+    false
+  end
+  alias_method :create?, :new?


Lost comment due to rebase:

@Davidslv:

What's the reason for these aliases?

@h-lame:

The policy stuff in pundit works at the controller level by asking if the user is allowed to perform the given controller action on the instance (or class). We're following the pattern established by some of the other policies by defining our rules for one action, and then aliasing the other actions onto it so they're all the same.

h-lame · 2017-11-06T16:29:51Z

app/models/bulk_grant_permission_set.rb

+    BulkGrantPermissionSetJob.perform_later(self.id)
+  end
+
+  def perform(_options = {})


Lost comment due to rebase:

@Davidslv:

what's the reason to pass options here if none is being used?

@h-lame:

Mostly so it looks the same as BatchInvitation which has a similar "create an instance, queue up a job, run the job by finding the instance and calling perform on it". BatchInivitation#perform takes options and so I though this should too. I think the reason for it is that the job takes options by default and we pass them through (even if they're ignored). I guess it makes it easy to pass options in later if we need to? Not strongly wedded to this, but I'd rather this and BatchInvitation look similar, so I could remove the options from both.

h-lame · 2017-11-06T16:36:19Z

db/migrate/20170818134716_add_default_flag_to_supported_permissions.rb

@@ -0,0 +1,5 @@
+class AddDefaultFlagToSupportedPermissions < ActiveRecord::Migration
+  def change
+    add_column :supported_permissions, :default, :boolean, default: false, null: false


Lost comment due to rebase:

@Davidslv:

How big is this table? Adding a default value will lock the table until all records are written

@h-lame:

At the time this migration is run it'll be empty because we only just created it in the previous migration.

Originally this was a question on the changes to the bulk_grant_permission_sets table which is split over two commits (but ultimately only one migration). However, it's a question that's relevant here on this commit and migration. Unlike I suggest in my previous answer, the supported_permissions isn't empty and hasn't just been created, but it does have only 725 rows, so I think we're safe enough to add the column inline.

sihugh

A couple of minor comments, but approved for testing on integration etc. I'm on 2nd line as of tomorrow so will keep an eye on tickets

For endpoints made available to oauth clients we make sure that the user that owns the access token used on the request has signin permission on the application making the request on their behalf. To achieve this we customise the `doorkeeper_authorize!` method that doorkeeper provides to protect your controllers. Our version calls super to let the default implementation take care of parsing tokens and doing the real work of authenticating the request. Once this is complete we check that the user has signin permission on the applicaiton (we obtain the user and application from the access token). If not we redirect to the signin required path, if they do we continue as before. It's not 100% clear to me that we need to do this. All these requests will have been preceded by a call to the oauth endpoints to obtain the access token that is used in this request. The oauth endpoints check that the user has signin permission on the application so it should not be possible to obtain an access token without this permission. However, I'm not sure how long lived our access tokens are, and by making sure each api request to signon checks this, we are able to invalidate requests as soon as the user's permission is revoked rather than waiting for their access token to expire.

They're invalid without one, so we should make sure a raw factory call works.

We follow the pattern established by BatchInvitation and have a model that is stored in the DB with a list of permissions, and then we create a job that performs the work. The work in this case is to iterate through all users and grant them each permission in the permission set. We use a job for this because there are 1,000s of users and iterating through them all in a single request/response cylce seems unlikely to be fast.

`status_message` is a bit generic and we're about to introduce something similar for our bulk_grant_permission_sets.

These counters allow us to track progress during the perform method in a similar fashion to how the batch invitations does it with it's user association. In rare circumstances we might end up with processed users > total users if someone is creats a user while our job is processing, but it's not a huge problem. If it is we might want to fix the user ids we're going to process into the permission set at creation time. This would pave the way towards us being able to choose a subset of users to bulk grant to, which might be interesting, but we don't need it now.

This is the same as batch invitations, org admins can't do it but full admins can.

This presents the standard permissions selection UI and completing the form will show a automatically refreshing page that shows progress of how many users have been processed. It's a bit rough and ready, but should suffice to explain what's going on.

Most likely that the user didn't choose any permissions.

This flag indicates if this is a permission that should be granted by default to all new users. For example, we want all users to be able to use the support application, so rather than asking the person creating the user to remember to give them support permissions we should be able to flag it as default once and have the system grant it for us. This commit adds the flag and allows admins to edit permissions to say which are default or not. Future commits will change the system to act on this flag when creating users.

This check was added in 4474af0 - but the supplied list of permissions is not used other than for this check. The only permission granted to a user via this task is 'signin' on the list of applications provided via the ENV.

This lets us test it and make it easier to extend the functionality later without breaking it.

There are 3 places a user can be created: 1. the /users form 2. the /batch_invitations form 3. the users:create rake task None of which use exactly the same mechanism so we have to introduce the code in each place. It may be worth refactoring so that all three places use the same user creation service object (perhaps we can extend the one we extracted from the rake task). One obvious alternative would be to use an after_create callback, but I'm wary of that as callbacks can end up getting expensive. Particularly in tests.

The dashboard would always provide a link to the support app regardless of the permissions the user has on the support app. This was because the support app didn't explicitly require "signin" permissions and we wanted everyone to have a link to it. Now that signon handles the 'signin' permission itself this link wouldn't always work and would confuse users who didn't really have the permission so we remove it. To keep the existing experience though we'll be giving everyone "signin" permission via the bulk grant functionality and making it a default permission for new users.

This means we don't lose any events when permissions are bulk granted.

We extracted the code in SigninRequiredAuthorizationsController from version 4.2.0 of doorkeeper. If we upgrade doorkeeper we want to be reminded to review the new version of doorkeeper and compare our controller against the default one to see if we need to make any changes. If we are lucky it becomes easier to augment the auth behaviour without overriding the whole controller. The intention is that someone does a gem upgrade, this test fails and reminds them to do the check. When they're happy they make any relevant changes to our implementation and tests and then change the EXPECTED_DOORKEEPER_VERSION constant in SigninRequiredAuthorizationsController to match the new version and the test will pass.

After our [audit of applications that use signon][audit] we found that we only need two default permissions: 1. signin on the support app 2. signin on the content preview app Both of these apps are intended to be used by every user so we make them default permissions, and queue up a bulk grant to make sure all existing users have these permissions. There were other apps that did not check the signin permission, but all of these turned out to be an error of implementation. [audit]: https://docs.google.com/spreadsheets/d/1ypHdDHaHJZELkdQC3xQprMxJpkPCLTfnJPsEtPhhENI/edit#gid=0

h-lame · 2017-11-07T16:33:46Z

@sihugh's comments were lost in a rebase, but I did address them, honest.

sihugh · 2017-11-07T16:43:42Z

I approve this message

Davidslv · 2017-11-08T09:26:18Z

I finally found them on the notifications page. All good from me too

Davidslv

I have found your answers, all good from me.

Without this we can't render the signon layout for any of the paths through the controller where it will cause a render instead of a redirect. We need it because the layout uses the policy helper that pundit provides to make sure we don't show non-admin users links to the list of users or applications or other things they're not allowed to do or see.

h-lame · 2017-11-10T10:22:56Z

We've tested this branch on integration and staging and encountered no blocking problems so we're gonna merge this. We have written up a card for some enhancements about communication of permission changes between signon and apps - but we don't consider it a blocker to progress.

h-lame mentioned this pull request Aug 16, 2017

Deprecate require_signin_permission! controller method alphagov/gds-sso#119

Merged

h-lame force-pushed the handle-signin-permission-during-oauth branch 3 times, most recently from 3c46f9e to 951df52 Compare August 18, 2017 12:18

h-lame changed the title ~~[WIP] Handle signin permission during oauth~~ Handle signin permission during oauth Aug 21, 2017

tijmenb reviewed Sep 1, 2017

View reviewed changes

This was referenced Sep 6, 2017

Handle signin permission during oauth (PR copy) bitzesty/signonotron2#4

Closed

Handle signin permission during oauth (COPY) bitzesty/signonotron2#6

Merged

h-lame force-pushed the handle-signin-permission-during-oauth branch 2 times, most recently from 3618791 to 6a673ea Compare September 11, 2017 14:19

h-lame force-pushed the handle-signin-permission-during-oauth branch 2 times, most recently from 01d1495 to dadcc6b Compare October 31, 2017 12:19

h-lame force-pushed the handle-signin-permission-during-oauth branch from dadcc6b to 66508a3 Compare November 6, 2017 15:37

h-lame force-pushed the handle-signin-permission-during-oauth branch from 66508a3 to f4d92b7 Compare November 6, 2017 16:13

h-lame commented Nov 6, 2017

View reviewed changes

sihugh approved these changes Nov 7, 2017

View reviewed changes

h-lame added 8 commits November 7, 2017 16:17

Add name to supported_permissions factory

b6284df

They're invalid without one, so we should make sure a raw factory call works.

Rename status_message helper to batch_invite_status_message

e4dc2de

`status_message` is a bit generic and we're about to introduce something similar for our bulk_grant_permission_sets.

Restrict bulk grant permission set to admins and superadmins

19cbf38

This is the same as batch invitations, org admins can't do it but full admins can.

Represent the bulk grant form if there are errors

c93ba2e

Most likely that the user didn't choose any permissions.

h-lame added 8 commits November 7, 2017 16:19

Remove unsupported permissions check from rake users:create

f4078ec

This check was added in 4474af0 - but the supplied list of permissions is not used other than for this check. The only permission granted to a user via this task is 'signin' on the list of applications provided via the ENV.

Extract internals of rake users:create to an object in lib

b463cb1

This lets us test it and make it easier to extend the functionality later without breaking it.

Record an EventLog when bulk granting permissions

04bbe79

This means we don't lose any events when permissions are bulk granted.

h-lame force-pushed the handle-signin-permission-during-oauth branch from f4d92b7 to 45d3fff Compare November 7, 2017 16:19

Davidslv approved these changes Nov 8, 2017

View reviewed changes

h-lame mentioned this pull request Nov 9, 2017

Model multiple orgs correctly #585

Merged

sihugh approved these changes Nov 10, 2017

View reviewed changes

h-lame merged commit 6d5c34c into master Nov 10, 2017

h-lame deleted the handle-signin-permission-during-oauth branch November 10, 2017 10:23

tijmenb mentioned this pull request Jan 8, 2019

Revoking app access for user should revoke additional permissions #535

Closed

floehopper mentioned this pull request Oct 26, 2023

Move UsersController#show -> OauthUsersController #2469

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle signin permission during oauth #560

Handle signin permission during oauth #560

h-lame commented Aug 15, 2017 •

edited

h-lame commented Aug 21, 2017 •

edited

boffbowsh commented Aug 31, 2017

tijmenb Sep 1, 2017

h-lame Sep 8, 2017

fofr commented Nov 6, 2017

h-lame commented Nov 6, 2017

Davidslv commented Nov 6, 2017

h-lame left a comment

h-lame Nov 6, 2017

h-lame Nov 6, 2017

h-lame Nov 6, 2017

h-lame Nov 6, 2017

h-lame Nov 6, 2017

sihugh left a comment

h-lame commented Nov 7, 2017

sihugh commented Nov 7, 2017

Davidslv commented Nov 8, 2017

Davidslv left a comment

h-lame commented Nov 10, 2017

	EventLog.record_event(
	user,
	EventLog::PERMISSIONS_ADDED,
	initiator: current_user,
	application_id: application_id,
	trailing_message: "(#{permissions.map(&:name).join(', ')})",
	)

Handle signin permission during oauth #560

Handle signin permission during oauth #560

Conversation

h-lame commented Aug 15, 2017 • edited

Todo

Screenshots

h-lame commented Aug 21, 2017 • edited

boffbowsh commented Aug 31, 2017

tijmenb Sep 1, 2017

Choose a reason for hiding this comment

h-lame Sep 8, 2017

Choose a reason for hiding this comment

fofr commented Nov 6, 2017

h-lame commented Nov 6, 2017

Davidslv commented Nov 6, 2017

h-lame left a comment

Choose a reason for hiding this comment

h-lame Nov 6, 2017

Choose a reason for hiding this comment

h-lame Nov 6, 2017

Choose a reason for hiding this comment

h-lame Nov 6, 2017

Choose a reason for hiding this comment

h-lame Nov 6, 2017

Choose a reason for hiding this comment

h-lame Nov 6, 2017

Choose a reason for hiding this comment

sihugh left a comment

Choose a reason for hiding this comment

h-lame commented Nov 7, 2017

sihugh commented Nov 7, 2017

Davidslv commented Nov 8, 2017

Davidslv left a comment

Choose a reason for hiding this comment

h-lame commented Nov 10, 2017

h-lame commented Aug 15, 2017 •

edited

h-lame commented Aug 21, 2017 •

edited