Authorization model #544

naamashoresh · 2017-05-04T12:00:17Z

A new PR instead of #435 with fixes according to comments by @mitchell852

mitchell852

Some more comments for you. thanks!

mitchell852 · 2017-05-10T21:16:02Z

docs/source/development/traffic_ops_api/v12/api_capability.rst

+
+|
+
+**GET /api/1.2/api_capabilities/capability/:name**


usually when filtering a result set, query params are used instead of route params. i.e. GET /api/1.2/api_capabilities?capName=ds-read

that way you don't have to create 2 routes and 2 documentation snippets, etc:

GET /api/1.2/api_capabilities
GET /api/1.2/api_capabilities/capability/:name

^^ no need for 2 routes. only this one is required:

GET /api/1.2/api_capabilities

Fixed, thanks

mitchell852 · 2017-05-10T21:19:48Z

docs/source/development/traffic_ops_api/v12/api_capability.rst

+  +-------------------+--------+--------------------------------------------------+
+  |    Parameter      |  Type  |                   Description                    |
+  +===================+========+==================================================+
+  | ``httpMethod``    | enum   | One of: 'GET', 'POST', 'PUT', 'PATCH', 'DELETE'. |


if you want your documentation to be perfect :) you could add a column for "required" with yes | no

mitchell852 · 2017-05-10T21:21:35Z

docs/source/development/traffic_ops_api/v12/api_capability.rst

+  **Request Route Parameters**
+
+  +-------------------+----------+------------------------------------------------+
+  | Name              |   Type   |                 Description                    |


I think you meant this column header to say "Required" and not type

Fixed, thanks

mitchell852 · 2017-05-10T21:22:47Z

docs/source/development/traffic_ops_api/v12/api_capability.rst

+  +-----------------+----------+------------------------------------------------+
+  | Name            | Required | Description                                    |
+  +=================+==========+================================================+
+  |   ``id``          |   yes    | Mapping id.                                  |


make sure you get these pipes in the right place or the docs won't build.

mitchell852 · 2017-05-10T21:25:14Z

docs/source/development/traffic_ops_api/v12/capability.rst

+  **Request Parameters**
+
+  +-------------------+--------+-------------------------------------------------+
+  |    Parameter      |  Type  |                   Description                   |


can you add a required column here too?

mitchell852 · 2017-05-10T21:57:30Z

traffic_ops/app/lib/API/Capability.pm

+	my $name = $self->param('name');
+
+	my $rs_data = $self->db->resultset("Capability")->search( 'me.name' => $name );
+	my @data = ();


you might want to do this

if ( !defined($rs_data) ) { return $self->not_found(); }

so they get back a 404 instead of an empty result set if id is invalid

mitchell852 · 2017-05-10T21:59:34Z

traffic_ops/app/lib/API/Capability.pm

