Skip to content
This repository has been archived by the owner on Apr 17, 2023. It is now read-only.

introduce more optional user restrictions #1003

Merged
merged 1 commit into from
Jul 25, 2016
Merged

introduce more optional user restrictions #1003

merged 1 commit into from
Jul 25, 2016

Conversation

monstermunchkin
Copy link
Contributor

Options regarding user permissions can be found under the
user_permission node in the config file.

The three options are:

  • change_visibility
    permits the user to change the visibility of namespaces he/she owns
  • modify_team
    permits the user to change team attributes, e.g. name or description,
    given that he/she is an owners
  • modify_namespace
    permits the user to change namespace attributes, e.g. name, team, or
    description, given that he/she is an owners

Note that the previous user restriction regarding namespace visibility
only applied to a user's personal namespace. This has been extended to
all namespaces the user owns.

Resolves #676

Signed-off-by: Thomas Hipp thipp@suse.de

@@ -48,6 +48,14 @@
APP_CONFIG["registry"] = {
"jwt_expiration_time" => { "value" => 5 },
"catalog_page" => { "value" => 100 }
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing comma. That's the compiler error you're getting 😉

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, it should be a missing curly brace 😉

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, sure. I was reading it as if it was all in the same hash 😁

@mssola
Copy link
Collaborator

mssola commented Jul 21, 2016

Moreover, you have to add options for portusctl. Bear in mind that they don't necessarily have to be named like user-permission-change-visiblity. Take a look at the format_key method in the packaging/suse/portusctl/spec/options_spec.rb file.

Other than that, everything looks good to me 👍

@monstermunchkin
Copy link
Contributor Author

Oh right, totally forgot about those additions. Thanks!

@monstermunchkin
Copy link
Contributor Author

Done.

Now, that I'm thinking about it, we should also hide the create/edit buttons from users if needed. Otherwise, these would only lead to confusion.

@monstermunchkin monstermunchkin changed the title introduce more optional user restrictions [WIP] introduce more optional user restrictions Jul 21, 2016
@monstermunchkin
Copy link
Contributor Author

This should be it.

@monstermunchkin monstermunchkin changed the title [WIP] introduce more optional user restrictions introduce more optional user restrictions Jul 21, 2016
(namespace.global? && user.admin?) || \
(!namespace.global? && (user.admin? || namespace.team.owners.exists?(user.id)))
user.admin? || (APP_CONFIG.enabled?("user_permission.change_visibility") &&
!namespace.global? && namespace.team.owners.exists?(user.id))
Copy link

@holgerreif holgerreif Jul 21, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The message "must be logged in" as at least misleading (to be correct: it is wrong) in the case of prohibition by APP_CONFIG. Actually the user is logged in but he is not allowed to do the change.

This applies to other code changes in this commit as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The message "must be logged in" is only shown if a user is not logged in. In case of prohibition by APP_CONFIG, the user should just get a 401. The Ruby code might look a bit confusing at first.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, now I see. The raise statement ends with "unless user". Sorry for confusion and thanks for explaination.

@holgerreif
Copy link

In #676 there was a consence about

In my opinion, it should be possible to disable team/namespace creation for normal users.
Admins should be able to create teams/repos on behalf of normal users.

From looking at the code I do not see, that this is accomplished with this commit (of course, I'm not much of a ruby expert)

@monstermunchkin
Copy link
Contributor Author

The options modify_team and modify_namespace take care of this. If they are disabled, users cannot manage (create/modify) any of these. This is checked in the *_policy.rb files. Does this help?

@holgerreif
Copy link

The options modify_team and modify_namespace take care of this.

Thanks, I wasn't sure that creation is deemed a modifaction ;-)

We will test this (together with some other fixes) after it gets merged and an images is published on dockerhub.

@monstermunchkin
Copy link
Contributor Author

Thanks, I wasn't sure that creation is deemed a modifaction ;-)

Good point. Perhaps we should rename them to something like manage_team and manage_namespace. On the other hand, the comments above both options clearly state what effect these options have.

@mssola
Copy link
Collaborator

mssola commented Jul 25, 2016

@monstermunchkin maybe renaming it to manage_team and manage_namespace is more clear. That being said, I don't have any strong feelings on this.

def update?
!@team.hidden? && owner?
(APP_CONFIG.enabled?("user_permission.modify_team") || user.admin?) && !@team.hidden? && owner?
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

create? && !@team.hidden? && owner?

Options regarding user permissions can be found under the
`user_permission` node in the config file.

The three options are:

* **change_visibility**
permits the user to change the visibility of namespaces he/she owns
* **modify_team**
permits the user to change team attributes, e.g. name or description,
given that he/she is an owners
* **modify_namespace**
permits the user to change namespace attributes, e.g. name, team, or
description, given that he/she is an owners

Note that the previous user restriction regarding namespace visibility
only applied to a user's personal namespace. This has been extended to
all namespaces the user owns.

Resolves #676

Signed-off-by: Thomas Hipp <thipp@suse.de>
@monstermunchkin
Copy link
Contributor Author

The options have been renamed to manage_team and manage_namespace.

@mssola mssola merged commit 16b188b into SUSE:master Jul 25, 2016
@mssola
Copy link
Collaborator

mssola commented Jul 25, 2016

Thanks 👏

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants