Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Users without the Publish Permission on a namespace can see the Publish button #2243

Closed
kolotu opened this issue Jan 20, 2020 · 11 comments
Closed
Labels
Milestone

Comments

@kolotu
Copy link
Contributor

kolotu commented Jan 20, 2020

Users that do not have PUBLISH permission can see the publish button on the details view. Nothing happens when the user clicks on the button, but it should not be visible to them in the first place.

@kolotu kolotu added the bug label Jan 20, 2020
@ghost
Copy link

ghost commented Jan 20, 2020

Will take a look.

@kolotu kolotu assigned ghost Jan 20, 2020
@ghost
Copy link

ghost commented Jan 20, 2020

Replicated. Also worth noting, clicking the Publish button does bring up the warning modal, and accepting the modal does strictly nothing.
Should be solved by just not having the button there, which might however require some magic behind the scenes.

@ghost
Copy link

ghost commented Jan 20, 2020

Looks like the GET call to /rest/models/[the model id]/policy for a user without publish permissions gives me

adminPolicy: false
permission: "FULL_ACCESS"
principalId: "MODEL_CREATOR"
principalType: "Role"

... which subsequently allows showing the Publish button.

There might be something wrong with that.

@ghost
Copy link

ghost commented Jan 20, 2020

I guess the issue is that the PolicyEntry DTO returned by that controller does not convey the required information on the "tenant user"'s roles for that namespace, which is the one that would have sufficient granularity to differentiate between single buttons such as the "Publish" one.

Not sure if there's an easy way to change that without breaking things, will need some more research.

@ghost
Copy link

ghost commented Jan 20, 2020

We have an existing REST endpoint that does not look like it's being used at all:

/rest/tenants/{tenantId}/users/{userId} (in AccountController).

A call to this would return the namespace user ("tenant user") permissions (which we somehow ended up calling "roles").

Can probably cook up some POC for this, although I will have to think harder in order to have only one REST call within context, before considering for merge.

@ghost
Copy link

ghost commented Jan 21, 2020

Discovered what seems to be a related bug, albeit cosmetic: a non-private namespace owner is not advertised to have the right to publish (although they obviously have the manage right).

Suspect the right to publish is implicitly derived from the higher manage privilege, or somehow derived from the namespace ownership any other way, since owners can obviously publish models in their own ns.

It still leads to weird situations where it looks like lambda users may have the right to publish whereas owners do not in appearance.

See example below:

Screenshot from 2020-01-21 13-32-03

  • mena-bosch is the owner here, and does not seem to have the right to publish (although he does)
  • menajrl is a user mena-bosch added with all privileges, aka nominally "one more" than mena-bosch
  • in reality, both users will have the right to publish as is

TL;DR

Will see if I can bundle a fix for this within this task (as it's more of a cosmetic bug) - otherwise will open another ticket...

Edit I think the fix for the main task deprecates this issue.

@ghost
Copy link

ghost commented Jan 21, 2020

Another somehow related bug (this last one might have been introduced by myself with #2208 or more probably, #2088: the first time one adds a user to a namespace, the permission fields are not sent to the back-end, which results in the PUT request failing and the front-end stating "You cannot change your own permission" (completely unrelated) as a boilerplate error message.
Will need to create a separate issue about this one at some point...

Edit this one is still valid and I've formalized it in #2247.

@ghost
Copy link

ghost commented Jan 21, 2020

Back to this specific issue, the /rest/tenants/{tenantId}/users/{userId} doesn't seem to be usable as there is no tenantId available in that context.

However, the policy DTO seems to return the "best" policy for that user for that model.
Replicating the logic but sending all "policies" instead of the "best" one would allow the right granularity for the job at hand, since we'd just need to check whether the one with principalId equals MODEL_PUBLISHER is present.

Note that so far however, updates on a current user for a given namespace do not seem to work, even after logging the user off and back in.

@ghost
Copy link

ghost commented Jan 21, 2020

And another note on the above: the REST endpoint already exists - hadn't spotted it at first.
Will see if can use as is.

@ghost
Copy link

ghost commented Jan 21, 2020

PR here.

@aedelmann if time allows, could you advise on whether lowering the pre-authorization for the endpoint in question constitutes a risk I don't understand, or is it good to go as is?

@kolotu kolotu added this to the 0.13 milestone Jan 22, 2020
@ghost
Copy link

ghost commented Jan 24, 2020

@kolotu since the PR is merged, I think you can close this.

@kolotu kolotu closed this as completed Jan 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

1 participant