Fix wrong 'archiveLink' for operations user #3060

josefkarasek · 2018-08-14T10:29:40Z

Opening this PR mostly to get feedback. I'll open a new PR against 3.9 and 3.10 ones the fix is ready, with proper grunt build and test.

fixes https://bugzilla.redhat.com/show_bug.cgi?id=1587807

spadgett · 2018-08-14T13:10:26Z

app/scripts/directives/logViewer.js


+                var canI = $filter('canI');
+                var podsLogVersion = APIService.getPreferredVersion('pods/log');
+                var canViewOperationsLogs = canI(podsLogVersion, 'view');


This is checking that you can view the log you're already viewing... @jcantrill Is this really what we want?

This specific issue is that we changed the available index patterns if you are an operations user. You used to get a list of every project in the cluster which was exceedingly long and we modified the seeding to provide only one which matched them all with 'project.*'. The complementary change was not captured in the web console. Operations users are being redirected to Kibana to use an index-pattern which does not exist.

Here we are trying to determine if the use is an operations user by using the same SAR we use in the multi-tenant plugin.

Here we are trying to determine if the use is an operations user by using the same SAR we use in the multi-tenant plugin.

What SAR specifically do you have?

Only operations users should be able to view pods/log from the logging project, so:

oc auth can-i view pods/log -n openshift-logging

spadgett · 2018-08-14T13:11:28Z

app/scripts/services/logLinks.js

-      var archiveUri = function(opts, prefix) {
+      var archiveUri = function(opts, prefix, canViewOperationsLogs) {
+        if(canViewOperationsLogs) {
+          if(!prefix || (prefix && prefix.startsWith('project.'))) {


These two lines can be collapsed into a single if condition

josefkarasek · 2018-08-14T10:38:48Z

app/scripts/directives/logViewer.js


+                var canI = $filter('canI');
+                var podsLogVersion = APIService.getPreferredVersion('pods/log');
+                var canViewOperationsLogs = canI(podsLogVersion, 'view');


@spadgett This function is confusing me - the third parameter, project, makes the function behave in a way that I don't understand.

When I run the following code while browsing any project other than openshift-logging, logged in as cluster-admin

canI(podsLogVersion, 'view', 'openshift-logging'');

the function evaluates to false. But when I browse pods/log from openshift-logging project, the function evaluates to true. I'm trying to write something like:

oc adm policy who-can view pods/log -n openshift-logging

and weirdly enough, I get that functionality when I don't specify the project in the function call.
I have to take in consideration that the user is either operations user (who can view project & operations logs) or a regular user.

Any idea how to translate the above oc adm command to a API call? Thank you.

If you need to check a specific permission in another project, you should use SelfSubjectAccessReview

That's what the canI filter does under the hood, isn't it?
I'm trying to use its projectName argument, I assume that

AuthorizationService.canI(resource, verb, projectName)

will issue a request against the Authorization API and SSAR is used on the server, or not?

No, that's not what it does... You should run a separate SelfSubjectAccessReview request to check access in another project.

Is there some support for it in the code base?

No, but it should be pretty easy to call DataService.create with a SelfSubjectAccessReview object:

https://kubernetes.io/docs/reference/access-authn-authz/authorization/#checking-api-access

That's the hint I was hoping for, thanks a lot!

josefkarasek · 2018-08-16T10:30:33Z

app/scripts/directives/logViewer.js

-                                        containername: $scope.options.container,
-                                        backlink: URI.encode($window.location.href)
-                                      }, $filter('annotation')($scope.context.project,'loggingDataPrefix')))
+                // var ssarVersion = APIService.getPreferredVersion('selfsubjectaccessreviews');


I tried to add SSAR to preferredVersions but somehow this change didn't take effect, even after grunt build.
@spadgett can you advice how to add it?

Don't worry about adding a preferred version. You'd need to update the origin-web-common repo and cut a release. I don't think it's worth it here.

josefkarasek · 2018-08-16T10:33:00Z

app/scripts/directives/logViewer.js

+                  return data.status.allowed;
+                }, function() {
+                  return false;
+                }).then(function(canViewOperationsLogs) {


@spadgett @jcantrill I'm struggling with the async nature of promises. As this function is not called externally (click on a button etc), at this point log view is blocked. Can you help me how to make sure this promise gets evaluated?

I had an issue with my VM, all log files disappeared from the file system. That's why I was seeing weird behavior in the console. The code should be fine.

josefkarasek · 2018-08-16T10:33:35Z

app/scripts/services/logLinks.js

+        }
        prefix = prefix || 'project.' + opts.namespace + '.' + opts.namespaceUid;
        opts.index = prefix + '.*';
+        console.log('link: ' + opts.index);


Will delete once the fix is ready.

spadgett · 2018-08-16T17:04:14Z

app/scripts/directives/logViewer.js

+                    }
+                  }
+                };
+                var promise = DataService.create('selfsubjectaccessreviews', null, ssar, {namespace: 'openshift-logging'});


