-
Notifications
You must be signed in to change notification settings - Fork 37
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
Adds parameter for security classification #451
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggest using an enum, but it's up to you.
django_app/redbox_app/settings.py
Outdated
# Security classifications | ||
# https://www.gov.uk/government/publications/government-security-classifications/ | ||
|
||
MAX_SECURITY_CLASSIFICATION = env.str("MAX_SECURITY_CLASSIFICATION") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could make this an enum:
class Classification(enum.StrEnum):
OFFICIAL = "Official"
OFFICIAL_SENSITIVE = "Official Sensitive"
...
MAX_SECURITY_CLASSIFICATION = Classification[env.str("MAX_SECURITY_CLASSIFICATION")]
We'd need to use underscores rather than dashes in the names for them, of course.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also - would we want a default value?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine withw/without an enum. r.e. default, we want it to fall over if they haven't set it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thought about it and decided to use the enum.
django_app/redbox_app/jinja2.py
Outdated
@@ -63,6 +63,7 @@ def environment(**options): | |||
"url": url, | |||
"humanize_timedelta": humanize_timedelta, | |||
"environment": settings.ENVIRONMENT, | |||
"security": settings.MAX_SECURITY_REPR, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we were to use an enum, we'd want MAX_SECURITY_CLASSIFICATION.value
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
Context
REDBOX-294. Just as we allow many different LLMs to be used, different departments will permit different levels of security assurance on documents staff are permitted to use with Redbox. This ticket is to add an environment variable to allow the frontend to make changes based on an app-wide setting.
Changes proposed in this pull request
MAX_SECURITY_CLASSIFICATION
Guidance to review
case switch
because I think an extension of this kind of work would involve quite a few different settings that result from the max security level of documents permittedRelevant links
Things to check