+	my $description = $params->{description} if defined( $params->{description} );
+
+	my $capability = $self->db->resultset('Capability')->find( { name => $name } );
+	if ( !defined($capability) or $capability eq "" ) {


i'm not sure this is needed:

or $capability eq ""

Right, done.

mitchell852 · 2017-05-11T02:34:06Z

traffic_ops/app/lib/TrafficOpsRoutes.pm

+	$r->put("/api/$version/api_capabilities/:id")->over( authenticated => 1 )->to( 'ApiCapability#update', namespace => $namespace );
+	$r->post("/api/$version/api_capabilities")->over( authenticated => 1 )->to( 'ApiCapability#create', namespace => $namespace );
+	$r->delete("/api/$version/api_capabilities/:id")->over( authenticated => 1 )->to( 'ApiCapability#delete', namespace => $namespace );
+


do you plan to create apis for associating capabilities with roles?

mitchell852 · 2017-05-11T02:36:30Z

traffic_ops/app/t/api/1.2/api_capabilities.t

+my $dbh    = Schema->database_handle;
+my $t      = Test::Mojo->new('TrafficOps');
+
+my $false = 0;


I don't see where you are using these variables. plus, per our resident perl expert, @dangogh, this is not a good idea.

Removed, thanks.

mitchell852 · 2017-05-11T02:39:18Z

traffic_ops/app/t/api/1.2/capabilities.t

+my $dbh    = Schema->database_handle;
+my $t      = Test::Mojo->new('TrafficOps');
+
+my $false = 0;


again, i don't think you need these.

Right. Removed.

mitchell852

and a few more comments...

mitchell852 · 2017-05-11T02:44:29Z

traffic_ops/app/db/seeds.sql

@@ -81,10 +81,260 @@ insert into role (name, description, priv_level) values ('steering', 'Role for S
 insert into role (name, description, priv_level) values ('read-only user', 'Read-Only user', 10) ON CONFLICT (name) DO NOTHING;
 insert into role (name, description, priv_level) values ('portal', 'Portal User', 2) ON CONFLICT (name) DO NOTHING;
 insert into role (name, description, priv_level) values ('disallowed', 'Block all access', 0) ON CONFLICT (name) DO NOTHING;
+insert into role (name, description, priv_level) values ('root', 'Role for full capabilities - super-user ', 30) ON CONFLICT DO NOTHING;


i still don't think this is needed as we already have the "admin" user which has always been the "super user".

mitchell852 · 2017-05-11T02:48:17Z

traffic_ops/app/db/seeds.sql

+insert into capability (name, description) values ('user-write', 'Create, edit or delete user configuration') ON CONFLICT (name) DO NOTHING;
+
+-- roles_capabilities
+insert into role_capability (role_id, cap_name) values ((select id from role where name='root'), 'all-read') ON CONFLICT (role_id, cap_name) DO NOTHING;


again, unless you really feel strongly about creating a 2nd "super user" called root (in addition to admin), i'd change this to

insert into role_capability (role_id, cap_name) values ((select id from role where name='admin'), 'all-read') ON CONFLICT (role_id, cap_name) DO NOTHING;
insert into role_capability (role_id, cap_name) values ((select id from role where name='admin'), 'all-write') ON CONFLICT (role_id, cap_name) DO NOTHING;

I think having 2 super users will only create unnecessary confusion.

mitchell852 · 2017-05-11T02:52:57Z

traffic_ops/app/db/seeds.sql

+insert into role_capability (role_id, cap_name) values ((select id from role where name='root'), 'all-write') ON CONFLICT (role_id, cap_name) DO NOTHING;
+
+-- api_capabilities
+insert into api_capability (http_method, route, capability) values ('GET', '/', 'all-read') ON CONFLICT (http_method, route, capability) DO NOTHING;


I feel like these should be /api as this authorization model is used by the api gateway and therefore only applicable to /api. However, I understand that there are a few that start with /internal but my understanding is that those that start with /internal should NOT be exposed thru the api.

mitchell852 · 2017-05-11T02:53:21Z

traffic_ops/app/db/seeds.sql

+insert into api_capability (http_method, route, capability) values ('PUT', '/api/*/asns/*', 'asn-write') ON CONFLICT (http_method, route, capability) DO NOTHING; -- 7
+insert into api_capability (http_method, route, capability) values ('DELETE', '/api/*/asns/*', 'asn-write') ON CONFLICT (http_method, route, capability) DO NOTHING; -- 8
+insert into api_capability (http_method, route, capability) values ('GET', '/api/*/cache_stats', 'cache-stats-read') ON CONFLICT (http_method, route, capability) DO NOTHING; -- 11
+insert into api_capability (http_method, route, capability) values ('GET', '/internal/api/*/daily_summary', 'cache-stats-read') ON CONFLICT (http_method, route, capability) DO NOTHING; -- 12


I would take out the /internal ones

We should probably bring this up in an email to the dev list. I'm not 100% familiar with the behavior of the api gateway but I assumed because of its name it handles /api requests. For example, if a request came in to traffic-ops.domain.com/foo.jpg then foo.jpg would be served and the api gateway would not be concerned. however, if a call to traffic-ops.domain.com/api/whatever came in that request would go thru the api gateway where roles/capabilites would be checked for the route. Therefore, this would mean the /internal ones would not go thru the api gateway.

So, my advice would be to duplicate the "authenticated" /internal routes in TrafficOpsRoutes.pm and add those to your seeds file. for example, in trafficopsroutes.pm you'll find this:

$r->get("/internal/api/$version/steering")->over( authenticated => 1 )->to( 'Steering#index', namespace => 'API::DeliveryService' );

I would create a clone like this in trafficopsroutes.pm:

$r->get("/api/$version/steering")->over( authenticated => 1 )->to( 'Steering#index', namespace => 'API::DeliveryService' );

^^ and that is the one I'd add to your seeds file.

I think I created an email a little while ago to address /internal routes. I can see if I can get that email thread started again.

Since the first implementation of the API gateway reads the mapping from a file, and not from the DB table, I tend to either leave it as is, or remove everything from this table, for now, until the discussion yields some conclusions. What do you say?

ok, let's leave the /internal ones in as I have not been able to get any conclusions on the matter.

mitchell852 · 2017-05-11T03:02:00Z

traffic_ops/app/db/seeds.sql

+insert into api_capability (http_method, route, capability) values ('DELETE', '/api/*/types/*', 'type-write') ON CONFLICT (http_method, route, capability) DO NOTHING; -- 283
+insert into api_capability (http_method, route, capability) values ('GET', '/api/*/users', 'user-read') ON CONFLICT (http_method, route, capability) DO NOTHING; -- 289
+insert into api_capability (http_method, route, capability) values ('GET', '/api/*/users/*', 'user-read') ON CONFLICT (http_method, route, capability) DO NOTHING; -- 290
+insert into api_capability (http_method, route, capability) values ('PUT', '/api/*/users/*', 'user-write') ON CONFLICT (http_method, route, capability) DO NOTHING; -- 292


you might want to do a quick check against the TrafficOpsRoutes.pm file to make sure you got everything. I'm sure some new routes have been added since the PR was first submitted.

Also, what about the new routes you created in this PR. are they in this seeds file?

Same here. Will give it another look once we decide about the way this data is used by the API gateway.

mitchell852 · 2017-05-22T16:10:05Z

I'm ready to merge this PR however it says "This branch cannot be rebased safely". To be honest I'm not really sure what that means but it doesn't sound great. So here's what I'd suggest:

update your master branch
git checkout authorization_model
git rebase master

if all goes smoothly:

git push origin authorization_model --force (which will update your PR)

if all doesn't go smoothly:

resolve conflicts (this is the tricky part of course do a git status to see where the conflicts are)
git rebase --continue (i think. it will tell you on the command line)
git push origin authorization_model --force (which will update your PR)

of course if that doesn't work you can always do:

git rebase --abort

and we'll sync up and see what the issue is...

Adding tables: capability (list of available capabilities), api_capability mapping, role_capability mapping & user_role. Seeding capability & api_capability tables. Also seeding root role. (cherry picked from commit 4879a2c)

(cherry picked from commit db31205)

(cherry picked from commit 78f2f84)

(cherry picked from commit d79f76c)

(cherry picked from commit 0dcee94)

Added "capability" as a query parameter to the api_capabilities API as a filter, and removed the dedicated API Updated the documentation and UT accordingly.

perltidy

naamashoresh · 2017-05-23T04:32:38Z

Branch rebased, PR updated. I believe it can be merged now.
Thanks

mitchell852 reviewed May 11, 2017

View reviewed changes

naamashoresh added 12 commits May 23, 2017 07:28

Authorization model -

c3c06d3

Adding tables: capability (list of available capabilities), api_capability mapping, role_capability mapping & user_role. Seeding capability & api_capability tables. Also seeding root role. (cherry picked from commit 4879a2c)

Adding capabilities & api_capalities APIs

72cf6d4

(cherry picked from commit db31205)

Unit tests for new capabilities & api_capabilities APIs

55bd848

(cherry picked from commit 78f2f84)

Documentation for new capabilities & api_capabilities APIs.

521e83e

(cherry picked from commit d79f76c)

Applying review comments by mitchell852

a7e171f

(cherry picked from commit 0dcee94)

Schema changes which are part of the previous commit.

adfcb32

Changed parmaeter name from "capName" to "capability".

ff5b7cb

Added "capability" as a query parameter to the api_capabilities API as a filter, and removed the dedicated API Updated the documentation and UT accordingly.

Cancelled "root" role, using the existing "admin" role instead.

d57067d

removed hardcoding row index

51a0427

perltidy

added "not_found" for non-existent capability

5d5f07d

Removed redundant check for empty string

8911898

Removed redundant true & false variables

f04b4db

naamashoresh force-pushed the authorization_model branch from 8914d63 to f04b4db Compare May 23, 2017 04:30

asfgit closed this in 95c1f38 May 24, 2017

mitchell852 self-assigned this Jan 19, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Authorization model #544

Authorization model #544

naamashoresh commented May 4, 2017

mitchell852 left a comment

mitchell852 May 10, 2017 •

edited

naamashoresh May 21, 2017

mitchell852 May 10, 2017

naamashoresh May 21, 2017

mitchell852 May 10, 2017

naamashoresh May 21, 2017

mitchell852 May 10, 2017

naamashoresh May 21, 2017

mitchell852 May 10, 2017

naamashoresh May 21, 2017

mitchell852 May 10, 2017

naamashoresh May 21, 2017

mitchell852 May 10, 2017

naamashoresh May 21, 2017

mitchell852 May 11, 2017 •

edited

mitchell852 May 11, 2017

naamashoresh May 21, 2017

mitchell852 May 11, 2017

naamashoresh May 21, 2017

mitchell852 left a comment

mitchell852 May 11, 2017

mitchell852 May 11, 2017

mitchell852 May 11, 2017

mitchell852 May 11, 2017

mitchell852 May 11, 2017

naamashoresh May 21, 2017

mitchell852 May 22, 2017

mitchell852 May 11, 2017

naamashoresh May 21, 2017

mitchell852 commented May 22, 2017

naamashoresh commented May 23, 2017


		\|

		GET /api/1.2/api_capabilities/capability/:name

Authorization model #544

Authorization model #544

Conversation

naamashoresh commented May 4, 2017

mitchell852 left a comment

Choose a reason for hiding this comment

mitchell852 May 10, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mitchell852 May 11, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mitchell852 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mitchell852 commented May 22, 2017

naamashoresh commented May 23, 2017

mitchell852 May 10, 2017 •

edited

mitchell852 May 11, 2017 •

edited