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.

Sign up for GitHub

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

Jump to bottom

Remove member group checks and check for permission instead #1275

Merged
merged 29 commits into from
Jul 19, 2023
Merged

Remove member group checks and check for permission instead #1275

merged 29 commits into from
Jul 19, 2023

Conversation

Rieven
Copy link
Contributor

@Rieven Rieven commented Jun 21, 2023 •
edited

Changes

Remove group checks in especially onboarding and general checks for groups at certain place in the code base.

Issue link

#1050

Closes #1050

Proof

Please add some proof of your working change here, unless this is not required (e.g. this PR is trivial).

Code Checklist

  • All the commits in this PR are properly PGP-signed and verified;
  • This PR only contains functionality relevant to the issue; tickets have been created for newly discovered issues.
  • I have written unit tests for the changes or fixes I made.
  • For any non-trivial functionality, I have added integration and/or end-to-end tests.
  • I have performed a self-review of my code and refactored it to the best of my abilities.

Communication

  • I have informed others of any required .env changes files if required and changed the .env-dist accordingly.
  • I have made corresponding changes to the documentation, if necessary.

Checklist for code reviewers:

Copy-paste the checklist from the docs/source/templates folder into your comment.

Checklist for QA:

Copy-paste the checklist from the docs/source/templates folder into your comment.

@Rieven Rieven marked this pull request as ready for review June 26, 2023 09:49
@Rieven Rieven requested a review from a team as a code owner June 26, 2023 09:49
ammar92
ammar92 reviewed Jun 27, 2023
View reviewed changes
Copy link
Contributor

@ammar92 ammar92 left a comment

Choose a reason for hiding this comment

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

A few minor remarks, looks good otherwise

rocky/onboarding/view_helpers.py Outdated
@@ -3,6 +3,8 @@
from django.utils.translation import gettext_lazy as _
from tools.view_helpers import StepsMixin

ONBOARIDNG_PERMS = ["tools.can_scan_organization", "tools.can_set_clearance_level", "tools.can_enable_disable_boefje"]
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
ONBOARIDNG_PERMS = ["tools.can_scan_organization", "tools.can_set_clearance_level", "tools.can_enable_disable_boefje"]
ONBOARIDNG_PERMISSIONS = ["tools.can_scan_organization", "tools.can_set_clearance_level", "tools.can_enable_disable_boefje"]

Copy link
Contributor

Choose a reason for hiding this comment

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

You're also better of with a tuple instead of a list

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see there is a typo also in ONBOARDING...fix it in c82ccb9

ammar92
ammar92 previously approved these changes Jul 4, 2023
View reviewed changes
@Darwinkel
Copy link
Contributor

Checklist for QA:

  • I have checked out this branch, and successfully ran a fresh make reset.
  • I confirmed that there are no unintended functional regressions in this branch:
    • I have managed to pass the onboarding flow
    • Objects and Findings are created properly
    • Tasks are created and completed properly
  • I confirmed that the PR's advertised feature or hotfix works as intended.

What doesn't work:

  • During the onboarding there are no longer any steps for creating a DNSReport / adding an object. After creating users (or skipping that step), the user immediately lands in the crisis room.

@TwistMeister
Copy link
Contributor

I noticed that when going through the onboarding as a superuser the "create report" steps are skipped. Is this intentional? I can understand that it could be, but for me personally its a bit undesirable because I liked the option to create the report in the onboarding flow to populate KAT with some basic objects and tasks. And of course you could skip this if you wanted. So I feel it's less flexible now than it was before, and am wondering if that's really what we want.

@TwistMeister
Copy link
Contributor

@Rieven I verified and it was intentional for superusers to follow the whole onboarding flow, so it's regression when a part of it gets skipped.

@Darwinkel
Copy link
Contributor

Checklist for QA:

  • I have checked out this branch, and successfully ran a fresh make reset.
  • I confirmed that there are no unintended functional regressions in this branch:
    • I have managed to pass the onboarding flow
    • Objects and Findings are created properly
    • Tasks are created and completed properly
  • I confirmed that the PR's advertised feature or hotfix works as intended.

What works:

  • Original onboarding flow has been restored 👍

