What can user do #82

jhadvig · 2016-06-20T19:13:52Z

Created a new AuthorizationService to determine actions that user can/can't do.
The new service consists from two main methods:

reviewUserRules
- which serves for reviewing multiple resource/verb pairs (namespaced and also non-namespaced)
- resource/verb pair combinations are listed in the $scope.canI variable which should be set in each controller which will call this method.
- obtained rules will be cached, until refreshing the page,changing projects, when 3 minute interval passes
canI
- lightweight version of the first method (response only takes info if user is allowed to perform action on the ressource)
- servers for reviewing single resource/verb pair (namespaced and also non-namespaced)
- does not need $scope.canI variable, will create one if not present in controller and adds the result of the resource/verb review to it.

@jwforres PTAL

Implements https://trello.com/c/UQEUgVrY

spadgett · 2016-06-20T19:16:42Z

app/scripts/services/authorization.js

+      if (pageRules) {
+        _.each(this.rules, function(rule) {
+          _.each(pageRules, function(verbs, resource) {
+            if (_.indexOf(rule.resources, resource) !== -1) {


Does this work (here and below)?

_.includes(rule.resources, resource)

jhadvig · 2016-06-20T19:18:02Z

will add bindata after review :)

jwforres · 2016-06-20T19:27:46Z

I suspect @liggitt will be interested in this one

jwforres · 2016-06-20T20:07:24Z

So first, keep your existing changes in a side branch since you have them working ...
But I'd like you to try something else since I would prefer to see no changes to the controllers. Here is what I am thinking:

Add a filter that passes through to AuthorizationService.canI (similar to what we do with the navigate filters), AuthorizationService.canI should just return false if a rules review has not yet been performed. Views should call the canI filter for the individual actions.
To determine whether to show the Actions dropdown menu, you'll want to add a filter which is canIDoAny that accepts an array of actions it wants to check and will return true as soon as it determines the user can perform any of the actions.
Instead of having all the controllers for namespace scoped pages requesting to have a rules review performed, we should request a rules review as part of ProjectsService.getProject

I've at least run this by @spadgett and @benjaminapetersen and we agree in theory

liggitt · 2016-06-20T21:07:38Z

app/scripts/services/authorization.js

+    // added into the $scope.canI variable.
+    // In case a resource shall to be added into the $scope.canI variable under a different unique name(eg. not as a 
+    // resource kind) but a different unique name(eg. namespace), use the 'identifier' parameter.  
+    AuthorizationService.prototype.canI = function(ns, verb, kind, $scope, identifier) {


should this be more specific regarding api group, kind, and resource?

spadgett · 2016-06-21T13:00:16Z

app/scripts/services/authorization.js

+      this.projectNameRules = null;
+    }
+
+    AuthorizationService.prototype.reviewUserRules = function($scope) {


I know we do this in other places, but we shouldn't pass scope to services. Services shouldn't know anything about a controller's scope.

spadgett · 2016-06-23T13:15:05Z

Related to https://bugzilla.redhat.com/show_bug.cgi?id=1293791

jhadvig · 2016-06-23T16:07:20Z

@jwforres I've updated the PR(not final) to get additional feedback.
Things I want to ask:

I've add the AuthorizationService.getProjectRules call to the projects.js service, but not sure if that s the right place, since at the settings page im experiancing that the $digest is not triggered when the $http get call response with the selfsubjectrulesreviews.
Whats you opinion on showing the 'missing health check' notification, specifically the links to 'Add Health Check'. Also for metrics, as QA asked in the card https://trello.com/c/UQEUgVrY/661-3-only-show-users-actions-they-have-authority-to-perform

benjaminapetersen · 2016-06-23T19:22:25Z

app/scripts/services/authorization.js

+    AuthorizationService.prototype.canI = function(resource, verb) {
+      var rules = this.getRules();
+      if (rules) {
+        if (resource) {


You can probably just return _.contains(rules[resource], verb) here since underscore will return true or false for you.

jwforres · 2016-07-06T13:09:02Z

app/views/browse/build.html

                  <!-- Primary Actions -->
                  <button class="btn btn-default hidden-xs"
                          ng-click="cancelBuild()"
-                          ng-if="!build.metadata.deletionTimestamp && (build | isIncompleteBuild)">Cancel Build</button>
+                          ng-if="!build.metadata.deletionTimestamp && (build | isIncompleteBuild) && ('builds/clone' | canI : 'create')">Cancel Build</button>


create on builds/clone isnt the right check for cancel build, copy/paste error i'm guessing, same thing on line 42 below. Should just be checking update on builds i think.

jwforres · 2016-07-06T13:56:19Z

Can you go ahead and add this to otherResources.js so that it will only put things in the dropdown that can actually be listed by the user:

   // for context, goes underneath the .then
    ProjectsService
      .get($routeParams.project)
      .then(_.spread(function(project, context
...
...
       // add this
        $scope.kinds = _.filter($scope.kinds, function(kind){
          var resource = APIService.kindToResource(kind.kind);
          return AuthorizationService.canI("list", kind.group ? kind.group + "/" + resource : resource, $scope.projectName);
        });

You'll also need to include AuthorizationService in the list of deps at the top of the service. You'll know its working if you can't see PetSets.

Then in constants.js please remove

      "LocalResourceAccessReview",
      "LocalSubjectAccessReview",
      "ResourceAccessReview",
      "SubjectAccessReview",
      "ReplicationControllerDummy",

from the blacklist, they'll now be cleanly removed from the list because the List verb isn't allowed on them. You need to leave DeploymentConfigRollback for now.

liggitt · 2016-07-06T13:58:33Z

app/scripts/services/authorization.js

+      //  - subresource taht contains '/', eg: 'builds/source', 'builds/logs', ...
+      //  - resource ending with 'review' string: 'localsubjectaccessreviews', 'selfsubjectrulesreviews', 'subjectaccessreviews' 
+      var checkResource = function(resource) {
+        if (resource === "projectrequests" || _.contains(resource, "/") || resource.endsWith("reviews")) {


checking if it contains / is fine, but I'd rather not use endsWith... we should have an explicit list (or hash) of resources we don't care about for this

jwforres · 2016-07-06T14:00:24Z

also remove ReplicaSet, PodTemplate, and ThirdPartyResource from the blacklist

jwforres · 2016-07-06T17:17:09Z

There is another View Log link that needs a canI check here https://github.com/openshift/origin-web-console/pull/82/files#diff-d40b418167447e1277db88f1163bb5d9L117

jwforres · 2016-07-06T17:24:23Z

app/views/browse/pod.html

@@ -122,7 +122,7 @@
                  <uib-tab active="selectedTab.terminal"
                    select="terminalTabWasSelected = true"
                    ng-init="containers = pod.status.containerStatuses"
-                    ng-if="containersRunning(pod.status.containerStatuses) > 0">
+                    ng-if="containersRunning(pod.status.containerStatuses) > 0 && ('pods/attach' | canI : 'get')">


this should be pods/exec

jhadvig · 2016-07-11T14:39:17Z

@jwforres @benjaminapetersen @liggitt I've update the PR based on your comments, expect the one that suggests to return the rules together with the project and context, since we have canI, canIDoAny, canIScale, canIAddToProject filters that take care of any users actions they have authority to perform

benjaminapetersen · 2016-07-11T16:00:24Z

Cool, I'm good with that.

jwforres · 2016-07-11T17:06:52Z

app/scripts/services/authorization.js

+          }
+        });
+        if (canAddToProject) {
+          return false;


why is this not returning true?

ignore this comment, i'd rather see https://github.com/openshift/origin-web-console/pull/82/files#r70297639 happen

jwforres · 2016-07-11T17:27:44Z

[merge]

jwforres · 2016-07-11T18:50:00Z

looks like it might have just been a flake [merge]

jwforres · 2016-07-11T19:04:47Z

failed on the same test, @jhadvig can you run grunt test-integration locally and see if its passing

jwforres · 2016-07-11T19:11:22Z

[merge] will let it go one more time, but i suspect this may be related to additional delay before the delete action is being shown

openshift-bot · 2016-07-11T19:31:10Z

Origin Web Console Merge Results: SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin_web_console/104/)

jwforres · 2016-07-11T20:59:58Z

[merge]

openshift-bot · 2016-07-11T21:01:00Z

Evaluated for origin web console merge up to 332bffe

fixed bug ORS-73

spadgett reviewed Jun 20, 2016
View reviewed changes

liggitt reviewed Jun 20, 2016
View reviewed changes

openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 20, 2016

spadgett reviewed Jun 21, 2016
View reviewed changes

jhadvig force-pushed the can_i branch 2 times, most recently from c377e4f to 701a714 Compare June 23, 2016 15:55

openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 23, 2016

benjaminapetersen reviewed Jun 23, 2016
View reviewed changes

openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 28, 2016

jhadvig force-pushed the can_i branch from 701a714 to 1b7fc62 Compare June 29, 2016 15:56

jwforres reviewed Jul 6, 2016
View reviewed changes

liggitt reviewed Jul 6, 2016
View reviewed changes

jwforres reviewed Jul 6, 2016
View reviewed changes

jhadvig force-pushed the can_i branch 3 times, most recently from cf3e23c to 155e7ef Compare July 11, 2016 14:38

jhadvig force-pushed the can_i branch from 155e7ef to 599c40c Compare July 11, 2016 15:37

jwforres reviewed Jul 11, 2016
View reviewed changes

jhadvig force-pushed the can_i branch from 599c40c to 545f505 Compare July 11, 2016 17:20

what can user do

332bffe

jhadvig force-pushed the can_i branch from 545f505 to 332bffe Compare July 11, 2016 20:57

openshift-bot merged commit 563e73a into openshift:master Jul 11, 2016

rassie mentioned this pull request Jul 14, 2016

Broken with Origin 1.2 API since introduction of what-can-i-do #176

Closed

f0x11 pushed a commit to f0x11/origin-web-console that referenced this pull request Jan 11, 2018

Merge pull request openshift#82 from pwzgorilla/poc

83c04ad

fixed bug ORS-73

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

What can user do #82

What can user do #82

jhadvig commented Jun 20, 2016 •

edited by jwforres

Loading

spadgett Jun 20, 2016

jhadvig commented Jun 20, 2016

jwforres commented Jun 20, 2016

jwforres commented Jun 20, 2016

liggitt Jun 20, 2016

spadgett Jun 21, 2016

spadgett commented Jun 23, 2016

jhadvig commented Jun 23, 2016

benjaminapetersen Jun 23, 2016

jwforres Jul 6, 2016

jwforres commented Jul 6, 2016

liggitt Jul 6, 2016

jwforres commented Jul 6, 2016

jwforres commented Jul 6, 2016

jwforres Jul 6, 2016

jhadvig commented Jul 11, 2016 •

edited

Loading

benjaminapetersen commented Jul 11, 2016

jwforres Jul 11, 2016

jwforres Jul 11, 2016

jwforres commented Jul 11, 2016

jwforres commented Jul 11, 2016

jwforres commented Jul 11, 2016

jwforres commented Jul 11, 2016

openshift-bot commented Jul 11, 2016 •

edited

Loading

jwforres commented Jul 11, 2016

openshift-bot commented Jul 11, 2016

What can user do #82

What can user do #82

Conversation

jhadvig commented Jun 20, 2016 • edited by jwforres Loading

Choose a reason for hiding this comment

jhadvig commented Jun 20, 2016

jwforres commented Jun 20, 2016

jwforres commented Jun 20, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spadgett commented Jun 23, 2016

jhadvig commented Jun 23, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jwforres commented Jul 6, 2016

Choose a reason for hiding this comment

jwforres commented Jul 6, 2016

jwforres commented Jul 6, 2016

Choose a reason for hiding this comment

jhadvig commented Jul 11, 2016 • edited Loading

benjaminapetersen commented Jul 11, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jwforres commented Jul 11, 2016

jwforres commented Jul 11, 2016

jwforres commented Jul 11, 2016

jwforres commented Jul 11, 2016

openshift-bot commented Jul 11, 2016 • edited Loading

jwforres commented Jul 11, 2016

openshift-bot commented Jul 11, 2016

jhadvig commented Jun 20, 2016 •

edited by jwforres

Loading

jhadvig commented Jul 11, 2016 •

edited

Loading

openshift-bot commented Jul 11, 2016 •

edited

Loading