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

Add deny list and validator for organization code #800

Merged
merged 38 commits into from
May 11, 2023

Conversation

Rieven
Copy link
Contributor

@Rieven Rieven commented Apr 24, 2023

Changes

We must exclude some organization codes that cannot be used when creating a new organization. Some organization codes like admin conflicts with the Django admin. We might extend the deny list when more confliction occurs or things we really want to deny.

Issue link

Closes #510

Proof

image

From admin:
image

From the api:
image


Checklists for authors:

Code Checklist

  • 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:

  • The code does not violate Model-View-Template and our other architectural principles.
  • The code prioritizes readability over performance where appropriate.
  • The code does not bypass authentication or security mechanisms.
  • The code does not introduce any dependency on a library that has not been properly vetted.
  • The code contains docstrings, comments, and documentation where needed.

Checklist for QA:

  • I have checked out this branch, and successfully ran a fresh make kat.
  • 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:

  • bullet point + screenshot (if useful) per tested functionality

What doesn't work:

  • bullet point + screenshot (if useful) per tested functionality

Bug or feature?:

  • bullet point + screenshot (if useful) if it is unclear whether something is a bug or an intended feature.

@Rieven Rieven requested a review from a team as a code owner April 24, 2023 14:00
rocky/tools/models.py Outdated Show resolved Hide resolved
rocky/tools/models.py Outdated Show resolved Hide resolved
rocky/tools/models.py Outdated Show resolved Hide resolved
rocky/tests/test_organization.py Outdated Show resolved Hide resolved
@github-actions
Copy link
Contributor

github-actions bot commented Apr 25, 2023

File Coverage
All files 65%
bits/definitions.py 65%
bits/runner.py 56%
bits/https_availability/https_availability.py 93%
bits/oois_in_headers/oois_in_headers.py 57%
bits/spf_discovery/internetnl_spf_parser.py 55%
bits/spf_discovery/spf_discovery.py 72%
octopoes/api/api.py 58%
octopoes/api/models.py 75%
octopoes/api/router.py 46%
octopoes/core/app.py 80%
octopoes/core/service.py 44%
octopoes/events/events.py 96%
octopoes/events/manager.py 65%
octopoes/models/__init__.py 86%
octopoes/models/datetime.py 66%
octopoes/models/exception.py 83%
octopoes/models/origin.py 70%
octopoes/models/path.py 99%
octopoes/models/types.py 95%
octopoes/models/ooi/certificate.py 96%
octopoes/models/ooi/config.py 89%
octopoes/models/ooi/email_security.py 95%
octopoes/models/ooi/findings.py 94%
octopoes/models/ooi/network.py 97%
octopoes/models/ooi/service.py 91%
octopoes/models/ooi/software.py 71%
octopoes/models/ooi/web.py 81%
octopoes/models/ooi/dns/records.py 95%
octopoes/models/ooi/dns/zone.py 77%
octopoes/repositories/ooi_repository.py 39%
octopoes/repositories/origin_parameter_repository.py 53%
octopoes/repositories/origin_repository.py 52%
octopoes/repositories/scan_profile_repository.py 45%
octopoes/xtdb/client.py 49%
octopoes/xtdb/query_builder.py 69%
octopoes/xtdb/related_field_generator.py 71%
tests/conftest.py 88%
tests/integration/test_xtdb_client.py 34%

Minimum allowed coverage is 75%

Generated by 🐒 cobertura-action against 1178a8c

"octopoes",
"rocky",
"fmea",
]
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure this should be a setting. Making this a setting implies that a user can change it, but a user should not removes codes from this list because things will be broken if such codes are used.

Copy link
Contributor

Choose a reason for hiding this comment

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

Being able to add extra denied names is a nice feature though, as we might run into situation where people install other apps, or just don' want some words to be used in their install.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What is also possible is to have 2 lists, one for internal usage and an extra extended one for reserved codes. Let me know what can be decided here @underdarknl @dekkers

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see how we can run into a situation where people install other apps and even if they do that they have to modify the code anyway. Adding organisations is also not something that can be done by everyone. I don't think we should turn this simple PR that denies the names that conflict with urls into something more complex feature that supports users being able to configure denying certain words in organisation names. I think we have more important things to work on...

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed, let keep it a non-user-configurable setting.

@underdarknl
Copy link
Contributor

this closes #510

@underdarknl underdarknl linked an issue May 4, 2023 that may be closed by this pull request
@dekkers
Copy link
Contributor

dekkers commented May 10, 2023

Looks good, moving to QA.

rocky/tools/models.py Show resolved Hide resolved
@Darwinkel
Copy link
Contributor

Darwinkel commented May 11, 2023

Checklist for QA:

  • I have checked out this branch, and successfully ran a fresh make kat.
  • 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:

  • Deny list when creating an organization works correctly during the onboarding as well as through the organization management page

Can be merged if @Donnype agrees.

@dekkers dekkers merged commit 1c2981f into main May 11, 2023
@dekkers dekkers deleted the feature/deny-list-org-code branch May 11, 2023 12:08
praseodym pushed a commit to praseodym/kat that referenced this pull request May 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

admin organization code causes route priority issues
5 participants