dekkers
dekkers reviewed Jul 18, 2023
View reviewed changes
rocky/tools/management/commands/setup_dev_account.py Show resolved Hide resolved
rocky/account/forms/account_setup.py
@@ -159,7 +159,7 @@ def save(self, **kwargs):
status=OrganizationMember.STATUSES.ACTIVE,
)
member.groups.add(selected_group.id)
if member.is_admin or self.user.is_superuser:
if member.has_perm("change_organizationmember") or self.user.is_superuser:
Copy link
Contributor

@dekkers dekkers Jul 18, 2023 •
edited

Choose a reason for hiding this comment

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

Why does a new member that can change organization members automatically get scan level 4? Wouldn't it be better to have the user set this explicitly? And if we want to have this way, shouldn't we document this and probably make this also clear in the user interface?

This is something we can change in a later PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here admins have by default trusted and acknowledged clearance levels by L4 so they can start scans but do not have clearance permission on objects, so redteamers are the only ones that can increase clearance levels on objects.

We can refactor this part to put defaults at member creation when member is an admin.

@dekkers dekkers merged commit 38753e3 into main Jul 19, 2023
17 checks passed
@dekkers dekkers deleted the feature/remove-group-checks-onboarding branch July 19, 2023 08:12
jpbruinsslot added a commit that referenced this pull request Jul 24, 2023
@jpbruinsslot
Merge branch 'main' into feature/mula/rate-limit
f70c8fb 
* main:
  Fix robot test (#1420)
  Use the correct clearance level variable in organization member list template (#1427)
  Fix translation in Debian package (#1432)
  Reschedule tasks when no results in bytes are found after grace period (#1410)
  Don't scan hostname nmap in nmap boefje (#1415)
  Add and use our own CVE API (#1383)
  Add `task_id` as a query parameter to the `GET /origins` endpoint (#1414)
  Remove member group checks and check for permission instead (#1275)
  Bump cryptography from 41.0.0 to 41.0.2 in /boefjes/boefjes/plugins/kat_ssl_certificates (#1396)
  Bump cryptography from 41.0.1 to 41.0.2 in /bytes (#1397)
  Build the Debian build image on the main branch (#1387)
  Add explicit `black` config to all modules (#1395)
  Fix <no title> in the user guide docs (#1391)
  Add configurable octpoes request timeout (#1382)
  Remove hardcoded clearance level in member list for superusers (#1390)
  Add Debian build depends for CVE API package (#1384)
  Add buttons to manual rerun tasks, both boefjes or normalizers (#1339)
  Use fix multiprocessing bug on macOS where `qsize()` is not implemented (#1374)
jpbruinsslot added a commit that referenced this pull request Jul 25, 2023
@jpbruinsslot
Merge branch 'main' into feature/mula/rescheduling
3607738 
* main: (95 commits)
  Translations for release 1.11 - EN -> NL, PAP (#1439)
  Add Question ooi model and create the first bit that generates a question (#1407)
  make port classification configurable (#1418)
  KATalogus API filtering and pagination (#1405)
  Fix robot test (#1420)
  Use the correct clearance level variable in organization member list template (#1427)
  Fix translation in Debian package (#1432)
  Reschedule tasks when no results in bytes are found after grace period (#1410)
  Don't scan hostname nmap in nmap boefje (#1415)
  Add and use our own CVE API (#1383)
  Add `task_id` as a query parameter to the `GET /origins` endpoint (#1414)
  Remove member group checks and check for permission instead (#1275)
  Bump cryptography from 41.0.0 to 41.0.2 in /boefjes/boefjes/plugins/kat_ssl_certificates (#1396)
  Bump cryptography from 41.0.1 to 41.0.2 in /bytes (#1397)
  Build the Debian build image on the main branch (#1387)
  Add explicit `black` config to all modules (#1395)
  Fix <no title> in the user guide docs (#1391)
  Add configurable octpoes request timeout (#1382)
  Remove hardcoded clearance level in member list for superusers (#1390)
  Add Debian build depends for CVE API package (#1384)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dekkers dekkers dekkers approved these changes

@ammar92 ammar92 ammar92 left review comments

Assignees

@Rieven Rieven

Labels
None yet
Projects
KAT
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Remove dependency on hardcoded admin/redteam/clients groups in onboarding
5 participants
@Rieven @Darwinkel @TwistMeister @dekkers @ammar92