You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@trafficcontrol.apache.org by GitBox <gi...@apache.org> on 2021/05/14 02:48:19 UTC
[GitHub] [trafficcontrol] ocket8888 opened a new pull request #5848: User Permissions Blueprint
ocket8888 opened a new pull request #5848:
URL: https://github.com/apache/trafficcontrol/pull/5848
## What does this PR (Pull Request) do?
- [ ] This PR fixes #REPLACE_ME OR is not related to any Issue <!-- You can check for an issue here: https://github.com/apache/trafficcontrol/issues -->
## Which Traffic Control components are affected by this PR?
None
## What is the best way to verify this PR?
Read the blueprint.
## The following criteria are ALL met by this PR
- [x] Tests are unnecessary
- [x] Documentation is unnecessary
- [x] An update to CHANGELOG.md is not necessary
- [x] This PR includes any and all required license headers
- [x] This PR **DOES NOT FIX A SERIOUS SECURITY VULNERABILITY**
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #5848: User Permissions Blueprint
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #5848:
URL: https://github.com/apache/trafficcontrol/pull/5848#discussion_r681194358
##########
File path: blueprints/user.permissions.md
##########
@@ -0,0 +1,286 @@
+<!--
+Licensed to the Apache Software Foundation (ASF) under one
+or more contributor license agreements. See the NOTICE file
+distributed with this work for additional information
+regarding copyright ownership. The ASF licenses this file
+to you under the Apache License, Version 2.0 (the
+"License"); you may not use this file except in compliance
+with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing,
+software distributed under the License is distributed on an
+"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+KIND, either express or implied. See the License for the
+specific language governing permissions and limitations
+under the License.
+-->
+# Enforced User Permissions
+
+## Problem Description
+Currently, the Permissions (currently "Capabilities", henceforth referred to as
+"Permissions" to avoid confusion with Server Capabilities) afforded to a user
+are defined by the "Privilege Level" of their Role. It essentially defines a
+scale on the interval [0,30], where a higher number allows more things. This
+isn't as scalable or configurable as it could be, and involves granting more
+permissions to a user than they actually need just because every Privilege
+Level encapsulates all of the permissions of every Privilege Level below it,
+making disjoint Permission sets for disparate Roles logically impossible.
+
+## Proposed Change
+Instead of a system of rigid supersets of Permissions, this blueprint proposes
+that the system of user "Capabilities" be expanded upon, renamed to Permissions
+(again, to avoid confusion) and enforced by the API.
+
+Under this system, each endpoint will declare the Permissions it requires for
+specific operations and check that the requesting user has those required, if
+any.
+
+This information is _not_ to be stored in the database nor exposed through the
Review comment:
Role to Permission mappings aren't documented. Though, the Roles that come with ATC by default could be documented somewhere. That's outside the scope of this blueprint, though.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] rawlinp commented on pull request #5848: User Permissions Blueprint
Posted by GitBox <gi...@apache.org>.
rawlinp commented on pull request #5848:
URL: https://github.com/apache/trafficcontrol/pull/5848#issuecomment-891994128
> If Roles are taken out of the API, then would we not still need to make API changes, so that the configuration file contents can be returned through the API?
Not necessarily, we don't _need_ the API to return which permissions belong to which roles.
> How would we migrate existing Roles to the on-disk configuration? How could that migration be undone for downgrades?
Since User Permissions are optional, it seems like we'll still have to define privilege levels for every route we add. So, I'd think our existing roles wouldn't even belong in the on-disk config. Their permissions would continue to be defined by the privilege levels we assign to routes. If people created custom roles with different privilege levels, they will have to translate those custom roles into the new User Permissions model.
> Testing this would also be difficult to do in the current framework, since creating or deleting Roles would require restarting Traffic Ops, which for the moment makes it a manual process during testing.
Testing permissions would be pretty easy via unit tests in this manner. You just simply update TO's internal memory representation (e.g. `map[string]map[string]struct{}`), then make an HTTP request. For the most part, you wouldn't have to mock the DB very much, since the request would be 403'd before even needing most DB queries. You'd probably just have to mock the DB for the user data (i.e. which role the user has). Long-term, testing via this method would probably be easier than writing API tests just to verify permissions functionality. In fact, the API tests we currently have to test privileges are pretty convoluted and confusing to begin with, since they require creating a new session, logging into the new session, and using the new session to attempt the requests.
> I also think that being able to manipulate user permissions through Traffic Portal, and in particular the ability to create a new service account with permissions for only a few endpoints it needs quickly through Traffic Portal, would be a shame to trade away for the very manual process
It wouldn't be a shame if the trade-offs were well-understood and taken into consideration. I think creating roles and assigning permissions will be one of the most rare operations that ever occurs in an ATC environment. The default roles have covered 99% of all cases up to this point, we just want something better now (for security). Since better security is the main goal here, it might make sense to make the implementation of this security feature as secure as possible.
> of ensuring everyone's okay with restarting TO, ssh in and edit a configuration file, then perform the restart and signal the all-clear (or, using Ansible as you said, which would probably be a longer process but safer than ssh-ing manually)
It wouldn't necessarily require a restart; we could make it reloadable if restarting TO was a problem. IMO, the way TO is designed, restarting is not really an issue in terms of availability. We can rolling-restart TO and still maintain 100% availability. Also, I would argue that most permissions requests wouldn't be so time-sensitive that we'd need to be able to quickly use TP to add permissions. If something _was_ really time-sensitive, then any admin should be able to perform the time-sensitive task or get the user the time-sensitive data.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] mitchell852 commented on a change in pull request #5848: User Permissions Blueprint
Posted by GitBox <gi...@apache.org>.
mitchell852 commented on a change in pull request #5848:
URL: https://github.com/apache/trafficcontrol/pull/5848#discussion_r681286881
##########
File path: blueprints/user.permissions.md
##########
@@ -0,0 +1,286 @@
+<!--
+Licensed to the Apache Software Foundation (ASF) under one
+or more contributor license agreements. See the NOTICE file
+distributed with this work for additional information
+regarding copyright ownership. The ASF licenses this file
+to you under the Apache License, Version 2.0 (the
+"License"); you may not use this file except in compliance
+with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing,
+software distributed under the License is distributed on an
+"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+KIND, either express or implied. See the License for the
+specific language governing permissions and limitations
+under the License.
+-->
+# Enforced User Permissions
+
+## Problem Description
+Currently, the Permissions (currently "Capabilities", henceforth referred to as
+"Permissions" to avoid confusion with Server Capabilities) afforded to a user
+are defined by the "Privilege Level" of their Role. It essentially defines a
+scale on the interval [0,30], where a higher number allows more things. This
+isn't as scalable or configurable as it could be, and involves granting more
+permissions to a user than they actually need just because every Privilege
+Level encapsulates all of the permissions of every Privilege Level below it,
+making disjoint Permission sets for disparate Roles logically impossible.
+
+## Proposed Change
+Instead of a system of rigid supersets of Permissions, this blueprint proposes
+that the system of user "Capabilities" be expanded upon, renamed to Permissions
+(again, to avoid confusion) and enforced by the API.
+
+Under this system, each endpoint will declare the Permissions it requires for
+specific operations and check that the requesting user has those required, if
+any.
+
+This information is _not_ to be stored in the database nor exposed through the
+API, but documented. This change is discussed in greater detail in the
+"Alternatives" section.
+
+The enforcement of these Permissions is proposed to be configurable via an
+entry in the Traffic Ops configuration file. Thus, for one major version the
+API will need to support Permissions-based authorization as well as Privilege
+Level-based authorization.
+
+## Data Model Impact
+"Capabilities" as they are known today will be removed from the data model.
+Also, the current model of a Role will need to be modified. The current model
+is as follows:
+
+```typescript
+interface Role {
+ capabilities: Array<string> | null;
+ description: string | null;
+ id?: number | null;
+ name: string | null;
+ privLevel: number | null;
+}
+```
+
+The proposed new model is shown below.
+
+```typescript
+interface Role {
+ description: string;
+ lastUpdated?: Date;
+ name: string;
Review comment:
at one time, i did think roles could/should be scoped per tenant but i'm kinda thinking that is overkill atm. but it is something to think about. without tenancy, any new role that is created will be visible to all "tenants". is that ok? maybe it is ok if creating new roles is a semi-rare event...
```
root
- tenant 1
-- tenant 1.1
-- tenant 1.2
- tenant 2
```
so basically if i'm a user with the tenant 1 tenant and i create the foo role, would that role be scope to tenant 1? meaning users of tenant 1, 1.1 and 1.2 would see the role but not users in tenant 2? seems to make sense...but again, would need to think about that more. could cause potential issues.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 commented on pull request #5848: User Permissions Blueprint
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on pull request #5848:
URL: https://github.com/apache/trafficcontrol/pull/5848#issuecomment-899513167
Yeah you're right, it should be possible to roll back for at least one major release because of that. I'm sold; I'll update the blueprint to mention that the `admin` Role will need some extra protections and special handling.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 commented on pull request #5848: User Permissions Blueprint
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on pull request #5848:
URL: https://github.com/apache/trafficcontrol/pull/5848#issuecomment-897966018
> ...we'll need to manually add any new permissions from new APIs after we upgrade, in order to be able to use them.
Sometimes. For new objects, that is true. But if, for example, we wanted to restrict the modification of DS DSCP properties to people with the `ds-dscp-modify` Permission, we can preserve existing behavior by granting that Permission to all Roles that already have the `ds-write` Permission which had been allowing them to do that, in the same migration that adds the new Permission. Or if we split servers into cache servers and "other", then people with the `servers-read` and `servers-write` Permissions would have, respectively, the read and write Permissions for both of the two new server categories.
> ... we shouldn't have to explicitly assign every single permission to the `admin` role... I guess as long as at least the admin role is special in that it implicitly has all possible permissions, we wouldn't have to jump through permissions hoops with every upgrade.
That's actually already true. The `admin`, `read-only`, and `operations` Roles are handled somewhat specially in that they are modified by `seeds.sql`, which is to say they're expected to exist in any new ATC installation. In particular, the `admin` Role has two special Permissions that no other Role is given (by default): `all-read` and `all-write` which are intended to convey exactly what it sounds like: `admin` can view and change anything. I didn't bring that up in the blueprint just because the concept already exists, if not implemented - but I can absolutely mention it if you like.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 commented on pull request #5848: User Permissions Blueprint
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on pull request #5848:
URL: https://github.com/apache/trafficcontrol/pull/5848#issuecomment-891943276
That would also preclude the tenancy of Roles, that you brought up earlier.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #5848: User Permissions Blueprint
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #5848:
URL: https://github.com/apache/trafficcontrol/pull/5848#discussion_r681194793
##########
File path: blueprints/user.permissions.md
##########
@@ -0,0 +1,286 @@
+<!--
+Licensed to the Apache Software Foundation (ASF) under one
+or more contributor license agreements. See the NOTICE file
+distributed with this work for additional information
+regarding copyright ownership. The ASF licenses this file
+to you under the Apache License, Version 2.0 (the
+"License"); you may not use this file except in compliance
+with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing,
+software distributed under the License is distributed on an
+"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+KIND, either express or implied. See the License for the
+specific language governing permissions and limitations
+under the License.
+-->
+# Enforced User Permissions
+
+## Problem Description
+Currently, the Permissions (currently "Capabilities", henceforth referred to as
+"Permissions" to avoid confusion with Server Capabilities) afforded to a user
+are defined by the "Privilege Level" of their Role. It essentially defines a
+scale on the interval [0,30], where a higher number allows more things. This
+isn't as scalable or configurable as it could be, and involves granting more
+permissions to a user than they actually need just because every Privilege
+Level encapsulates all of the permissions of every Privilege Level below it,
+making disjoint Permission sets for disparate Roles logically impossible.
+
+## Proposed Change
+Instead of a system of rigid supersets of Permissions, this blueprint proposes
+that the system of user "Capabilities" be expanded upon, renamed to Permissions
+(again, to avoid confusion) and enforced by the API.
+
+Under this system, each endpoint will declare the Permissions it requires for
+specific operations and check that the requesting user has those required, if
+any.
+
+This information is _not_ to be stored in the database nor exposed through the
+API, but documented. This change is discussed in greater detail in the
+"Alternatives" section.
+
+The enforcement of these Permissions is proposed to be configurable via an
+entry in the Traffic Ops configuration file. Thus, for one major version the
+API will need to support Permissions-based authorization as well as Privilege
+Level-based authorization.
+
+## Data Model Impact
+"Capabilities" as they are known today will be removed from the data model.
+Also, the current model of a Role will need to be modified. The current model
+is as follows:
+
+```typescript
+interface Role {
+ capabilities: Array<string> | null;
+ description: string | null;
+ id?: number | null;
+ name: string | null;
+ privLevel: number | null;
+}
+```
+
+The proposed new model is shown below.
+
+```typescript
+interface Role {
+ description: string;
+ lastUpdated?: Date;
+ name: string;
+ permissions: Array<string>;
+}
+```
+
+Also, because the ID is no longer exposed via the API, the model for users will
+need to change to only reference Role name, not ID. So it'll change from...
+
+```typescript
+interface User {
+ addressLine1: string | null;
+ addressLine2: string | null;
+ city: string | null;
+ company: string | null;
+ confirmLocalPassword?: string | null;
+ country: string | null;
+ email: string | null;
+ fullName: string | null;
+ gid: number | null;
+ id?: number | null;
+ lastUpdated?: string | null;
+ localPassword?: string | null;
+ newUser: boolean | null;
+ phoneNumber: string | null;
+ postalCode: string | null;
+ publicSSHKey: string | null;
+ registrationSent: string | null;
+ role: number | null;
+ roleName?: string | null;
+ stateOrProvince: string | null;
+ tenant?: string | null;
+ tenantID: string | null;
+ uid: number | null;
+ username: string | null;
+}
+```
+
+... to ...
+
+```typescript
+interface User {
+ addressLine1: string | null;
+ addressLine2: string | null;
+ city: string | null;
+ company: string | null;
+ confirmLocalPassword?: string | null;
+ country: string | null;
+ email: string | null;
+ fullName: string;
+ gid: number | null;
+ id?: number;
+ lastUpdated?: Date;
+ localPassword?: string | null;
+ newUser: boolean;
+ phoneNumber: string | null;
+ postalCode: string | null;
+ publicSSHKey: string | null;
+ registrationSent: string | null;
+ role: string;
+ stateOrProvince: string | null;
+ tenant?: string;
+ tenantID: number;
+ uid: number | null;
+ username: string;
+}
+```
+
+... replacing the required `role` field that was a Role ID with one that is
+the Role's name (and therefore removing `roleName` as it's now redundant).
+
+(Note that `lastUpdated` changed from `string | null` to `Date`, which
+represents the transition to using proper RFC3339 formatting, and also some
+fields which cannot be `null` for a valid user are no longer `| null`. Those
+aren't changes necessary to implement Permissions, but can be done easily at
+the same time as necessary changes to User.)
+
+## Component Impact
+This change primarily concerns Traffic Ops and Traffic Portal, though any
+client of the API will need to be aware of the changes to its authorization
+system.
+
+### Traffic Portal Impact
+There is a "Capabilities" view currently in Traffic Portal that provides a
+table of user Permissions, though it is not linked to the side navigation bar.
+This should be removed.
+
+The Roles' details pages will need to be augmented with controls for adding and
+removing Permissions. The Permissions available for adding to a user should
Review comment:
yeah, that should say Role.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] rawlinp merged pull request #5848: User Permissions Blueprint
Posted by GitBox <gi...@apache.org>.
rawlinp merged pull request #5848:
URL: https://github.com/apache/trafficcontrol/pull/5848
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #5848: User Permissions Blueprint
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #5848:
URL: https://github.com/apache/trafficcontrol/pull/5848#discussion_r681299425
##########
File path: blueprints/user.permissions.md
##########
@@ -0,0 +1,286 @@
+<!--
+Licensed to the Apache Software Foundation (ASF) under one
+or more contributor license agreements. See the NOTICE file
+distributed with this work for additional information
+regarding copyright ownership. The ASF licenses this file
+to you under the Apache License, Version 2.0 (the
+"License"); you may not use this file except in compliance
+with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing,
+software distributed under the License is distributed on an
+"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+KIND, either express or implied. See the License for the
+specific language governing permissions and limitations
+under the License.
+-->
+# Enforced User Permissions
+
+## Problem Description
+Currently, the Permissions (currently "Capabilities", henceforth referred to as
+"Permissions" to avoid confusion with Server Capabilities) afforded to a user
+are defined by the "Privilege Level" of their Role. It essentially defines a
+scale on the interval [0,30], where a higher number allows more things. This
+isn't as scalable or configurable as it could be, and involves granting more
+permissions to a user than they actually need just because every Privilege
+Level encapsulates all of the permissions of every Privilege Level below it,
+making disjoint Permission sets for disparate Roles logically impossible.
+
+## Proposed Change
+Instead of a system of rigid supersets of Permissions, this blueprint proposes
+that the system of user "Capabilities" be expanded upon, renamed to Permissions
+(again, to avoid confusion) and enforced by the API.
+
+Under this system, each endpoint will declare the Permissions it requires for
+specific operations and check that the requesting user has those required, if
+any.
+
+This information is _not_ to be stored in the database nor exposed through the
+API, but documented. This change is discussed in greater detail in the
+"Alternatives" section.
+
+The enforcement of these Permissions is proposed to be configurable via an
+entry in the Traffic Ops configuration file. Thus, for one major version the
+API will need to support Permissions-based authorization as well as Privilege
+Level-based authorization.
+
+## Data Model Impact
+"Capabilities" as they are known today will be removed from the data model.
+Also, the current model of a Role will need to be modified. The current model
+is as follows:
+
+```typescript
+interface Role {
+ capabilities: Array<string> | null;
+ description: string | null;
+ id?: number | null;
+ name: string | null;
+ privLevel: number | null;
+}
+```
+
+The proposed new model is shown below.
+
+```typescript
+interface Role {
+ description: string;
+ lastUpdated?: Date;
+ name: string;
+ permissions: Array<string>;
+}
+```
+
+Also, because the ID is no longer exposed via the API, the model for users will
+need to change to only reference Role name, not ID. So it'll change from...
+
+```typescript
+interface User {
+ addressLine1: string | null;
+ addressLine2: string | null;
+ city: string | null;
+ company: string | null;
+ confirmLocalPassword?: string | null;
+ country: string | null;
+ email: string | null;
+ fullName: string | null;
+ gid: number | null;
+ id?: number | null;
+ lastUpdated?: string | null;
+ localPassword?: string | null;
+ newUser: boolean | null;
+ phoneNumber: string | null;
+ postalCode: string | null;
+ publicSSHKey: string | null;
+ registrationSent: string | null;
+ role: number | null;
+ roleName?: string | null;
+ stateOrProvince: string | null;
+ tenant?: string | null;
+ tenantID: string | null;
+ uid: number | null;
+ username: string | null;
+}
+```
+
+... to ...
+
+```typescript
+interface User {
+ addressLine1: string | null;
+ addressLine2: string | null;
+ city: string | null;
+ company: string | null;
+ confirmLocalPassword?: string | null;
+ country: string | null;
+ email: string | null;
+ fullName: string;
+ gid: number | null;
+ id?: number;
+ lastUpdated?: Date;
+ localPassword?: string | null;
+ newUser: boolean;
+ phoneNumber: string | null;
+ postalCode: string | null;
+ publicSSHKey: string | null;
+ registrationSent: string | null;
+ role: string;
+ stateOrProvince: string | null;
+ tenant?: string;
+ tenantID: number;
+ uid: number | null;
+ username: string;
+}
+```
+
+... replacing the required `role` field that was a Role ID with one that is
+the Role's name (and therefore removing `roleName` as it's now redundant).
+
+(Note that `lastUpdated` changed from `string | null` to `Date`, which
+represents the transition to using proper RFC3339 formatting, and also some
+fields which cannot be `null` for a valid user are no longer `| null`. Those
+aren't changes necessary to implement Permissions, but can be done easily at
+the same time as necessary changes to User.)
+
+## Component Impact
+This change primarily concerns Traffic Ops and Traffic Portal, though any
+client of the API will need to be aware of the changes to its authorization
+system.
+
+### Traffic Portal Impact
+There is a "Capabilities" view currently in Traffic Portal that provides a
+table of user Permissions, though it is not linked to the side navigation bar.
+This should be removed.
+
+The Roles' details pages will need to be augmented with controls for adding and
+removing Permissions. The Permissions available for adding to a user should
+include an autocompletion of those known by Traffic Portal to exist, but
+shouldn't restrict the addition of Permissions not known to exist to accomodate
+running older versions of Traffic Portal with newer versions of Traffic Ops, as
+well as any plugins unbeknownst to Traffic Portal that add Traffic Ops API
+endpoints and may declare their own Permissions that do not exist in a vanilla
+Traffic Control environment.
+
+Also, the Roles table will be augmented with the ability to filter Roles by the
+presence or absence of a Permission, although the full itemization of the
+Permissions afforded to a Role would, in most cases, be far too large to
+comfortably display in the table itself.
+
+Under-the-hood, API services will need to change from using Role IDs to Role
+names.
+
+### Traffic Ops Impact
+Traffic Ops will reqire changes to its Roles and Permissions API, chiefly, and
+the database tables that back them. Enforcement of API Permissions, though,
+will also have more wide-ranging impact that will require changes to very
+nearly every API endpoint.
+
+Additionally, a configuation file field will be added named `usePermissions`
+that is an optional boolean which, when present and `true` causes Traffic Ops
+to use Permissions rather than Role Privilege Level to determine a user's
+authorization for a given operation.
+
+#### API Impact
+Structurally, the only necessary changes are to the `/roles` and `/users`
+endpoints, which will need to be updated to output structures consistent with
+the changes outlined in the Data Model Impact section. The `/capabilities` and
+`/api_capabilities` endpoints will be removed (and we might consider renaming
Review comment:
I definitely remember the confusion between Capabilities and Server Capabilities being discussed explicitly when deciding on the names of API routes - but that doesn't really matter. I just thought it was worth calling out, but it's a waste of everyone's time to have this point of contention for something that really is pretty unrelated to what the blueprint's actually about. I'll just remove it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] mitchell852 commented on a change in pull request #5848: User Permissions Blueprint
Posted by GitBox <gi...@apache.org>.
mitchell852 commented on a change in pull request #5848:
URL: https://github.com/apache/trafficcontrol/pull/5848#discussion_r680196470
##########
File path: blueprints/user.permissions.md
##########
@@ -0,0 +1,286 @@
+<!--
+Licensed to the Apache Software Foundation (ASF) under one
+or more contributor license agreements. See the NOTICE file
+distributed with this work for additional information
+regarding copyright ownership. The ASF licenses this file
+to you under the Apache License, Version 2.0 (the
+"License"); you may not use this file except in compliance
+with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing,
+software distributed under the License is distributed on an
+"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+KIND, either express or implied. See the License for the
+specific language governing permissions and limitations
+under the License.
+-->
+# Enforced User Permissions
+
+## Problem Description
+Currently, the Permissions (currently "Capabilities", henceforth referred to as
+"Permissions" to avoid confusion with Server Capabilities) afforded to a user
+are defined by the "Privilege Level" of their Role. It essentially defines a
+scale on the interval [0,30], where a higher number allows more things. This
+isn't as scalable or configurable as it could be, and involves granting more
+permissions to a user than they actually need just because every Privilege
+Level encapsulates all of the permissions of every Privilege Level below it,
+making disjoint Permission sets for disparate Roles logically impossible.
+
+## Proposed Change
+Instead of a system of rigid supersets of Permissions, this blueprint proposes
+that the system of user "Capabilities" be expanded upon, renamed to Permissions
+(again, to avoid confusion) and enforced by the API.
+
+Under this system, each endpoint will declare the Permissions it requires for
+specific operations and check that the requesting user has those required, if
+any.
+
+This information is _not_ to be stored in the database nor exposed through the
Review comment:
what exactly is documented? the perm(s) required of each api? the role to perm mappings? not sure how the 2nd can be documented as i thought it was dynamic.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #5848: User Permissions Blueprint
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #5848:
URL: https://github.com/apache/trafficcontrol/pull/5848#discussion_r681252784
##########
File path: blueprints/user.permissions.md
##########
@@ -0,0 +1,286 @@
+<!--
+Licensed to the Apache Software Foundation (ASF) under one
+or more contributor license agreements. See the NOTICE file
+distributed with this work for additional information
+regarding copyright ownership. The ASF licenses this file
+to you under the Apache License, Version 2.0 (the
+"License"); you may not use this file except in compliance
+with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing,
+software distributed under the License is distributed on an
+"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+KIND, either express or implied. See the License for the
+specific language governing permissions and limitations
+under the License.
+-->
+# Enforced User Permissions
+
+## Problem Description
+Currently, the Permissions (currently "Capabilities", henceforth referred to as
+"Permissions" to avoid confusion with Server Capabilities) afforded to a user
+are defined by the "Privilege Level" of their Role. It essentially defines a
+scale on the interval [0,30], where a higher number allows more things. This
+isn't as scalable or configurable as it could be, and involves granting more
+permissions to a user than they actually need just because every Privilege
+Level encapsulates all of the permissions of every Privilege Level below it,
+making disjoint Permission sets for disparate Roles logically impossible.
+
+## Proposed Change
+Instead of a system of rigid supersets of Permissions, this blueprint proposes
+that the system of user "Capabilities" be expanded upon, renamed to Permissions
+(again, to avoid confusion) and enforced by the API.
+
+Under this system, each endpoint will declare the Permissions it requires for
+specific operations and check that the requesting user has those required, if
+any.
+
+This information is _not_ to be stored in the database nor exposed through the
+API, but documented. This change is discussed in greater detail in the
+"Alternatives" section.
+
+The enforcement of these Permissions is proposed to be configurable via an
+entry in the Traffic Ops configuration file. Thus, for one major version the
+API will need to support Permissions-based authorization as well as Privilege
+Level-based authorization.
+
+## Data Model Impact
+"Capabilities" as they are known today will be removed from the data model.
+Also, the current model of a Role will need to be modified. The current model
+is as follows:
+
+```typescript
+interface Role {
+ capabilities: Array<string> | null;
+ description: string | null;
+ id?: number | null;
+ name: string | null;
+ privLevel: number | null;
+}
+```
+
+The proposed new model is shown below.
+
+```typescript
+interface Role {
+ description: string;
+ lastUpdated?: Date;
+ name: string;
+ permissions: Array<string>;
+}
+```
+
+Also, because the ID is no longer exposed via the API, the model for users will
+need to change to only reference Role name, not ID. So it'll change from...
+
+```typescript
+interface User {
+ addressLine1: string | null;
+ addressLine2: string | null;
+ city: string | null;
+ company: string | null;
+ confirmLocalPassword?: string | null;
+ country: string | null;
+ email: string | null;
+ fullName: string | null;
+ gid: number | null;
+ id?: number | null;
+ lastUpdated?: string | null;
+ localPassword?: string | null;
+ newUser: boolean | null;
+ phoneNumber: string | null;
+ postalCode: string | null;
+ publicSSHKey: string | null;
+ registrationSent: string | null;
+ role: number | null;
+ roleName?: string | null;
+ stateOrProvince: string | null;
+ tenant?: string | null;
+ tenantID: string | null;
+ uid: number | null;
+ username: string | null;
+}
+```
+
+... to ...
+
+```typescript
+interface User {
+ addressLine1: string | null;
+ addressLine2: string | null;
+ city: string | null;
+ company: string | null;
+ confirmLocalPassword?: string | null;
+ country: string | null;
+ email: string | null;
+ fullName: string;
+ gid: number | null;
+ id?: number;
+ lastUpdated?: Date;
+ localPassword?: string | null;
+ newUser: boolean;
+ phoneNumber: string | null;
+ postalCode: string | null;
+ publicSSHKey: string | null;
+ registrationSent: string | null;
+ role: string;
+ stateOrProvince: string | null;
+ tenant?: string;
+ tenantID: number;
+ uid: number | null;
+ username: string;
+}
+```
+
+... replacing the required `role` field that was a Role ID with one that is
+the Role's name (and therefore removing `roleName` as it's now redundant).
+
+(Note that `lastUpdated` changed from `string | null` to `Date`, which
+represents the transition to using proper RFC3339 formatting, and also some
+fields which cannot be `null` for a valid user are no longer `| null`. Those
+aren't changes necessary to implement Permissions, but can be done easily at
+the same time as necessary changes to User.)
+
+## Component Impact
+This change primarily concerns Traffic Ops and Traffic Portal, though any
+client of the API will need to be aware of the changes to its authorization
+system.
+
+### Traffic Portal Impact
+There is a "Capabilities" view currently in Traffic Portal that provides a
+table of user Permissions, though it is not linked to the side navigation bar.
+This should be removed.
+
+The Roles' details pages will need to be augmented with controls for adding and
+removing Permissions. The Permissions available for adding to a user should
+include an autocompletion of those known by Traffic Portal to exist, but
+shouldn't restrict the addition of Permissions not known to exist to accomodate
+running older versions of Traffic Portal with newer versions of Traffic Ops, as
+well as any plugins unbeknownst to Traffic Portal that add Traffic Ops API
+endpoints and may declare their own Permissions that do not exist in a vanilla
+Traffic Control environment.
+
+Also, the Roles table will be augmented with the ability to filter Roles by the
+presence or absence of a Permission, although the full itemization of the
+Permissions afforded to a Role would, in most cases, be far too large to
+comfortably display in the table itself.
+
+Under-the-hood, API services will need to change from using Role IDs to Role
+names.
+
+### Traffic Ops Impact
+Traffic Ops will reqire changes to its Roles and Permissions API, chiefly, and
+the database tables that back them. Enforcement of API Permissions, though,
+will also have more wide-ranging impact that will require changes to very
+nearly every API endpoint.
+
+Additionally, a configuation file field will be added named `usePermissions`
+that is an optional boolean which, when present and `true` causes Traffic Ops
+to use Permissions rather than Role Privilege Level to determine a user's
+authorization for a given operation.
+
+#### API Impact
+Structurally, the only necessary changes are to the `/roles` and `/users`
+endpoints, which will need to be updated to output structures consistent with
+the changes outlined in the Data Model Impact section. The `/capabilities` and
+`/api_capabilities` endpoints will be removed (and we might consider renaming
+the `/server_capabilities` and `/server_server_capabilities` to drop the
+now-unnecessary "server_" qualifier).
+
+The more pervasive changes will be to all authenticated API endpoints which
+shall be updated to respect Permissions given the correct configuration
+setting.
+
+The exact Permissions that need to exist for each endpoint are best left to
+debate on the changeset that implements them.
+
+When a user attempts an operation for which they do not have sufficient
+Permissions, the API MUST respond with a `403 Forbidden` response containing an
+error-level Alert that describes what operation is not permitted and what
+Permission the user is missing that would allow them to proceed.
+
+#### Client Impact
+As Permissions are defined by the Role of the authenticated user, the only
+client changes are necessary beyond those necessitated by the removal of two
+API endpoints and the renaming and restructuring of a third are to stop
+filling in a Role ID on user creation/update from a Role name if an ID was
+unset (this will now be handled by the API itself, which will save a full
+HTTP request on each user creation/modification).
+
+#### Database Impact
+The new model for a Role does not allow a `null` description; a simple
+migration that coalesces existing `NULL` values to an empty string and adds a
+check constraint should be all that's actually required. No other immediate
+changes should be made - including dropping the now-unused `api_capability`
+table -, since old API versions will still need access to the deprecated
+columns. However, the foreign key constraint on the `role_capability` table
+that links a "Capability" name to a row in the `capability` table should be
+dropped, as that is no longer the source of truth for valid Permissions.
+
+Optionally, a migration should really be added to make a user's username
+`NOT NULL`, since that field is actually required by the API and many things
+will break if it's `NULL`.
+
+## Documentation Impact
+The new configuration option will need to be documented, documentation for
+removed API endpoints will itself need to be removed, documentation for the
+`/roles` and `/users` endpoints will need to be updated to reflect the new
+request and response structures, and as Permissions are implemented on each
+endpoint the Permissions it requires for various actions will need to be
+defined.
+
+## Testing Impact
+The most significant testing changes will need to be made to the Go Traffic Ops
+client integration tests, which should verify each endpoint's proper
+Permissions-based authorization. Traffic Portal functionality will also require
+the appropriate end-to-end tests.
+
+## Performance Impact
+No significant performance impact is expected, since the Permissions of a user
+are already queried at the time of servicing a request by every authenticated
+endpoint. Some negligible, constant time will need to be spend determining how
+to authorize an authenticated user.
+
+## Security Impact
+Careful consideration must be given to the design and implementation of each
+Permission. For example, this author believes a Permission named
+`do-secrets-things` that allows a Role unrestricted read and write access to
+any and all DNSSEC, SSL, URL Signature, and URI signing keys would be a poor
+design from a security standpoint. Permissions should be broad enough to
+encompass a single, well-defined purpose, and no more. In many cases, though,
+the existing "Capabilities" concepts will be good enough to build from (e.g.
+`delivery-services-write`, `cdns-read`).
+
+## Upgrade Impact
+There will be a database migration to run, but since the default configuration
+will be to ignore Permissions and just use Privilege Level, there isn't much in
+the way of upgrade impact immediately. The bigger step is getting ready for the
+_next_ upgrade, when Permissions stop being optional.
+
+## Operations Impact
Review comment:
Hopefully that's mostly taken care of by re-using the `role_capability` table, but yeah, we'll need to be more explicit in case people have created other Roles. Seems like the safest thing to do is to "round down" the amount of privilege a Role grants. If a Role has a priv_level that isn't found exactly in routes.go, then we'll need to give it the same capabilities as the next-lowest that is, because we can't know the intention of such levels.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 commented on pull request #5848: User Permissions Blueprint
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on pull request #5848:
URL: https://github.com/apache/trafficcontrol/pull/5848#issuecomment-898005069
I'm good with all that, I just don't know what the upgrade path looks like. If there's multiple Roles with privLevel >= 30, do we delete all but one and rename it to `admin` (unless there's already one named `admin`)? If `admin` already exists but has privLevel < 30, do we escalate its privileges? Either way you're destroying data which makes downgrade difficult, though the first case is a bigger deal for downgrading than the second.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #5848: User Permissions Blueprint
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #5848:
URL: https://github.com/apache/trafficcontrol/pull/5848#discussion_r673340969
##########
File path: blueprints/user.permissions.md
##########
@@ -0,0 +1,205 @@
+<!--
+Licensed to the Apache Software Foundation (ASF) under one
+or more contributor license agreements. See the NOTICE file
+distributed with this work for additional information
+regarding copyright ownership. The ASF licenses this file
+to you under the Apache License, Version 2.0 (the
+"License"); you may not use this file except in compliance
+with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing,
+software distributed under the License is distributed on an
+"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+KIND, either express or implied. See the License for the
+specific language governing permissions and limitations
+under the License.
+-->
+# Enforced User Permissions
+
+## Problem Description
+Currently, the Permissions (currently "Capabilities", henceforth referred to as
+"Permissions" to avoid confusion with Server Capabilities) afforded to a user
+are defined by the "Privilege Level" of their Role. It essentially defines a
+scale on the interval [0,30], where a higher number allows more things. This
+isn't as scalable or configurable as it could be, and involves granting more
+permissions to a user than they actually need just because every Privilege
+Level encapsulates all of the permissions of every Privilege Level below it,
+making disjoint Permission sets for disparate Roles logically impossible.
+
+## Proposed Change
+Instead of a system of rigid supersets of Permissions, this blueprint proposes
+that the system of user "Capabilities" be expanded upon, renamed to Permissions
+(again, to avoid confusion) and enforced by the API.
+
+Under this system, each endpoint will declare the Permissions it requires for
+specific operations and check that the requesting user has those required, if
+any.
+
+This information is _not_ to be stored in the database nor exposed through the
+API, but documented. This change is discussed in greater detail in the
+"Alternatives" section.
+
+The enforcement of these Permissions is proposed to be configurable via an
+entry in the Traffic Ops configuration file. Thus, for one major version the
+API will need to support Permissions-based authorization as well as Privilege
+Level-based authorization.
+
+## Data Model Impact
+"Capabilities" as they are known today will be removed from the data model.
+Also, the current model of a Role will need to be modified. The current model
+is as follows<sup>1</sup>:
+
+```typescript
+interface Role {
+ capabilities: Array<string>;
+ description: string | null;
+ id?: number;
+ name: string;
+ privLevel: number;
+}
+```
+
+The proposed new model is shown below<sup>1</sup>.
+
+```typescript
+interface Role {
+ description: string;
+ name: string;
+ permissions: Array<string>;
+}
+```
+
+## Component Impact
+This change primarily concerns Traffic Ops and Traffic Portal, though any
+client of the API will need to be aware of the changes to its authorization
+system.
+
+### Traffic Portal Impact
+There is a "Capabilities" view currently in Traffic Portal that provides a
+table of user Permissions, though it is not linked to the side navigation bar.
+This should be removed.
+
+The Roles' details pages will need to be augmented with controls for adding and
+removing Permissions. The Permissions available for adding to a user should
+include an autocompletion of those known by Traffic Portal to exist, but
+shouldn't restrict the addition of Permissions not known to exist to accomodate
+running older versions of Traffic Portal with newer versions of Traffic Ops, as
+well as any plugins unbeknownst to Traffic Portal that add Traffic Ops API
+endpoints and may declare their own Permissions that do not exist in a vanilla
+Traffic Control environment.
+
+Also, the Roles table will be augmented with the ability to filter Roles by the
+presence or absence of a Permission, although the full itemization of the
+Permissions afforded to a Role would, in most cases, be far too large to
+comfortably display in the table itself.
+
+### Traffic Ops Impact
+Traffic Ops will reqire changes to its Roles and Permissions API, chiefly, and
+the database tables that back them. Enforcement of API Permissions, though,
+will also have more wide-ranging impact that will require changes to very
+nearly every API endpoint.
+
+Additionally, a configuation file field will be added named `usePermissions`
+that is an optional boolean which, when present and `true` causes Traffic Ops
+to use Permissions rather than Role Privilege Level to determine a user's
+authorization for a given operation.
+
+#### API Impact
+Structurally, the only necessary changes are to the `/roles` endpoint, which
+will need to be updated to output structures consistent with the changes
+outlined in the Data Model Impact section. The `/capabilities` and
+`/api_capabilities` endpoints will be removed (and we might consider renaming
+the `/server_capabilities` and `/server_server_capabilities` to drop the
+now-unnecessary "server_" qualifier).
+
+The more pervasive changes will be to all authenticated API endpoints which
+shall be updated to respect Permissions given the correct configuration
+setting.
+
+The exact Permissions that need to exist for each endpoint are best left to
+debate on the changeset that implements them.
+
+When a user attempts an operation for which they do not have sufficient
+Permissions, the API MUST respond with a `403 Forbidden` response containing an
+error-level Alert that describes what operation is not permitted and what
+Permission the user is missing that would allow them to proceed.
+
+#### Client Impact
+As Permissions are defined by the Role of the authenticated user, no client
+changes are necessary beyond those necessitated by the removal of two API
+endpoints and the renaming and restructuring of a third.
+
+#### Database Impact
+The new model for a Role does not allow a `null` description; a simple
+migration that coalesces existing `NULL` values to an empty string and adds a
+check constraint should be all that's actually required. No other immediate
+changes should be made, since old API versions will still need access to the
+deprecated colunms. However, the foreign key constraint on the
+`role_capability` table that links a "Capability" name to a row in the
+`capability` table should be dropped, as that is no longer the source of truth
+for valid Permissions.
+
+## Documentation Impact
+The new configuration option will need to be documented, documentation for
+removed API endpoints will itself need to be removed, documentation for the
+`/roles` endpoint will need to be updated to reflect the new request and
+response structures, and as Permissions are implemented on each endpoint the
+Permissions it requires for various actions will need to be defined.
+
+## Testing Impact
+The most significant testing changes will need to be made to the Go Traffic Ops
+client integration tests, which should verify each endpoint's proper
+Permissions-based authorization. Traffic Portal functionality will also require
+the appropriate end-to-end tests.
+
+## Performance Impact
+No significant performance impact is expected, since the Permissions of a user
+are already queried at the time of servicing a request by every authenticated
+endpoint. Some negligible, constant time will need to be spend determining how
+to authorize an authenticated user.
+
+## Security Impact
+Careful consideration must be given to the design and implementation of each
+Permission. For example, this author believes a Permission named
+`do-secrets-things` that allows a Role unrestricted read and write access to
+any and all DNSSEC, SSL, URL Signature, and URI signing keys would be a poor
+design from a security standpoint. Permissions should be broad enough to
+encompass a single, well-defined purpose, and no more. In many cases, though,
+the existing "Capabilities" concepts will be good enough to build from (e.g.
+`delivery-services-write`, `cdns-read`).
+
+## Upgrade Impact
+There will be a database migration to run, but since the default configuration
+will be to ignore Permissions and just use Privilege Level, there isn't much in
+the way of upgrade impact immediately. The bigger step is getting ready for the
+_next_ upgrade, when Permissions stop being optional.
+
+## Operations Impact
+Before enabling Permissions, operators will need to ensure that all Roles have
+the appropriate Permissions to accomplish their necessary tasks. They will also
+need to be aware of the Permissions required to accomplish any given task, or
+at least where in the documentation to look for guidance in that regard.
+
+### Developer Impact
+From now on, new endpoints will need to be designed with their Permissions in
+mind. For many endpoints this will probably be trivial, but must always be at
+least considered.
+
+## Alternatives
+The current system of "Capabilities", if enforced, was a possible alternative
+to the system herein described. However, that system lacks the ability to
+express a user's permission to do something beyond a combination of HTTP
+request method and path. For example, the CDN Locks blueprint (#5834) proposes
+a system by which a user may delete a lock that they created, but also states
+that "`admin` users" (Privilege Level 30 or above) can delete any lock created
+by any user. However, these two permissions use the same request method and
+path, meaning that under the current system it would be impossible to give
+admin users the ability to do that overriding deletion without also giving it
+to everyone else. The system described here is far more flexible. It could even
+eliminate the need for the `/deliveryservices/safe` API endpoint, which can be
+expressed instead as two separate Permissions: one that allows making changes
+to the "safe" Delivery Service fields and one that allows all others.
+
+[1] `lastUpdated` is omitted as it's a read-only, response-only field.
Review comment:
I just mean it's omitted from the example
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] mitchell852 commented on a change in pull request #5848: User Permissions Blueprint
Posted by GitBox <gi...@apache.org>.
mitchell852 commented on a change in pull request #5848:
URL: https://github.com/apache/trafficcontrol/pull/5848#discussion_r680197185
##########
File path: blueprints/user.permissions.md
##########
@@ -0,0 +1,286 @@
+<!--
+Licensed to the Apache Software Foundation (ASF) under one
+or more contributor license agreements. See the NOTICE file
+distributed with this work for additional information
+regarding copyright ownership. The ASF licenses this file
+to you under the Apache License, Version 2.0 (the
+"License"); you may not use this file except in compliance
+with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing,
+software distributed under the License is distributed on an
+"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+KIND, either express or implied. See the License for the
+specific language governing permissions and limitations
+under the License.
+-->
+# Enforced User Permissions
+
+## Problem Description
+Currently, the Permissions (currently "Capabilities", henceforth referred to as
+"Permissions" to avoid confusion with Server Capabilities) afforded to a user
+are defined by the "Privilege Level" of their Role. It essentially defines a
+scale on the interval [0,30], where a higher number allows more things. This
+isn't as scalable or configurable as it could be, and involves granting more
+permissions to a user than they actually need just because every Privilege
+Level encapsulates all of the permissions of every Privilege Level below it,
+making disjoint Permission sets for disparate Roles logically impossible.
+
+## Proposed Change
+Instead of a system of rigid supersets of Permissions, this blueprint proposes
+that the system of user "Capabilities" be expanded upon, renamed to Permissions
+(again, to avoid confusion) and enforced by the API.
+
+Under this system, each endpoint will declare the Permissions it requires for
+specific operations and check that the requesting user has those required, if
+any.
+
+This information is _not_ to be stored in the database nor exposed through the
+API, but documented. This change is discussed in greater detail in the
+"Alternatives" section.
+
+The enforcement of these Permissions is proposed to be configurable via an
+entry in the Traffic Ops configuration file. Thus, for one major version the
+API will need to support Permissions-based authorization as well as Privilege
+Level-based authorization.
+
+## Data Model Impact
+"Capabilities" as they are known today will be removed from the data model.
+Also, the current model of a Role will need to be modified. The current model
+is as follows:
+
+```typescript
+interface Role {
+ capabilities: Array<string> | null;
+ description: string | null;
+ id?: number | null;
+ name: string | null;
+ privLevel: number | null;
+}
+```
+
+The proposed new model is shown below.
+
+```typescript
+interface Role {
+ description: string;
+ lastUpdated?: Date;
+ name: string;
+ permissions: Array<string>;
+}
+```
+
+Also, because the ID is no longer exposed via the API, the model for users will
+need to change to only reference Role name, not ID. So it'll change from...
+
+```typescript
+interface User {
+ addressLine1: string | null;
+ addressLine2: string | null;
+ city: string | null;
+ company: string | null;
+ confirmLocalPassword?: string | null;
+ country: string | null;
+ email: string | null;
+ fullName: string | null;
+ gid: number | null;
+ id?: number | null;
+ lastUpdated?: string | null;
+ localPassword?: string | null;
+ newUser: boolean | null;
+ phoneNumber: string | null;
+ postalCode: string | null;
+ publicSSHKey: string | null;
+ registrationSent: string | null;
+ role: number | null;
+ roleName?: string | null;
+ stateOrProvince: string | null;
+ tenant?: string | null;
+ tenantID: string | null;
+ uid: number | null;
+ username: string | null;
+}
+```
+
+... to ...
+
+```typescript
+interface User {
+ addressLine1: string | null;
+ addressLine2: string | null;
+ city: string | null;
+ company: string | null;
+ confirmLocalPassword?: string | null;
+ country: string | null;
+ email: string | null;
+ fullName: string;
+ gid: number | null;
+ id?: number;
+ lastUpdated?: Date;
+ localPassword?: string | null;
+ newUser: boolean;
+ phoneNumber: string | null;
+ postalCode: string | null;
+ publicSSHKey: string | null;
+ registrationSent: string | null;
+ role: string;
+ stateOrProvince: string | null;
+ tenant?: string;
+ tenantID: number;
+ uid: number | null;
+ username: string;
+}
+```
+
+... replacing the required `role` field that was a Role ID with one that is
+the Role's name (and therefore removing `roleName` as it's now redundant).
+
+(Note that `lastUpdated` changed from `string | null` to `Date`, which
+represents the transition to using proper RFC3339 formatting, and also some
+fields which cannot be `null` for a valid user are no longer `| null`. Those
+aren't changes necessary to implement Permissions, but can be done easily at
+the same time as necessary changes to User.)
+
+## Component Impact
+This change primarily concerns Traffic Ops and Traffic Portal, though any
+client of the API will need to be aware of the changes to its authorization
+system.
+
+### Traffic Portal Impact
+There is a "Capabilities" view currently in Traffic Portal that provides a
+table of user Permissions, though it is not linked to the side navigation bar.
+This should be removed.
+
+The Roles' details pages will need to be augmented with controls for adding and
+removing Permissions. The Permissions available for adding to a user should
Review comment:
> The Permissions available for adding to a user
this makes it sound like you add perms _directly_ to a user. is that true? or do you add perms to a role (which belongs to a user?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] mitchell852 commented on a change in pull request #5848: User Permissions Blueprint
Posted by GitBox <gi...@apache.org>.
mitchell852 commented on a change in pull request #5848:
URL: https://github.com/apache/trafficcontrol/pull/5848#discussion_r680198376
##########
File path: blueprints/user.permissions.md
##########
@@ -0,0 +1,286 @@
+<!--
+Licensed to the Apache Software Foundation (ASF) under one
+or more contributor license agreements. See the NOTICE file
+distributed with this work for additional information
+regarding copyright ownership. The ASF licenses this file
+to you under the Apache License, Version 2.0 (the
+"License"); you may not use this file except in compliance
+with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing,
+software distributed under the License is distributed on an
+"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+KIND, either express or implied. See the License for the
+specific language governing permissions and limitations
+under the License.
+-->
+# Enforced User Permissions
+
+## Problem Description
+Currently, the Permissions (currently "Capabilities", henceforth referred to as
+"Permissions" to avoid confusion with Server Capabilities) afforded to a user
+are defined by the "Privilege Level" of their Role. It essentially defines a
+scale on the interval [0,30], where a higher number allows more things. This
+isn't as scalable or configurable as it could be, and involves granting more
+permissions to a user than they actually need just because every Privilege
+Level encapsulates all of the permissions of every Privilege Level below it,
+making disjoint Permission sets for disparate Roles logically impossible.
+
+## Proposed Change
+Instead of a system of rigid supersets of Permissions, this blueprint proposes
+that the system of user "Capabilities" be expanded upon, renamed to Permissions
+(again, to avoid confusion) and enforced by the API.
+
+Under this system, each endpoint will declare the Permissions it requires for
+specific operations and check that the requesting user has those required, if
+any.
+
+This information is _not_ to be stored in the database nor exposed through the
+API, but documented. This change is discussed in greater detail in the
+"Alternatives" section.
+
+The enforcement of these Permissions is proposed to be configurable via an
+entry in the Traffic Ops configuration file. Thus, for one major version the
+API will need to support Permissions-based authorization as well as Privilege
+Level-based authorization.
+
+## Data Model Impact
+"Capabilities" as they are known today will be removed from the data model.
+Also, the current model of a Role will need to be modified. The current model
+is as follows:
+
+```typescript
+interface Role {
+ capabilities: Array<string> | null;
+ description: string | null;
+ id?: number | null;
+ name: string | null;
+ privLevel: number | null;
+}
+```
+
+The proposed new model is shown below.
+
+```typescript
+interface Role {
+ description: string;
+ lastUpdated?: Date;
+ name: string;
+ permissions: Array<string>;
+}
+```
+
+Also, because the ID is no longer exposed via the API, the model for users will
+need to change to only reference Role name, not ID. So it'll change from...
+
+```typescript
+interface User {
+ addressLine1: string | null;
+ addressLine2: string | null;
+ city: string | null;
+ company: string | null;
+ confirmLocalPassword?: string | null;
+ country: string | null;
+ email: string | null;
+ fullName: string | null;
+ gid: number | null;
+ id?: number | null;
+ lastUpdated?: string | null;
+ localPassword?: string | null;
+ newUser: boolean | null;
+ phoneNumber: string | null;
+ postalCode: string | null;
+ publicSSHKey: string | null;
+ registrationSent: string | null;
+ role: number | null;
+ roleName?: string | null;
+ stateOrProvince: string | null;
+ tenant?: string | null;
+ tenantID: string | null;
+ uid: number | null;
+ username: string | null;
+}
+```
+
+... to ...
+
+```typescript
+interface User {
+ addressLine1: string | null;
+ addressLine2: string | null;
+ city: string | null;
+ company: string | null;
+ confirmLocalPassword?: string | null;
+ country: string | null;
+ email: string | null;
+ fullName: string;
+ gid: number | null;
+ id?: number;
+ lastUpdated?: Date;
+ localPassword?: string | null;
+ newUser: boolean;
+ phoneNumber: string | null;
+ postalCode: string | null;
+ publicSSHKey: string | null;
+ registrationSent: string | null;
+ role: string;
+ stateOrProvince: string | null;
+ tenant?: string;
+ tenantID: number;
+ uid: number | null;
+ username: string;
+}
+```
+
+... replacing the required `role` field that was a Role ID with one that is
+the Role's name (and therefore removing `roleName` as it's now redundant).
+
+(Note that `lastUpdated` changed from `string | null` to `Date`, which
+represents the transition to using proper RFC3339 formatting, and also some
+fields which cannot be `null` for a valid user are no longer `| null`. Those
+aren't changes necessary to implement Permissions, but can be done easily at
+the same time as necessary changes to User.)
+
+## Component Impact
+This change primarily concerns Traffic Ops and Traffic Portal, though any
+client of the API will need to be aware of the changes to its authorization
+system.
+
+### Traffic Portal Impact
+There is a "Capabilities" view currently in Traffic Portal that provides a
+table of user Permissions, though it is not linked to the side navigation bar.
+This should be removed.
+
+The Roles' details pages will need to be augmented with controls for adding and
+removing Permissions. The Permissions available for adding to a user should
+include an autocompletion of those known by Traffic Portal to exist, but
+shouldn't restrict the addition of Permissions not known to exist to accomodate
+running older versions of Traffic Portal with newer versions of Traffic Ops, as
+well as any plugins unbeknownst to Traffic Portal that add Traffic Ops API
+endpoints and may declare their own Permissions that do not exist in a vanilla
+Traffic Control environment.
+
+Also, the Roles table will be augmented with the ability to filter Roles by the
+presence or absence of a Permission, although the full itemization of the
+Permissions afforded to a Role would, in most cases, be far too large to
+comfortably display in the table itself.
+
+Under-the-hood, API services will need to change from using Role IDs to Role
+names.
+
+### Traffic Ops Impact
+Traffic Ops will reqire changes to its Roles and Permissions API, chiefly, and
+the database tables that back them. Enforcement of API Permissions, though,
+will also have more wide-ranging impact that will require changes to very
+nearly every API endpoint.
+
+Additionally, a configuation file field will be added named `usePermissions`
+that is an optional boolean which, when present and `true` causes Traffic Ops
+to use Permissions rather than Role Privilege Level to determine a user's
+authorization for a given operation.
+
+#### API Impact
+Structurally, the only necessary changes are to the `/roles` and `/users`
+endpoints, which will need to be updated to output structures consistent with
+the changes outlined in the Data Model Impact section. The `/capabilities` and
+`/api_capabilities` endpoints will be removed (and we might consider renaming
Review comment:
> and we might consider renaming the `/server_capabilities` and `/server_server_capabilities` to drop the now-unnecessary "server_" qualifier
seems beyond the scope of this change. any reason to mention it at all?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] rawlinp commented on pull request #5848: User Permissions Blueprint
Posted by GitBox <gi...@apache.org>.
rawlinp commented on pull request #5848:
URL: https://github.com/apache/trafficcontrol/pull/5848#issuecomment-893727312
For the record, I'm not -1 on having permissions in the DB; I just think config-based permissions have some benefits worth considering as an alternative. That is why we have the `## Alternatives` section in the blueprint template -- to explain how we weighed the pros/cons and why we chose one alternative over the other. Config-file based API permissions are not even my idea; I just recalled them from my OpenStack days: https://docs.openstack.org/oslo.policy/latest/admin/policy-yaml-file.html.
The config file idea is more about security and performance than it is about dev/maintenance savings. However, I've raised some OpEx concerns as well, in that if we remove the legacy priv-level based permissions, we'll need to manually add any new permissions from new APIs after we upgrade, in order to be able to use them. For example, `admin` users should generally just have all permissions; we shouldn't have to explicitly assign every single permission to the `admin` role. The same probably applies to the `operations` role which might as well be `admin` anyways. However, for roles like `readonly` and `portal`, we probably wouldn't need to add more permissions as we add more routes. For the `disallowed` role, which by definition has no permissions, should we even allow assigning permissions to it?
I guess as long as at least the `admin` role is special in that it implicitly has all possible permissions, we wouldn't have to jump through permissions hoops with every upgrade. That would alleviate some of my concerns with this proposal.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #5848: User Permissions Blueprint
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #5848:
URL: https://github.com/apache/trafficcontrol/pull/5848#discussion_r681251009
##########
File path: blueprints/user.permissions.md
##########
@@ -0,0 +1,286 @@
+<!--
+Licensed to the Apache Software Foundation (ASF) under one
+or more contributor license agreements. See the NOTICE file
+distributed with this work for additional information
+regarding copyright ownership. The ASF licenses this file
+to you under the Apache License, Version 2.0 (the
+"License"); you may not use this file except in compliance
+with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing,
+software distributed under the License is distributed on an
+"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+KIND, either express or implied. See the License for the
+specific language governing permissions and limitations
+under the License.
+-->
+# Enforced User Permissions
+
+## Problem Description
+Currently, the Permissions (currently "Capabilities", henceforth referred to as
+"Permissions" to avoid confusion with Server Capabilities) afforded to a user
+are defined by the "Privilege Level" of their Role. It essentially defines a
+scale on the interval [0,30], where a higher number allows more things. This
+isn't as scalable or configurable as it could be, and involves granting more
+permissions to a user than they actually need just because every Privilege
+Level encapsulates all of the permissions of every Privilege Level below it,
+making disjoint Permission sets for disparate Roles logically impossible.
+
+## Proposed Change
+Instead of a system of rigid supersets of Permissions, this blueprint proposes
+that the system of user "Capabilities" be expanded upon, renamed to Permissions
+(again, to avoid confusion) and enforced by the API.
+
+Under this system, each endpoint will declare the Permissions it requires for
+specific operations and check that the requesting user has those required, if
+any.
+
+This information is _not_ to be stored in the database nor exposed through the
Review comment:
yes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 edited a comment on pull request #5848: User Permissions Blueprint
Posted by GitBox <gi...@apache.org>.
ocket8888 edited a comment on pull request #5848:
URL: https://github.com/apache/trafficcontrol/pull/5848#issuecomment-891932043
If Roles are taken out of the API, then would we not still need to make API changes, so that the configuration file contents can be returned through the API? It's less work than manipulable objects from the database, to be sure, but it's not nothing. How would we migrate existing Roles to the on-disk configuration? How could that migration be undone for downgrades? What happens to users with the "Foo" Role if that Role is removed from the configuration file? Testing this would also be difficult to do in the current framework, since creating or deleting Roles would require restarting Traffic Ops, which for the moment makes it a manual process during testing. I also think that being able to manipulate user permissions through Traffic Portal, and in particular the ability to create a new service account with permissions for only a few endpoints it needs quickly through Traffic Portal, would be a shame to trade away for the very manual process of ensuring everyone's okay with restarti
ng TO, ssh in and edit a configuration file, then perform the restart and signal the all-clear (or, using Ansible as you said, which would probably be a longer process but safer than ssh-ing manually)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] mitchell852 commented on a change in pull request #5848: User Permissions Blueprint
Posted by GitBox <gi...@apache.org>.
mitchell852 commented on a change in pull request #5848:
URL: https://github.com/apache/trafficcontrol/pull/5848#discussion_r681235408
##########
File path: blueprints/user.permissions.md
##########
@@ -0,0 +1,286 @@
+<!--
+Licensed to the Apache Software Foundation (ASF) under one
+or more contributor license agreements. See the NOTICE file
+distributed with this work for additional information
+regarding copyright ownership. The ASF licenses this file
+to you under the Apache License, Version 2.0 (the
+"License"); you may not use this file except in compliance
+with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing,
+software distributed under the License is distributed on an
+"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+KIND, either express or implied. See the License for the
+specific language governing permissions and limitations
+under the License.
+-->
+# Enforced User Permissions
+
+## Problem Description
+Currently, the Permissions (currently "Capabilities", henceforth referred to as
+"Permissions" to avoid confusion with Server Capabilities) afforded to a user
+are defined by the "Privilege Level" of their Role. It essentially defines a
+scale on the interval [0,30], where a higher number allows more things. This
+isn't as scalable or configurable as it could be, and involves granting more
+permissions to a user than they actually need just because every Privilege
+Level encapsulates all of the permissions of every Privilege Level below it,
+making disjoint Permission sets for disparate Roles logically impossible.
+
+## Proposed Change
+Instead of a system of rigid supersets of Permissions, this blueprint proposes
+that the system of user "Capabilities" be expanded upon, renamed to Permissions
+(again, to avoid confusion) and enforced by the API.
+
+Under this system, each endpoint will declare the Permissions it requires for
+specific operations and check that the requesting user has those required, if
+any.
+
+This information is _not_ to be stored in the database nor exposed through the
+API, but documented. This change is discussed in greater detail in the
+"Alternatives" section.
+
+The enforcement of these Permissions is proposed to be configurable via an
+entry in the Traffic Ops configuration file. Thus, for one major version the
+API will need to support Permissions-based authorization as well as Privilege
+Level-based authorization.
+
+## Data Model Impact
+"Capabilities" as they are known today will be removed from the data model.
+Also, the current model of a Role will need to be modified. The current model
+is as follows:
+
+```typescript
+interface Role {
+ capabilities: Array<string> | null;
+ description: string | null;
+ id?: number | null;
+ name: string | null;
+ privLevel: number | null;
+}
+```
+
+The proposed new model is shown below.
+
+```typescript
+interface Role {
+ description: string;
+ lastUpdated?: Date;
+ name: string;
+ permissions: Array<string>;
+}
+```
+
+Also, because the ID is no longer exposed via the API, the model for users will
+need to change to only reference Role name, not ID. So it'll change from...
+
+```typescript
+interface User {
+ addressLine1: string | null;
+ addressLine2: string | null;
+ city: string | null;
+ company: string | null;
+ confirmLocalPassword?: string | null;
+ country: string | null;
+ email: string | null;
+ fullName: string | null;
+ gid: number | null;
+ id?: number | null;
+ lastUpdated?: string | null;
+ localPassword?: string | null;
+ newUser: boolean | null;
+ phoneNumber: string | null;
+ postalCode: string | null;
+ publicSSHKey: string | null;
+ registrationSent: string | null;
+ role: number | null;
+ roleName?: string | null;
+ stateOrProvince: string | null;
+ tenant?: string | null;
+ tenantID: string | null;
+ uid: number | null;
+ username: string | null;
+}
+```
+
+... to ...
+
+```typescript
+interface User {
+ addressLine1: string | null;
+ addressLine2: string | null;
+ city: string | null;
+ company: string | null;
+ confirmLocalPassword?: string | null;
+ country: string | null;
+ email: string | null;
+ fullName: string;
+ gid: number | null;
+ id?: number;
+ lastUpdated?: Date;
+ localPassword?: string | null;
+ newUser: boolean;
+ phoneNumber: string | null;
+ postalCode: string | null;
+ publicSSHKey: string | null;
+ registrationSent: string | null;
+ role: string;
+ stateOrProvince: string | null;
+ tenant?: string;
+ tenantID: number;
+ uid: number | null;
+ username: string;
+}
+```
+
+... replacing the required `role` field that was a Role ID with one that is
+the Role's name (and therefore removing `roleName` as it's now redundant).
+
+(Note that `lastUpdated` changed from `string | null` to `Date`, which
+represents the transition to using proper RFC3339 formatting, and also some
+fields which cannot be `null` for a valid user are no longer `| null`. Those
+aren't changes necessary to implement Permissions, but can be done easily at
+the same time as necessary changes to User.)
+
+## Component Impact
+This change primarily concerns Traffic Ops and Traffic Portal, though any
+client of the API will need to be aware of the changes to its authorization
+system.
+
+### Traffic Portal Impact
+There is a "Capabilities" view currently in Traffic Portal that provides a
+table of user Permissions, though it is not linked to the side navigation bar.
+This should be removed.
+
+The Roles' details pages will need to be augmented with controls for adding and
+removing Permissions. The Permissions available for adding to a user should
+include an autocompletion of those known by Traffic Portal to exist, but
+shouldn't restrict the addition of Permissions not known to exist to accomodate
+running older versions of Traffic Portal with newer versions of Traffic Ops, as
+well as any plugins unbeknownst to Traffic Portal that add Traffic Ops API
+endpoints and may declare their own Permissions that do not exist in a vanilla
+Traffic Control environment.
+
+Also, the Roles table will be augmented with the ability to filter Roles by the
+presence or absence of a Permission, although the full itemization of the
+Permissions afforded to a Role would, in most cases, be far too large to
+comfortably display in the table itself.
+
+Under-the-hood, API services will need to change from using Role IDs to Role
+names.
+
+### Traffic Ops Impact
+Traffic Ops will reqire changes to its Roles and Permissions API, chiefly, and
+the database tables that back them. Enforcement of API Permissions, though,
+will also have more wide-ranging impact that will require changes to very
+nearly every API endpoint.
+
+Additionally, a configuation file field will be added named `usePermissions`
+that is an optional boolean which, when present and `true` causes Traffic Ops
+to use Permissions rather than Role Privilege Level to determine a user's
+authorization for a given operation.
+
+#### API Impact
+Structurally, the only necessary changes are to the `/roles` and `/users`
+endpoints, which will need to be updated to output structures consistent with
+the changes outlined in the Data Model Impact section. The `/capabilities` and
+`/api_capabilities` endpoints will be removed (and we might consider renaming
+the `/server_capabilities` and `/server_server_capabilities` to drop the
+now-unnecessary "server_" qualifier).
+
+The more pervasive changes will be to all authenticated API endpoints which
+shall be updated to respect Permissions given the correct configuration
+setting.
+
+The exact Permissions that need to exist for each endpoint are best left to
+debate on the changeset that implements them.
+
+When a user attempts an operation for which they do not have sufficient
+Permissions, the API MUST respond with a `403 Forbidden` response containing an
+error-level Alert that describes what operation is not permitted and what
+Permission the user is missing that would allow them to proceed.
+
+#### Client Impact
+As Permissions are defined by the Role of the authenticated user, the only
+client changes are necessary beyond those necessitated by the removal of two
+API endpoints and the renaming and restructuring of a third are to stop
+filling in a Role ID on user creation/update from a Role name if an ID was
+unset (this will now be handled by the API itself, which will save a full
+HTTP request on each user creation/modification).
+
+#### Database Impact
+The new model for a Role does not allow a `null` description; a simple
+migration that coalesces existing `NULL` values to an empty string and adds a
+check constraint should be all that's actually required. No other immediate
+changes should be made - including dropping the now-unused `api_capability`
+table -, since old API versions will still need access to the deprecated
+columns. However, the foreign key constraint on the `role_capability` table
Review comment:
> Of course.
that's what i thought. just double-checking.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] dneuman64 commented on pull request #5848: User Permissions Blueprint
Posted by GitBox <gi...@apache.org>.
dneuman64 commented on pull request #5848:
URL: https://github.com/apache/trafficcontrol/pull/5848#issuecomment-893701967
-1 on trying to use a config file over the database. It is an interesting idea for sure, but I don't think it really makes sense for something like that.
For whatever perceived development savings we think there will be, there will be more operational expense and, to me, I would rather have less operational expense. I don't want to try to manage permissions across n servers, no matter how good ansible/puppet/etc is at it.
It seems pretty straightforward to just put this in the database so that it can be managed via API and via the UI, I don't really see why we wouldn't just go with that plan.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #5848: User Permissions Blueprint
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #5848:
URL: https://github.com/apache/trafficcontrol/pull/5848#discussion_r681298317
##########
File path: blueprints/user.permissions.md
##########
@@ -0,0 +1,286 @@
+<!--
+Licensed to the Apache Software Foundation (ASF) under one
+or more contributor license agreements. See the NOTICE file
+distributed with this work for additional information
+regarding copyright ownership. The ASF licenses this file
+to you under the Apache License, Version 2.0 (the
+"License"); you may not use this file except in compliance
+with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing,
+software distributed under the License is distributed on an
+"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+KIND, either express or implied. See the License for the
+specific language governing permissions and limitations
+under the License.
+-->
+# Enforced User Permissions
+
+## Problem Description
+Currently, the Permissions (currently "Capabilities", henceforth referred to as
+"Permissions" to avoid confusion with Server Capabilities) afforded to a user
+are defined by the "Privilege Level" of their Role. It essentially defines a
+scale on the interval [0,30], where a higher number allows more things. This
+isn't as scalable or configurable as it could be, and involves granting more
+permissions to a user than they actually need just because every Privilege
+Level encapsulates all of the permissions of every Privilege Level below it,
+making disjoint Permission sets for disparate Roles logically impossible.
+
+## Proposed Change
+Instead of a system of rigid supersets of Permissions, this blueprint proposes
+that the system of user "Capabilities" be expanded upon, renamed to Permissions
+(again, to avoid confusion) and enforced by the API.
+
+Under this system, each endpoint will declare the Permissions it requires for
+specific operations and check that the requesting user has those required, if
+any.
+
+This information is _not_ to be stored in the database nor exposed through the
+API, but documented. This change is discussed in greater detail in the
+"Alternatives" section.
+
+The enforcement of these Permissions is proposed to be configurable via an
+entry in the Traffic Ops configuration file. Thus, for one major version the
+API will need to support Permissions-based authorization as well as Privilege
+Level-based authorization.
+
+## Data Model Impact
+"Capabilities" as they are known today will be removed from the data model.
+Also, the current model of a Role will need to be modified. The current model
+is as follows:
+
+```typescript
+interface Role {
+ capabilities: Array<string> | null;
+ description: string | null;
+ id?: number | null;
+ name: string | null;
+ privLevel: number | null;
+}
+```
+
+The proposed new model is shown below.
+
+```typescript
+interface Role {
+ description: string;
+ lastUpdated?: Date;
+ name: string;
Review comment:
I think the issue there was because users in Tenant A can modify the permissions, effectively, of users in Tenant B even though users in Tenant A are not, in general, able to access Tenant B.
We could make Roles "Tenantable", but IMO that's somewhat separate to this notion of using Permissions instead of privilege level. Plus, you wouldn't need to make Roles "Tenantable" to prevent that, although it could possibly be more annoying without it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 commented on pull request #5848: User Permissions Blueprint
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on pull request #5848:
URL: https://github.com/apache/trafficcontrol/pull/5848#issuecomment-892022273
> Not necessarily, we don't need the API to return which permissions belong to which roles.
You would at the very least need to return which Roles exist, so that new users can be created. I also would be against only allowing users to be created with unknown Permissions, though that would be technically possible.
Also, the new Traffic Portal is banking on visibility into what a user is allowed to do, in order to hide or disable UI elements to deliver different experiences to different users according to their needs. That's pretty heavily requested, and - although I'm not sure if it's totally true - has actually been called "required" for the self-service aspect on which it currently focuses to be production-ready.
> Since User Permissions are optional...
Well, at first, yes, but only for a deprecation period. Manual translation of Roles does seem to be the only upgrade path, unless we're going to write a custom tool a la Traffic Vault and MySQL -> Postgresql.
> Testing permissions would be pretty easy via unit tests in this manner. You just simply update TO's internal memory representation (e.g. map[string]map[string]struct{}), then make an HTTP request.
The existing API tests do not have access to the internal memory of TO, though. We could very easily make unit tests like you describe, but end-to-end testing would be much harder without testing framework changes.
> ... this method would probably be easier than writing API tests... In fact, the API tests we currently have to test privileges are pretty convoluted and confusing...
That's not wrong, it would be much easier and they are convoluted. But we could actually test very nearly every single API (calls to external servers and system commands aside, all of them) with unit tests that use mock databases and mock HTTP requests and response recorders - but we don't because firstly that leaves the client untested, and secondly because there's value in an end-to-end test period. Our current tests just happen to fill both of those needs at once (arguably; there are things the client tests can't test for e.g. erroneous extra response fields). As far as being convoluted, the reality is that to use a new Role, one must first log in as the new Role. We can make it easier to access a session structure for a generic Role by writing some testing code to support that, but we can't change that flow because that simply is how the API works.
> Since better security is the main goal here, it might make sense to make the implementation of this security feature as secure as possible.
If Permissions aren't good enough security to manage Roles and Permissions, why are they secure enough for everything else? It's sort of like planning for defeat, seems to me.
> Also, I would argue that most permissions requests wouldn't be so time-sensitive that we'd need to be able to quickly use TP to add permissions.
*Adding* Permissions is unlikely to be time-sensitive, I agree, though I could probably imagine some contrived scenario for a poorly behaved script or something. But *removing* Permissions can be a security issue that needs to be addressed very quickly, which isn't addressed by just having "any admin... perform... the... task or get the user the time-sensitive data".
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] mitchell852 commented on a change in pull request #5848: User Permissions Blueprint
Posted by GitBox <gi...@apache.org>.
mitchell852 commented on a change in pull request #5848:
URL: https://github.com/apache/trafficcontrol/pull/5848#discussion_r680199594
##########
File path: blueprints/user.permissions.md
##########
@@ -0,0 +1,286 @@
+<!--
+Licensed to the Apache Software Foundation (ASF) under one
+or more contributor license agreements. See the NOTICE file
+distributed with this work for additional information
+regarding copyright ownership. The ASF licenses this file
+to you under the Apache License, Version 2.0 (the
+"License"); you may not use this file except in compliance
+with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing,
+software distributed under the License is distributed on an
+"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+KIND, either express or implied. See the License for the
+specific language governing permissions and limitations
+under the License.
+-->
+# Enforced User Permissions
+
+## Problem Description
+Currently, the Permissions (currently "Capabilities", henceforth referred to as
+"Permissions" to avoid confusion with Server Capabilities) afforded to a user
+are defined by the "Privilege Level" of their Role. It essentially defines a
+scale on the interval [0,30], where a higher number allows more things. This
+isn't as scalable or configurable as it could be, and involves granting more
+permissions to a user than they actually need just because every Privilege
+Level encapsulates all of the permissions of every Privilege Level below it,
+making disjoint Permission sets for disparate Roles logically impossible.
+
+## Proposed Change
+Instead of a system of rigid supersets of Permissions, this blueprint proposes
+that the system of user "Capabilities" be expanded upon, renamed to Permissions
+(again, to avoid confusion) and enforced by the API.
+
+Under this system, each endpoint will declare the Permissions it requires for
+specific operations and check that the requesting user has those required, if
+any.
+
+This information is _not_ to be stored in the database nor exposed through the
+API, but documented. This change is discussed in greater detail in the
+"Alternatives" section.
+
+The enforcement of these Permissions is proposed to be configurable via an
+entry in the Traffic Ops configuration file. Thus, for one major version the
+API will need to support Permissions-based authorization as well as Privilege
+Level-based authorization.
+
+## Data Model Impact
+"Capabilities" as they are known today will be removed from the data model.
+Also, the current model of a Role will need to be modified. The current model
+is as follows:
+
+```typescript
+interface Role {
+ capabilities: Array<string> | null;
+ description: string | null;
+ id?: number | null;
+ name: string | null;
+ privLevel: number | null;
+}
+```
+
+The proposed new model is shown below.
+
+```typescript
+interface Role {
+ description: string;
+ lastUpdated?: Date;
+ name: string;
+ permissions: Array<string>;
+}
+```
+
+Also, because the ID is no longer exposed via the API, the model for users will
+need to change to only reference Role name, not ID. So it'll change from...
+
+```typescript
+interface User {
+ addressLine1: string | null;
+ addressLine2: string | null;
+ city: string | null;
+ company: string | null;
+ confirmLocalPassword?: string | null;
+ country: string | null;
+ email: string | null;
+ fullName: string | null;
+ gid: number | null;
+ id?: number | null;
+ lastUpdated?: string | null;
+ localPassword?: string | null;
+ newUser: boolean | null;
+ phoneNumber: string | null;
+ postalCode: string | null;
+ publicSSHKey: string | null;
+ registrationSent: string | null;
+ role: number | null;
+ roleName?: string | null;
+ stateOrProvince: string | null;
+ tenant?: string | null;
+ tenantID: string | null;
+ uid: number | null;
+ username: string | null;
+}
+```
+
+... to ...
+
+```typescript
+interface User {
+ addressLine1: string | null;
+ addressLine2: string | null;
+ city: string | null;
+ company: string | null;
+ confirmLocalPassword?: string | null;
+ country: string | null;
+ email: string | null;
+ fullName: string;
+ gid: number | null;
+ id?: number;
+ lastUpdated?: Date;
+ localPassword?: string | null;
+ newUser: boolean;
+ phoneNumber: string | null;
+ postalCode: string | null;
+ publicSSHKey: string | null;
+ registrationSent: string | null;
+ role: string;
+ stateOrProvince: string | null;
+ tenant?: string;
+ tenantID: number;
+ uid: number | null;
+ username: string;
+}
+```
+
+... replacing the required `role` field that was a Role ID with one that is
+the Role's name (and therefore removing `roleName` as it's now redundant).
+
+(Note that `lastUpdated` changed from `string | null` to `Date`, which
+represents the transition to using proper RFC3339 formatting, and also some
+fields which cannot be `null` for a valid user are no longer `| null`. Those
+aren't changes necessary to implement Permissions, but can be done easily at
+the same time as necessary changes to User.)
+
+## Component Impact
+This change primarily concerns Traffic Ops and Traffic Portal, though any
+client of the API will need to be aware of the changes to its authorization
+system.
+
+### Traffic Portal Impact
+There is a "Capabilities" view currently in Traffic Portal that provides a
+table of user Permissions, though it is not linked to the side navigation bar.
+This should be removed.
+
+The Roles' details pages will need to be augmented with controls for adding and
+removing Permissions. The Permissions available for adding to a user should
+include an autocompletion of those known by Traffic Portal to exist, but
+shouldn't restrict the addition of Permissions not known to exist to accomodate
+running older versions of Traffic Portal with newer versions of Traffic Ops, as
+well as any plugins unbeknownst to Traffic Portal that add Traffic Ops API
+endpoints and may declare their own Permissions that do not exist in a vanilla
+Traffic Control environment.
+
+Also, the Roles table will be augmented with the ability to filter Roles by the
+presence or absence of a Permission, although the full itemization of the
+Permissions afforded to a Role would, in most cases, be far too large to
+comfortably display in the table itself.
+
+Under-the-hood, API services will need to change from using Role IDs to Role
+names.
+
+### Traffic Ops Impact
+Traffic Ops will reqire changes to its Roles and Permissions API, chiefly, and
+the database tables that back them. Enforcement of API Permissions, though,
+will also have more wide-ranging impact that will require changes to very
+nearly every API endpoint.
+
+Additionally, a configuation file field will be added named `usePermissions`
+that is an optional boolean which, when present and `true` causes Traffic Ops
+to use Permissions rather than Role Privilege Level to determine a user's
+authorization for a given operation.
+
+#### API Impact
+Structurally, the only necessary changes are to the `/roles` and `/users`
+endpoints, which will need to be updated to output structures consistent with
+the changes outlined in the Data Model Impact section. The `/capabilities` and
+`/api_capabilities` endpoints will be removed (and we might consider renaming
+the `/server_capabilities` and `/server_server_capabilities` to drop the
+now-unnecessary "server_" qualifier).
+
+The more pervasive changes will be to all authenticated API endpoints which
+shall be updated to respect Permissions given the correct configuration
+setting.
+
+The exact Permissions that need to exist for each endpoint are best left to
+debate on the changeset that implements them.
+
+When a user attempts an operation for which they do not have sufficient
+Permissions, the API MUST respond with a `403 Forbidden` response containing an
+error-level Alert that describes what operation is not permitted and what
+Permission the user is missing that would allow them to proceed.
+
+#### Client Impact
+As Permissions are defined by the Role of the authenticated user, the only
+client changes are necessary beyond those necessitated by the removal of two
+API endpoints and the renaming and restructuring of a third are to stop
+filling in a Role ID on user creation/update from a Role name if an ID was
+unset (this will now be handled by the API itself, which will save a full
+HTTP request on each user creation/modification).
+
+#### Database Impact
+The new model for a Role does not allow a `null` description; a simple
+migration that coalesces existing `NULL` values to an empty string and adds a
+check constraint should be all that's actually required. No other immediate
+changes should be made - including dropping the now-unused `api_capability`
+table -, since old API versions will still need access to the deprecated
+columns. However, the foreign key constraint on the `role_capability` table
Review comment:
is there now going to be a role_permission table instead to capture the mappings? also, can i create new roles as needed. i.e. a foo role with one permission (ds-read or whatever)
##########
File path: blueprints/user.permissions.md
##########
@@ -0,0 +1,286 @@
+<!--
+Licensed to the Apache Software Foundation (ASF) under one
+or more contributor license agreements. See the NOTICE file
+distributed with this work for additional information
+regarding copyright ownership. The ASF licenses this file
+to you under the Apache License, Version 2.0 (the
+"License"); you may not use this file except in compliance
+with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing,
+software distributed under the License is distributed on an
+"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+KIND, either express or implied. See the License for the
+specific language governing permissions and limitations
+under the License.
+-->
+# Enforced User Permissions
+
+## Problem Description
+Currently, the Permissions (currently "Capabilities", henceforth referred to as
+"Permissions" to avoid confusion with Server Capabilities) afforded to a user
+are defined by the "Privilege Level" of their Role. It essentially defines a
+scale on the interval [0,30], where a higher number allows more things. This
+isn't as scalable or configurable as it could be, and involves granting more
+permissions to a user than they actually need just because every Privilege
+Level encapsulates all of the permissions of every Privilege Level below it,
+making disjoint Permission sets for disparate Roles logically impossible.
+
+## Proposed Change
+Instead of a system of rigid supersets of Permissions, this blueprint proposes
+that the system of user "Capabilities" be expanded upon, renamed to Permissions
+(again, to avoid confusion) and enforced by the API.
+
+Under this system, each endpoint will declare the Permissions it requires for
+specific operations and check that the requesting user has those required, if
+any.
+
+This information is _not_ to be stored in the database nor exposed through the
+API, but documented. This change is discussed in greater detail in the
+"Alternatives" section.
+
+The enforcement of these Permissions is proposed to be configurable via an
+entry in the Traffic Ops configuration file. Thus, for one major version the
+API will need to support Permissions-based authorization as well as Privilege
+Level-based authorization.
+
+## Data Model Impact
+"Capabilities" as they are known today will be removed from the data model.
+Also, the current model of a Role will need to be modified. The current model
+is as follows:
+
+```typescript
+interface Role {
+ capabilities: Array<string> | null;
+ description: string | null;
+ id?: number | null;
+ name: string | null;
+ privLevel: number | null;
+}
+```
+
+The proposed new model is shown below.
+
+```typescript
+interface Role {
+ description: string;
+ lastUpdated?: Date;
+ name: string;
+ permissions: Array<string>;
+}
+```
+
+Also, because the ID is no longer exposed via the API, the model for users will
+need to change to only reference Role name, not ID. So it'll change from...
+
+```typescript
+interface User {
+ addressLine1: string | null;
+ addressLine2: string | null;
+ city: string | null;
+ company: string | null;
+ confirmLocalPassword?: string | null;
+ country: string | null;
+ email: string | null;
+ fullName: string | null;
+ gid: number | null;
+ id?: number | null;
+ lastUpdated?: string | null;
+ localPassword?: string | null;
+ newUser: boolean | null;
+ phoneNumber: string | null;
+ postalCode: string | null;
+ publicSSHKey: string | null;
+ registrationSent: string | null;
+ role: number | null;
+ roleName?: string | null;
+ stateOrProvince: string | null;
+ tenant?: string | null;
+ tenantID: string | null;
+ uid: number | null;
+ username: string | null;
+}
+```
+
+... to ...
+
+```typescript
+interface User {
+ addressLine1: string | null;
+ addressLine2: string | null;
+ city: string | null;
+ company: string | null;
+ confirmLocalPassword?: string | null;
+ country: string | null;
+ email: string | null;
+ fullName: string;
+ gid: number | null;
+ id?: number;
+ lastUpdated?: Date;
+ localPassword?: string | null;
+ newUser: boolean;
+ phoneNumber: string | null;
+ postalCode: string | null;
+ publicSSHKey: string | null;
+ registrationSent: string | null;
+ role: string;
+ stateOrProvince: string | null;
+ tenant?: string;
+ tenantID: number;
+ uid: number | null;
+ username: string;
+}
+```
+
+... replacing the required `role` field that was a Role ID with one that is
+the Role's name (and therefore removing `roleName` as it's now redundant).
+
+(Note that `lastUpdated` changed from `string | null` to `Date`, which
+represents the transition to using proper RFC3339 formatting, and also some
+fields which cannot be `null` for a valid user are no longer `| null`. Those
+aren't changes necessary to implement Permissions, but can be done easily at
+the same time as necessary changes to User.)
+
+## Component Impact
+This change primarily concerns Traffic Ops and Traffic Portal, though any
+client of the API will need to be aware of the changes to its authorization
+system.
+
+### Traffic Portal Impact
+There is a "Capabilities" view currently in Traffic Portal that provides a
+table of user Permissions, though it is not linked to the side navigation bar.
+This should be removed.
+
+The Roles' details pages will need to be augmented with controls for adding and
+removing Permissions. The Permissions available for adding to a user should
+include an autocompletion of those known by Traffic Portal to exist, but
+shouldn't restrict the addition of Permissions not known to exist to accomodate
+running older versions of Traffic Portal with newer versions of Traffic Ops, as
+well as any plugins unbeknownst to Traffic Portal that add Traffic Ops API
+endpoints and may declare their own Permissions that do not exist in a vanilla
+Traffic Control environment.
+
+Also, the Roles table will be augmented with the ability to filter Roles by the
+presence or absence of a Permission, although the full itemization of the
+Permissions afforded to a Role would, in most cases, be far too large to
+comfortably display in the table itself.
+
+Under-the-hood, API services will need to change from using Role IDs to Role
+names.
+
+### Traffic Ops Impact
+Traffic Ops will reqire changes to its Roles and Permissions API, chiefly, and
+the database tables that back them. Enforcement of API Permissions, though,
+will also have more wide-ranging impact that will require changes to very
+nearly every API endpoint.
+
+Additionally, a configuation file field will be added named `usePermissions`
+that is an optional boolean which, when present and `true` causes Traffic Ops
+to use Permissions rather than Role Privilege Level to determine a user's
+authorization for a given operation.
+
+#### API Impact
+Structurally, the only necessary changes are to the `/roles` and `/users`
+endpoints, which will need to be updated to output structures consistent with
+the changes outlined in the Data Model Impact section. The `/capabilities` and
+`/api_capabilities` endpoints will be removed (and we might consider renaming
+the `/server_capabilities` and `/server_server_capabilities` to drop the
+now-unnecessary "server_" qualifier).
+
+The more pervasive changes will be to all authenticated API endpoints which
+shall be updated to respect Permissions given the correct configuration
+setting.
+
+The exact Permissions that need to exist for each endpoint are best left to
+debate on the changeset that implements them.
+
+When a user attempts an operation for which they do not have sufficient
+Permissions, the API MUST respond with a `403 Forbidden` response containing an
+error-level Alert that describes what operation is not permitted and what
+Permission the user is missing that would allow them to proceed.
+
+#### Client Impact
+As Permissions are defined by the Role of the authenticated user, the only
+client changes are necessary beyond those necessitated by the removal of two
+API endpoints and the renaming and restructuring of a third are to stop
+filling in a Role ID on user creation/update from a Role name if an ID was
+unset (this will now be handled by the API itself, which will save a full
+HTTP request on each user creation/modification).
+
+#### Database Impact
+The new model for a Role does not allow a `null` description; a simple
+migration that coalesces existing `NULL` values to an empty string and adds a
+check constraint should be all that's actually required. No other immediate
+changes should be made - including dropping the now-unused `api_capability`
+table -, since old API versions will still need access to the deprecated
+columns. However, the foreign key constraint on the `role_capability` table
+that links a "Capability" name to a row in the `capability` table should be
+dropped, as that is no longer the source of truth for valid Permissions.
+
+Optionally, a migration should really be added to make a user's username
+`NOT NULL`, since that field is actually required by the API and many things
+will break if it's `NULL`.
+
+## Documentation Impact
+The new configuration option will need to be documented, documentation for
+removed API endpoints will itself need to be removed, documentation for the
+`/roles` and `/users` endpoints will need to be updated to reflect the new
+request and response structures, and as Permissions are implemented on each
+endpoint the Permissions it requires for various actions will need to be
+defined.
+
+## Testing Impact
+The most significant testing changes will need to be made to the Go Traffic Ops
+client integration tests, which should verify each endpoint's proper
+Permissions-based authorization. Traffic Portal functionality will also require
+the appropriate end-to-end tests.
+
+## Performance Impact
+No significant performance impact is expected, since the Permissions of a user
+are already queried at the time of servicing a request by every authenticated
+endpoint. Some negligible, constant time will need to be spend determining how
+to authorize an authenticated user.
+
+## Security Impact
+Careful consideration must be given to the design and implementation of each
+Permission. For example, this author believes a Permission named
+`do-secrets-things` that allows a Role unrestricted read and write access to
+any and all DNSSEC, SSL, URL Signature, and URI signing keys would be a poor
+design from a security standpoint. Permissions should be broad enough to
+encompass a single, well-defined purpose, and no more. In many cases, though,
+the existing "Capabilities" concepts will be good enough to build from (e.g.
+`delivery-services-write`, `cdns-read`).
+
+## Upgrade Impact
+There will be a database migration to run, but since the default configuration
+will be to ignore Permissions and just use Privilege Level, there isn't much in
+the way of upgrade impact immediately. The bigger step is getting ready for the
+_next_ upgrade, when Permissions stop being optional.
+
+## Operations Impact
Review comment:
granted i read thru this pretty quick but do you want to mention anything about how the current roles (admin, ops, r/o, etc) will be given perms to match their current behavior as dictated by priv level?
##########
File path: blueprints/user.permissions.md
##########
@@ -0,0 +1,286 @@
+<!--
+Licensed to the Apache Software Foundation (ASF) under one
+or more contributor license agreements. See the NOTICE file
+distributed with this work for additional information
+regarding copyright ownership. The ASF licenses this file
+to you under the Apache License, Version 2.0 (the
+"License"); you may not use this file except in compliance
+with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing,
+software distributed under the License is distributed on an
+"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+KIND, either express or implied. See the License for the
+specific language governing permissions and limitations
+under the License.
+-->
+# Enforced User Permissions
+
+## Problem Description
+Currently, the Permissions (currently "Capabilities", henceforth referred to as
+"Permissions" to avoid confusion with Server Capabilities) afforded to a user
+are defined by the "Privilege Level" of their Role. It essentially defines a
+scale on the interval [0,30], where a higher number allows more things. This
+isn't as scalable or configurable as it could be, and involves granting more
+permissions to a user than they actually need just because every Privilege
+Level encapsulates all of the permissions of every Privilege Level below it,
+making disjoint Permission sets for disparate Roles logically impossible.
+
+## Proposed Change
+Instead of a system of rigid supersets of Permissions, this blueprint proposes
+that the system of user "Capabilities" be expanded upon, renamed to Permissions
+(again, to avoid confusion) and enforced by the API.
+
+Under this system, each endpoint will declare the Permissions it requires for
+specific operations and check that the requesting user has those required, if
+any.
+
+This information is _not_ to be stored in the database nor exposed through the
+API, but documented. This change is discussed in greater detail in the
+"Alternatives" section.
+
+The enforcement of these Permissions is proposed to be configurable via an
+entry in the Traffic Ops configuration file. Thus, for one major version the
+API will need to support Permissions-based authorization as well as Privilege
+Level-based authorization.
+
+## Data Model Impact
+"Capabilities" as they are known today will be removed from the data model.
+Also, the current model of a Role will need to be modified. The current model
+is as follows:
+
+```typescript
+interface Role {
+ capabilities: Array<string> | null;
+ description: string | null;
+ id?: number | null;
+ name: string | null;
+ privLevel: number | null;
+}
+```
+
+The proposed new model is shown below.
+
+```typescript
+interface Role {
+ description: string;
+ lastUpdated?: Date;
+ name: string;
+ permissions: Array<string>;
+}
+```
+
+Also, because the ID is no longer exposed via the API, the model for users will
+need to change to only reference Role name, not ID. So it'll change from...
+
+```typescript
+interface User {
+ addressLine1: string | null;
+ addressLine2: string | null;
+ city: string | null;
+ company: string | null;
+ confirmLocalPassword?: string | null;
+ country: string | null;
+ email: string | null;
+ fullName: string | null;
+ gid: number | null;
+ id?: number | null;
+ lastUpdated?: string | null;
+ localPassword?: string | null;
+ newUser: boolean | null;
+ phoneNumber: string | null;
+ postalCode: string | null;
+ publicSSHKey: string | null;
+ registrationSent: string | null;
+ role: number | null;
+ roleName?: string | null;
+ stateOrProvince: string | null;
+ tenant?: string | null;
+ tenantID: string | null;
+ uid: number | null;
+ username: string | null;
+}
+```
+
+... to ...
+
+```typescript
+interface User {
+ addressLine1: string | null;
+ addressLine2: string | null;
+ city: string | null;
+ company: string | null;
+ confirmLocalPassword?: string | null;
+ country: string | null;
+ email: string | null;
+ fullName: string;
+ gid: number | null;
+ id?: number;
+ lastUpdated?: Date;
+ localPassword?: string | null;
+ newUser: boolean;
+ phoneNumber: string | null;
+ postalCode: string | null;
+ publicSSHKey: string | null;
+ registrationSent: string | null;
+ role: string;
+ stateOrProvince: string | null;
+ tenant?: string;
+ tenantID: number;
+ uid: number | null;
+ username: string;
+}
+```
+
+... replacing the required `role` field that was a Role ID with one that is
+the Role's name (and therefore removing `roleName` as it's now redundant).
+
+(Note that `lastUpdated` changed from `string | null` to `Date`, which
+represents the transition to using proper RFC3339 formatting, and also some
+fields which cannot be `null` for a valid user are no longer `| null`. Those
+aren't changes necessary to implement Permissions, but can be done easily at
+the same time as necessary changes to User.)
+
+## Component Impact
+This change primarily concerns Traffic Ops and Traffic Portal, though any
+client of the API will need to be aware of the changes to its authorization
+system.
+
+### Traffic Portal Impact
+There is a "Capabilities" view currently in Traffic Portal that provides a
+table of user Permissions, though it is not linked to the side navigation bar.
+This should be removed.
+
+The Roles' details pages will need to be augmented with controls for adding and
+removing Permissions. The Permissions available for adding to a user should
+include an autocompletion of those known by Traffic Portal to exist, but
+shouldn't restrict the addition of Permissions not known to exist to accomodate
+running older versions of Traffic Portal with newer versions of Traffic Ops, as
+well as any plugins unbeknownst to Traffic Portal that add Traffic Ops API
+endpoints and may declare their own Permissions that do not exist in a vanilla
+Traffic Control environment.
+
+Also, the Roles table will be augmented with the ability to filter Roles by the
+presence or absence of a Permission, although the full itemization of the
+Permissions afforded to a Role would, in most cases, be far too large to
+comfortably display in the table itself.
+
+Under-the-hood, API services will need to change from using Role IDs to Role
+names.
+
+### Traffic Ops Impact
+Traffic Ops will reqire changes to its Roles and Permissions API, chiefly, and
+the database tables that back them. Enforcement of API Permissions, though,
+will also have more wide-ranging impact that will require changes to very
+nearly every API endpoint.
+
+Additionally, a configuation file field will be added named `usePermissions`
+that is an optional boolean which, when present and `true` causes Traffic Ops
+to use Permissions rather than Role Privilege Level to determine a user's
+authorization for a given operation.
+
+#### API Impact
+Structurally, the only necessary changes are to the `/roles` and `/users`
+endpoints, which will need to be updated to output structures consistent with
+the changes outlined in the Data Model Impact section. The `/capabilities` and
+`/api_capabilities` endpoints will be removed (and we might consider renaming
+the `/server_capabilities` and `/server_server_capabilities` to drop the
+now-unnecessary "server_" qualifier).
+
+The more pervasive changes will be to all authenticated API endpoints which
+shall be updated to respect Permissions given the correct configuration
+setting.
+
+The exact Permissions that need to exist for each endpoint are best left to
+debate on the changeset that implements them.
+
+When a user attempts an operation for which they do not have sufficient
+Permissions, the API MUST respond with a `403 Forbidden` response containing an
+error-level Alert that describes what operation is not permitted and what
+Permission the user is missing that would allow them to proceed.
Review comment:
> and what Permission the user is missing that would allow them to proceed
I like that. seems like a very necessary thing.
##########
File path: blueprints/user.permissions.md
##########
@@ -0,0 +1,286 @@
+<!--
+Licensed to the Apache Software Foundation (ASF) under one
+or more contributor license agreements. See the NOTICE file
+distributed with this work for additional information
+regarding copyright ownership. The ASF licenses this file
+to you under the Apache License, Version 2.0 (the
+"License"); you may not use this file except in compliance
+with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing,
+software distributed under the License is distributed on an
+"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+KIND, either express or implied. See the License for the
+specific language governing permissions and limitations
+under the License.
+-->
+# Enforced User Permissions
+
+## Problem Description
+Currently, the Permissions (currently "Capabilities", henceforth referred to as
+"Permissions" to avoid confusion with Server Capabilities) afforded to a user
+are defined by the "Privilege Level" of their Role. It essentially defines a
+scale on the interval [0,30], where a higher number allows more things. This
+isn't as scalable or configurable as it could be, and involves granting more
+permissions to a user than they actually need just because every Privilege
+Level encapsulates all of the permissions of every Privilege Level below it,
+making disjoint Permission sets for disparate Roles logically impossible.
+
+## Proposed Change
+Instead of a system of rigid supersets of Permissions, this blueprint proposes
+that the system of user "Capabilities" be expanded upon, renamed to Permissions
+(again, to avoid confusion) and enforced by the API.
+
+Under this system, each endpoint will declare the Permissions it requires for
+specific operations and check that the requesting user has those required, if
+any.
+
+This information is _not_ to be stored in the database nor exposed through the
+API, but documented. This change is discussed in greater detail in the
+"Alternatives" section.
+
+The enforcement of these Permissions is proposed to be configurable via an
+entry in the Traffic Ops configuration file. Thus, for one major version the
+API will need to support Permissions-based authorization as well as Privilege
+Level-based authorization.
+
+## Data Model Impact
+"Capabilities" as they are known today will be removed from the data model.
+Also, the current model of a Role will need to be modified. The current model
+is as follows:
+
+```typescript
+interface Role {
+ capabilities: Array<string> | null;
+ description: string | null;
+ id?: number | null;
+ name: string | null;
+ privLevel: number | null;
+}
+```
+
+The proposed new model is shown below.
+
+```typescript
+interface Role {
+ description: string;
+ lastUpdated?: Date;
+ name: string;
+ permissions: Array<string>;
+}
+```
+
+Also, because the ID is no longer exposed via the API, the model for users will
+need to change to only reference Role name, not ID. So it'll change from...
+
+```typescript
+interface User {
+ addressLine1: string | null;
+ addressLine2: string | null;
+ city: string | null;
+ company: string | null;
+ confirmLocalPassword?: string | null;
+ country: string | null;
+ email: string | null;
+ fullName: string | null;
+ gid: number | null;
+ id?: number | null;
+ lastUpdated?: string | null;
+ localPassword?: string | null;
+ newUser: boolean | null;
+ phoneNumber: string | null;
+ postalCode: string | null;
+ publicSSHKey: string | null;
+ registrationSent: string | null;
+ role: number | null;
+ roleName?: string | null;
+ stateOrProvince: string | null;
+ tenant?: string | null;
+ tenantID: string | null;
+ uid: number | null;
+ username: string | null;
+}
+```
+
+... to ...
+
+```typescript
+interface User {
+ addressLine1: string | null;
+ addressLine2: string | null;
+ city: string | null;
+ company: string | null;
+ confirmLocalPassword?: string | null;
+ country: string | null;
+ email: string | null;
+ fullName: string;
+ gid: number | null;
+ id?: number;
+ lastUpdated?: Date;
+ localPassword?: string | null;
+ newUser: boolean;
+ phoneNumber: string | null;
+ postalCode: string | null;
+ publicSSHKey: string | null;
+ registrationSent: string | null;
+ role: string;
+ stateOrProvince: string | null;
+ tenant?: string;
+ tenantID: number;
+ uid: number | null;
+ username: string;
+}
+```
+
+... replacing the required `role` field that was a Role ID with one that is
+the Role's name (and therefore removing `roleName` as it's now redundant).
+
+(Note that `lastUpdated` changed from `string | null` to `Date`, which
+represents the transition to using proper RFC3339 formatting, and also some
+fields which cannot be `null` for a valid user are no longer `| null`. Those
+aren't changes necessary to implement Permissions, but can be done easily at
+the same time as necessary changes to User.)
+
+## Component Impact
+This change primarily concerns Traffic Ops and Traffic Portal, though any
+client of the API will need to be aware of the changes to its authorization
+system.
+
+### Traffic Portal Impact
+There is a "Capabilities" view currently in Traffic Portal that provides a
+table of user Permissions, though it is not linked to the side navigation bar.
+This should be removed.
+
+The Roles' details pages will need to be augmented with controls for adding and
+removing Permissions. The Permissions available for adding to a user should
+include an autocompletion of those known by Traffic Portal to exist, but
+shouldn't restrict the addition of Permissions not known to exist to accomodate
+running older versions of Traffic Portal with newer versions of Traffic Ops, as
+well as any plugins unbeknownst to Traffic Portal that add Traffic Ops API
+endpoints and may declare their own Permissions that do not exist in a vanilla
+Traffic Control environment.
+
+Also, the Roles table will be augmented with the ability to filter Roles by the
+presence or absence of a Permission, although the full itemization of the
+Permissions afforded to a Role would, in most cases, be far too large to
+comfortably display in the table itself.
+
+Under-the-hood, API services will need to change from using Role IDs to Role
+names.
+
+### Traffic Ops Impact
+Traffic Ops will reqire changes to its Roles and Permissions API, chiefly, and
+the database tables that back them. Enforcement of API Permissions, though,
+will also have more wide-ranging impact that will require changes to very
+nearly every API endpoint.
+
+Additionally, a configuation file field will be added named `usePermissions`
+that is an optional boolean which, when present and `true` causes Traffic Ops
+to use Permissions rather than Role Privilege Level to determine a user's
+authorization for a given operation.
+
+#### API Impact
+Structurally, the only necessary changes are to the `/roles` and `/users`
+endpoints, which will need to be updated to output structures consistent with
+the changes outlined in the Data Model Impact section. The `/capabilities` and
+`/api_capabilities` endpoints will be removed (and we might consider renaming
Review comment:
> and we might consider renaming the `/server_capabilities` and `/server_server_capabilities` to drop the now-unnecessary "server_" qualifier
seems beyond the scope of this change. if you agree, any reason to mention it?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] rawlinp commented on pull request #5848: User Permissions Blueprint
Posted by GitBox <gi...@apache.org>.
rawlinp commented on pull request #5848:
URL: https://github.com/apache/trafficcontrol/pull/5848#issuecomment-893021860
>> Not necessarily, we don't need the API to return which permissions belong to which roles.
> You would at the very least need to return which Roles exist, so that new users can be created. I also would be against only allowing users to be created with unknown Permissions, though that would be technically possible.
This is what I meant by keeping Roles in the API but not Permissions. You'd only be able to create Users with Roles that exist in the DB, but the permissions would be defined via config.
> Also, the new Traffic Portal is banking on visibility into what a user is allowed to do, in order to hide or disable UI elements to deliver different experiences to different users according to their needs.
One way to solve that would be to simply use the same config file for both TO and TP. The TO one would be authoritative, of course, while the TP one would be simply for hiding everything that their role doesn't allow. Or, we could have an API to return which permissions are currently assigned in TO's config file but still not allow assigning permissions via the API.
>> Since User Permissions are optional...
> Well, at first, yes, but only for a deprecation period. Manual translation of Roles does seem to be the only upgrade path
The new permissions config file could define the current permissions of the legacy Roles, but if we will eventually make User Permissions required, won't that mean that we'd always have to update the legacy Roles to add new permissions as we add new routes/functionality? I.e. every upgrade, we'd have to get all the new permissions and manually add them to the legacy Roles? Why can't we continue to assign privilege levels to routes instead of having to add permissions to legacy Roles with every upgrade? It seems like at least for admin, we shouldn't have to manually assign new permissions to it with every upgrade. Or do you think every new route that's added needs to have a DB migration to add the permission to the legacy Roles?
> The existing API tests do not have access to the internal memory of TO, though. We could very easily make unit tests like you describe, but end-to-end testing would be much harder without testing framework changes.
Permissions don't necessarily need tested in an end-to-end manner. We would still obviously have end-to-end tests for every route, but those end-to-end tests could focus on other things besides permissions. In fact, it would probably be overkill to test permissions in an end-to-end test for every route. The unit tests could cover permissions functionality as a whole, and we wouldn't necessarily require the specific permission to be tested on every route (although we could).
> > Since better security is the main goal here, it might make sense to make the implementation of this security feature as secure as possible.
> If Permissions aren't good enough security to manage Roles and Permissions, why are they secure enough for everything else? It's sort of like planning for defeat, seems to me.
I'm not really sure what you mean here, but storing permissions in a config file simply means there are fewer attack vectors in which an attacker could gain access to more permissions. DBs are usually available over the network, whereas on-disk config files typically aren't. Permissions stored in the DB data means they can be vulnerable to SQL injection. You also have to make sure your DB settings are secure. With a filesystem, you just have to set the proper file permissions so that even if an attacker was able to login to the system, they wouldn't be able to edit the permissions unless they had root (at which point they could do lots of nastier things anyways). I'm not saying Permissions aren't "good enough security"; I'm just saying that on-disk permissions could be considered "better security".
> *Adding* Permissions is unlikely to be time-sensitive, I agree, though I could probably imagine some contrived scenario for a poorly behaved script or something. But *removing* Permissions can be a security issue that needs to be addressed very quickly, which isn't addressed by just having "any admin... perform... the... task or get the user the time-sensitive data".
Even with on-disk permissions, we could still use the API to quickly change a user's role to one without permissions.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] mitchell852 commented on a change in pull request #5848: User Permissions Blueprint
Posted by GitBox <gi...@apache.org>.
mitchell852 commented on a change in pull request #5848:
URL: https://github.com/apache/trafficcontrol/pull/5848#discussion_r680198376
##########
File path: blueprints/user.permissions.md
##########
@@ -0,0 +1,286 @@
+<!--
+Licensed to the Apache Software Foundation (ASF) under one
+or more contributor license agreements. See the NOTICE file
+distributed with this work for additional information
+regarding copyright ownership. The ASF licenses this file
+to you under the Apache License, Version 2.0 (the
+"License"); you may not use this file except in compliance
+with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing,
+software distributed under the License is distributed on an
+"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+KIND, either express or implied. See the License for the
+specific language governing permissions and limitations
+under the License.
+-->
+# Enforced User Permissions
+
+## Problem Description
+Currently, the Permissions (currently "Capabilities", henceforth referred to as
+"Permissions" to avoid confusion with Server Capabilities) afforded to a user
+are defined by the "Privilege Level" of their Role. It essentially defines a
+scale on the interval [0,30], where a higher number allows more things. This
+isn't as scalable or configurable as it could be, and involves granting more
+permissions to a user than they actually need just because every Privilege
+Level encapsulates all of the permissions of every Privilege Level below it,
+making disjoint Permission sets for disparate Roles logically impossible.
+
+## Proposed Change
+Instead of a system of rigid supersets of Permissions, this blueprint proposes
+that the system of user "Capabilities" be expanded upon, renamed to Permissions
+(again, to avoid confusion) and enforced by the API.
+
+Under this system, each endpoint will declare the Permissions it requires for
+specific operations and check that the requesting user has those required, if
+any.
+
+This information is _not_ to be stored in the database nor exposed through the
+API, but documented. This change is discussed in greater detail in the
+"Alternatives" section.
+
+The enforcement of these Permissions is proposed to be configurable via an
+entry in the Traffic Ops configuration file. Thus, for one major version the
+API will need to support Permissions-based authorization as well as Privilege
+Level-based authorization.
+
+## Data Model Impact
+"Capabilities" as they are known today will be removed from the data model.
+Also, the current model of a Role will need to be modified. The current model
+is as follows:
+
+```typescript
+interface Role {
+ capabilities: Array<string> | null;
+ description: string | null;
+ id?: number | null;
+ name: string | null;
+ privLevel: number | null;
+}
+```
+
+The proposed new model is shown below.
+
+```typescript
+interface Role {
+ description: string;
+ lastUpdated?: Date;
+ name: string;
+ permissions: Array<string>;
+}
+```
+
+Also, because the ID is no longer exposed via the API, the model for users will
+need to change to only reference Role name, not ID. So it'll change from...
+
+```typescript
+interface User {
+ addressLine1: string | null;
+ addressLine2: string | null;
+ city: string | null;
+ company: string | null;
+ confirmLocalPassword?: string | null;
+ country: string | null;
+ email: string | null;
+ fullName: string | null;
+ gid: number | null;
+ id?: number | null;
+ lastUpdated?: string | null;
+ localPassword?: string | null;
+ newUser: boolean | null;
+ phoneNumber: string | null;
+ postalCode: string | null;
+ publicSSHKey: string | null;
+ registrationSent: string | null;
+ role: number | null;
+ roleName?: string | null;
+ stateOrProvince: string | null;
+ tenant?: string | null;
+ tenantID: string | null;
+ uid: number | null;
+ username: string | null;
+}
+```
+
+... to ...
+
+```typescript
+interface User {
+ addressLine1: string | null;
+ addressLine2: string | null;
+ city: string | null;
+ company: string | null;
+ confirmLocalPassword?: string | null;
+ country: string | null;
+ email: string | null;
+ fullName: string;
+ gid: number | null;
+ id?: number;
+ lastUpdated?: Date;
+ localPassword?: string | null;
+ newUser: boolean;
+ phoneNumber: string | null;
+ postalCode: string | null;
+ publicSSHKey: string | null;
+ registrationSent: string | null;
+ role: string;
+ stateOrProvince: string | null;
+ tenant?: string;
+ tenantID: number;
+ uid: number | null;
+ username: string;
+}
+```
+
+... replacing the required `role` field that was a Role ID with one that is
+the Role's name (and therefore removing `roleName` as it's now redundant).
+
+(Note that `lastUpdated` changed from `string | null` to `Date`, which
+represents the transition to using proper RFC3339 formatting, and also some
+fields which cannot be `null` for a valid user are no longer `| null`. Those
+aren't changes necessary to implement Permissions, but can be done easily at
+the same time as necessary changes to User.)
+
+## Component Impact
+This change primarily concerns Traffic Ops and Traffic Portal, though any
+client of the API will need to be aware of the changes to its authorization
+system.
+
+### Traffic Portal Impact
+There is a "Capabilities" view currently in Traffic Portal that provides a
+table of user Permissions, though it is not linked to the side navigation bar.
+This should be removed.
+
+The Roles' details pages will need to be augmented with controls for adding and
+removing Permissions. The Permissions available for adding to a user should
+include an autocompletion of those known by Traffic Portal to exist, but
+shouldn't restrict the addition of Permissions not known to exist to accomodate
+running older versions of Traffic Portal with newer versions of Traffic Ops, as
+well as any plugins unbeknownst to Traffic Portal that add Traffic Ops API
+endpoints and may declare their own Permissions that do not exist in a vanilla
+Traffic Control environment.
+
+Also, the Roles table will be augmented with the ability to filter Roles by the
+presence or absence of a Permission, although the full itemization of the
+Permissions afforded to a Role would, in most cases, be far too large to
+comfortably display in the table itself.
+
+Under-the-hood, API services will need to change from using Role IDs to Role
+names.
+
+### Traffic Ops Impact
+Traffic Ops will reqire changes to its Roles and Permissions API, chiefly, and
+the database tables that back them. Enforcement of API Permissions, though,
+will also have more wide-ranging impact that will require changes to very
+nearly every API endpoint.
+
+Additionally, a configuation file field will be added named `usePermissions`
+that is an optional boolean which, when present and `true` causes Traffic Ops
+to use Permissions rather than Role Privilege Level to determine a user's
+authorization for a given operation.
+
+#### API Impact
+Structurally, the only necessary changes are to the `/roles` and `/users`
+endpoints, which will need to be updated to output structures consistent with
+the changes outlined in the Data Model Impact section. The `/capabilities` and
+`/api_capabilities` endpoints will be removed (and we might consider renaming
Review comment:
> and we might consider renaming the `/server_capabilities` and `/server_server_capabilities` to drop the now-unnecessary "server_" qualifier
seems beyond the scope of this change. if you agree, any reason to mention it at all?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] mitchell852 commented on a change in pull request #5848: User Permissions Blueprint
Posted by GitBox <gi...@apache.org>.
mitchell852 commented on a change in pull request #5848:
URL: https://github.com/apache/trafficcontrol/pull/5848#discussion_r681234201
##########
File path: blueprints/user.permissions.md
##########
@@ -0,0 +1,286 @@
+<!--
+Licensed to the Apache Software Foundation (ASF) under one
+or more contributor license agreements. See the NOTICE file
+distributed with this work for additional information
+regarding copyright ownership. The ASF licenses this file
+to you under the Apache License, Version 2.0 (the
+"License"); you may not use this file except in compliance
+with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing,
+software distributed under the License is distributed on an
+"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+KIND, either express or implied. See the License for the
+specific language governing permissions and limitations
+under the License.
+-->
+# Enforced User Permissions
+
+## Problem Description
+Currently, the Permissions (currently "Capabilities", henceforth referred to as
+"Permissions" to avoid confusion with Server Capabilities) afforded to a user
+are defined by the "Privilege Level" of their Role. It essentially defines a
+scale on the interval [0,30], where a higher number allows more things. This
+isn't as scalable or configurable as it could be, and involves granting more
+permissions to a user than they actually need just because every Privilege
+Level encapsulates all of the permissions of every Privilege Level below it,
+making disjoint Permission sets for disparate Roles logically impossible.
+
+## Proposed Change
+Instead of a system of rigid supersets of Permissions, this blueprint proposes
+that the system of user "Capabilities" be expanded upon, renamed to Permissions
+(again, to avoid confusion) and enforced by the API.
+
+Under this system, each endpoint will declare the Permissions it requires for
+specific operations and check that the requesting user has those required, if
+any.
+
+This information is _not_ to be stored in the database nor exposed through the
Review comment:
ok, so what is documented would be this: the perm(s) required of each api.
that sounds sufficient as it should be static in nature.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] mitchell852 commented on a change in pull request #5848: User Permissions Blueprint
Posted by GitBox <gi...@apache.org>.
mitchell852 commented on a change in pull request #5848:
URL: https://github.com/apache/trafficcontrol/pull/5848#discussion_r681289892
##########
File path: blueprints/user.permissions.md
##########
@@ -0,0 +1,286 @@
+<!--
+Licensed to the Apache Software Foundation (ASF) under one
+or more contributor license agreements. See the NOTICE file
+distributed with this work for additional information
+regarding copyright ownership. The ASF licenses this file
+to you under the Apache License, Version 2.0 (the
+"License"); you may not use this file except in compliance
+with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing,
+software distributed under the License is distributed on an
+"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+KIND, either express or implied. See the License for the
+specific language governing permissions and limitations
+under the License.
+-->
+# Enforced User Permissions
+
+## Problem Description
+Currently, the Permissions (currently "Capabilities", henceforth referred to as
+"Permissions" to avoid confusion with Server Capabilities) afforded to a user
+are defined by the "Privilege Level" of their Role. It essentially defines a
+scale on the interval [0,30], where a higher number allows more things. This
+isn't as scalable or configurable as it could be, and involves granting more
+permissions to a user than they actually need just because every Privilege
+Level encapsulates all of the permissions of every Privilege Level below it,
+making disjoint Permission sets for disparate Roles logically impossible.
+
+## Proposed Change
+Instead of a system of rigid supersets of Permissions, this blueprint proposes
+that the system of user "Capabilities" be expanded upon, renamed to Permissions
+(again, to avoid confusion) and enforced by the API.
+
+Under this system, each endpoint will declare the Permissions it requires for
+specific operations and check that the requesting user has those required, if
+any.
+
+This information is _not_ to be stored in the database nor exposed through the
+API, but documented. This change is discussed in greater detail in the
+"Alternatives" section.
+
+The enforcement of these Permissions is proposed to be configurable via an
+entry in the Traffic Ops configuration file. Thus, for one major version the
+API will need to support Permissions-based authorization as well as Privilege
+Level-based authorization.
+
+## Data Model Impact
+"Capabilities" as they are known today will be removed from the data model.
+Also, the current model of a Role will need to be modified. The current model
+is as follows:
+
+```typescript
+interface Role {
+ capabilities: Array<string> | null;
+ description: string | null;
+ id?: number | null;
+ name: string | null;
+ privLevel: number | null;
+}
+```
+
+The proposed new model is shown below.
+
+```typescript
+interface Role {
+ description: string;
+ lastUpdated?: Date;
+ name: string;
Review comment:
anyhow, easier to implement now than later if that seems important.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] srijeet0406 commented on pull request #5848: User Permissions Blueprint
Posted by GitBox <gi...@apache.org>.
srijeet0406 commented on pull request #5848:
URL: https://github.com/apache/trafficcontrol/pull/5848#issuecomment-890128763
> > Check if the user is trying to delete their own lock. If so, check the user’s role(and perms). If they have the `cdn-locks-delete-cap` permission, then let them proceed with the request.
> > However, if the user is trying to delete somebody else’s lock, check to see if they have the `cdn-locks-admin-delete-cap` . If they have it, then proceed with the request. If not, then return a 403.
>
> i would just suggest that the "cap" aka permission is named intuitively. `cdn-locks-admin-delete-cap` doesn't mean anything to me and it has a "role" embedded in the name which seems like a no-no. this would make more sense to me: `cdn-locks-delete` permission (to delete your own locks) and `cdn-locks-delete-all` permission (to delete any/all locks).
Yeah makes sense. I just wanted to jot things down so that I dont forget later.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] mitchell852 commented on a change in pull request #5848: User Permissions Blueprint
Posted by GitBox <gi...@apache.org>.
mitchell852 commented on a change in pull request #5848:
URL: https://github.com/apache/trafficcontrol/pull/5848#discussion_r680197185
##########
File path: blueprints/user.permissions.md
##########
@@ -0,0 +1,286 @@
+<!--
+Licensed to the Apache Software Foundation (ASF) under one
+or more contributor license agreements. See the NOTICE file
+distributed with this work for additional information
+regarding copyright ownership. The ASF licenses this file
+to you under the Apache License, Version 2.0 (the
+"License"); you may not use this file except in compliance
+with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing,
+software distributed under the License is distributed on an
+"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+KIND, either express or implied. See the License for the
+specific language governing permissions and limitations
+under the License.
+-->
+# Enforced User Permissions
+
+## Problem Description
+Currently, the Permissions (currently "Capabilities", henceforth referred to as
+"Permissions" to avoid confusion with Server Capabilities) afforded to a user
+are defined by the "Privilege Level" of their Role. It essentially defines a
+scale on the interval [0,30], where a higher number allows more things. This
+isn't as scalable or configurable as it could be, and involves granting more
+permissions to a user than they actually need just because every Privilege
+Level encapsulates all of the permissions of every Privilege Level below it,
+making disjoint Permission sets for disparate Roles logically impossible.
+
+## Proposed Change
+Instead of a system of rigid supersets of Permissions, this blueprint proposes
+that the system of user "Capabilities" be expanded upon, renamed to Permissions
+(again, to avoid confusion) and enforced by the API.
+
+Under this system, each endpoint will declare the Permissions it requires for
+specific operations and check that the requesting user has those required, if
+any.
+
+This information is _not_ to be stored in the database nor exposed through the
+API, but documented. This change is discussed in greater detail in the
+"Alternatives" section.
+
+The enforcement of these Permissions is proposed to be configurable via an
+entry in the Traffic Ops configuration file. Thus, for one major version the
+API will need to support Permissions-based authorization as well as Privilege
+Level-based authorization.
+
+## Data Model Impact
+"Capabilities" as they are known today will be removed from the data model.
+Also, the current model of a Role will need to be modified. The current model
+is as follows:
+
+```typescript
+interface Role {
+ capabilities: Array<string> | null;
+ description: string | null;
+ id?: number | null;
+ name: string | null;
+ privLevel: number | null;
+}
+```
+
+The proposed new model is shown below.
+
+```typescript
+interface Role {
+ description: string;
+ lastUpdated?: Date;
+ name: string;
+ permissions: Array<string>;
+}
+```
+
+Also, because the ID is no longer exposed via the API, the model for users will
+need to change to only reference Role name, not ID. So it'll change from...
+
+```typescript
+interface User {
+ addressLine1: string | null;
+ addressLine2: string | null;
+ city: string | null;
+ company: string | null;
+ confirmLocalPassword?: string | null;
+ country: string | null;
+ email: string | null;
+ fullName: string | null;
+ gid: number | null;
+ id?: number | null;
+ lastUpdated?: string | null;
+ localPassword?: string | null;
+ newUser: boolean | null;
+ phoneNumber: string | null;
+ postalCode: string | null;
+ publicSSHKey: string | null;
+ registrationSent: string | null;
+ role: number | null;
+ roleName?: string | null;
+ stateOrProvince: string | null;
+ tenant?: string | null;
+ tenantID: string | null;
+ uid: number | null;
+ username: string | null;
+}
+```
+
+... to ...
+
+```typescript
+interface User {
+ addressLine1: string | null;
+ addressLine2: string | null;
+ city: string | null;
+ company: string | null;
+ confirmLocalPassword?: string | null;
+ country: string | null;
+ email: string | null;
+ fullName: string;
+ gid: number | null;
+ id?: number;
+ lastUpdated?: Date;
+ localPassword?: string | null;
+ newUser: boolean;
+ phoneNumber: string | null;
+ postalCode: string | null;
+ publicSSHKey: string | null;
+ registrationSent: string | null;
+ role: string;
+ stateOrProvince: string | null;
+ tenant?: string;
+ tenantID: number;
+ uid: number | null;
+ username: string;
+}
+```
+
+... replacing the required `role` field that was a Role ID with one that is
+the Role's name (and therefore removing `roleName` as it's now redundant).
+
+(Note that `lastUpdated` changed from `string | null` to `Date`, which
+represents the transition to using proper RFC3339 formatting, and also some
+fields which cannot be `null` for a valid user are no longer `| null`. Those
+aren't changes necessary to implement Permissions, but can be done easily at
+the same time as necessary changes to User.)
+
+## Component Impact
+This change primarily concerns Traffic Ops and Traffic Portal, though any
+client of the API will need to be aware of the changes to its authorization
+system.
+
+### Traffic Portal Impact
+There is a "Capabilities" view currently in Traffic Portal that provides a
+table of user Permissions, though it is not linked to the side navigation bar.
+This should be removed.
+
+The Roles' details pages will need to be augmented with controls for adding and
+removing Permissions. The Permissions available for adding to a user should
Review comment:
> The Permissions available for adding to a user
this sounds like you add perms to a user. is that true? or do you add perms to a role (which belongs to a user0?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #5848: User Permissions Blueprint
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #5848:
URL: https://github.com/apache/trafficcontrol/pull/5848#discussion_r681197726
##########
File path: blueprints/user.permissions.md
##########
@@ -0,0 +1,286 @@
+<!--
+Licensed to the Apache Software Foundation (ASF) under one
+or more contributor license agreements. See the NOTICE file
+distributed with this work for additional information
+regarding copyright ownership. The ASF licenses this file
+to you under the Apache License, Version 2.0 (the
+"License"); you may not use this file except in compliance
+with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing,
+software distributed under the License is distributed on an
+"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+KIND, either express or implied. See the License for the
+specific language governing permissions and limitations
+under the License.
+-->
+# Enforced User Permissions
+
+## Problem Description
+Currently, the Permissions (currently "Capabilities", henceforth referred to as
+"Permissions" to avoid confusion with Server Capabilities) afforded to a user
+are defined by the "Privilege Level" of their Role. It essentially defines a
+scale on the interval [0,30], where a higher number allows more things. This
+isn't as scalable or configurable as it could be, and involves granting more
+permissions to a user than they actually need just because every Privilege
+Level encapsulates all of the permissions of every Privilege Level below it,
+making disjoint Permission sets for disparate Roles logically impossible.
+
+## Proposed Change
+Instead of a system of rigid supersets of Permissions, this blueprint proposes
+that the system of user "Capabilities" be expanded upon, renamed to Permissions
+(again, to avoid confusion) and enforced by the API.
+
+Under this system, each endpoint will declare the Permissions it requires for
+specific operations and check that the requesting user has those required, if
+any.
+
+This information is _not_ to be stored in the database nor exposed through the
+API, but documented. This change is discussed in greater detail in the
+"Alternatives" section.
+
+The enforcement of these Permissions is proposed to be configurable via an
+entry in the Traffic Ops configuration file. Thus, for one major version the
+API will need to support Permissions-based authorization as well as Privilege
+Level-based authorization.
+
+## Data Model Impact
+"Capabilities" as they are known today will be removed from the data model.
+Also, the current model of a Role will need to be modified. The current model
+is as follows:
+
+```typescript
+interface Role {
+ capabilities: Array<string> | null;
+ description: string | null;
+ id?: number | null;
+ name: string | null;
+ privLevel: number | null;
+}
+```
+
+The proposed new model is shown below.
+
+```typescript
+interface Role {
+ description: string;
+ lastUpdated?: Date;
+ name: string;
+ permissions: Array<string>;
+}
+```
+
+Also, because the ID is no longer exposed via the API, the model for users will
+need to change to only reference Role name, not ID. So it'll change from...
+
+```typescript
+interface User {
+ addressLine1: string | null;
+ addressLine2: string | null;
+ city: string | null;
+ company: string | null;
+ confirmLocalPassword?: string | null;
+ country: string | null;
+ email: string | null;
+ fullName: string | null;
+ gid: number | null;
+ id?: number | null;
+ lastUpdated?: string | null;
+ localPassword?: string | null;
+ newUser: boolean | null;
+ phoneNumber: string | null;
+ postalCode: string | null;
+ publicSSHKey: string | null;
+ registrationSent: string | null;
+ role: number | null;
+ roleName?: string | null;
+ stateOrProvince: string | null;
+ tenant?: string | null;
+ tenantID: string | null;
+ uid: number | null;
+ username: string | null;
+}
+```
+
+... to ...
+
+```typescript
+interface User {
+ addressLine1: string | null;
+ addressLine2: string | null;
+ city: string | null;
+ company: string | null;
+ confirmLocalPassword?: string | null;
+ country: string | null;
+ email: string | null;
+ fullName: string;
+ gid: number | null;
+ id?: number;
+ lastUpdated?: Date;
+ localPassword?: string | null;
+ newUser: boolean;
+ phoneNumber: string | null;
+ postalCode: string | null;
+ publicSSHKey: string | null;
+ registrationSent: string | null;
+ role: string;
+ stateOrProvince: string | null;
+ tenant?: string;
+ tenantID: number;
+ uid: number | null;
+ username: string;
+}
+```
+
+... replacing the required `role` field that was a Role ID with one that is
+the Role's name (and therefore removing `roleName` as it's now redundant).
+
+(Note that `lastUpdated` changed from `string | null` to `Date`, which
+represents the transition to using proper RFC3339 formatting, and also some
+fields which cannot be `null` for a valid user are no longer `| null`. Those
+aren't changes necessary to implement Permissions, but can be done easily at
+the same time as necessary changes to User.)
+
+## Component Impact
+This change primarily concerns Traffic Ops and Traffic Portal, though any
+client of the API will need to be aware of the changes to its authorization
+system.
+
+### Traffic Portal Impact
+There is a "Capabilities" view currently in Traffic Portal that provides a
+table of user Permissions, though it is not linked to the side navigation bar.
+This should be removed.
+
+The Roles' details pages will need to be augmented with controls for adding and
+removing Permissions. The Permissions available for adding to a user should
+include an autocompletion of those known by Traffic Portal to exist, but
+shouldn't restrict the addition of Permissions not known to exist to accomodate
+running older versions of Traffic Portal with newer versions of Traffic Ops, as
+well as any plugins unbeknownst to Traffic Portal that add Traffic Ops API
+endpoints and may declare their own Permissions that do not exist in a vanilla
+Traffic Control environment.
+
+Also, the Roles table will be augmented with the ability to filter Roles by the
+presence or absence of a Permission, although the full itemization of the
+Permissions afforded to a Role would, in most cases, be far too large to
+comfortably display in the table itself.
+
+Under-the-hood, API services will need to change from using Role IDs to Role
+names.
+
+### Traffic Ops Impact
+Traffic Ops will reqire changes to its Roles and Permissions API, chiefly, and
+the database tables that back them. Enforcement of API Permissions, though,
+will also have more wide-ranging impact that will require changes to very
+nearly every API endpoint.
+
+Additionally, a configuation file field will be added named `usePermissions`
+that is an optional boolean which, when present and `true` causes Traffic Ops
+to use Permissions rather than Role Privilege Level to determine a user's
+authorization for a given operation.
+
+#### API Impact
+Structurally, the only necessary changes are to the `/roles` and `/users`
+endpoints, which will need to be updated to output structures consistent with
+the changes outlined in the Data Model Impact section. The `/capabilities` and
+`/api_capabilities` endpoints will be removed (and we might consider renaming
+the `/server_capabilities` and `/server_server_capabilities` to drop the
+now-unnecessary "server_" qualifier).
+
+The more pervasive changes will be to all authenticated API endpoints which
+shall be updated to respect Permissions given the correct configuration
+setting.
+
+The exact Permissions that need to exist for each endpoint are best left to
+debate on the changeset that implements them.
+
+When a user attempts an operation for which they do not have sufficient
+Permissions, the API MUST respond with a `403 Forbidden` response containing an
+error-level Alert that describes what operation is not permitted and what
+Permission the user is missing that would allow them to proceed.
+
+#### Client Impact
+As Permissions are defined by the Role of the authenticated user, the only
+client changes are necessary beyond those necessitated by the removal of two
+API endpoints and the renaming and restructuring of a third are to stop
+filling in a Role ID on user creation/update from a Role name if an ID was
+unset (this will now be handled by the API itself, which will save a full
+HTTP request on each user creation/modification).
+
+#### Database Impact
+The new model for a Role does not allow a `null` description; a simple
+migration that coalesces existing `NULL` values to an empty string and adds a
+check constraint should be all that's actually required. No other immediate
+changes should be made - including dropping the now-unused `api_capability`
+table -, since old API versions will still need access to the deprecated
+columns. However, the foreign key constraint on the `role_capability` table
Review comment:
> is there now going to be a role_permission table instead to capture the mappings?
No need, we can use the existing `role_capability` table by just dropping the foreign key constraint.
> also, can i create new roles as needed. i.e. a foo role with one permission (ds-read or whatever)
Of course.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] mitchell852 commented on a change in pull request #5848: User Permissions Blueprint
Posted by GitBox <gi...@apache.org>.
mitchell852 commented on a change in pull request #5848:
URL: https://github.com/apache/trafficcontrol/pull/5848#discussion_r681286881
##########
File path: blueprints/user.permissions.md
##########
@@ -0,0 +1,286 @@
+<!--
+Licensed to the Apache Software Foundation (ASF) under one
+or more contributor license agreements. See the NOTICE file
+distributed with this work for additional information
+regarding copyright ownership. The ASF licenses this file
+to you under the Apache License, Version 2.0 (the
+"License"); you may not use this file except in compliance
+with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing,
+software distributed under the License is distributed on an
+"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+KIND, either express or implied. See the License for the
+specific language governing permissions and limitations
+under the License.
+-->
+# Enforced User Permissions
+
+## Problem Description
+Currently, the Permissions (currently "Capabilities", henceforth referred to as
+"Permissions" to avoid confusion with Server Capabilities) afforded to a user
+are defined by the "Privilege Level" of their Role. It essentially defines a
+scale on the interval [0,30], where a higher number allows more things. This
+isn't as scalable or configurable as it could be, and involves granting more
+permissions to a user than they actually need just because every Privilege
+Level encapsulates all of the permissions of every Privilege Level below it,
+making disjoint Permission sets for disparate Roles logically impossible.
+
+## Proposed Change
+Instead of a system of rigid supersets of Permissions, this blueprint proposes
+that the system of user "Capabilities" be expanded upon, renamed to Permissions
+(again, to avoid confusion) and enforced by the API.
+
+Under this system, each endpoint will declare the Permissions it requires for
+specific operations and check that the requesting user has those required, if
+any.
+
+This information is _not_ to be stored in the database nor exposed through the
+API, but documented. This change is discussed in greater detail in the
+"Alternatives" section.
+
+The enforcement of these Permissions is proposed to be configurable via an
+entry in the Traffic Ops configuration file. Thus, for one major version the
+API will need to support Permissions-based authorization as well as Privilege
+Level-based authorization.
+
+## Data Model Impact
+"Capabilities" as they are known today will be removed from the data model.
+Also, the current model of a Role will need to be modified. The current model
+is as follows:
+
+```typescript
+interface Role {
+ capabilities: Array<string> | null;
+ description: string | null;
+ id?: number | null;
+ name: string | null;
+ privLevel: number | null;
+}
+```
+
+The proposed new model is shown below.
+
+```typescript
+interface Role {
+ description: string;
+ lastUpdated?: Date;
+ name: string;
Review comment:
at one time, i did think roles could/should be scoped per tenant but i'm kinda thinking that is overkill atm. but it is something to think about. without tenancy, any new role that is created will be visible to all "tenants". is that ok? maybe if creating new roles is a semi-rare event...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] mitchell852 commented on pull request #5848: User Permissions Blueprint
Posted by GitBox <gi...@apache.org>.
mitchell852 commented on pull request #5848:
URL: https://github.com/apache/trafficcontrol/pull/5848#issuecomment-890127894
> Check if the user is trying to delete their own lock. If so, check the user’s role(and perms). If they have the `cdn-locks-delete-cap` permission, then let them proceed with the request.
> However, if the user is trying to delete somebody else’s lock, check to see if they have the `cdn-locks-admin-delete-cap` . If they have it, then proceed with the request. If not, then return a 403.
i would just suggest that the "cap" aka permission is named intuitively. `cdn-locks-admin-delete-cap` doesn't mean anything to me and it has a "role" embedded in the name which seems like a no-no. this would make more sense to me: `cdn-locks-delete` permission (to delete your own locks) and `cdn-locks-delete-all` permission (to delete any/all locks).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] mitchell852 commented on a change in pull request #5848: User Permissions Blueprint
Posted by GitBox <gi...@apache.org>.
mitchell852 commented on a change in pull request #5848:
URL: https://github.com/apache/trafficcontrol/pull/5848#discussion_r680197185
##########
File path: blueprints/user.permissions.md
##########
@@ -0,0 +1,286 @@
+<!--
+Licensed to the Apache Software Foundation (ASF) under one
+or more contributor license agreements. See the NOTICE file
+distributed with this work for additional information
+regarding copyright ownership. The ASF licenses this file
+to you under the Apache License, Version 2.0 (the
+"License"); you may not use this file except in compliance
+with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing,
+software distributed under the License is distributed on an
+"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+KIND, either express or implied. See the License for the
+specific language governing permissions and limitations
+under the License.
+-->
+# Enforced User Permissions
+
+## Problem Description
+Currently, the Permissions (currently "Capabilities", henceforth referred to as
+"Permissions" to avoid confusion with Server Capabilities) afforded to a user
+are defined by the "Privilege Level" of their Role. It essentially defines a
+scale on the interval [0,30], where a higher number allows more things. This
+isn't as scalable or configurable as it could be, and involves granting more
+permissions to a user than they actually need just because every Privilege
+Level encapsulates all of the permissions of every Privilege Level below it,
+making disjoint Permission sets for disparate Roles logically impossible.
+
+## Proposed Change
+Instead of a system of rigid supersets of Permissions, this blueprint proposes
+that the system of user "Capabilities" be expanded upon, renamed to Permissions
+(again, to avoid confusion) and enforced by the API.
+
+Under this system, each endpoint will declare the Permissions it requires for
+specific operations and check that the requesting user has those required, if
+any.
+
+This information is _not_ to be stored in the database nor exposed through the
+API, but documented. This change is discussed in greater detail in the
+"Alternatives" section.
+
+The enforcement of these Permissions is proposed to be configurable via an
+entry in the Traffic Ops configuration file. Thus, for one major version the
+API will need to support Permissions-based authorization as well as Privilege
+Level-based authorization.
+
+## Data Model Impact
+"Capabilities" as they are known today will be removed from the data model.
+Also, the current model of a Role will need to be modified. The current model
+is as follows:
+
+```typescript
+interface Role {
+ capabilities: Array<string> | null;
+ description: string | null;
+ id?: number | null;
+ name: string | null;
+ privLevel: number | null;
+}
+```
+
+The proposed new model is shown below.
+
+```typescript
+interface Role {
+ description: string;
+ lastUpdated?: Date;
+ name: string;
+ permissions: Array<string>;
+}
+```
+
+Also, because the ID is no longer exposed via the API, the model for users will
+need to change to only reference Role name, not ID. So it'll change from...
+
+```typescript
+interface User {
+ addressLine1: string | null;
+ addressLine2: string | null;
+ city: string | null;
+ company: string | null;
+ confirmLocalPassword?: string | null;
+ country: string | null;
+ email: string | null;
+ fullName: string | null;
+ gid: number | null;
+ id?: number | null;
+ lastUpdated?: string | null;
+ localPassword?: string | null;
+ newUser: boolean | null;
+ phoneNumber: string | null;
+ postalCode: string | null;
+ publicSSHKey: string | null;
+ registrationSent: string | null;
+ role: number | null;
+ roleName?: string | null;
+ stateOrProvince: string | null;
+ tenant?: string | null;
+ tenantID: string | null;
+ uid: number | null;
+ username: string | null;
+}
+```
+
+... to ...
+
+```typescript
+interface User {
+ addressLine1: string | null;
+ addressLine2: string | null;
+ city: string | null;
+ company: string | null;
+ confirmLocalPassword?: string | null;
+ country: string | null;
+ email: string | null;
+ fullName: string;
+ gid: number | null;
+ id?: number;
+ lastUpdated?: Date;
+ localPassword?: string | null;
+ newUser: boolean;
+ phoneNumber: string | null;
+ postalCode: string | null;
+ publicSSHKey: string | null;
+ registrationSent: string | null;
+ role: string;
+ stateOrProvince: string | null;
+ tenant?: string;
+ tenantID: number;
+ uid: number | null;
+ username: string;
+}
+```
+
+... replacing the required `role` field that was a Role ID with one that is
+the Role's name (and therefore removing `roleName` as it's now redundant).
+
+(Note that `lastUpdated` changed from `string | null` to `Date`, which
+represents the transition to using proper RFC3339 formatting, and also some
+fields which cannot be `null` for a valid user are no longer `| null`. Those
+aren't changes necessary to implement Permissions, but can be done easily at
+the same time as necessary changes to User.)
+
+## Component Impact
+This change primarily concerns Traffic Ops and Traffic Portal, though any
+client of the API will need to be aware of the changes to its authorization
+system.
+
+### Traffic Portal Impact
+There is a "Capabilities" view currently in Traffic Portal that provides a
+table of user Permissions, though it is not linked to the side navigation bar.
+This should be removed.
+
+The Roles' details pages will need to be augmented with controls for adding and
+removing Permissions. The Permissions available for adding to a user should
Review comment:
> The Permissions available for adding to a user
this makes it sound like you add perms _directly_ to a user. is that true? or do you add perms to a role (which in turn belongs to a user)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #5848: User Permissions Blueprint
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #5848:
URL: https://github.com/apache/trafficcontrol/pull/5848#discussion_r681251555
##########
File path: blueprints/user.permissions.md
##########
@@ -0,0 +1,286 @@
+<!--
+Licensed to the Apache Software Foundation (ASF) under one
+or more contributor license agreements. See the NOTICE file
+distributed with this work for additional information
+regarding copyright ownership. The ASF licenses this file
+to you under the Apache License, Version 2.0 (the
+"License"); you may not use this file except in compliance
+with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing,
+software distributed under the License is distributed on an
+"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+KIND, either express or implied. See the License for the
+specific language governing permissions and limitations
+under the License.
+-->
+# Enforced User Permissions
+
+## Problem Description
+Currently, the Permissions (currently "Capabilities", henceforth referred to as
+"Permissions" to avoid confusion with Server Capabilities) afforded to a user
+are defined by the "Privilege Level" of their Role. It essentially defines a
+scale on the interval [0,30], where a higher number allows more things. This
+isn't as scalable or configurable as it could be, and involves granting more
+permissions to a user than they actually need just because every Privilege
+Level encapsulates all of the permissions of every Privilege Level below it,
+making disjoint Permission sets for disparate Roles logically impossible.
+
+## Proposed Change
+Instead of a system of rigid supersets of Permissions, this blueprint proposes
+that the system of user "Capabilities" be expanded upon, renamed to Permissions
+(again, to avoid confusion) and enforced by the API.
+
+Under this system, each endpoint will declare the Permissions it requires for
+specific operations and check that the requesting user has those required, if
+any.
+
+This information is _not_ to be stored in the database nor exposed through the
+API, but documented. This change is discussed in greater detail in the
+"Alternatives" section.
+
+The enforcement of these Permissions is proposed to be configurable via an
+entry in the Traffic Ops configuration file. Thus, for one major version the
+API will need to support Permissions-based authorization as well as Privilege
+Level-based authorization.
+
+## Data Model Impact
+"Capabilities" as they are known today will be removed from the data model.
+Also, the current model of a Role will need to be modified. The current model
+is as follows:
+
+```typescript
+interface Role {
+ capabilities: Array<string> | null;
+ description: string | null;
+ id?: number | null;
+ name: string | null;
+ privLevel: number | null;
+}
+```
+
+The proposed new model is shown below.
+
+```typescript
+interface Role {
+ description: string;
+ lastUpdated?: Date;
+ name: string;
+ permissions: Array<string>;
+}
+```
+
+Also, because the ID is no longer exposed via the API, the model for users will
+need to change to only reference Role name, not ID. So it'll change from...
+
+```typescript
+interface User {
+ addressLine1: string | null;
+ addressLine2: string | null;
+ city: string | null;
+ company: string | null;
+ confirmLocalPassword?: string | null;
+ country: string | null;
+ email: string | null;
+ fullName: string | null;
+ gid: number | null;
+ id?: number | null;
+ lastUpdated?: string | null;
+ localPassword?: string | null;
+ newUser: boolean | null;
+ phoneNumber: string | null;
+ postalCode: string | null;
+ publicSSHKey: string | null;
+ registrationSent: string | null;
+ role: number | null;
+ roleName?: string | null;
+ stateOrProvince: string | null;
+ tenant?: string | null;
+ tenantID: string | null;
+ uid: number | null;
+ username: string | null;
+}
+```
+
+... to ...
+
+```typescript
+interface User {
+ addressLine1: string | null;
+ addressLine2: string | null;
+ city: string | null;
+ company: string | null;
+ confirmLocalPassword?: string | null;
+ country: string | null;
+ email: string | null;
+ fullName: string;
+ gid: number | null;
+ id?: number;
+ lastUpdated?: Date;
+ localPassword?: string | null;
+ newUser: boolean;
+ phoneNumber: string | null;
+ postalCode: string | null;
+ publicSSHKey: string | null;
+ registrationSent: string | null;
+ role: string;
+ stateOrProvince: string | null;
+ tenant?: string;
+ tenantID: number;
+ uid: number | null;
+ username: string;
+}
+```
+
+... replacing the required `role` field that was a Role ID with one that is
+the Role's name (and therefore removing `roleName` as it's now redundant).
+
+(Note that `lastUpdated` changed from `string | null` to `Date`, which
+represents the transition to using proper RFC3339 formatting, and also some
+fields which cannot be `null` for a valid user are no longer `| null`. Those
+aren't changes necessary to implement Permissions, but can be done easily at
+the same time as necessary changes to User.)
+
+## Component Impact
+This change primarily concerns Traffic Ops and Traffic Portal, though any
+client of the API will need to be aware of the changes to its authorization
+system.
+
+### Traffic Portal Impact
+There is a "Capabilities" view currently in Traffic Portal that provides a
+table of user Permissions, though it is not linked to the side navigation bar.
+This should be removed.
+
+The Roles' details pages will need to be augmented with controls for adding and
+removing Permissions. The Permissions available for adding to a user should
+include an autocompletion of those known by Traffic Portal to exist, but
+shouldn't restrict the addition of Permissions not known to exist to accomodate
+running older versions of Traffic Portal with newer versions of Traffic Ops, as
+well as any plugins unbeknownst to Traffic Portal that add Traffic Ops API
+endpoints and may declare their own Permissions that do not exist in a vanilla
+Traffic Control environment.
+
+Also, the Roles table will be augmented with the ability to filter Roles by the
+presence or absence of a Permission, although the full itemization of the
+Permissions afforded to a Role would, in most cases, be far too large to
+comfortably display in the table itself.
+
+Under-the-hood, API services will need to change from using Role IDs to Role
+names.
+
+### Traffic Ops Impact
+Traffic Ops will reqire changes to its Roles and Permissions API, chiefly, and
+the database tables that back them. Enforcement of API Permissions, though,
+will also have more wide-ranging impact that will require changes to very
+nearly every API endpoint.
+
+Additionally, a configuation file field will be added named `usePermissions`
+that is an optional boolean which, when present and `true` causes Traffic Ops
+to use Permissions rather than Role Privilege Level to determine a user's
+authorization for a given operation.
+
+#### API Impact
+Structurally, the only necessary changes are to the `/roles` and `/users`
+endpoints, which will need to be updated to output structures consistent with
+the changes outlined in the Data Model Impact section. The `/capabilities` and
+`/api_capabilities` endpoints will be removed (and we might consider renaming
+the `/server_capabilities` and `/server_server_capabilities` to drop the
+now-unnecessary "server_" qualifier).
+
+The more pervasive changes will be to all authenticated API endpoints which
+shall be updated to respect Permissions given the correct configuration
+setting.
+
+The exact Permissions that need to exist for each endpoint are best left to
+debate on the changeset that implements them.
+
+When a user attempts an operation for which they do not have sufficient
+Permissions, the API MUST respond with a `403 Forbidden` response containing an
+error-level Alert that describes what operation is not permitted and what
+Permission the user is missing that would allow them to proceed.
+
+#### Client Impact
+As Permissions are defined by the Role of the authenticated user, the only
+client changes are necessary beyond those necessitated by the removal of two
+API endpoints and the renaming and restructuring of a third are to stop
+filling in a Role ID on user creation/update from a Role name if an ID was
+unset (this will now be handled by the API itself, which will save a full
+HTTP request on each user creation/modification).
+
+#### Database Impact
+The new model for a Role does not allow a `null` description; a simple
+migration that coalesces existing `NULL` values to an empty string and adds a
+check constraint should be all that's actually required. No other immediate
+changes should be made - including dropping the now-unused `api_capability`
+table -, since old API versions will still need access to the deprecated
+columns. However, the foreign key constraint on the `role_capability` table
Review comment:
Yeah, that's the easiest path forward.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] mitchell852 commented on a change in pull request #5848: User Permissions Blueprint
Posted by GitBox <gi...@apache.org>.
mitchell852 commented on a change in pull request #5848:
URL: https://github.com/apache/trafficcontrol/pull/5848#discussion_r681286881
##########
File path: blueprints/user.permissions.md
##########
@@ -0,0 +1,286 @@
+<!--
+Licensed to the Apache Software Foundation (ASF) under one
+or more contributor license agreements. See the NOTICE file
+distributed with this work for additional information
+regarding copyright ownership. The ASF licenses this file
+to you under the Apache License, Version 2.0 (the
+"License"); you may not use this file except in compliance
+with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing,
+software distributed under the License is distributed on an
+"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+KIND, either express or implied. See the License for the
+specific language governing permissions and limitations
+under the License.
+-->
+# Enforced User Permissions
+
+## Problem Description
+Currently, the Permissions (currently "Capabilities", henceforth referred to as
+"Permissions" to avoid confusion with Server Capabilities) afforded to a user
+are defined by the "Privilege Level" of their Role. It essentially defines a
+scale on the interval [0,30], where a higher number allows more things. This
+isn't as scalable or configurable as it could be, and involves granting more
+permissions to a user than they actually need just because every Privilege
+Level encapsulates all of the permissions of every Privilege Level below it,
+making disjoint Permission sets for disparate Roles logically impossible.
+
+## Proposed Change
+Instead of a system of rigid supersets of Permissions, this blueprint proposes
+that the system of user "Capabilities" be expanded upon, renamed to Permissions
+(again, to avoid confusion) and enforced by the API.
+
+Under this system, each endpoint will declare the Permissions it requires for
+specific operations and check that the requesting user has those required, if
+any.
+
+This information is _not_ to be stored in the database nor exposed through the
+API, but documented. This change is discussed in greater detail in the
+"Alternatives" section.
+
+The enforcement of these Permissions is proposed to be configurable via an
+entry in the Traffic Ops configuration file. Thus, for one major version the
+API will need to support Permissions-based authorization as well as Privilege
+Level-based authorization.
+
+## Data Model Impact
+"Capabilities" as they are known today will be removed from the data model.
+Also, the current model of a Role will need to be modified. The current model
+is as follows:
+
+```typescript
+interface Role {
+ capabilities: Array<string> | null;
+ description: string | null;
+ id?: number | null;
+ name: string | null;
+ privLevel: number | null;
+}
+```
+
+The proposed new model is shown below.
+
+```typescript
+interface Role {
+ description: string;
+ lastUpdated?: Date;
+ name: string;
Review comment:
at one time, i did think roles could/should be scoped per tenant but i'm kinda thinking that is overkill atm. but it is something to think about. without tenancy, any new role that is created will be visible to all "tenants". is that ok? maybe it is ok if creating new roles is a semi-rare event...
```
root
- tenant 1
-- tenant 1.1
-- tenant 1.2
- tenant 2
```
so basically if i'm a user with the tenant 1 tenant and i create the foo role, would that role be scope to tenant 1? meaning users of tenant 1, 1.1 and 1.2 would see the role but not users in tenant 2? seems to make sense...but again, would need to think about that more. could cause potential issues. curious what others think.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] srijeet0406 commented on a change in pull request #5848: User Permissions Blueprint
Posted by GitBox <gi...@apache.org>.
srijeet0406 commented on a change in pull request #5848:
URL: https://github.com/apache/trafficcontrol/pull/5848#discussion_r673308927
##########
File path: blueprints/user.permissions.md
##########
@@ -0,0 +1,205 @@
+<!--
+Licensed to the Apache Software Foundation (ASF) under one
+or more contributor license agreements. See the NOTICE file
+distributed with this work for additional information
+regarding copyright ownership. The ASF licenses this file
+to you under the Apache License, Version 2.0 (the
+"License"); you may not use this file except in compliance
+with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing,
+software distributed under the License is distributed on an
+"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+KIND, either express or implied. See the License for the
+specific language governing permissions and limitations
+under the License.
+-->
+# Enforced User Permissions
+
+## Problem Description
+Currently, the Permissions (currently "Capabilities", henceforth referred to as
+"Permissions" to avoid confusion with Server Capabilities) afforded to a user
+are defined by the "Privilege Level" of their Role. It essentially defines a
+scale on the interval [0,30], where a higher number allows more things. This
+isn't as scalable or configurable as it could be, and involves granting more
+permissions to a user than they actually need just because every Privilege
+Level encapsulates all of the permissions of every Privilege Level below it,
+making disjoint Permission sets for disparate Roles logically impossible.
+
+## Proposed Change
+Instead of a system of rigid supersets of Permissions, this blueprint proposes
+that the system of user "Capabilities" be expanded upon, renamed to Permissions
+(again, to avoid confusion) and enforced by the API.
+
+Under this system, each endpoint will declare the Permissions it requires for
+specific operations and check that the requesting user has those required, if
+any.
+
+This information is _not_ to be stored in the database nor exposed through the
+API, but documented. This change is discussed in greater detail in the
+"Alternatives" section.
+
+The enforcement of these Permissions is proposed to be configurable via an
+entry in the Traffic Ops configuration file. Thus, for one major version the
+API will need to support Permissions-based authorization as well as Privilege
+Level-based authorization.
+
+## Data Model Impact
+"Capabilities" as they are known today will be removed from the data model.
+Also, the current model of a Role will need to be modified. The current model
+is as follows<sup>1</sup>:
+
+```typescript
+interface Role {
+ capabilities: Array<string>;
+ description: string | null;
+ id?: number;
+ name: string;
+ privLevel: number;
+}
+```
+
+The proposed new model is shown below<sup>1</sup>.
+
+```typescript
+interface Role {
+ description: string;
+ name: string;
+ permissions: Array<string>;
+}
+```
+
+## Component Impact
+This change primarily concerns Traffic Ops and Traffic Portal, though any
+client of the API will need to be aware of the changes to its authorization
+system.
+
+### Traffic Portal Impact
+There is a "Capabilities" view currently in Traffic Portal that provides a
+table of user Permissions, though it is not linked to the side navigation bar.
+This should be removed.
+
+The Roles' details pages will need to be augmented with controls for adding and
+removing Permissions. The Permissions available for adding to a user should
+include an autocompletion of those known by Traffic Portal to exist, but
+shouldn't restrict the addition of Permissions not known to exist to accomodate
+running older versions of Traffic Portal with newer versions of Traffic Ops, as
+well as any plugins unbeknownst to Traffic Portal that add Traffic Ops API
+endpoints and may declare their own Permissions that do not exist in a vanilla
+Traffic Control environment.
+
+Also, the Roles table will be augmented with the ability to filter Roles by the
+presence or absence of a Permission, although the full itemization of the
+Permissions afforded to a Role would, in most cases, be far too large to
+comfortably display in the table itself.
+
+### Traffic Ops Impact
+Traffic Ops will reqire changes to its Roles and Permissions API, chiefly, and
+the database tables that back them. Enforcement of API Permissions, though,
+will also have more wide-ranging impact that will require changes to very
+nearly every API endpoint.
+
+Additionally, a configuation file field will be added named `usePermissions`
+that is an optional boolean which, when present and `true` causes Traffic Ops
+to use Permissions rather than Role Privilege Level to determine a user's
+authorization for a given operation.
+
+#### API Impact
+Structurally, the only necessary changes are to the `/roles` endpoint, which
+will need to be updated to output structures consistent with the changes
+outlined in the Data Model Impact section. The `/capabilities` and
+`/api_capabilities` endpoints will be removed (and we might consider renaming
+the `/server_capabilities` and `/server_server_capabilities` to drop the
+now-unnecessary "server_" qualifier).
+
+The more pervasive changes will be to all authenticated API endpoints which
+shall be updated to respect Permissions given the correct configuration
+setting.
+
+The exact Permissions that need to exist for each endpoint are best left to
+debate on the changeset that implements them.
+
+When a user attempts an operation for which they do not have sufficient
+Permissions, the API MUST respond with a `403 Forbidden` response containing an
+error-level Alert that describes what operation is not permitted and what
+Permission the user is missing that would allow them to proceed.
+
+#### Client Impact
+As Permissions are defined by the Role of the authenticated user, no client
+changes are necessary beyond those necessitated by the removal of two API
+endpoints and the renaming and restructuring of a third.
+
+#### Database Impact
+The new model for a Role does not allow a `null` description; a simple
+migration that coalesces existing `NULL` values to an empty string and adds a
+check constraint should be all that's actually required. No other immediate
+changes should be made, since old API versions will still need access to the
+deprecated colunms. However, the foreign key constraint on the
Review comment:
typo: columns
##########
File path: blueprints/user.permissions.md
##########
@@ -0,0 +1,205 @@
+<!--
+Licensed to the Apache Software Foundation (ASF) under one
+or more contributor license agreements. See the NOTICE file
+distributed with this work for additional information
+regarding copyright ownership. The ASF licenses this file
+to you under the Apache License, Version 2.0 (the
+"License"); you may not use this file except in compliance
+with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing,
+software distributed under the License is distributed on an
+"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+KIND, either express or implied. See the License for the
+specific language governing permissions and limitations
+under the License.
+-->
+# Enforced User Permissions
+
+## Problem Description
+Currently, the Permissions (currently "Capabilities", henceforth referred to as
+"Permissions" to avoid confusion with Server Capabilities) afforded to a user
+are defined by the "Privilege Level" of their Role. It essentially defines a
+scale on the interval [0,30], where a higher number allows more things. This
+isn't as scalable or configurable as it could be, and involves granting more
+permissions to a user than they actually need just because every Privilege
+Level encapsulates all of the permissions of every Privilege Level below it,
+making disjoint Permission sets for disparate Roles logically impossible.
+
+## Proposed Change
+Instead of a system of rigid supersets of Permissions, this blueprint proposes
+that the system of user "Capabilities" be expanded upon, renamed to Permissions
+(again, to avoid confusion) and enforced by the API.
+
+Under this system, each endpoint will declare the Permissions it requires for
+specific operations and check that the requesting user has those required, if
+any.
+
+This information is _not_ to be stored in the database nor exposed through the
+API, but documented. This change is discussed in greater detail in the
+"Alternatives" section.
+
+The enforcement of these Permissions is proposed to be configurable via an
+entry in the Traffic Ops configuration file. Thus, for one major version the
+API will need to support Permissions-based authorization as well as Privilege
+Level-based authorization.
+
+## Data Model Impact
+"Capabilities" as they are known today will be removed from the data model.
+Also, the current model of a Role will need to be modified. The current model
+is as follows<sup>1</sup>:
+
+```typescript
+interface Role {
+ capabilities: Array<string>;
+ description: string | null;
+ id?: number;
+ name: string;
+ privLevel: number;
+}
+```
+
+The proposed new model is shown below<sup>1</sup>.
+
+```typescript
+interface Role {
+ description: string;
+ name: string;
+ permissions: Array<string>;
+}
+```
+
+## Component Impact
+This change primarily concerns Traffic Ops and Traffic Portal, though any
+client of the API will need to be aware of the changes to its authorization
+system.
+
+### Traffic Portal Impact
+There is a "Capabilities" view currently in Traffic Portal that provides a
+table of user Permissions, though it is not linked to the side navigation bar.
+This should be removed.
+
+The Roles' details pages will need to be augmented with controls for adding and
+removing Permissions. The Permissions available for adding to a user should
+include an autocompletion of those known by Traffic Portal to exist, but
+shouldn't restrict the addition of Permissions not known to exist to accomodate
+running older versions of Traffic Portal with newer versions of Traffic Ops, as
+well as any plugins unbeknownst to Traffic Portal that add Traffic Ops API
+endpoints and may declare their own Permissions that do not exist in a vanilla
+Traffic Control environment.
+
+Also, the Roles table will be augmented with the ability to filter Roles by the
+presence or absence of a Permission, although the full itemization of the
+Permissions afforded to a Role would, in most cases, be far too large to
+comfortably display in the table itself.
+
+### Traffic Ops Impact
+Traffic Ops will reqire changes to its Roles and Permissions API, chiefly, and
+the database tables that back them. Enforcement of API Permissions, though,
+will also have more wide-ranging impact that will require changes to very
+nearly every API endpoint.
+
+Additionally, a configuation file field will be added named `usePermissions`
+that is an optional boolean which, when present and `true` causes Traffic Ops
+to use Permissions rather than Role Privilege Level to determine a user's
+authorization for a given operation.
+
+#### API Impact
+Structurally, the only necessary changes are to the `/roles` endpoint, which
+will need to be updated to output structures consistent with the changes
+outlined in the Data Model Impact section. The `/capabilities` and
+`/api_capabilities` endpoints will be removed (and we might consider renaming
+the `/server_capabilities` and `/server_server_capabilities` to drop the
+now-unnecessary "server_" qualifier).
+
+The more pervasive changes will be to all authenticated API endpoints which
+shall be updated to respect Permissions given the correct configuration
+setting.
+
+The exact Permissions that need to exist for each endpoint are best left to
+debate on the changeset that implements them.
+
+When a user attempts an operation for which they do not have sufficient
+Permissions, the API MUST respond with a `403 Forbidden` response containing an
+error-level Alert that describes what operation is not permitted and what
+Permission the user is missing that would allow them to proceed.
+
+#### Client Impact
+As Permissions are defined by the Role of the authenticated user, no client
+changes are necessary beyond those necessitated by the removal of two API
+endpoints and the renaming and restructuring of a third.
+
+#### Database Impact
+The new model for a Role does not allow a `null` description; a simple
+migration that coalesces existing `NULL` values to an empty string and adds a
+check constraint should be all that's actually required. No other immediate
+changes should be made, since old API versions will still need access to the
+deprecated colunms. However, the foreign key constraint on the
+`role_capability` table that links a "Capability" name to a row in the
+`capability` table should be dropped, as that is no longer the source of truth
+for valid Permissions.
+
+## Documentation Impact
+The new configuration option will need to be documented, documentation for
+removed API endpoints will itself need to be removed, documentation for the
+`/roles` endpoint will need to be updated to reflect the new request and
+response structures, and as Permissions are implemented on each endpoint the
+Permissions it requires for various actions will need to be defined.
+
+## Testing Impact
+The most significant testing changes will need to be made to the Go Traffic Ops
+client integration tests, which should verify each endpoint's proper
+Permissions-based authorization. Traffic Portal functionality will also require
+the appropriate end-to-end tests.
+
+## Performance Impact
+No significant performance impact is expected, since the Permissions of a user
+are already queried at the time of servicing a request by every authenticated
+endpoint. Some negligible, constant time will need to be spend determining how
+to authorize an authenticated user.
+
+## Security Impact
+Careful consideration must be given to the design and implementation of each
+Permission. For example, this author believes a Permission named
+`do-secrets-things` that allows a Role unrestricted read and write access to
+any and all DNSSEC, SSL, URL Signature, and URI signing keys would be a poor
+design from a security standpoint. Permissions should be broad enough to
+encompass a single, well-defined purpose, and no more. In many cases, though,
+the existing "Capabilities" concepts will be good enough to build from (e.g.
+`delivery-services-write`, `cdns-read`).
+
+## Upgrade Impact
+There will be a database migration to run, but since the default configuration
+will be to ignore Permissions and just use Privilege Level, there isn't much in
+the way of upgrade impact immediately. The bigger step is getting ready for the
+_next_ upgrade, when Permissions stop being optional.
+
+## Operations Impact
+Before enabling Permissions, operators will need to ensure that all Roles have
+the appropriate Permissions to accomplish their necessary tasks. They will also
+need to be aware of the Permissions required to accomplish any given task, or
+at least where in the documentation to look for guidance in that regard.
+
+### Developer Impact
+From now on, new endpoints will need to be designed with their Permissions in
+mind. For many endpoints this will probably be trivial, but must always be at
+least considered.
+
+## Alternatives
+The current system of "Capabilities", if enforced, was a possible alternative
+to the system herein described. However, that system lacks the ability to
+express a user's permission to do something beyond a combination of HTTP
+request method and path. For example, the CDN Locks blueprint (#5834) proposes
+a system by which a user may delete a lock that they created, but also states
+that "`admin` users" (Privilege Level 30 or above) can delete any lock created
+by any user. However, these two permissions use the same request method and
+path, meaning that under the current system it would be impossible to give
+admin users the ability to do that overriding deletion without also giving it
+to everyone else. The system described here is far more flexible. It could even
+eliminate the need for the `/deliveryservices/safe` API endpoint, which can be
+expressed instead as two separate Permissions: one that allows making changes
+to the "safe" Delivery Service fields and one that allows all others.
+
+[1] `lastUpdated` is omitted as it's a read-only, response-only field.
Review comment:
The `roles` endpoint currently supports If-Modified-Since, If-Unmodified-Since and If-Match headers. When you say `lastUpdated` is omitted, do you mean it is removed from the structure as well as the database, or just the former? If it is just the former, then the IMS, IUMS and IM requests should still work fine.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 commented on pull request #5848: User Permissions Blueprint
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on pull request #5848:
URL: https://github.com/apache/trafficcontrol/pull/5848#issuecomment-891932043
If Roles are taken out of the API, then would we not still need to make API changes, so that the configuration file contents can be returned through the API? It's less work than manipulable objects from the database, to be sure, but it's not nothing. How would we migrate existing Roles to the on-disk configuration? What happens to users with the "Foo" Role if that Role is removed from the configuration file? Testing this would also be difficult to do in the current framework, since creating or deleting Roles would require restarting Traffic Ops, which for the moment makes it a manual process during testing. I also think that being able to manipulate user permissions through Traffic Portal, and in particular the ability to create a new service account with permissions for only a few endpoints it needs quickly through Traffic Portal, would be a shame to trade away for the very manual process of ensuring everyone's okay with restarting TO, ssh in and edit a configuration file, then p
erform the restart and signal the all-clear (or, using Ansible as you said, which would probably be a longer process but safer than ssh-ing manually)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] mitchell852 edited a comment on pull request #5848: User Permissions Blueprint
Posted by GitBox <gi...@apache.org>.
mitchell852 edited a comment on pull request #5848:
URL: https://github.com/apache/trafficcontrol/pull/5848#issuecomment-890127894
> Check if the user is trying to delete their own lock. If so, check the user’s role(and perms). If they have the `cdn-locks-delete-cap` permission, then let them proceed with the request.
> However, if the user is trying to delete somebody else’s lock, check to see if they have the `cdn-locks-admin-delete-cap` . If they have it, then proceed with the request. If not, then return a 403.
i would just suggest that the "cap" aka permission is named intuitively. `cdn-locks-admin-delete-cap` doesn't mean anything to me and it has a "role" embedded in the name which seems like a no-no. this would make more sense to me: `cdn-locks-delete` permission (to delete your own locks) and `cdn-locks-delete-all` permission (to delete any/all locks). i would expect the existing `admin` role to have the `cdn-locks-delete-all` perm and the existing operations role to have the `cdn-locks-delete` perm.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] rawlinp commented on pull request #5848: User Permissions Blueprint
Posted by GitBox <gi...@apache.org>.
rawlinp commented on pull request #5848:
URL: https://github.com/apache/trafficcontrol/pull/5848#issuecomment-897996569
> I don't hate the idea of admin being special in that way, it's just hard to know how to handle people redefining admin between install and upgrade.
That's the thing -- it shouldn't be possible to redefine `admin`. It would be defined via the code in that it specifically bypasses all permissions checks. I agree just having `all` would be better than having both `all-write` and `all-read`, but having an assignable permission that grants all permissions seems a bit risky. If the `admin` somehow removed its own `all` permission, and it was the only one that had it, that role would be locked out of everything and would require manually running some SQL in order to get the permission back. If `admin` is special in that it bypasses permissions checks, that scenario isn't possible.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] mitchell852 commented on a change in pull request #5848: User Permissions Blueprint
Posted by GitBox <gi...@apache.org>.
mitchell852 commented on a change in pull request #5848:
URL: https://github.com/apache/trafficcontrol/pull/5848#discussion_r681286881
##########
File path: blueprints/user.permissions.md
##########
@@ -0,0 +1,286 @@
+<!--
+Licensed to the Apache Software Foundation (ASF) under one
+or more contributor license agreements. See the NOTICE file
+distributed with this work for additional information
+regarding copyright ownership. The ASF licenses this file
+to you under the Apache License, Version 2.0 (the
+"License"); you may not use this file except in compliance
+with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing,
+software distributed under the License is distributed on an
+"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+KIND, either express or implied. See the License for the
+specific language governing permissions and limitations
+under the License.
+-->
+# Enforced User Permissions
+
+## Problem Description
+Currently, the Permissions (currently "Capabilities", henceforth referred to as
+"Permissions" to avoid confusion with Server Capabilities) afforded to a user
+are defined by the "Privilege Level" of their Role. It essentially defines a
+scale on the interval [0,30], where a higher number allows more things. This
+isn't as scalable or configurable as it could be, and involves granting more
+permissions to a user than they actually need just because every Privilege
+Level encapsulates all of the permissions of every Privilege Level below it,
+making disjoint Permission sets for disparate Roles logically impossible.
+
+## Proposed Change
+Instead of a system of rigid supersets of Permissions, this blueprint proposes
+that the system of user "Capabilities" be expanded upon, renamed to Permissions
+(again, to avoid confusion) and enforced by the API.
+
+Under this system, each endpoint will declare the Permissions it requires for
+specific operations and check that the requesting user has those required, if
+any.
+
+This information is _not_ to be stored in the database nor exposed through the
+API, but documented. This change is discussed in greater detail in the
+"Alternatives" section.
+
+The enforcement of these Permissions is proposed to be configurable via an
+entry in the Traffic Ops configuration file. Thus, for one major version the
+API will need to support Permissions-based authorization as well as Privilege
+Level-based authorization.
+
+## Data Model Impact
+"Capabilities" as they are known today will be removed from the data model.
+Also, the current model of a Role will need to be modified. The current model
+is as follows:
+
+```typescript
+interface Role {
+ capabilities: Array<string> | null;
+ description: string | null;
+ id?: number | null;
+ name: string | null;
+ privLevel: number | null;
+}
+```
+
+The proposed new model is shown below.
+
+```typescript
+interface Role {
+ description: string;
+ lastUpdated?: Date;
+ name: string;
Review comment:
at one time, i did think roles could/should be scoped per tenant but i'm kinda thinking that is overkill atm. but it is something to think about. without tenancy, any new role that is created will be visible to all "tenants". is that ok? maybe it is ok if creating new roles is a semi-rare event...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] mitchell852 commented on a change in pull request #5848: User Permissions Blueprint
Posted by GitBox <gi...@apache.org>.
mitchell852 commented on a change in pull request #5848:
URL: https://github.com/apache/trafficcontrol/pull/5848#discussion_r681289892
##########
File path: blueprints/user.permissions.md
##########
@@ -0,0 +1,286 @@
+<!--
+Licensed to the Apache Software Foundation (ASF) under one
+or more contributor license agreements. See the NOTICE file
+distributed with this work for additional information
+regarding copyright ownership. The ASF licenses this file
+to you under the Apache License, Version 2.0 (the
+"License"); you may not use this file except in compliance
+with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing,
+software distributed under the License is distributed on an
+"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+KIND, either express or implied. See the License for the
+specific language governing permissions and limitations
+under the License.
+-->
+# Enforced User Permissions
+
+## Problem Description
+Currently, the Permissions (currently "Capabilities", henceforth referred to as
+"Permissions" to avoid confusion with Server Capabilities) afforded to a user
+are defined by the "Privilege Level" of their Role. It essentially defines a
+scale on the interval [0,30], where a higher number allows more things. This
+isn't as scalable or configurable as it could be, and involves granting more
+permissions to a user than they actually need just because every Privilege
+Level encapsulates all of the permissions of every Privilege Level below it,
+making disjoint Permission sets for disparate Roles logically impossible.
+
+## Proposed Change
+Instead of a system of rigid supersets of Permissions, this blueprint proposes
+that the system of user "Capabilities" be expanded upon, renamed to Permissions
+(again, to avoid confusion) and enforced by the API.
+
+Under this system, each endpoint will declare the Permissions it requires for
+specific operations and check that the requesting user has those required, if
+any.
+
+This information is _not_ to be stored in the database nor exposed through the
+API, but documented. This change is discussed in greater detail in the
+"Alternatives" section.
+
+The enforcement of these Permissions is proposed to be configurable via an
+entry in the Traffic Ops configuration file. Thus, for one major version the
+API will need to support Permissions-based authorization as well as Privilege
+Level-based authorization.
+
+## Data Model Impact
+"Capabilities" as they are known today will be removed from the data model.
+Also, the current model of a Role will need to be modified. The current model
+is as follows:
+
+```typescript
+interface Role {
+ capabilities: Array<string> | null;
+ description: string | null;
+ id?: number | null;
+ name: string | null;
+ privLevel: number | null;
+}
+```
+
+The proposed new model is shown below.
+
+```typescript
+interface Role {
+ description: string;
+ lastUpdated?: Date;
+ name: string;
Review comment:
anyhow, easier to implement tenancy now than later if that seems important.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] mitchell852 commented on a change in pull request #5848: User Permissions Blueprint
Posted by GitBox <gi...@apache.org>.
mitchell852 commented on a change in pull request #5848:
URL: https://github.com/apache/trafficcontrol/pull/5848#discussion_r680196470
##########
File path: blueprints/user.permissions.md
##########
@@ -0,0 +1,286 @@
+<!--
+Licensed to the Apache Software Foundation (ASF) under one
+or more contributor license agreements. See the NOTICE file
+distributed with this work for additional information
+regarding copyright ownership. The ASF licenses this file
+to you under the Apache License, Version 2.0 (the
+"License"); you may not use this file except in compliance
+with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing,
+software distributed under the License is distributed on an
+"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+KIND, either express or implied. See the License for the
+specific language governing permissions and limitations
+under the License.
+-->
+# Enforced User Permissions
+
+## Problem Description
+Currently, the Permissions (currently "Capabilities", henceforth referred to as
+"Permissions" to avoid confusion with Server Capabilities) afforded to a user
+are defined by the "Privilege Level" of their Role. It essentially defines a
+scale on the interval [0,30], where a higher number allows more things. This
+isn't as scalable or configurable as it could be, and involves granting more
+permissions to a user than they actually need just because every Privilege
+Level encapsulates all of the permissions of every Privilege Level below it,
+making disjoint Permission sets for disparate Roles logically impossible.
+
+## Proposed Change
+Instead of a system of rigid supersets of Permissions, this blueprint proposes
+that the system of user "Capabilities" be expanded upon, renamed to Permissions
+(again, to avoid confusion) and enforced by the API.
+
+Under this system, each endpoint will declare the Permissions it requires for
+specific operations and check that the requesting user has those required, if
+any.
+
+This information is _not_ to be stored in the database nor exposed through the
Review comment:
what exactly is documented? the perm(s) required of each api? the role to perm mappings? both or just the first?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] mitchell852 commented on a change in pull request #5848: User Permissions Blueprint
Posted by GitBox <gi...@apache.org>.
mitchell852 commented on a change in pull request #5848:
URL: https://github.com/apache/trafficcontrol/pull/5848#discussion_r680200803
##########
File path: blueprints/user.permissions.md
##########
@@ -0,0 +1,286 @@
+<!--
+Licensed to the Apache Software Foundation (ASF) under one
+or more contributor license agreements. See the NOTICE file
+distributed with this work for additional information
+regarding copyright ownership. The ASF licenses this file
+to you under the Apache License, Version 2.0 (the
+"License"); you may not use this file except in compliance
+with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing,
+software distributed under the License is distributed on an
+"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+KIND, either express or implied. See the License for the
+specific language governing permissions and limitations
+under the License.
+-->
+# Enforced User Permissions
+
+## Problem Description
+Currently, the Permissions (currently "Capabilities", henceforth referred to as
+"Permissions" to avoid confusion with Server Capabilities) afforded to a user
+are defined by the "Privilege Level" of their Role. It essentially defines a
+scale on the interval [0,30], where a higher number allows more things. This
+isn't as scalable or configurable as it could be, and involves granting more
+permissions to a user than they actually need just because every Privilege
+Level encapsulates all of the permissions of every Privilege Level below it,
+making disjoint Permission sets for disparate Roles logically impossible.
+
+## Proposed Change
+Instead of a system of rigid supersets of Permissions, this blueprint proposes
+that the system of user "Capabilities" be expanded upon, renamed to Permissions
+(again, to avoid confusion) and enforced by the API.
+
+Under this system, each endpoint will declare the Permissions it requires for
+specific operations and check that the requesting user has those required, if
+any.
+
+This information is _not_ to be stored in the database nor exposed through the
+API, but documented. This change is discussed in greater detail in the
+"Alternatives" section.
+
+The enforcement of these Permissions is proposed to be configurable via an
+entry in the Traffic Ops configuration file. Thus, for one major version the
+API will need to support Permissions-based authorization as well as Privilege
+Level-based authorization.
+
+## Data Model Impact
+"Capabilities" as they are known today will be removed from the data model.
+Also, the current model of a Role will need to be modified. The current model
+is as follows:
+
+```typescript
+interface Role {
+ capabilities: Array<string> | null;
+ description: string | null;
+ id?: number | null;
+ name: string | null;
+ privLevel: number | null;
+}
+```
+
+The proposed new model is shown below.
+
+```typescript
+interface Role {
+ description: string;
+ lastUpdated?: Date;
+ name: string;
+ permissions: Array<string>;
+}
+```
+
+Also, because the ID is no longer exposed via the API, the model for users will
+need to change to only reference Role name, not ID. So it'll change from...
+
+```typescript
+interface User {
+ addressLine1: string | null;
+ addressLine2: string | null;
+ city: string | null;
+ company: string | null;
+ confirmLocalPassword?: string | null;
+ country: string | null;
+ email: string | null;
+ fullName: string | null;
+ gid: number | null;
+ id?: number | null;
+ lastUpdated?: string | null;
+ localPassword?: string | null;
+ newUser: boolean | null;
+ phoneNumber: string | null;
+ postalCode: string | null;
+ publicSSHKey: string | null;
+ registrationSent: string | null;
+ role: number | null;
+ roleName?: string | null;
+ stateOrProvince: string | null;
+ tenant?: string | null;
+ tenantID: string | null;
+ uid: number | null;
+ username: string | null;
+}
+```
+
+... to ...
+
+```typescript
+interface User {
+ addressLine1: string | null;
+ addressLine2: string | null;
+ city: string | null;
+ company: string | null;
+ confirmLocalPassword?: string | null;
+ country: string | null;
+ email: string | null;
+ fullName: string;
+ gid: number | null;
+ id?: number;
+ lastUpdated?: Date;
+ localPassword?: string | null;
+ newUser: boolean;
+ phoneNumber: string | null;
+ postalCode: string | null;
+ publicSSHKey: string | null;
+ registrationSent: string | null;
+ role: string;
+ stateOrProvince: string | null;
+ tenant?: string;
+ tenantID: number;
+ uid: number | null;
+ username: string;
+}
+```
+
+... replacing the required `role` field that was a Role ID with one that is
+the Role's name (and therefore removing `roleName` as it's now redundant).
+
+(Note that `lastUpdated` changed from `string | null` to `Date`, which
+represents the transition to using proper RFC3339 formatting, and also some
+fields which cannot be `null` for a valid user are no longer `| null`. Those
+aren't changes necessary to implement Permissions, but can be done easily at
+the same time as necessary changes to User.)
+
+## Component Impact
+This change primarily concerns Traffic Ops and Traffic Portal, though any
+client of the API will need to be aware of the changes to its authorization
+system.
+
+### Traffic Portal Impact
+There is a "Capabilities" view currently in Traffic Portal that provides a
+table of user Permissions, though it is not linked to the side navigation bar.
+This should be removed.
+
+The Roles' details pages will need to be augmented with controls for adding and
+removing Permissions. The Permissions available for adding to a user should
+include an autocompletion of those known by Traffic Portal to exist, but
+shouldn't restrict the addition of Permissions not known to exist to accomodate
+running older versions of Traffic Portal with newer versions of Traffic Ops, as
+well as any plugins unbeknownst to Traffic Portal that add Traffic Ops API
+endpoints and may declare their own Permissions that do not exist in a vanilla
+Traffic Control environment.
+
+Also, the Roles table will be augmented with the ability to filter Roles by the
+presence or absence of a Permission, although the full itemization of the
+Permissions afforded to a Role would, in most cases, be far too large to
+comfortably display in the table itself.
+
+Under-the-hood, API services will need to change from using Role IDs to Role
+names.
+
+### Traffic Ops Impact
+Traffic Ops will reqire changes to its Roles and Permissions API, chiefly, and
+the database tables that back them. Enforcement of API Permissions, though,
+will also have more wide-ranging impact that will require changes to very
+nearly every API endpoint.
+
+Additionally, a configuation file field will be added named `usePermissions`
+that is an optional boolean which, when present and `true` causes Traffic Ops
+to use Permissions rather than Role Privilege Level to determine a user's
+authorization for a given operation.
+
+#### API Impact
+Structurally, the only necessary changes are to the `/roles` and `/users`
+endpoints, which will need to be updated to output structures consistent with
+the changes outlined in the Data Model Impact section. The `/capabilities` and
+`/api_capabilities` endpoints will be removed (and we might consider renaming
+the `/server_capabilities` and `/server_server_capabilities` to drop the
+now-unnecessary "server_" qualifier).
+
+The more pervasive changes will be to all authenticated API endpoints which
+shall be updated to respect Permissions given the correct configuration
+setting.
+
+The exact Permissions that need to exist for each endpoint are best left to
+debate on the changeset that implements them.
+
+When a user attempts an operation for which they do not have sufficient
+Permissions, the API MUST respond with a `403 Forbidden` response containing an
+error-level Alert that describes what operation is not permitted and what
+Permission the user is missing that would allow them to proceed.
+
+#### Client Impact
+As Permissions are defined by the Role of the authenticated user, the only
+client changes are necessary beyond those necessitated by the removal of two
+API endpoints and the renaming and restructuring of a third are to stop
+filling in a Role ID on user creation/update from a Role name if an ID was
+unset (this will now be handled by the API itself, which will save a full
+HTTP request on each user creation/modification).
+
+#### Database Impact
+The new model for a Role does not allow a `null` description; a simple
+migration that coalesces existing `NULL` values to an empty string and adds a
+check constraint should be all that's actually required. No other immediate
+changes should be made - including dropping the now-unused `api_capability`
+table -, since old API versions will still need access to the deprecated
+columns. However, the foreign key constraint on the `role_capability` table
+that links a "Capability" name to a row in the `capability` table should be
+dropped, as that is no longer the source of truth for valid Permissions.
+
+Optionally, a migration should really be added to make a user's username
+`NOT NULL`, since that field is actually required by the API and many things
+will break if it's `NULL`.
+
+## Documentation Impact
+The new configuration option will need to be documented, documentation for
+removed API endpoints will itself need to be removed, documentation for the
+`/roles` and `/users` endpoints will need to be updated to reflect the new
+request and response structures, and as Permissions are implemented on each
+endpoint the Permissions it requires for various actions will need to be
+defined.
+
+## Testing Impact
+The most significant testing changes will need to be made to the Go Traffic Ops
+client integration tests, which should verify each endpoint's proper
+Permissions-based authorization. Traffic Portal functionality will also require
+the appropriate end-to-end tests.
+
+## Performance Impact
+No significant performance impact is expected, since the Permissions of a user
+are already queried at the time of servicing a request by every authenticated
+endpoint. Some negligible, constant time will need to be spend determining how
+to authorize an authenticated user.
+
+## Security Impact
+Careful consideration must be given to the design and implementation of each
+Permission. For example, this author believes a Permission named
+`do-secrets-things` that allows a Role unrestricted read and write access to
+any and all DNSSEC, SSL, URL Signature, and URI signing keys would be a poor
+design from a security standpoint. Permissions should be broad enough to
+encompass a single, well-defined purpose, and no more. In many cases, though,
+the existing "Capabilities" concepts will be good enough to build from (e.g.
+`delivery-services-write`, `cdns-read`).
+
+## Upgrade Impact
+There will be a database migration to run, but since the default configuration
+will be to ignore Permissions and just use Privilege Level, there isn't much in
+the way of upgrade impact immediately. The bigger step is getting ready for the
+_next_ upgrade, when Permissions stop being optional.
+
+## Operations Impact
Review comment:
granted i read thru this pretty quick so maybe you mentioned this but do you want to mention anything about how the current roles (admin, ops, r/o, etc) will be given perms to match their current behavior as dictated by priv level?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] rawlinp commented on a change in pull request #5848: User Permissions Blueprint
Posted by GitBox <gi...@apache.org>.
rawlinp commented on a change in pull request #5848:
URL: https://github.com/apache/trafficcontrol/pull/5848#discussion_r681238214
##########
File path: blueprints/user.permissions.md
##########
@@ -0,0 +1,286 @@
+<!--
+Licensed to the Apache Software Foundation (ASF) under one
+or more contributor license agreements. See the NOTICE file
+distributed with this work for additional information
+regarding copyright ownership. The ASF licenses this file
+to you under the Apache License, Version 2.0 (the
+"License"); you may not use this file except in compliance
+with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing,
+software distributed under the License is distributed on an
+"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+KIND, either express or implied. See the License for the
+specific language governing permissions and limitations
+under the License.
+-->
+# Enforced User Permissions
+
+## Problem Description
+Currently, the Permissions (currently "Capabilities", henceforth referred to as
+"Permissions" to avoid confusion with Server Capabilities) afforded to a user
+are defined by the "Privilege Level" of their Role. It essentially defines a
+scale on the interval [0,30], where a higher number allows more things. This
+isn't as scalable or configurable as it could be, and involves granting more
+permissions to a user than they actually need just because every Privilege
+Level encapsulates all of the permissions of every Privilege Level below it,
+making disjoint Permission sets for disparate Roles logically impossible.
+
+## Proposed Change
+Instead of a system of rigid supersets of Permissions, this blueprint proposes
+that the system of user "Capabilities" be expanded upon, renamed to Permissions
+(again, to avoid confusion) and enforced by the API.
+
+Under this system, each endpoint will declare the Permissions it requires for
+specific operations and check that the requesting user has those required, if
+any.
+
+This information is _not_ to be stored in the database nor exposed through the
+API, but documented. This change is discussed in greater detail in the
+"Alternatives" section.
+
+The enforcement of these Permissions is proposed to be configurable via an
+entry in the Traffic Ops configuration file. Thus, for one major version the
+API will need to support Permissions-based authorization as well as Privilege
+Level-based authorization.
+
+## Data Model Impact
+"Capabilities" as they are known today will be removed from the data model.
+Also, the current model of a Role will need to be modified. The current model
+is as follows:
+
+```typescript
+interface Role {
+ capabilities: Array<string> | null;
+ description: string | null;
+ id?: number | null;
+ name: string | null;
+ privLevel: number | null;
+}
+```
+
+The proposed new model is shown below.
+
+```typescript
+interface Role {
+ description: string;
+ lastUpdated?: Date;
+ name: string;
Review comment:
I thought someone said that roles will be per-tenant. Doesn't this mean it should have a `tenantId`?
##########
File path: blueprints/user.permissions.md
##########
@@ -0,0 +1,286 @@
+<!--
+Licensed to the Apache Software Foundation (ASF) under one
+or more contributor license agreements. See the NOTICE file
+distributed with this work for additional information
+regarding copyright ownership. The ASF licenses this file
+to you under the Apache License, Version 2.0 (the
+"License"); you may not use this file except in compliance
+with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing,
+software distributed under the License is distributed on an
+"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+KIND, either express or implied. See the License for the
+specific language governing permissions and limitations
+under the License.
+-->
+# Enforced User Permissions
+
+## Problem Description
+Currently, the Permissions (currently "Capabilities", henceforth referred to as
+"Permissions" to avoid confusion with Server Capabilities) afforded to a user
+are defined by the "Privilege Level" of their Role. It essentially defines a
+scale on the interval [0,30], where a higher number allows more things. This
+isn't as scalable or configurable as it could be, and involves granting more
+permissions to a user than they actually need just because every Privilege
+Level encapsulates all of the permissions of every Privilege Level below it,
+making disjoint Permission sets for disparate Roles logically impossible.
+
+## Proposed Change
+Instead of a system of rigid supersets of Permissions, this blueprint proposes
+that the system of user "Capabilities" be expanded upon, renamed to Permissions
+(again, to avoid confusion) and enforced by the API.
+
+Under this system, each endpoint will declare the Permissions it requires for
+specific operations and check that the requesting user has those required, if
+any.
+
+This information is _not_ to be stored in the database nor exposed through the
+API, but documented. This change is discussed in greater detail in the
+"Alternatives" section.
+
+The enforcement of these Permissions is proposed to be configurable via an
+entry in the Traffic Ops configuration file. Thus, for one major version the
+API will need to support Permissions-based authorization as well as Privilege
+Level-based authorization.
+
+## Data Model Impact
+"Capabilities" as they are known today will be removed from the data model.
+Also, the current model of a Role will need to be modified. The current model
+is as follows:
+
+```typescript
+interface Role {
+ capabilities: Array<string> | null;
+ description: string | null;
+ id?: number | null;
+ name: string | null;
+ privLevel: number | null;
+}
+```
+
+The proposed new model is shown below.
+
+```typescript
+interface Role {
+ description: string;
+ lastUpdated?: Date;
+ name: string;
+ permissions: Array<string>;
+}
+```
+
+Also, because the ID is no longer exposed via the API, the model for users will
+need to change to only reference Role name, not ID. So it'll change from...
+
+```typescript
+interface User {
+ addressLine1: string | null;
+ addressLine2: string | null;
+ city: string | null;
+ company: string | null;
+ confirmLocalPassword?: string | null;
+ country: string | null;
+ email: string | null;
+ fullName: string | null;
+ gid: number | null;
+ id?: number | null;
+ lastUpdated?: string | null;
+ localPassword?: string | null;
+ newUser: boolean | null;
+ phoneNumber: string | null;
+ postalCode: string | null;
+ publicSSHKey: string | null;
+ registrationSent: string | null;
+ role: number | null;
+ roleName?: string | null;
+ stateOrProvince: string | null;
+ tenant?: string | null;
+ tenantID: string | null;
+ uid: number | null;
+ username: string | null;
+}
+```
+
+... to ...
+
+```typescript
+interface User {
+ addressLine1: string | null;
+ addressLine2: string | null;
+ city: string | null;
+ company: string | null;
+ confirmLocalPassword?: string | null;
+ country: string | null;
+ email: string | null;
+ fullName: string;
+ gid: number | null;
+ id?: number;
+ lastUpdated?: Date;
+ localPassword?: string | null;
+ newUser: boolean;
+ phoneNumber: string | null;
+ postalCode: string | null;
+ publicSSHKey: string | null;
+ registrationSent: string | null;
+ role: string;
+ stateOrProvince: string | null;
+ tenant?: string;
+ tenantID: number;
+ uid: number | null;
+ username: string;
+}
+```
+
+... replacing the required `role` field that was a Role ID with one that is
+the Role's name (and therefore removing `roleName` as it's now redundant).
+
+(Note that `lastUpdated` changed from `string | null` to `Date`, which
+represents the transition to using proper RFC3339 formatting, and also some
+fields which cannot be `null` for a valid user are no longer `| null`. Those
+aren't changes necessary to implement Permissions, but can be done easily at
+the same time as necessary changes to User.)
+
+## Component Impact
+This change primarily concerns Traffic Ops and Traffic Portal, though any
+client of the API will need to be aware of the changes to its authorization
+system.
+
+### Traffic Portal Impact
+There is a "Capabilities" view currently in Traffic Portal that provides a
+table of user Permissions, though it is not linked to the side navigation bar.
+This should be removed.
+
+The Roles' details pages will need to be augmented with controls for adding and
+removing Permissions. The Permissions available for adding to a user should
+include an autocompletion of those known by Traffic Portal to exist, but
+shouldn't restrict the addition of Permissions not known to exist to accomodate
+running older versions of Traffic Portal with newer versions of Traffic Ops, as
+well as any plugins unbeknownst to Traffic Portal that add Traffic Ops API
+endpoints and may declare their own Permissions that do not exist in a vanilla
+Traffic Control environment.
+
+Also, the Roles table will be augmented with the ability to filter Roles by the
+presence or absence of a Permission, although the full itemization of the
+Permissions afforded to a Role would, in most cases, be far too large to
+comfortably display in the table itself.
+
+Under-the-hood, API services will need to change from using Role IDs to Role
+names.
+
+### Traffic Ops Impact
+Traffic Ops will reqire changes to its Roles and Permissions API, chiefly, and
+the database tables that back them. Enforcement of API Permissions, though,
+will also have more wide-ranging impact that will require changes to very
+nearly every API endpoint.
+
+Additionally, a configuation file field will be added named `usePermissions`
+that is an optional boolean which, when present and `true` causes Traffic Ops
+to use Permissions rather than Role Privilege Level to determine a user's
+authorization for a given operation.
+
+#### API Impact
+Structurally, the only necessary changes are to the `/roles` and `/users`
+endpoints, which will need to be updated to output structures consistent with
+the changes outlined in the Data Model Impact section. The `/capabilities` and
+`/api_capabilities` endpoints will be removed (and we might consider renaming
Review comment:
I don't think they were named that way just to distinguish them from Role Capabilities. They were specifically called Server Capabilities because they are the capabilities of a server. Renaming the endpoints would seem like an avoidable breaking change anyways.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] rawlinp commented on pull request #5848: User Permissions Blueprint
Posted by GitBox <gi...@apache.org>.
rawlinp commented on pull request #5848:
URL: https://github.com/apache/trafficcontrol/pull/5848#issuecomment-897982277
Ok, I was thinking more along the lines of `admin` being special in that it specifically doesn't have any permissions that would need checking, rather than having the existence of `all-read` and `all-write` permissions which could be assigned to other roles as well. It doesn't really make sense to have more than one super `admin` role. Plus, having `all-read` and `all-write` implies that every route also needs to identify as either a "write" route or a "read" route, and the middleware would have to check those special `all-write` or `all-read` permissions (in addition to the per-route permissions). That seems like unnecessary identification and handling when we could just start the check with `if role == "admin" || ...`. We can't really depend on the method to determine read vs write since someone could sneak a write into a read method or vice versa.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] mitchell852 commented on a change in pull request #5848: User Permissions Blueprint
Posted by GitBox <gi...@apache.org>.
mitchell852 commented on a change in pull request #5848:
URL: https://github.com/apache/trafficcontrol/pull/5848#discussion_r680197185
##########
File path: blueprints/user.permissions.md
##########
@@ -0,0 +1,286 @@
+<!--
+Licensed to the Apache Software Foundation (ASF) under one
+or more contributor license agreements. See the NOTICE file
+distributed with this work for additional information
+regarding copyright ownership. The ASF licenses this file
+to you under the Apache License, Version 2.0 (the
+"License"); you may not use this file except in compliance
+with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing,
+software distributed under the License is distributed on an
+"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+KIND, either express or implied. See the License for the
+specific language governing permissions and limitations
+under the License.
+-->
+# Enforced User Permissions
+
+## Problem Description
+Currently, the Permissions (currently "Capabilities", henceforth referred to as
+"Permissions" to avoid confusion with Server Capabilities) afforded to a user
+are defined by the "Privilege Level" of their Role. It essentially defines a
+scale on the interval [0,30], where a higher number allows more things. This
+isn't as scalable or configurable as it could be, and involves granting more
+permissions to a user than they actually need just because every Privilege
+Level encapsulates all of the permissions of every Privilege Level below it,
+making disjoint Permission sets for disparate Roles logically impossible.
+
+## Proposed Change
+Instead of a system of rigid supersets of Permissions, this blueprint proposes
+that the system of user "Capabilities" be expanded upon, renamed to Permissions
+(again, to avoid confusion) and enforced by the API.
+
+Under this system, each endpoint will declare the Permissions it requires for
+specific operations and check that the requesting user has those required, if
+any.
+
+This information is _not_ to be stored in the database nor exposed through the
+API, but documented. This change is discussed in greater detail in the
+"Alternatives" section.
+
+The enforcement of these Permissions is proposed to be configurable via an
+entry in the Traffic Ops configuration file. Thus, for one major version the
+API will need to support Permissions-based authorization as well as Privilege
+Level-based authorization.
+
+## Data Model Impact
+"Capabilities" as they are known today will be removed from the data model.
+Also, the current model of a Role will need to be modified. The current model
+is as follows:
+
+```typescript
+interface Role {
+ capabilities: Array<string> | null;
+ description: string | null;
+ id?: number | null;
+ name: string | null;
+ privLevel: number | null;
+}
+```
+
+The proposed new model is shown below.
+
+```typescript
+interface Role {
+ description: string;
+ lastUpdated?: Date;
+ name: string;
+ permissions: Array<string>;
+}
+```
+
+Also, because the ID is no longer exposed via the API, the model for users will
+need to change to only reference Role name, not ID. So it'll change from...
+
+```typescript
+interface User {
+ addressLine1: string | null;
+ addressLine2: string | null;
+ city: string | null;
+ company: string | null;
+ confirmLocalPassword?: string | null;
+ country: string | null;
+ email: string | null;
+ fullName: string | null;
+ gid: number | null;
+ id?: number | null;
+ lastUpdated?: string | null;
+ localPassword?: string | null;
+ newUser: boolean | null;
+ phoneNumber: string | null;
+ postalCode: string | null;
+ publicSSHKey: string | null;
+ registrationSent: string | null;
+ role: number | null;
+ roleName?: string | null;
+ stateOrProvince: string | null;
+ tenant?: string | null;
+ tenantID: string | null;
+ uid: number | null;
+ username: string | null;
+}
+```
+
+... to ...
+
+```typescript
+interface User {
+ addressLine1: string | null;
+ addressLine2: string | null;
+ city: string | null;
+ company: string | null;
+ confirmLocalPassword?: string | null;
+ country: string | null;
+ email: string | null;
+ fullName: string;
+ gid: number | null;
+ id?: number;
+ lastUpdated?: Date;
+ localPassword?: string | null;
+ newUser: boolean;
+ phoneNumber: string | null;
+ postalCode: string | null;
+ publicSSHKey: string | null;
+ registrationSent: string | null;
+ role: string;
+ stateOrProvince: string | null;
+ tenant?: string;
+ tenantID: number;
+ uid: number | null;
+ username: string;
+}
+```
+
+... replacing the required `role` field that was a Role ID with one that is
+the Role's name (and therefore removing `roleName` as it's now redundant).
+
+(Note that `lastUpdated` changed from `string | null` to `Date`, which
+represents the transition to using proper RFC3339 formatting, and also some
+fields which cannot be `null` for a valid user are no longer `| null`. Those
+aren't changes necessary to implement Permissions, but can be done easily at
+the same time as necessary changes to User.)
+
+## Component Impact
+This change primarily concerns Traffic Ops and Traffic Portal, though any
+client of the API will need to be aware of the changes to its authorization
+system.
+
+### Traffic Portal Impact
+There is a "Capabilities" view currently in Traffic Portal that provides a
+table of user Permissions, though it is not linked to the side navigation bar.
+This should be removed.
+
+The Roles' details pages will need to be augmented with controls for adding and
+removing Permissions. The Permissions available for adding to a user should
Review comment:
> The Permissions available for adding to a user
this sounds like you add perms to a user. is that true? or do you add perms to a role (which belongs to a user?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] rawlinp commented on a change in pull request #5848: User Permissions Blueprint
Posted by GitBox <gi...@apache.org>.
rawlinp commented on a change in pull request #5848:
URL: https://github.com/apache/trafficcontrol/pull/5848#discussion_r681829889
##########
File path: blueprints/user.permissions.md
##########
@@ -0,0 +1,286 @@
+<!--
+Licensed to the Apache Software Foundation (ASF) under one
+or more contributor license agreements. See the NOTICE file
+distributed with this work for additional information
+regarding copyright ownership. The ASF licenses this file
+to you under the Apache License, Version 2.0 (the
+"License"); you may not use this file except in compliance
+with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing,
+software distributed under the License is distributed on an
+"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+KIND, either express or implied. See the License for the
+specific language governing permissions and limitations
+under the License.
+-->
+# Enforced User Permissions
+
+## Problem Description
+Currently, the Permissions (currently "Capabilities", henceforth referred to as
+"Permissions" to avoid confusion with Server Capabilities) afforded to a user
+are defined by the "Privilege Level" of their Role. It essentially defines a
+scale on the interval [0,30], where a higher number allows more things. This
+isn't as scalable or configurable as it could be, and involves granting more
+permissions to a user than they actually need just because every Privilege
+Level encapsulates all of the permissions of every Privilege Level below it,
+making disjoint Permission sets for disparate Roles logically impossible.
+
+## Proposed Change
+Instead of a system of rigid supersets of Permissions, this blueprint proposes
+that the system of user "Capabilities" be expanded upon, renamed to Permissions
+(again, to avoid confusion) and enforced by the API.
+
+Under this system, each endpoint will declare the Permissions it requires for
+specific operations and check that the requesting user has those required, if
+any.
+
+This information is _not_ to be stored in the database nor exposed through the
+API, but documented. This change is discussed in greater detail in the
+"Alternatives" section.
+
+The enforcement of these Permissions is proposed to be configurable via an
+entry in the Traffic Ops configuration file. Thus, for one major version the
+API will need to support Permissions-based authorization as well as Privilege
+Level-based authorization.
+
+## Data Model Impact
+"Capabilities" as they are known today will be removed from the data model.
+Also, the current model of a Role will need to be modified. The current model
+is as follows:
+
+```typescript
+interface Role {
+ capabilities: Array<string> | null;
+ description: string | null;
+ id?: number | null;
+ name: string | null;
+ privLevel: number | null;
+}
+```
+
+The proposed new model is shown below.
+
+```typescript
+interface Role {
+ description: string;
+ lastUpdated?: Date;
+ name: string;
Review comment:
I don't think Roles should be tenantable; I just thought that's what someone said the current design was. But, it looks like the design has changed for the better. Disregard :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 commented on pull request #5848: User Permissions Blueprint
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on pull request #5848:
URL: https://github.com/apache/trafficcontrol/pull/5848#issuecomment-897985295
I don't hate the idea of `admin` being special in *that* way, it's just hard to know how to handle people redefining `admin` between install and upgrade.
It's also confusing to me why we have `all-read` and `all-write` instead of just `all` - `all-read` isn't used by anything else, not even `read-only` afaik, and `all-write` on its own just makes no sense at all.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] rawlinp commented on pull request #5848: User Permissions Blueprint
Posted by GitBox <gi...@apache.org>.
rawlinp commented on pull request #5848:
URL: https://github.com/apache/trafficcontrol/pull/5848#issuecomment-898011539
You still have to keep the priv-level around since the feature is optional (initially), right? So there shouldn't be any data loss, and I wouldn't really worry about escalating `admin` privileges if for some reason its priv-level was lowered, because in reality I'm having a hard time believing that anyone would've actually done that (although it is certainly _possible_, as are many things that shouldn't be done in TO).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] dneuman64 edited a comment on pull request #5848: User Permissions Blueprint
Posted by GitBox <gi...@apache.org>.
dneuman64 edited a comment on pull request #5848:
URL: https://github.com/apache/trafficcontrol/pull/5848#issuecomment-893701967
-1 on trying to use a config file over the database. It is an interesting idea for sure, but I don't think it really makes sense for something like this.
For whatever perceived development savings we think there will be, there will be more operational expense and, to me, I would rather have less operational expense. I don't want to try to manage permissions across n servers, no matter how good ansible/puppet/etc is at it.
It seems pretty straightforward to just put this in the database so that it can be managed via API and via the UI, I don't really see why we wouldn't just go with that plan.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] rawlinp commented on pull request #5848: User Permissions Blueprint
Posted by GitBox <gi...@apache.org>.
rawlinp commented on pull request #5848:
URL: https://github.com/apache/trafficcontrol/pull/5848#issuecomment-893021860
>> Not necessarily, we don't need the API to return which permissions belong to which roles.
> You would at the very least need to return which Roles exist, so that new users can be created. I also would be against only allowing users to be created with unknown Permissions, though that would be technically possible.
This is what I meant by keeping Roles in the API but not Permissions. You'd only be able to create Users with Roles that exist in the DB, but the permissions would be defined via config.
> Also, the new Traffic Portal is banking on visibility into what a user is allowed to do, in order to hide or disable UI elements to deliver different experiences to different users according to their needs.
One way to solve that would be to simply use the same config file for both TO and TP. The TO one would be authoritative, of course, while the TP one would be simply for hiding everything that their role doesn't allow. Or, we could have an API to return which permissions are currently assigned in TO's config file but still not allow assigning permissions via the API.
>> Since User Permissions are optional...
> Well, at first, yes, but only for a deprecation period. Manual translation of Roles does seem to be the only upgrade path
The new permissions config file could define the current permissions of the legacy Roles, but if we will eventually make User Permissions required, won't that mean that we'd always have to update the legacy Roles to add new permissions as we add new routes/functionality? I.e. every upgrade, we'd have to get all the new permissions and manually add them to the legacy Roles? Why can't we continue to assign privilege levels to routes instead of having to add permissions to legacy Roles with every upgrade? It seems like at least for admin, we shouldn't have to manually assign new permissions to it with every upgrade. Or do you think every new route that's added needs to have a DB migration to add the permission to the legacy Roles?
> The existing API tests do not have access to the internal memory of TO, though. We could very easily make unit tests like you describe, but end-to-end testing would be much harder without testing framework changes.
Permissions don't necessarily need tested in an end-to-end manner. We would still obviously have end-to-end tests for every route, but those end-to-end tests could focus on other things besides permissions. In fact, it would probably be overkill to test permissions in an end-to-end test for every route. The unit tests could cover permissions functionality as a whole, and we wouldn't necessarily require the specific permission to be tested on every route (although we could).
> > Since better security is the main goal here, it might make sense to make the implementation of this security feature as secure as possible.
> If Permissions aren't good enough security to manage Roles and Permissions, why are they secure enough for everything else? It's sort of like planning for defeat, seems to me.
I'm not really sure what you mean here, but storing permissions in a config file simply means there are fewer attack vectors in which an attacker could gain access to more permissions. DBs are usually available over the network, whereas on-disk config files typically aren't. Permissions stored in the DB data means they can be vulnerable to SQL injection. You also have to make sure your DB settings are secure. With a filesystem, you just have to set the proper file permissions so that even if an attacker was able to login to the system, they wouldn't be able to edit the permissions unless they had root (at which point they could do lots of nastier things anyways). I'm not saying Permissions aren't "good enough security"; I'm just saying that on-disk permissions could be considered "better security".
> *Adding* Permissions is unlikely to be time-sensitive, I agree, though I could probably imagine some contrived scenario for a poorly behaved script or something. But *removing* Permissions can be a security issue that needs to be addressed very quickly, which isn't addressed by just having "any admin... perform... the... task or get the user the time-sensitive data".
Even with on-disk permissions, we could still use the API to quickly change a user's role to one without permissions.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] mitchell852 commented on a change in pull request #5848: User Permissions Blueprint
Posted by GitBox <gi...@apache.org>.
mitchell852 commented on a change in pull request #5848:
URL: https://github.com/apache/trafficcontrol/pull/5848#discussion_r681241671
##########
File path: blueprints/user.permissions.md
##########
@@ -0,0 +1,286 @@
+<!--
+Licensed to the Apache Software Foundation (ASF) under one
+or more contributor license agreements. See the NOTICE file
+distributed with this work for additional information
+regarding copyright ownership. The ASF licenses this file
+to you under the Apache License, Version 2.0 (the
+"License"); you may not use this file except in compliance
+with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing,
+software distributed under the License is distributed on an
+"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+KIND, either express or implied. See the License for the
+specific language governing permissions and limitations
+under the License.
+-->
+# Enforced User Permissions
+
+## Problem Description
+Currently, the Permissions (currently "Capabilities", henceforth referred to as
+"Permissions" to avoid confusion with Server Capabilities) afforded to a user
+are defined by the "Privilege Level" of their Role. It essentially defines a
+scale on the interval [0,30], where a higher number allows more things. This
+isn't as scalable or configurable as it could be, and involves granting more
+permissions to a user than they actually need just because every Privilege
+Level encapsulates all of the permissions of every Privilege Level below it,
+making disjoint Permission sets for disparate Roles logically impossible.
+
+## Proposed Change
+Instead of a system of rigid supersets of Permissions, this blueprint proposes
+that the system of user "Capabilities" be expanded upon, renamed to Permissions
+(again, to avoid confusion) and enforced by the API.
+
+Under this system, each endpoint will declare the Permissions it requires for
+specific operations and check that the requesting user has those required, if
+any.
+
+This information is _not_ to be stored in the database nor exposed through the
+API, but documented. This change is discussed in greater detail in the
+"Alternatives" section.
+
+The enforcement of these Permissions is proposed to be configurable via an
+entry in the Traffic Ops configuration file. Thus, for one major version the
+API will need to support Permissions-based authorization as well as Privilege
+Level-based authorization.
+
+## Data Model Impact
+"Capabilities" as they are known today will be removed from the data model.
+Also, the current model of a Role will need to be modified. The current model
+is as follows:
+
+```typescript
+interface Role {
+ capabilities: Array<string> | null;
+ description: string | null;
+ id?: number | null;
+ name: string | null;
+ privLevel: number | null;
+}
+```
+
+The proposed new model is shown below.
+
+```typescript
+interface Role {
+ description: string;
+ lastUpdated?: Date;
+ name: string;
+ permissions: Array<string>;
+}
+```
+
+Also, because the ID is no longer exposed via the API, the model for users will
+need to change to only reference Role name, not ID. So it'll change from...
+
+```typescript
+interface User {
+ addressLine1: string | null;
+ addressLine2: string | null;
+ city: string | null;
+ company: string | null;
+ confirmLocalPassword?: string | null;
+ country: string | null;
+ email: string | null;
+ fullName: string | null;
+ gid: number | null;
+ id?: number | null;
+ lastUpdated?: string | null;
+ localPassword?: string | null;
+ newUser: boolean | null;
+ phoneNumber: string | null;
+ postalCode: string | null;
+ publicSSHKey: string | null;
+ registrationSent: string | null;
+ role: number | null;
+ roleName?: string | null;
+ stateOrProvince: string | null;
+ tenant?: string | null;
+ tenantID: string | null;
+ uid: number | null;
+ username: string | null;
+}
+```
+
+... to ...
+
+```typescript
+interface User {
+ addressLine1: string | null;
+ addressLine2: string | null;
+ city: string | null;
+ company: string | null;
+ confirmLocalPassword?: string | null;
+ country: string | null;
+ email: string | null;
+ fullName: string;
+ gid: number | null;
+ id?: number;
+ lastUpdated?: Date;
+ localPassword?: string | null;
+ newUser: boolean;
+ phoneNumber: string | null;
+ postalCode: string | null;
+ publicSSHKey: string | null;
+ registrationSent: string | null;
+ role: string;
+ stateOrProvince: string | null;
+ tenant?: string;
+ tenantID: number;
+ uid: number | null;
+ username: string;
+}
+```
+
+... replacing the required `role` field that was a Role ID with one that is
+the Role's name (and therefore removing `roleName` as it's now redundant).
+
+(Note that `lastUpdated` changed from `string | null` to `Date`, which
+represents the transition to using proper RFC3339 formatting, and also some
+fields which cannot be `null` for a valid user are no longer `| null`. Those
+aren't changes necessary to implement Permissions, but can be done easily at
+the same time as necessary changes to User.)
+
+## Component Impact
+This change primarily concerns Traffic Ops and Traffic Portal, though any
+client of the API will need to be aware of the changes to its authorization
+system.
+
+### Traffic Portal Impact
+There is a "Capabilities" view currently in Traffic Portal that provides a
+table of user Permissions, though it is not linked to the side navigation bar.
+This should be removed.
+
+The Roles' details pages will need to be augmented with controls for adding and
+removing Permissions. The Permissions available for adding to a user should
+include an autocompletion of those known by Traffic Portal to exist, but
+shouldn't restrict the addition of Permissions not known to exist to accomodate
+running older versions of Traffic Portal with newer versions of Traffic Ops, as
+well as any plugins unbeknownst to Traffic Portal that add Traffic Ops API
+endpoints and may declare their own Permissions that do not exist in a vanilla
+Traffic Control environment.
+
+Also, the Roles table will be augmented with the ability to filter Roles by the
+presence or absence of a Permission, although the full itemization of the
+Permissions afforded to a Role would, in most cases, be far too large to
+comfortably display in the table itself.
+
+Under-the-hood, API services will need to change from using Role IDs to Role
+names.
+
+### Traffic Ops Impact
+Traffic Ops will reqire changes to its Roles and Permissions API, chiefly, and
+the database tables that back them. Enforcement of API Permissions, though,
+will also have more wide-ranging impact that will require changes to very
+nearly every API endpoint.
+
+Additionally, a configuation file field will be added named `usePermissions`
+that is an optional boolean which, when present and `true` causes Traffic Ops
+to use Permissions rather than Role Privilege Level to determine a user's
+authorization for a given operation.
+
+#### API Impact
+Structurally, the only necessary changes are to the `/roles` and `/users`
+endpoints, which will need to be updated to output structures consistent with
+the changes outlined in the Data Model Impact section. The `/capabilities` and
+`/api_capabilities` endpoints will be removed (and we might consider renaming
+the `/server_capabilities` and `/server_server_capabilities` to drop the
+now-unnecessary "server_" qualifier).
+
+The more pervasive changes will be to all authenticated API endpoints which
+shall be updated to respect Permissions given the correct configuration
+setting.
+
+The exact Permissions that need to exist for each endpoint are best left to
+debate on the changeset that implements them.
+
+When a user attempts an operation for which they do not have sufficient
+Permissions, the API MUST respond with a `403 Forbidden` response containing an
+error-level Alert that describes what operation is not permitted and what
+Permission the user is missing that would allow them to proceed.
+
+#### Client Impact
+As Permissions are defined by the Role of the authenticated user, the only
+client changes are necessary beyond those necessitated by the removal of two
+API endpoints and the renaming and restructuring of a third are to stop
+filling in a Role ID on user creation/update from a Role name if an ID was
+unset (this will now be handled by the API itself, which will save a full
+HTTP request on each user creation/modification).
+
+#### Database Impact
+The new model for a Role does not allow a `null` description; a simple
+migration that coalesces existing `NULL` values to an empty string and adds a
+check constraint should be all that's actually required. No other immediate
+changes should be made - including dropping the now-unused `api_capability`
+table -, since old API versions will still need access to the deprecated
+columns. However, the foreign key constraint on the `role_capability` table
Review comment:
> the existing `role_capability` table by just dropping the foreign key constraint
so the existing role_cap table will hold the the perms for a role?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #5848: User Permissions Blueprint
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #5848:
URL: https://github.com/apache/trafficcontrol/pull/5848#discussion_r681196961
##########
File path: blueprints/user.permissions.md
##########
@@ -0,0 +1,286 @@
+<!--
+Licensed to the Apache Software Foundation (ASF) under one
+or more contributor license agreements. See the NOTICE file
+distributed with this work for additional information
+regarding copyright ownership. The ASF licenses this file
+to you under the Apache License, Version 2.0 (the
+"License"); you may not use this file except in compliance
+with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing,
+software distributed under the License is distributed on an
+"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+KIND, either express or implied. See the License for the
+specific language governing permissions and limitations
+under the License.
+-->
+# Enforced User Permissions
+
+## Problem Description
+Currently, the Permissions (currently "Capabilities", henceforth referred to as
+"Permissions" to avoid confusion with Server Capabilities) afforded to a user
+are defined by the "Privilege Level" of their Role. It essentially defines a
+scale on the interval [0,30], where a higher number allows more things. This
+isn't as scalable or configurable as it could be, and involves granting more
+permissions to a user than they actually need just because every Privilege
+Level encapsulates all of the permissions of every Privilege Level below it,
+making disjoint Permission sets for disparate Roles logically impossible.
+
+## Proposed Change
+Instead of a system of rigid supersets of Permissions, this blueprint proposes
+that the system of user "Capabilities" be expanded upon, renamed to Permissions
+(again, to avoid confusion) and enforced by the API.
+
+Under this system, each endpoint will declare the Permissions it requires for
+specific operations and check that the requesting user has those required, if
+any.
+
+This information is _not_ to be stored in the database nor exposed through the
+API, but documented. This change is discussed in greater detail in the
+"Alternatives" section.
+
+The enforcement of these Permissions is proposed to be configurable via an
+entry in the Traffic Ops configuration file. Thus, for one major version the
+API will need to support Permissions-based authorization as well as Privilege
+Level-based authorization.
+
+## Data Model Impact
+"Capabilities" as they are known today will be removed from the data model.
+Also, the current model of a Role will need to be modified. The current model
+is as follows:
+
+```typescript
+interface Role {
+ capabilities: Array<string> | null;
+ description: string | null;
+ id?: number | null;
+ name: string | null;
+ privLevel: number | null;
+}
+```
+
+The proposed new model is shown below.
+
+```typescript
+interface Role {
+ description: string;
+ lastUpdated?: Date;
+ name: string;
+ permissions: Array<string>;
+}
+```
+
+Also, because the ID is no longer exposed via the API, the model for users will
+need to change to only reference Role name, not ID. So it'll change from...
+
+```typescript
+interface User {
+ addressLine1: string | null;
+ addressLine2: string | null;
+ city: string | null;
+ company: string | null;
+ confirmLocalPassword?: string | null;
+ country: string | null;
+ email: string | null;
+ fullName: string | null;
+ gid: number | null;
+ id?: number | null;
+ lastUpdated?: string | null;
+ localPassword?: string | null;
+ newUser: boolean | null;
+ phoneNumber: string | null;
+ postalCode: string | null;
+ publicSSHKey: string | null;
+ registrationSent: string | null;
+ role: number | null;
+ roleName?: string | null;
+ stateOrProvince: string | null;
+ tenant?: string | null;
+ tenantID: string | null;
+ uid: number | null;
+ username: string | null;
+}
+```
+
+... to ...
+
+```typescript
+interface User {
+ addressLine1: string | null;
+ addressLine2: string | null;
+ city: string | null;
+ company: string | null;
+ confirmLocalPassword?: string | null;
+ country: string | null;
+ email: string | null;
+ fullName: string;
+ gid: number | null;
+ id?: number;
+ lastUpdated?: Date;
+ localPassword?: string | null;
+ newUser: boolean;
+ phoneNumber: string | null;
+ postalCode: string | null;
+ publicSSHKey: string | null;
+ registrationSent: string | null;
+ role: string;
+ stateOrProvince: string | null;
+ tenant?: string;
+ tenantID: number;
+ uid: number | null;
+ username: string;
+}
+```
+
+... replacing the required `role` field that was a Role ID with one that is
+the Role's name (and therefore removing `roleName` as it's now redundant).
+
+(Note that `lastUpdated` changed from `string | null` to `Date`, which
+represents the transition to using proper RFC3339 formatting, and also some
+fields which cannot be `null` for a valid user are no longer `| null`. Those
+aren't changes necessary to implement Permissions, but can be done easily at
+the same time as necessary changes to User.)
+
+## Component Impact
+This change primarily concerns Traffic Ops and Traffic Portal, though any
+client of the API will need to be aware of the changes to its authorization
+system.
+
+### Traffic Portal Impact
+There is a "Capabilities" view currently in Traffic Portal that provides a
+table of user Permissions, though it is not linked to the side navigation bar.
+This should be removed.
+
+The Roles' details pages will need to be augmented with controls for adding and
+removing Permissions. The Permissions available for adding to a user should
+include an autocompletion of those known by Traffic Portal to exist, but
+shouldn't restrict the addition of Permissions not known to exist to accomodate
+running older versions of Traffic Portal with newer versions of Traffic Ops, as
+well as any plugins unbeknownst to Traffic Portal that add Traffic Ops API
+endpoints and may declare their own Permissions that do not exist in a vanilla
+Traffic Control environment.
+
+Also, the Roles table will be augmented with the ability to filter Roles by the
+presence or absence of a Permission, although the full itemization of the
+Permissions afforded to a Role would, in most cases, be far too large to
+comfortably display in the table itself.
+
+Under-the-hood, API services will need to change from using Role IDs to Role
+names.
+
+### Traffic Ops Impact
+Traffic Ops will reqire changes to its Roles and Permissions API, chiefly, and
+the database tables that back them. Enforcement of API Permissions, though,
+will also have more wide-ranging impact that will require changes to very
+nearly every API endpoint.
+
+Additionally, a configuation file field will be added named `usePermissions`
+that is an optional boolean which, when present and `true` causes Traffic Ops
+to use Permissions rather than Role Privilege Level to determine a user's
+authorization for a given operation.
+
+#### API Impact
+Structurally, the only necessary changes are to the `/roles` and `/users`
+endpoints, which will need to be updated to output structures consistent with
+the changes outlined in the Data Model Impact section. The `/capabilities` and
+`/api_capabilities` endpoints will be removed (and we might consider renaming
Review comment:
Because the only reason those have that qualifier is to distinguish them from Role Capabilities, which will no longer exist.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org