Make sure the API version in the request is the same as the endpoint you're calling. I believe it would be

DataService.create({ group: 'authorization.k8s.io', version: 'v1', resource: 'selfsubjectaccessreviews'}, ...

We should be caching this response somewhere so we only check it once per session. You probably want to move this into an Angular service.

What if I remove role from a user? That could be a possible danger it that user is logged in a session and has dirty cash.

Disregard, we'll do another round of auth on our side.

spadgett · 2018-08-16T17:07:09Z

/cc @jcantrill

Jeff should review as well

josefkarasek · 2018-08-22T10:48:10Z

Depends on openshift/origin-web-common#342

josefkarasek · 2018-08-22T10:59:59Z

@jcantrill there are still a few issues that need to be resolved

The 'Archive link' button sends a kibana search query[1] in the request's body,
which is discarded during the oauth process (http 302 redirects remove the body).
As a result the user lands at default kibana index pattern
The token sent in the request body[1] is ignored by the proxy.
Oauth proxy can be configured to read Authorization: Bearer: token header.
Problem in our case is that the web console uses tokens with scope user:info user:check-access,
but we need user:info user:check-access user:list-projects scope so that Elasticsearch plugin can use this token to check whether the user is operations user.

[1] https://github.com/openshift/origin-web-console/blob/master/app/views/directives/logs/_log-viewer.html#L10-L16

spadgett · 2018-08-22T12:26:00Z

If you guys have switched to the OAuth proxy, I don't think we should be sending the token from the console at all anymore, correct?

cc @jwforres

jwforres · 2018-08-22T13:10:25Z

the original discussion was the switch to using the oauth proxy was supposed to be transparent to console users and that the existing token mechanism was going to behave the same

the discussion about changing the behavior to stop passing token had been a converged console discussion

jcantrill · 2018-08-22T14:14:06Z

@jwforres @spadgett I incorrectly was under the impression the oauth-proxy was able to function in the same manor as the original proxy. Pending investigation from @josefkarasek about if it its possible to function in the previous capacity, we may need to revert the proxy being used in order to not have a functional regression. We discussed within the logging team regarding the 4.0 release not having a link from console to kibana which means this work would be uneeded. It means however we need to continue to release the old proxy which I was hoping to avoid

app/views/directives/logs/_log-viewer.html

josefkarasek · 2018-08-29T11:58:22Z

@jcantrill @spadgett please let me know what you think. I will then update the PR so we can finish this. Thank you

jcantrill · 2018-08-29T14:40:41Z

@josefkarasek @spadgett this #3067 will resolve the form post/get issue

jcantrill · 2018-08-29T17:31:19Z

app/scripts/directives/logViewer.js

             DataService,
             HTMLService,
             ModalsService,
+             AggregatedLoggingService,


This is not defined in this repo. Please move to this repo per @spadgett request

It's in vendor.js

You are missing the point. The intention is not do any other release of origin-web-common. All the files in 'dist' are generated files that are vendored in through an automated process. The recommendation is o close #342 and move the file to origin-web-console

jcantrill · 2018-08-29T17:33:37Z

app/scripts/directives/logViewer.js

-                                      }, $filter('annotation')($scope.context.project,'loggingDataPrefix')))
+
+                var currentUser = AuthService.UserStore().getUser().metadata.name;
+                AggregatedLoggingService.isOperationsUser(currentUser).then(function(canViewOperationsLogs) {


I don't think this needs to be encapsulated this way but I defer to @spadgett. You should be able to simply call:
AggregatedLoggingService.isOperationsUser(currentUser) and assign it to a var. and then pass it in as a param to archiveURI

It has to be a promise since it makes a selfsubjectaccessreview request to the server, which is asynchronous

app/scripts/services/aggregatedLoggingService.js

spadgett · 2018-08-30T12:31:33Z

app/scripts/directives/logViewer.js

    'DataService',
    'HTMLService',
    'ModalsService',
+    'AggregatedLoggingService',


We try to keep these alphabetized

spadgett · 2018-08-30T12:32:57Z

app/scripts/services/aggregatedLoggingService.js

+    // Create SelfSubjectAccessReview request againt authorization API to check whether
+    // current user can view pods/log from 'openshift-logging' project.
+    // Users who can view such logs are 'operations' users
+    var isOperationsUser = function(userName) {


This should not need to take in a user name since it uses self subject access review. (The cached data will be cleared on logout because the page is reloaded, so that's not a problem here.)

spadgett · 2018-08-30T12:34:20Z

app/scripts/services/aggregatedLoggingService.js

+
+    var cachedUserPermissions = $cacheFactory('operationsUsersCache', {
+      number: 10
+    });


This seems like overkill since we are only ever storing one value (if the current user is an operations user)

spadgett · 2018-08-30T12:34:48Z

app/scripts/services/aggregatedLoggingService.js

+      number: 10
+    });
+
+    var inFlightPermissionsRequest = null;


I don't think this is needed since you should never have multiple in flight requests for this specific permission

spadgett · 2018-08-30T12:36:52Z

app/scripts/services/aggregatedLoggingService.js

+        // Using cached data.
+        Logger.log("AggregatedLoggingService, using cached user " + userName);
+        if ((_.now() - userAllowed.cacheTimestamp) >= 600000) {
+          userAllowed.forceRefresh = true;


I wouldn't worry about forcing refresh... This permission does not change often. Let's just always use the cached value.

spadgett · 2018-08-30T12:37:47Z

app/scripts/services/aggregatedLoggingService.js

+        if ((_.now() - userAllowed.cacheTimestamp) >= 600000) {
+          userAllowed.forceRefresh = true;
+        }
+        return $q.when(!!_.get(userAllowed, ['allowed']));


Why not just...

return $q.when(userAllowed.allowed);

spadgett · 2018-08-30T12:38:49Z

app/views/directives/logs/_log-viewer.html

-      </button>
-    </form>
+  <div ng-if="kibanaArchiveUrl">
+    <a href="{{kibanaArchiveUrl}}" class="btn btn-primary btn-lg">View Archive</a>


ng-href

Why does this have button styling? It should just be a normal link.

spadgett · 2018-08-30T12:40:18Z

app/scripts/services/logLinks.js


-      var archiveUri = function(opts, prefix) {
+      var archiveUri = function(opts, prefix, canViewOperationsLogs) {
+        if((canViewOperationsLogs && !prefix) || (canViewOperationsLogs && prefix && prefix.startsWith('project.'))) {


if (canViewOperationsLogs && (!prefix || prefix.startsWith('project.')) {

josefkarasek · 2018-08-31T13:16:36Z

@spadgett are you happy with the latest changes?
If yes, what do we need to do to have this patch merged? This PR is against master, is that ok?
I ran tests against okd 3.10 with latest logging images and they pass.

spadgett · 2018-08-31T14:36:23Z

app/scripts/services/aggregatedLoggingService.js

+                          null, ssar, {namespace: 'default'}).then(
+          function(data) {
+            userAllowed = data.status.allowed;
+            return Promise.resolve(userAllowed);


return userAllowed;

spadgett · 2018-08-31T14:36:51Z

app/scripts/services/aggregatedLoggingService.js

+            userAllowed = data.status.allowed;
+            return Promise.resolve(userAllowed);
+          }, function() {
+            return Promise.resolve(false);


return false;

spadgett · 2018-08-31T14:37:44Z

app/scripts/services/aggregatedLoggingService.js

+            }
+          }
+        };
+        authPromise = DataService.create({ group: 'authorization.k8s.io', version: 'v1', resource: 'selfsubjectaccessreviews'},


You don't need to create a Promise yourself. Just return the Promise from DataService.create

return DataService.create(...).then(...);

spadgett · 2018-08-31T14:38:27Z

app/scripts/services/logLinks.js


-      var archiveUri = function(opts, prefix) {
+      var archiveUri = function(opts, prefix, canViewOperationsLogs) {
+        if((canViewOperationsLogs) && (!prefix || prefix.startsWith('project.'))) {


style nit: i think the extra parens around canViewOperationsLogs actually make it harder to read

spadgett · 2018-08-31T14:45:01Z

app/scripts/services/aggregatedLoggingService.js

+    var isOperationsUser = function() {
+      var authPromise = null;
+
+      if (userAllowed === undefined) {


I'd switch this to an early return if we have a cached value. It's easier to reason about.

if (userAllowed !== undefined) { return $q.when(userAllowed); } var ssar = { ... }; return DataService.create(...).then(...);

spadgett · 2018-08-31T18:25:58Z

[FATAL] Built /dist does not match what is committed, run 'grunt build' and include the results in your commit.

spadgett

LGTM but there seems to be a dist mismatch

spadgett · 2018-08-31T18:26:51Z

app/scripts/directives/logViewer.js

+                  $scope.$watchGroup(['context.project.metadata.name', 'options.container', 'name'], function() {
+                    angular.extend($scope, {
+                      // The archive URL violates angular's built in same origin policy.
+                      // Need to explicitly tell it to trust this location or it will throw errors.


I'm not sure if this is still needed now that it's a link instead of a form. You can probably get rid of the trustAsResourceUrl call

Comment is out of date with the code now

spadgett · 2018-08-31T18:27:23Z

app/scripts/services/aggregatedLoggingService.js

+          return userAllowed;
+        }, function() {
+          return false;
+      });


👍 thanks, this looks much cleaner

josefkarasek · 2018-09-03T10:52:15Z

@spadgett Thank you for your help and feedback. If there's still something to be done please let me know.

spadgett

LGTM, except please remove the incorrect comment

spadgett · 2018-09-04T13:36:03Z

app/scripts/directives/logViewer.js

+                  $scope.$watchGroup(['context.project.metadata.name', 'options.container', 'name'], function() {
+                    angular.extend($scope, {
+                      // The archive URL violates angular's built in same origin policy.
+                      // Need to explicitly tell it to trust this location or it will throw errors.


Comment is out of date with the code now

spadgett · 2018-09-04T13:36:14Z

app/scripts/directives/logViewer.js

+                      // The archive URL violates angular's built in same origin policy.
+                      // Need to explicitly tell it to trust this location or it will throw errors.
+                      kibanaArchiveUrl: logLinks.archiveUri({
+                                          baseURL: url,


nit: indentation

not sure what's wrong with indentation here - a new block of code - indented by 2 spaces against its parent block

spadgett

/lgtm

Thanks @josefkarasek 👍

jcantrill · 2018-09-04T16:49:43Z

/cherrypick release-3.10

openshift-cherrypick-robot · 2018-09-04T16:50:02Z

@jcantrill: #3060 failed to apply on top of branch "release-3.10":

error: Failed to merge in the changes.
Using index info to reconstruct a base tree...
M	app/index.html
M	app/scripts/directives/logViewer.js
M	app/scripts/services/logLinks.js
M	app/views/directives/logs/_log-viewer.html
M	dist/scripts/scripts.js
M	dist/scripts/templates.js
M	dist/scripts/vendor.js
Falling back to patching base and 3-way merge...
Auto-merging dist/scripts/vendor.js
Auto-merging dist/scripts/templates.js
Auto-merging dist/scripts/scripts.js
CONFLICT (content): Merge conflict in dist/scripts/scripts.js
Auto-merging app/views/directives/logs/_log-viewer.html
Auto-merging app/scripts/services/logLinks.js
Auto-merging app/scripts/directives/logViewer.js
CONFLICT (content): Merge conflict in app/scripts/directives/logViewer.js
Auto-merging app/index.html
Patch failed at 0001 Fix wrong 'archiveLink' for operations user

In response to this:

/cherrypick release-3.10

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

jcantrill · 2018-09-04T16:58:48Z

@josefkarasek please backport to 3.10 and 3.9 to fix relevent issues

spadgett · 2018-09-04T17:09:41Z

Note that this will need some rework if we aren't using oauth proxy in 3.10 and 3.9

openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Aug 14, 2018

josefkarasek requested a review from spadgett August 14, 2018 10:29

spadgett requested changes Aug 14, 2018

View reviewed changes

josefkarasek commented Aug 14, 2018

View reviewed changes

openshift-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Aug 16, 2018

josefkarasek commented Aug 16, 2018

View reviewed changes

spadgett reviewed Aug 16, 2018

View reviewed changes

openshift-ci-robot requested a review from jcantrill August 16, 2018 17:07

josefkarasek force-pushed the bz1587807-archive-link branch from 6002f80 to 71ccb32 Compare August 29, 2018 11:54

josefkarasek commented Aug 29, 2018

View reviewed changes

app/views/directives/logs/_log-viewer.html Show resolved Hide resolved

josefkarasek force-pushed the bz1587807-archive-link branch from 71ccb32 to 3d5fa60 Compare August 29, 2018 16:05

openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 29, 2018

josefkarasek force-pushed the bz1587807-archive-link branch from b1ab31e to b2e3cdd Compare August 29, 2018 16:51

jcantrill reviewed Aug 29, 2018

View reviewed changes

josefkarasek commented Aug 30, 2018

View reviewed changes

app/scripts/services/aggregatedLoggingService.js Show resolved Hide resolved

josefkarasek mentioned this pull request Aug 30, 2018

Check whether current user is operations user openshift/origin-web-common#342

Closed

spadgett requested changes Aug 30, 2018

View reviewed changes

openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 30, 2018

josefkarasek force-pushed the bz1587807-archive-link branch 2 times, most recently from 27f7837 to fc1a1aa Compare August 30, 2018 18:46

jcantrill removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 31, 2018

openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 31, 2018

jcantrill changed the title ~~[WIP] Fix wrong 'archiveLink' for operations user~~ Fix wrong 'archiveLink' for operations user Aug 31, 2018

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 31, 2018

spadgett requested changes Aug 31, 2018

View reviewed changes

spadgett reviewed Aug 31, 2018

View reviewed changes

josefkarasek force-pushed the bz1587807-archive-link branch 2 times, most recently from cb82403 to 043ed71 Compare August 31, 2018 15:04

spadgett reviewed Aug 31, 2018

View reviewed changes

josefkarasek force-pushed the bz1587807-archive-link branch from 043ed71 to ef6e11c Compare September 3, 2018 10:47

spadgett reviewed Sep 4, 2018

View reviewed changes

Fix wrong 'archiveLink' for operations user

40e9714

josefkarasek force-pushed the bz1587807-archive-link branch from ef6e11c to 40e9714 Compare September 4, 2018 14:38

spadgett approved these changes Sep 4, 2018

View reviewed changes

openshift-ci-robot assigned spadgett Sep 4, 2018

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 4, 2018

openshift-merge-robot merged commit 926f65e into openshift:master Sep 4, 2018

This was referenced Oct 18, 2018

fix kibana archive link for operations #3086

Closed

[enterprise-3.10] fix kibana archive link for operations #3087

Merged

[enterprise-3.9] fix kibana archive link for operations #3088

Merged

Fix wrong 'archiveLink' for operations user #3060

Fix wrong 'archiveLink' for operations user #3060

Uh oh!

Conversation

josefkarasek commented Aug 14, 2018 • edited by jcantrill Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

spadgett commented Aug 16, 2018

Uh oh!

josefkarasek commented Aug 22, 2018

Uh oh!

josefkarasek commented Aug 22, 2018

Uh oh!

spadgett commented Aug 22, 2018

Uh oh!

jwforres commented Aug 22, 2018

Uh oh!

jcantrill commented Aug 22, 2018

Uh oh!

Uh oh!

josefkarasek commented Aug 29, 2018

Uh oh!

jcantrill commented Aug 29, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

josefkarasek commented Aug 14, 2018 •

edited by jcantrill

Loading