From 9ee253d8f5a460dfc8f519dfc64f92ff09641193 Mon Sep 17 00:00:00 2001 From: luis cielak Date: Thu, 16 Aug 2018 10:00:28 -0400 Subject: [PATCH 01/36] Align fields for clarity --- atst/forms/financial.py | 1 - .../requests/financial_verification.html | 123 +++++++++++++++--- 2 files changed, 102 insertions(+), 22 deletions(-) diff --git a/atst/forms/financial.py b/atst/forms/financial.py index 994e84bf..75f3d9aa 100644 --- a/atst/forms/financial.py +++ b/atst/forms/financial.py @@ -100,7 +100,6 @@ class FinancialForm(ValidatedForm): ) funding_type = SelectField( - validators=[Required()], choices=[ ("", "- Select -"), ("RDTE", "Research, Development, Testing & Evaluation (RDT&E)"), diff --git a/templates/requests/financial_verification.html b/templates/requests/financial_verification.html index e8236ece..6c9ba4ec 100644 --- a/templates/requests/financial_verification.html +++ b/templates/requests/financial_verification.html @@ -34,36 +34,117 @@

In order to get you access to the JEDI Cloud, we will need you to enter the details below that will help us verify and account for your Task Order.

- {{ TextInput(f.task_order_id,placeholder="e.g.: 1234567899C0001",tooltip="Note that there may be a lag between the time you have created and approved the task order to the time it is searchable within the electronic.
A Contracting Officer will likely be the best source for this number.") }} - {{ TextInput(f.uii_ids,paragraph=True,placeholder="e.g.: DI 0CVA5786950 \nUN1945326361234786950",tooltip="A Unique Item Identifer is a unique code that helps the Department of Defense track and report on where and how digital assets are stored.
Not all applications have an existing UII number assigned.") }} - {{ TextInput(f.pe_id,placeholder="e.g.: 0203752A",tooltip="Program Element numbers helps the Department of Defense identify which offices\\' budgets are contributing towards this resource use.") }} - {{ TextInput(f.treasury_code,placeholder="e.g.: 1200") }} - {{ TextInput(f.ba_code,placeholder="e.g.: 02") }} + {{ TextInput( + f.task_order_id, + placeholder="e.g.: 1234567899C0001", + tooltip="Note that there may be a lag between the time you have created and approved the task order to the time it is searchable within the electronic.
A Contracting Officer will likely be the best source for this number.", + validation="anything" + ) }} + + {{ TextInput(f.uii_ids, + paragraph=True, + placeholder="e.g.: DI 0CVA5786950 \nUN1945326361234786950", + tooltip="A Unique Item Identifer is a unique code that helps the Department of Defense track and report on where and how digital assets are stored.
Not all applications have an existing UII number assigned." + ) }} + + {{ TextInput(f.pe_id, + placeholder="e.g.: 0203752A", + tooltip="Program Element numbers helps the Department of Defense identify which offices\\' budgets are contributing towards this resource use." + ) }} + + {{ TextInput(f.treasury_code, + placeholder="e.g.: 1200" + ) }} + + {{ TextInput(f.ba_code, + placeholder="e.g.: 02" + ) }}

Contracting Officer (KO) Information

- {{ TextInput(f.fname_co,placeholder="Contracting Officer First Name") }} - {{ TextInput(f.lname_co,placeholder="Contracting Officer Last Name") }} - {{ TextInput(f.email_co,validation='email',placeholder="jane@mail.mil") }} - {{ TextInput(f.office_co,placeholder="e.g.: WHS") }} + {{ TextInput( + f.fname_co,placeholder="Contracting Officer First Name" + ) }} + + {{ TextInput( + f.lname_co,placeholder="Contracting Officer Last Name" + ) }} + + {{ TextInput( + f.email_co,validation='email', + placeholder="jane@mail.mil" + ) }} + + {{ TextInput( + f.office_co, + placeholder="e.g.: WHS" + ) }} +

Contracting Officer Representative (COR) Information

- {{ TextInput(f.fname_cor,placeholder="Contracting Officer Representative First Name") }} - {{ TextInput(f.lname_cor,placeholder="Contracting Officer Representative Last Name") }} - {{ TextInput(f.email_cor,validation='email',placeholder="jane@mail.mil") }} - {{ TextInput(f.office_cor,placeholder="e.g.: WHS") }} + {{ TextInput( + f.fname_cor,placeholder="Contracting Officer Representative First Name" + ) }} + + {{ TextInput( + f.lname_cor,placeholder="Contracting Officer Representative Last Name" + ) }} + + {{ TextInput( + f.email_cor,validation='email',placeholder="jane@mail.mil" + ) }} + + {{ TextInput( + f.office_cor,placeholder="e.g.: WHS" + ) }} +

↓ FIELDS NEEDED FOR MANUAL ENTRY OF TASK ORDER INFORMATION (only necessary if EDA info not available) - {{ OptionsInput(f.funding_type) }} - {{ TextInput(f.funding_type_other) }} - {{ TextInput(f.clin_0001,placeholder="50,000", validation='integer', tooltip="Review your task order document, the amounts for each CLIN must match exactly here.") }} - {{ TextInput(f.clin_0003,placeholder="13,000", validation='integer', tooltip="Review your task order document, the amounts for each CLIN must match exactly here.") }} - {{ TextInput(f.clin_1001,placeholder="30,000", validation='integer', tooltip="Review your task order document, the amounts for each CLIN must match exactly here.") }} - {{ TextInput(f.clin_1003,placeholder="7,000", validation='integer', tooltip="Review your task order document, the amounts for each CLIN must match exactly here.") }} - {{ TextInput(f.clin_2001,placeholder="30,000", validation='integer', tooltip="Review your task order document, the amounts for each CLIN must match exactly here.") }} - {{ TextInput(f.clin_2003,placeholder="7,000", validation='integer', tooltip="Review your task order document, the amounts for each CLIN must match exactly here.") }} + {{ OptionsInput( + f.funding_type, + ) }} + + {{ TextInput( + f.funding_type_other + ) }} + + {{ TextInput( + f.clin_0001,placeholder="50,000", + validation='integer', + tooltip="Review your task order document, the amounts for each CLIN must match exactly here." + ) }} + + {{ TextInput( + f.clin_0003,placeholder="13,000", + validation='integer', + tooltip="Review your task order document, the amounts for each CLIN must match exactly here." + ) }} + + {{ TextInput( + f.clin_1001,placeholder="30,000", + validation='integer', + tooltip="Review your task order document, the amounts for each CLIN must match exactly here." + ) }} + + {{ TextInput( + f.clin_1003,placeholder="7,000", + validation='integer', + tooltip="Review your task order document, the amounts for each CLIN must match exactly here." + ) }} + + {{ TextInput( + f.clin_2001,placeholder="30,000", + validation='integer', + tooltip="Review your task order document, the amounts for each CLIN must match exactly here." + ) }} + + {{ TextInput(f.clin_2003,placeholder="7,000", + validation='integer', + tooltip="Review your task order document, the amounts for each CLIN must match exactly here." + ) }} + {% endautoescape %} {% endblock form %} From 9e57908a2cce9d907bc01cce8903e15744e6bfc6 Mon Sep 17 00:00:00 2001 From: luis cielak Date: Thu, 16 Aug 2018 10:33:26 -0400 Subject: [PATCH 02/36] Adding more validation --- atst/forms/financial.py | 5 +++-- atst/forms/poc.py | 2 +- atst/forms/request.py | 6 +++++- templates/requests/financial_verification.html | 7 +++++-- templates/requests/screen-1.html | 6 +++--- 5 files changed, 17 insertions(+), 9 deletions(-) diff --git a/atst/forms/financial.py b/atst/forms/financial.py index 75f3d9aa..f7368f46 100644 --- a/atst/forms/financial.py +++ b/atst/forms/financial.py @@ -71,9 +71,9 @@ class FinancialForm(ValidatedForm): pe_id = StringField("Program Element (PE) Number related to your request", validators=[Required()]) - treasury_code = StringField("Program Treasury Code") + treasury_code = StringField("Program Treasury Code", validators=[Required()]) - ba_code = StringField("Program BA Code") + ba_code = StringField("Program BA Code", validators=[Required()]) fname_co = StringField("Contracting Officer First Name", validators=[Required()]) lname_co = StringField("Contracting Officer Last Name", validators=[Required()]) @@ -107,6 +107,7 @@ class FinancialForm(ValidatedForm): ("PROC", "Procurement (PROC)"), ("OTHER", "Other"), ], + validators=[Required()] ) funding_type_other = StringField( diff --git a/atst/forms/poc.py b/atst/forms/poc.py index 827bf1ae..fc0d8459 100644 --- a/atst/forms/poc.py +++ b/atst/forms/poc.py @@ -20,7 +20,7 @@ class POCForm(ValidatedForm): am_poc = BooleanField( - "I am the Workspace Owner.", + "I am the Workspace Owner", default=False, false_values=(False, "false", "False", "no", "") ) diff --git a/atst/forms/request.py b/atst/forms/request.py index 31b1967d..e40e02ff 100644 --- a/atst/forms/request.py +++ b/atst/forms/request.py @@ -85,11 +85,13 @@ class RequestForm(ValidatedForm): ("US Transportation Command (USTRANSCOM)", "US Transportation Command (USTRANSCOM)"), ("Washington Headquarters Services", "Washington Headquarters Services"), ], + validators=[Required()] ) jedi_usage = TextAreaField( "JEDI Usage", description="Your answer will help us provide tangible examples to DoD leadership how and why commercial cloud resources are accelerating the Department's missions", + validators=[Required()] ) @@ -195,5 +197,7 @@ class RequestForm(ValidatedForm): ) start_date = DateField( - description="When do you expect to start using the JEDI Cloud (not for billing purposes)?" + description="When do you expect to start using the JEDI Cloud (not for billing purposes)?", + validators=[ + Required()] ) diff --git a/templates/requests/financial_verification.html b/templates/requests/financial_verification.html index 6c9ba4ec..37141b68 100644 --- a/templates/requests/financial_verification.html +++ b/templates/requests/financial_verification.html @@ -98,9 +98,12 @@ f.office_cor,placeholder="e.g.: WHS" ) }} +
-

- ↓ FIELDS NEEDED FOR MANUAL ENTRY OF TASK ORDER INFORMATION (only necessary if EDA info not available) + {{ Alert('Task Order not found in EDA', + message="Since the Task Order (TO) number was not found in our system of record, EDA, please populate the additional fields in the form below.", + level='warning' + ) }} {{ OptionsInput( f.funding_type, diff --git a/templates/requests/screen-1.html b/templates/requests/screen-1.html index c6ea9a05..cc3c9c25 100644 --- a/templates/requests/screen-1.html +++ b/templates/requests/screen-1.html @@ -25,7 +25,7 @@

General

{{ OptionsInput(f.dod_component) }} - {{ TextInput(f.jedi_usage, paragraph=True, placeholder="Briefly describe how you are expecting to use the JEDI Cloud. \n e.g. We are migrating XYZ application to the cloud so that...") }} + {{ TextInput(f.jedi_usage, paragraph=True, placeholder="Briefly describe how you are expecting to use the JEDI Cloud. e.g. We are migrating XYZ application to the cloud so that...") }}

Cloud Readiness

{{ TextInput(f.num_software_systems,validation="integer",tooltip="A software system can be any code that you plan to host on cloud infrastructure. For example, it could be a custom-developed web application, or a large ERP system.",placeholder="0") }} @@ -75,8 +75,8 @@ - {{ TextInput(f.dollar_value, validation='dollars', placeholder="$0") }} - {{ TextInput(f.start_date, validation='date', placeholder='MM / DD / YYYY') }} + {{ TextInput(f.dollar_value, validation='dollars', placeholder='$0') }} + {{ TextInput(f.start_date, placeholder='MM / DD / YYYY', validation='date') }} From 12668cd98751f042c6aafb28bcbb5f45d8be6c15 Mon Sep 17 00:00:00 2001 From: luis cielak Date: Thu, 16 Aug 2018 10:34:00 -0400 Subject: [PATCH 03/36] Update styles --- styles/elements/_inputs.scss | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/styles/elements/_inputs.scss b/styles/elements/_inputs.scss index 35640fbe..46718b78 100644 --- a/styles/elements/_inputs.scss +++ b/styles/elements/_inputs.scss @@ -110,6 +110,7 @@ margin: 0; box-sizing: border-box; max-width: 32em; + resize: none; &:hover, &:focus { @@ -133,6 +134,10 @@ } + select { + max-width: 30em; + } + ul { list-style: none; margin: 0; @@ -181,16 +186,17 @@ &--anything, &--email { input { - max-width: 26em; + max-width: 30em; } .icon-validation { - left: 26em; + left: 30em; } } &--paragraph { + max-width: 30em; .icon-validation { - left: 32em; + left: 30em; } } From 7471d062e12f79bbbc16d83bf9e331fbb3127b6c Mon Sep 17 00:00:00 2001 From: luis cielak Date: Thu, 16 Aug 2018 10:44:42 -0400 Subject: [PATCH 04/36] Update tooltips --- atst/forms/financial.py | 13 ++++++++---- .../requests/financial_verification.html | 20 +++++++------------ 2 files changed, 16 insertions(+), 17 deletions(-) diff --git a/atst/forms/financial.py b/atst/forms/financial.py index f7368f46..d76692b7 100644 --- a/atst/forms/financial.py +++ b/atst/forms/financial.py @@ -62,11 +62,11 @@ class FinancialForm(ValidatedForm): return valid task_order_id = StringField( - "Task Order Number associated with this request.", validators=[Required()] + "Task Order Number associated with this request", validators=[Required()] ) uii_ids = NewlineListField( - "Unique Item Identifier (UII)s related to your application(s) if you already have them." + "Unique Item Identifier (UII)s related to your application(s) if you already have them" ) pe_id = StringField("Program Element (PE) Number related to your request", validators=[Required()]) @@ -106,8 +106,7 @@ class FinancialForm(ValidatedForm): ("OM", "Operations & Maintenance (O&M)"), ("PROC", "Procurement (PROC)"), ("OTHER", "Other"), - ], - validators=[Required()] + ] ) funding_type_other = StringField( @@ -117,29 +116,35 @@ class FinancialForm(ValidatedForm): clin_0001 = StringField( "
CLIN 0001
-
Unclassified IaaS and PaaS Amount
", validators=[Required()], + description="Review your task order document, the amounts for each CLIN must match exactly here" ) clin_0003 = StringField( "
CLIN 0003
-
Unclassified Cloud Support Package
", validators=[Required()], + description="Review your task order document, the amounts for each CLIN must match exactly here" ) clin_1001 = StringField( "
CLIN 1001
-
Unclassified IaaS and PaaS Amount
OPTION PERIOD 1
", validators=[Required()], + description="Review your task order document, the amounts for each CLIN must match exactly here" ) clin_1003 = StringField( "
CLIN 1003
-
Unclassified Cloud Support Package
OPTION PERIOD 1
", validators=[Required()], + description="Review your task order document, the amounts for each CLIN must match exactly here" ) clin_2001 = StringField( "
CLIN 2001
-
Unclassified IaaS and PaaS Amount
OPTION PERIOD 2
", validators=[Required()], + description="Review your task order document, the amounts for each CLIN must match exactly here" ) clin_2003 = StringField( "
CLIN 2003
-
Unclassified Cloud Support Package
OPTION PERIOD 2
", validators=[Required()], + description="Review your task order document, the amounts for each CLIN must match exactly here" ) diff --git a/templates/requests/financial_verification.html b/templates/requests/financial_verification.html index 37141b68..cec36d27 100644 --- a/templates/requests/financial_verification.html +++ b/templates/requests/financial_verification.html @@ -106,7 +106,7 @@ ) }} {{ OptionsInput( - f.funding_type, + f.funding_type ) }} {{ TextInput( @@ -115,37 +115,31 @@ {{ TextInput( f.clin_0001,placeholder="50,000", - validation='integer', - tooltip="Review your task order document, the amounts for each CLIN must match exactly here." + validation='integer' ) }} {{ TextInput( f.clin_0003,placeholder="13,000", - validation='integer', - tooltip="Review your task order document, the amounts for each CLIN must match exactly here." + validation='integer' ) }} {{ TextInput( f.clin_1001,placeholder="30,000", - validation='integer', - tooltip="Review your task order document, the amounts for each CLIN must match exactly here." + validation='integer' ) }} {{ TextInput( f.clin_1003,placeholder="7,000", - validation='integer', - tooltip="Review your task order document, the amounts for each CLIN must match exactly here." + validation='integer' ) }} {{ TextInput( f.clin_2001,placeholder="30,000", - validation='integer', - tooltip="Review your task order document, the amounts for each CLIN must match exactly here." + validation='integer' ) }} {{ TextInput(f.clin_2003,placeholder="7,000", - validation='integer', - tooltip="Review your task order document, the amounts for each CLIN must match exactly here." + validation='integer' ) }} From 453d45bf36f6ba4baa9c4ac6e84e7cc38344515f Mon Sep 17 00:00:00 2001 From: luis cielak Date: Thu, 16 Aug 2018 10:53:59 -0400 Subject: [PATCH 05/36] Add fieldset background to TO fields --- styles/components/_forms.scss | 8 ++++ .../requests/financial_verification.html | 45 ++++++++++--------- 2 files changed, 32 insertions(+), 21 deletions(-) diff --git a/styles/components/_forms.scss b/styles/components/_forms.scss index 83ab5b83..b2ca60f0 100644 --- a/styles/components/_forms.scss +++ b/styles/components/_forms.scss @@ -73,5 +73,13 @@ } } } + + &--warning { + @include alert-level('warning'); + } + + &--error { + @include alert-level('error'); + } } diff --git a/templates/requests/financial_verification.html b/templates/requests/financial_verification.html index cec36d27..dd12ea01 100644 --- a/templates/requests/financial_verification.html +++ b/templates/requests/financial_verification.html @@ -105,42 +105,45 @@ level='warning' ) }} - {{ OptionsInput( - f.funding_type +
+ {{ OptionsInput( + f.funding_type ) }} - {{ TextInput( - f.funding_type_other + {{ TextInput( + f.funding_type_other ) }} - {{ TextInput( - f.clin_0001,placeholder="50,000", - validation='integer' + {{ TextInput( + f.clin_0001,placeholder="50,000", + validation='integer' ) }} - {{ TextInput( - f.clin_0003,placeholder="13,000", - validation='integer' + {{ TextInput( + f.clin_0003,placeholder="13,000", + validation='integer' ) }} - {{ TextInput( - f.clin_1001,placeholder="30,000", - validation='integer' + {{ TextInput( + f.clin_1001,placeholder="30,000", + validation='integer' ) }} - {{ TextInput( - f.clin_1003,placeholder="7,000", - validation='integer' + {{ TextInput( + f.clin_1003,placeholder="7,000", + validation='integer' ) }} - {{ TextInput( - f.clin_2001,placeholder="30,000", - validation='integer' + {{ TextInput( + f.clin_2001,placeholder="30,000", + validation='integer' ) }} - {{ TextInput(f.clin_2003,placeholder="7,000", - validation='integer' + {{ TextInput( + f.clin_2003,placeholder="7,000", + validation='integer' ) }} +
{% endautoescape %} From af962372316928c568d20184447fc0455ac96eb2 Mon Sep 17 00:00:00 2001 From: luis cielak Date: Thu, 16 Aug 2018 10:58:08 -0400 Subject: [PATCH 06/36] Adjust formatting --- .../requests/financial_verification.html | 40 +++++-------------- 1 file changed, 10 insertions(+), 30 deletions(-) diff --git a/templates/requests/financial_verification.html b/templates/requests/financial_verification.html index dd12ea01..40f38af0 100644 --- a/templates/requests/financial_verification.html +++ b/templates/requests/financial_verification.html @@ -52,22 +52,14 @@ tooltip="Program Element numbers helps the Department of Defense identify which offices\\' budgets are contributing towards this resource use." ) }} - {{ TextInput(f.treasury_code, - placeholder="e.g.: 1200" - ) }} + {{ TextInput(f.treasury_code,placeholder="e.g.: 1200") }} - {{ TextInput(f.ba_code, - placeholder="e.g.: 02" - ) }} + {{ TextInput(f.ba_code,placeholder="e.g.: 02") }}

Contracting Officer (KO) Information

- {{ TextInput( - f.fname_co,placeholder="Contracting Officer First Name" - ) }} + {{ TextInput(f.fname_co,placeholder="Contracting Officer First Name") }} - {{ TextInput( - f.lname_co,placeholder="Contracting Officer Last Name" - ) }} + {{ TextInput(f.lname_co,placeholder="Contracting Officer Last Name") }} {{ TextInput( f.email_co,validation='email', @@ -82,21 +74,13 @@

Contracting Officer Representative (COR) Information

- {{ TextInput( - f.fname_cor,placeholder="Contracting Officer Representative First Name" - ) }} + {{ TextInput(f.fname_cor,placeholder="Contracting Officer Representative First Name") }} - {{ TextInput( - f.lname_cor,placeholder="Contracting Officer Representative Last Name" - ) }} + {{ TextInput(f.lname_cor,placeholder="Contracting Officer Representative Last Name") }} - {{ TextInput( - f.email_cor,validation='email',placeholder="jane@mail.mil" - ) }} + {{ TextInput(f.email_cor,validation='email',placeholder="jane@mail.mil") }} - {{ TextInput( - f.office_cor,placeholder="e.g.: WHS" - ) }} + {{ TextInput(f.office_cor,placeholder="e.g.: WHS") }}
@@ -106,13 +90,9 @@ ) }}
- {{ OptionsInput( - f.funding_type - ) }} + {{ OptionsInput(f.funding_type) }} - {{ TextInput( - f.funding_type_other - ) }} + {{ TextInput(f.funding_type_other) }} {{ TextInput( f.clin_0001,placeholder="50,000", From 070fe823d35c5c8cf72aab379727b7777948856a Mon Sep 17 00:00:00 2001 From: richard-dds Date: Thu, 16 Aug 2018 13:24:11 -0400 Subject: [PATCH 07/36] Fix bug in UII field Was displaying an empty list of UIIs as [''] in the textarea --- atst/forms/fields.py | 2 +- tests/forms/test_fields.py | 43 +++++++++++++++++++++++++++++++---- tests/forms/test_financial.py | 6 ++--- 3 files changed, 42 insertions(+), 9 deletions(-) diff --git a/atst/forms/fields.py b/atst/forms/fields.py index 2c06154a..f573f179 100644 --- a/atst/forms/fields.py +++ b/atst/forms/fields.py @@ -32,6 +32,6 @@ class NewlineListField(Field): def process_formdata(self, valuelist): if valuelist: - self.data = [l.strip() for l in valuelist[0].split("\n")] + self.data = [l.strip() for l in valuelist[0].split("\n") if l] else: self.data = [] diff --git a/tests/forms/test_fields.py b/tests/forms/test_fields.py index ceece7c5..04dd88c1 100644 --- a/tests/forms/test_fields.py +++ b/tests/forms/test_fields.py @@ -1,25 +1,58 @@ import pytest from wtforms import Form import pendulum +from werkzeug.datastructures import ImmutableMultiDict -from atst.forms.fields import DateField +from atst.forms.fields import DateField, NewlineListField -class MyForm(Form): +class DateForm(Form): date = DateField() +class NewlineListForm(Form): + newline_list = NewlineListField() + + def test_date_ie_format(): - form = MyForm(data={"date": "12/24/2018"}) + form = DateForm(data={"date": "12/24/2018"}) assert form.date._value() == pendulum.date(2018, 12, 24) def test_date_sane_format(): - form = MyForm(data={"date": "2018-12-24"}) + form = DateForm(data={"date": "2018-12-24"}) assert form.date._value() == pendulum.date(2018, 12, 24) def test_date_insane_format(): - form = MyForm(data={"date": "hello"}) + form = DateForm(data={"date": "hello"}) with pytest.raises(ValueError): form.date._value() + + +@pytest.mark.parametrize("input_,expected", [ + ("", []), + ("hello", ["hello"]), + ("hello\n", ["hello"]), + ("hello\nworld", ["hello", "world"]), + ("hello\nworld\n", ["hello", "world"]) +]) +def test_newline_list_process(input_, expected): + form_data = ImmutableMultiDict({"newline_list": input_}) + form = NewlineListForm(form_data) + + assert form.validate() + assert form.data == {"newline_list": expected} + + +@pytest.mark.parametrize("input_,expected", [ + ([], ""), + (["hello"], "hello"), + (["hello", "world"], "hello\nworld") +]) +def test_newline_list_value(input_, expected): + form_data = {"newline_list": input_} + form = NewlineListForm(data=form_data) + + assert form.validate() + assert form.newline_list._value() == expected diff --git a/tests/forms/test_financial.py b/tests/forms/test_financial.py index d58aa61c..c24c742d 100644 --- a/tests/forms/test_financial.py +++ b/tests/forms/test_financial.py @@ -3,12 +3,12 @@ import pytest from atst.forms.financial import suggest_pe_id -@pytest.mark.parametrize("input,expected", [ +@pytest.mark.parametrize("input_,expected", [ ('0603502N', None), ('0603502NZ', None), ('603502N', '0603502N'), ('063502N', '0603502N'), ('63502N', '0603502N'), ]) -def test_suggest_pe_id(input, expected): - assert suggest_pe_id(input) == expected +def test_suggest_pe_id(input_, expected): + assert suggest_pe_id(input_) == expected From 4a1a3571bc99961dc8aaf136a60de8f3c9858dc3 Mon Sep 17 00:00:00 2001 From: richard-dds Date: Thu, 16 Aug 2018 13:28:08 -0400 Subject: [PATCH 08/36] Require financial verification funding field to have a selection --- atst/forms/financial.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/atst/forms/financial.py b/atst/forms/financial.py index d76692b7..d8e21653 100644 --- a/atst/forms/financial.py +++ b/atst/forms/financial.py @@ -1,7 +1,7 @@ import re from wtforms.fields.html5 import EmailField from wtforms.fields import StringField, SelectField -from wtforms.validators import Required, Email +from wtforms.validators import Required, Email, InputRequired from atst.domain.exceptions import NotFoundError from atst.domain.pe_numbers import PENumbers @@ -106,7 +106,8 @@ class FinancialForm(ValidatedForm): ("OM", "Operations & Maintenance (O&M)"), ("PROC", "Procurement (PROC)"), ("OTHER", "Other"), - ] + ], + validators=[InputRequired()] ) funding_type_other = StringField( From 59b5e19c79fb410cf530a7aa1630b6eaa65313ab Mon Sep 17 00:00:00 2001 From: richard-dds Date: Thu, 16 Aug 2018 13:52:17 -0400 Subject: [PATCH 09/36] Funding type other input is only required if funding type is other --- atst/forms/financial.py | 12 ++++-- js/components/forms/financial.js | 41 +++++++++++++++++++ js/index.js | 2 + .../requests/financial_verification.html | 9 ++-- tests/forms/test_financial.py | 20 ++++++++- 5 files changed, 76 insertions(+), 8 deletions(-) create mode 100644 js/components/forms/financial.js diff --git a/atst/forms/financial.py b/atst/forms/financial.py index d8e21653..21fde2c9 100644 --- a/atst/forms/financial.py +++ b/atst/forms/financial.py @@ -1,7 +1,7 @@ import re from wtforms.fields.html5 import EmailField from wtforms.fields import StringField, SelectField -from wtforms.validators import Required, Email, InputRequired +from wtforms.validators import Required, Email, InputRequired, Optional from atst.domain.exceptions import NotFoundError from atst.domain.pe_numbers import PENumbers @@ -55,6 +55,12 @@ def validate_pe_id(field, existing_request): class FinancialForm(ValidatedForm): + def validate(self, *args, **kwargs): + if self.funding_type.data == "OTHER": + self.funding_type_other.validators.append(Required()) + return super().validate(*args, **kwargs) + + def perform_extra_validation(self, existing_request): valid = True if not existing_request or existing_request.get("pe_id") != self.pe_id.data: @@ -110,9 +116,7 @@ class FinancialForm(ValidatedForm): validators=[InputRequired()] ) - funding_type_other = StringField( - "If other, please specify", validators=[Required()] - ) + funding_type_other = StringField("If other, please specify") clin_0001 = StringField( "
CLIN 0001
-
Unclassified IaaS and PaaS Amount
", diff --git a/js/components/forms/financial.js b/js/components/forms/financial.js new file mode 100644 index 00000000..2a1fb43b --- /dev/null +++ b/js/components/forms/financial.js @@ -0,0 +1,41 @@ +import optionsinput from '../options_input' +import textinput from '../text_input' + +export default { + name: 'financial', + + components: { + optionsinput, + textinput, + }, + + props: { + initialData: { + type: Object, + default: () => ({}) + } + }, + + data: function () { + const { + funding_type = "" + } = this.initialData + + return { + funding_type + } + }, + + mounted: function () { + this.$root.$on('field-change', this.handleFieldChange) + }, + + methods: { + handleFieldChange: function (event) { + const { value, name } = event + if (typeof this[name] !== undefined) { + this[name] = value + } + }, + } +} diff --git a/js/index.js b/js/index.js index afe6961d..4ecd6aa7 100644 --- a/js/index.js +++ b/js/index.js @@ -7,6 +7,7 @@ import textinput from './components/text_input' import checkboxinput from './components/checkbox_input' import DetailsOfUse from './components/forms/details_of_use' import poc from './components/forms/poc' +import financial from './components/forms/financial' Vue.use(VTooltip) @@ -19,6 +20,7 @@ const app = new Vue({ checkboxinput, DetailsOfUse, poc, + financial, }, methods: { closeModal: function(name) { diff --git a/templates/requests/financial_verification.html b/templates/requests/financial_verification.html index 40f38af0..5f4c14e7 100644 --- a/templates/requests/financial_verification.html +++ b/templates/requests/financial_verification.html @@ -6,6 +6,7 @@ {% block content %} +
@@ -92,7 +93,9 @@
{{ OptionsInput(f.funding_type) }} - {{ TextInput(f.funding_type_other) }} + {{ TextInput( f.clin_0001,placeholder="50,000", @@ -132,9 +135,9 @@ {% endblock %} -
+
-{% endblock %} \ No newline at end of file +{% endblock %} diff --git a/tests/forms/test_financial.py b/tests/forms/test_financial.py index c24c742d..62cea8ec 100644 --- a/tests/forms/test_financial.py +++ b/tests/forms/test_financial.py @@ -1,6 +1,6 @@ import pytest -from atst.forms.financial import suggest_pe_id +from atst.forms.financial import suggest_pe_id, FinancialForm @pytest.mark.parametrize("input_,expected", [ @@ -12,3 +12,21 @@ from atst.forms.financial import suggest_pe_id ]) def test_suggest_pe_id(input_, expected): assert suggest_pe_id(input_) == expected + + +def test_funding_type_other_not_required_if_funding_type_is_not_other(): + form_data = { + "funding_type": "PROC" + } + form = FinancialForm(data=form_data) + form.validate() + assert "funding_type_other" not in form.errors + + +def test_funding_type_other_required_if_funding_type_is_other(): + form_data = { + "funding_type": "OTHER" + } + form = FinancialForm(data=form_data) + form.validate() + assert "funding_type_other" in form.errors From 9b8dd464f682ad831f9a4fa88155a2cfd4422b4c Mon Sep 17 00:00:00 2001 From: richard-dds Date: Thu, 16 Aug 2018 14:32:32 -0400 Subject: [PATCH 10/36] Unused import --- atst/forms/financial.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/atst/forms/financial.py b/atst/forms/financial.py index 21fde2c9..c923be97 100644 --- a/atst/forms/financial.py +++ b/atst/forms/financial.py @@ -1,7 +1,7 @@ import re from wtforms.fields.html5 import EmailField from wtforms.fields import StringField, SelectField -from wtforms.validators import Required, Email, InputRequired, Optional +from wtforms.validators import Required, Email, InputRequired from atst.domain.exceptions import NotFoundError from atst.domain.pe_numbers import PENumbers From 3e6dc59f046a879c20a149ed9f47d96e53f2256c Mon Sep 17 00:00:00 2001 From: luis cielak Date: Thu, 16 Aug 2018 14:43:17 -0400 Subject: [PATCH 11/36] Fix fieldset flexbox bug in Firefox and Safari --- styles/components/_forms.scss | 3 +++ 1 file changed, 3 insertions(+) diff --git a/styles/components/_forms.scss b/styles/components/_forms.scss index b2ca60f0..6bfd17d2 100644 --- a/styles/components/_forms.scss +++ b/styles/components/_forms.scss @@ -47,6 +47,7 @@ .form__sub-fields { @include alert; @include alert-level('default'); + display: block; .usa-input { &:first-child { @@ -76,10 +77,12 @@ &--warning { @include alert-level('warning'); + display: block; } &--error { @include alert-level('error'); + display: block; } } From fda8c134a80584c1bfb7859e37da971170fdce32 Mon Sep 17 00:00:00 2001 From: richard-dds Date: Thu, 16 Aug 2018 15:29:41 -0400 Subject: [PATCH 12/36] Fix UII issue (hopefully for good) --- atst/forms/fields.py | 6 ++++++ atst/routes/requests/financial_verification.py | 2 +- 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/atst/forms/fields.py b/atst/forms/fields.py index f573f179..0714c847 100644 --- a/atst/forms/fields.py +++ b/atst/forms/fields.py @@ -35,3 +35,9 @@ class NewlineListField(Field): self.data = [l.strip() for l in valuelist[0].split("\n") if l] else: self.data = [] + + def process_data(self, value): + if isinstance(value, list): + self.data = "\n".join(value) + else: + self.data = value diff --git a/atst/routes/requests/financial_verification.py b/atst/routes/requests/financial_verification.py index f6b8cfda..972e9351 100644 --- a/atst/routes/requests/financial_verification.py +++ b/atst/routes/requests/financial_verification.py @@ -24,7 +24,7 @@ def update_financial_verification(request_id): rerender_args = dict(request_id=request_id, f=form) if form.validate(): - request_data = {"financial_verification": post_data} + request_data = {"financial_verification": form.data} valid = form.perform_extra_validation( existing_request.body.get("financial_verification") ) From 57fd5eb57c37d58065145e0c3a953284f76a4190 Mon Sep 17 00:00:00 2001 From: richard-dds Date: Thu, 16 Aug 2018 15:30:04 -0400 Subject: [PATCH 13/36] Add regex validations for treasury_code and ba_code --- atst/forms/financial.py | 9 ++++++--- tests/forms/test_financial.py | 37 +++++++++++++++++++++++++++++++++++ 2 files changed, 43 insertions(+), 3 deletions(-) diff --git a/atst/forms/financial.py b/atst/forms/financial.py index c923be97..21274e9b 100644 --- a/atst/forms/financial.py +++ b/atst/forms/financial.py @@ -1,7 +1,7 @@ import re from wtforms.fields.html5 import EmailField from wtforms.fields import StringField, SelectField -from wtforms.validators import Required, Email, InputRequired +from wtforms.validators import Required, Email, InputRequired, Regexp from atst.domain.exceptions import NotFoundError from atst.domain.pe_numbers import PENumbers @@ -21,6 +21,9 @@ PE_REGEX = re.compile( re.X, ) +TREASURY_CODE_REGEX = re.compile(r"^0*([1-9]{4}|[1-9]{6})$") + +BA_CODE_REGEX = re.compile(r"^0*[1-9]{2}\w?$") def suggest_pe_id(pe_id): suggestion = pe_id @@ -77,9 +80,9 @@ class FinancialForm(ValidatedForm): pe_id = StringField("Program Element (PE) Number related to your request", validators=[Required()]) - treasury_code = StringField("Program Treasury Code", validators=[Required()]) + treasury_code = StringField("Program Treasury Code", validators=[Required(), Regexp(TREASURY_CODE_REGEX)]) - ba_code = StringField("Program BA Code", validators=[Required()]) + ba_code = StringField("Program BA Code", validators=[Required(), Regexp(BA_CODE_REGEX)]) fname_co = StringField("Contracting Officer First Name", validators=[Required()]) lname_co = StringField("Contracting Officer Last Name", validators=[Required()]) diff --git a/tests/forms/test_financial.py b/tests/forms/test_financial.py index 62cea8ec..9bc82421 100644 --- a/tests/forms/test_financial.py +++ b/tests/forms/test_financial.py @@ -30,3 +30,40 @@ def test_funding_type_other_required_if_funding_type_is_other(): form = FinancialForm(data=form_data) form.validate() assert "funding_type_other" in form.errors + + +@pytest.mark.parametrize("input_,expected", [ + ("1234", True), + ("123456", True), + ("0001234", True), + ("000123456", True), + ("12345", False), + ("00012345", False), + ("0001234567", False), + ("000000", False), +]) +def test_treasury_code_validation(input_, expected): + form_data = {"treasury_code": input_} + form = FinancialForm(data=form_data) + form.validate() + is_valid = "treasury_code" not in form.errors + + assert is_valid == expected + + +@pytest.mark.parametrize("input_,expected", [ + ("12", True), + ("00012", True), + ("12A", True), + ("000123", True), + ("00012A", True), + ("0001", False), + ("00012AB", False), +]) +def test_ba_code_validation(input_, expected): + form_data = {"ba_code": input_} + form = FinancialForm(data=form_data) + form.validate() + is_valid = "ba_code" not in form.errors + + assert is_valid == expected From 35d8158f3c706b979992c30a4420c6be5da8dbc9 Mon Sep 17 00:00:00 2001 From: richard-dds Date: Thu, 16 Aug 2018 15:53:53 -0400 Subject: [PATCH 14/36] Really stupid fix for the UII issue --- atst/routes/requests/financial_verification.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/atst/routes/requests/financial_verification.py b/atst/routes/requests/financial_verification.py index 972e9351..2a92b938 100644 --- a/atst/routes/requests/financial_verification.py +++ b/atst/routes/requests/financial_verification.py @@ -32,6 +32,8 @@ def update_financial_verification(request_id): if valid: return redirect(url_for("requests.financial_verification_submitted")) else: + # TODO: This is a horrible workaround. + form.uii_ids.process_data(form.uii_ids.data) return render_template( "requests/financial_verification.html", **rerender_args ) From 0356d619609b8db08dd20e99facdac6e73a06605 Mon Sep 17 00:00:00 2001 From: luis cielak Date: Thu, 16 Aug 2018 15:56:05 -0400 Subject: [PATCH 15/36] Fix typos and some copy text tweaks --- atst/forms/financial.py | 2 +- atst/forms/org.py | 2 +- atst/forms/request.py | 32 ++++++++++++++++---------------- styles/elements/_inputs.scss | 4 +++- 4 files changed, 21 insertions(+), 19 deletions(-) diff --git a/atst/forms/financial.py b/atst/forms/financial.py index 21274e9b..0f8c7715 100644 --- a/atst/forms/financial.py +++ b/atst/forms/financial.py @@ -89,7 +89,7 @@ class FinancialForm(ValidatedForm): email_co = EmailField("Contracting Officer Email", validators=[Required(), Email()]) - office_co = StringField("Contracting Office Office", validators=[Required()]) + office_co = StringField("Contracting Officer Office", validators=[Required()]) fname_cor = StringField( "Contracting Officer Representative (COR) First Name", validators=[Required()] diff --git a/atst/forms/org.py b/atst/forms/org.py index a812ce4e..275d62c8 100644 --- a/atst/forms/org.py +++ b/atst/forms/org.py @@ -93,7 +93,7 @@ class OrgForm(ValidatedForm): ) date_latest_training = DateField( - "Latest Information Assurance (IA) Training completion date", + "Latest Information Assurance (IA) Training Completion Date", description="To complete the training, you can find it in Information Assurance Cyber Awareness Challange website.", validators=[ Required(), diff --git a/atst/forms/request.py b/atst/forms/request.py index e40e02ff..c14dacf2 100644 --- a/atst/forms/request.py +++ b/atst/forms/request.py @@ -97,7 +97,7 @@ class RequestForm(ValidatedForm): # Details of Use: Cloud Readiness num_software_systems = IntegerField( - "Number of Software System", + "Number of Software Systems", description="Estimate the number of software systems that will be supported by this JEDI Cloud access request", ) @@ -141,16 +141,16 @@ class RequestForm(ValidatedForm): description="How much data is being transferred to the cloud?", choices=[ ("null", "Select an option"), - ("less_than_100gb", "Less than 100GB"), - ("100gb-500gb", "100GB-500GB"), - ("500gb-1tb", "500GB-1TB"), - ("1tb-50tb", "1TB-50TB"), - ("50tb-100tb", "50TB-100TB"), - ("100tb-500tb", "100TB-500TB"), - ("500tb-1pb", "500TB-1PB"), - ("1pb-5pb", "1PB-5PB"), - ("5pb-10pb", "5PB-10PB"), - ("above_10pb", "Above 10PB"), + ("Less than 100GB", "Less than 100GB"), + ("100GB-500GB", "100GB-500GB"), + ("500GB-1TB", "500GB-1TB"), + ("1TB-50TB", "1TB-50TB"), + ("50TB-100TB", "50TB-100TB"), + ("100TB-500TB", "100TB-500TB"), + ("500TB-1PB", "500TB-1PB"), + ("1PB-5PB", "1PB-5PB"), + ("5PB-10PB", "5PB-10PB"), + ("Above 10PB", "Above 10PB"), ], ) @@ -158,10 +158,10 @@ class RequestForm(ValidatedForm): description="When do you expect to complete your migration to the JEDI Cloud?", choices=[ ("null", "Select an option"), - ("less_than_1_month", "Less than 1 month"), - ("1_to_3_months", "1-3 months"), - ("3_to_6_months", "3-6 months"), - ("above_12_months", "Above 12 months"), + ("Less than 1 month", "Less than 1 month"), + ("1-3 months", "1-3 months"), + ("3-6 months", "3-6 months"), + ("Above 12 months", "Above 12 months"), ], ) @@ -173,7 +173,7 @@ class RequestForm(ValidatedForm): # Details of Use: Financial Usage estimated_monthly_spend = IntegerField( - "Estimated monthly spend", + "Estimated Monthly Spend", description='Use the JEDI CSP Calculator to estimate your monthly cloud resource usage and enter the dollar amount below. Note these estimates are for initial approval only. After the request is approved, you will be asked to provide a valid Task Order number with specific CLIN amounts for cloud services.', ) diff --git a/styles/elements/_inputs.scss b/styles/elements/_inputs.scss index 46718b78..14b508ab 100644 --- a/styles/elements/_inputs.scss +++ b/styles/elements/_inputs.scss @@ -194,7 +194,9 @@ } &--paragraph { - max-width: 30em; + textarea { + max-width: 30em; + } .icon-validation { left: 30em; } From e065bd27c10d30adcf76f68695fc8403bbbbf0ef Mon Sep 17 00:00:00 2001 From: richard-dds Date: Thu, 16 Aug 2018 16:32:35 -0400 Subject: [PATCH 16/36] Don't use html validation for funding type field --- atst/forms/fields.py | 12 +++++++++++- atst/forms/financial.py | 9 +++++---- 2 files changed, 16 insertions(+), 5 deletions(-) diff --git a/atst/forms/fields.py b/atst/forms/fields.py index 0714c847..9ac4371b 100644 --- a/atst/forms/fields.py +++ b/atst/forms/fields.py @@ -1,5 +1,5 @@ from wtforms.fields.html5 import DateField -from wtforms.fields import Field +from wtforms.fields import Field, SelectField as SelectField_ from wtforms.widgets import TextArea from atst.domain.date import parse_date @@ -41,3 +41,13 @@ class NewlineListField(Field): self.data = "\n".join(value) else: self.data = value + + +class SelectField(SelectField_): + def __init__(self, *args, **kwargs): + render_kw = kwargs.get("render_kw", {}) + kwargs["render_kw"] = { + **render_kw, + "required": False + } + super().__init__(*args, **kwargs) diff --git a/atst/forms/financial.py b/atst/forms/financial.py index 0f8c7715..8a444bf7 100644 --- a/atst/forms/financial.py +++ b/atst/forms/financial.py @@ -1,12 +1,12 @@ import re from wtforms.fields.html5 import EmailField -from wtforms.fields import StringField, SelectField -from wtforms.validators import Required, Email, InputRequired, Regexp +from wtforms.fields import StringField +from wtforms.validators import Required, Email, InputRequired, Regexp, DataRequired from atst.domain.exceptions import NotFoundError from atst.domain.pe_numbers import PENumbers -from .fields import NewlineListField +from .fields import NewlineListField, SelectField from .forms import ValidatedForm @@ -116,7 +116,8 @@ class FinancialForm(ValidatedForm): ("PROC", "Procurement (PROC)"), ("OTHER", "Other"), ], - validators=[InputRequired()] + validators=[Required()], + render_kw={"required": False} ) funding_type_other = StringField("If other, please specify") From 6898f86611d8bb2e871a78b4acc346b6edd837e3 Mon Sep 17 00:00:00 2001 From: richard-dds Date: Thu, 16 Aug 2018 16:32:53 -0400 Subject: [PATCH 17/36] UIIs are required --- atst/forms/financial.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/atst/forms/financial.py b/atst/forms/financial.py index 8a444bf7..f7aa76a6 100644 --- a/atst/forms/financial.py +++ b/atst/forms/financial.py @@ -75,7 +75,8 @@ class FinancialForm(ValidatedForm): ) uii_ids = NewlineListField( - "Unique Item Identifier (UII)s related to your application(s) if you already have them" + "Unique Item Identifier (UII)s related to your application(s) if you already have them", + validators=[Required()] ) pe_id = StringField("Program Element (PE) Number related to your request", validators=[Required()]) From b680b3478a06b36d578fc07dc35b37cb06cfc117 Mon Sep 17 00:00:00 2001 From: richard-dds Date: Thu, 16 Aug 2018 16:38:25 -0400 Subject: [PATCH 18/36] Fix validations in other select fields --- atst/forms/org.py | 55 ++++--------------------------------------- atst/forms/request.py | 54 ++++-------------------------------------- 2 files changed, 9 insertions(+), 100 deletions(-) diff --git a/atst/forms/org.py b/atst/forms/org.py index 275d62c8..ff531d9a 100644 --- a/atst/forms/org.py +++ b/atst/forms/org.py @@ -1,10 +1,12 @@ from wtforms.fields.html5 import EmailField, TelField -from wtforms.fields import RadioField, StringField, SelectField +from wtforms.fields import RadioField, StringField from wtforms.validators import Required, Email import pendulum -from .fields import DateField + +from .fields import DateField, SelectField from .forms import ValidatedForm from .validators import DateRange, PhoneNumber, Alphabet +from .data import SERVICE_BRANCHES class OrgForm(ValidatedForm): @@ -21,54 +23,7 @@ class OrgForm(ValidatedForm): service_branch = SelectField( "Service Branch or Agency", description="Which services and organizations do you belong to within the DoD?", - choices=[ - ("null", "Select an option"), - ("Air Force, Department of the", "Air Force, Department of the"), - ("Army and Air Force Exchange Service", "Army and Air Force Exchange Service"), - ("Army, Department of the", "Army, Department of the"), - ("Defense Advanced Research Projects Agency", "Defense Advanced Research Projects Agency"), - ("Defense Commissary Agency", "Defense Commissary Agency"), - ("Defense Contract Audit Agency", "Defense Contract Audit Agency"), - ("Defense Contract Management Agency", "Defense Contract Management Agency"), - ("Defense Finance & Accounting Service", "Defense Finance & Accounting Service"), - ("Defense Health Agency", "Defense Health Agency"), - ("Defense Information System Agency", "Defense Information System Agency"), - ("Defense Intelligence Agency", "Defense Intelligence Agency"), - ("Defense Legal Services Agency", "Defense Legal Services Agency"), - ("Defense Logistics Agency", "Defense Logistics Agency"), - ("Defense Media Activity", "Defense Media Activity"), - ("Defense Micro Electronics Activity", "Defense Micro Electronics Activity"), - ("Defense POW-MIA Accounting Agency", "Defense POW-MIA Accounting Agency"), - ("Defense Security Cooperation Agency", "Defense Security Cooperation Agency"), - ("Defense Security Service", "Defense Security Service"), - ("Defense Technical Information Center", "Defense Technical Information Center"), - ("Defense Technology Security Administration", "Defense Technology Security Administration"), - ("Defense Threat Reduction Agency", "Defense Threat Reduction Agency"), - ("DoD Education Activity", "DoD Education Activity"), - ("DoD Human Recourses Activity", "DoD Human Recourses Activity"), - ("DoD Inspector General", "DoD Inspector General"), - ("DoD Test Resource Management Center", "DoD Test Resource Management Center"), - ("Headquarters Defense Human Resource Activity ", "Headquarters Defense Human Resource Activity "), - ("Joint Staff", "Joint Staff"), - ("Missile Defense Agency", "Missile Defense Agency"), - ("National Defense University", "National Defense University"), - ("National Geospatial Intelligence Agency (NGA)", "National Geospatial Intelligence Agency (NGA)"), - ("National Oceanic and Atmospheric Administration (NOAA)", "National Oceanic and Atmospheric Administration (NOAA)"), - ("National Reconnaissance Office", "National Reconnaissance Office"), - ("National Reconnaissance Office (NRO)", "National Reconnaissance Office (NRO)"), - ("National Security Agency (NSA)", "National Security Agency (NSA)"), - ("National Security Agency-Central Security Service", "National Security Agency-Central Security Service"), - ("Navy, Department of the", "Navy, Department of the"), - ("Office of Economic Adjustment", "Office of Economic Adjustment"), - ("Office of the Secretary of Defense", "Office of the Secretary of Defense"), - ("Pentagon Force Protection Agency", "Pentagon Force Protection Agency"), - ("Uniform Services University of the Health Sciences", "Uniform Services University of the Health Sciences"), - ("US Cyber Command (USCYBERCOM)", "US Cyber Command (USCYBERCOM)"), - ("US Special Operations Command (USSOCOM)", "US Special Operations Command (USSOCOM)"), - ("US Strategic Command (USSTRATCOM)", "US Strategic Command (USSTRATCOM)"), - ("US Transportation Command (USTRANSCOM)", "US Transportation Command (USTRANSCOM)"), - ("Washington Headquarters Services", "Washington Headquarters Services"), - ], + choices=SERVICE_BRANCHES, ) citizenship = RadioField( diff --git a/atst/forms/request.py b/atst/forms/request.py index c14dacf2..576d9879 100644 --- a/atst/forms/request.py +++ b/atst/forms/request.py @@ -1,9 +1,10 @@ from wtforms.fields.html5 import IntegerField -from wtforms.fields import RadioField, TextAreaField, SelectField +from wtforms.fields import RadioField, TextAreaField from wtforms.validators import Optional, Required -from .fields import DateField +from .fields import DateField, SelectField from .forms import ValidatedForm +from .data import SERVICE_BRANCHES from atst.domain.requests import Requests @@ -37,54 +38,7 @@ class RequestForm(ValidatedForm): dod_component = SelectField( "DoD Component", description="Identify the DoD component that is requesting access to the JEDI Cloud", - choices=[ - ("null", "Select an option"), - ("Air Force, Department of the", "Air Force, Department of the"), - ("Army and Air Force Exchange Service", "Army and Air Force Exchange Service"), - ("Army, Department of the", "Army, Department of the"), - ("Defense Advanced Research Projects Agency", "Defense Advanced Research Projects Agency"), - ("Defense Commissary Agency", "Defense Commissary Agency"), - ("Defense Contract Audit Agency", "Defense Contract Audit Agency"), - ("Defense Contract Management Agency", "Defense Contract Management Agency"), - ("Defense Finance & Accounting Service", "Defense Finance & Accounting Service"), - ("Defense Health Agency", "Defense Health Agency"), - ("Defense Information System Agency", "Defense Information System Agency"), - ("Defense Intelligence Agency", "Defense Intelligence Agency"), - ("Defense Legal Services Agency", "Defense Legal Services Agency"), - ("Defense Logistics Agency", "Defense Logistics Agency"), - ("Defense Media Activity", "Defense Media Activity"), - ("Defense Micro Electronics Activity", "Defense Micro Electronics Activity"), - ("Defense POW-MIA Accounting Agency", "Defense POW-MIA Accounting Agency"), - ("Defense Security Cooperation Agency", "Defense Security Cooperation Agency"), - ("Defense Security Service", "Defense Security Service"), - ("Defense Technical Information Center", "Defense Technical Information Center"), - ("Defense Technology Security Administration", "Defense Technology Security Administration"), - ("Defense Threat Reduction Agency", "Defense Threat Reduction Agency"), - ("DoD Education Activity", "DoD Education Activity"), - ("DoD Human Recourses Activity", "DoD Human Recourses Activity"), - ("DoD Inspector General", "DoD Inspector General"), - ("DoD Test Resource Management Center", "DoD Test Resource Management Center"), - ("Headquarters Defense Human Resource Activity ", "Headquarters Defense Human Resource Activity "), - ("Joint Staff", "Joint Staff"), - ("Missile Defense Agency", "Missile Defense Agency"), - ("National Defense University", "National Defense University"), - ("National Geospatial Intelligence Agency (NGA)", "National Geospatial Intelligence Agency (NGA)"), - ("National Oceanic and Atmospheric Administration (NOAA)", "National Oceanic and Atmospheric Administration (NOAA)"), - ("National Reconnaissance Office", "National Reconnaissance Office"), - ("National Reconnaissance Office (NRO)", "National Reconnaissance Office (NRO)"), - ("National Security Agency (NSA)", "National Security Agency (NSA)"), - ("National Security Agency-Central Security Service", "National Security Agency-Central Security Service"), - ("Navy, Department of the", "Navy, Department of the"), - ("Office of Economic Adjustment", "Office of Economic Adjustment"), - ("Office of the Secretary of Defense", "Office of the Secretary of Defense"), - ("Pentagon Force Protection Agency", "Pentagon Force Protection Agency"), - ("Uniform Services University of the Health Sciences", "Uniform Services University of the Health Sciences"), - ("US Cyber Command (USCYBERCOM)", "US Cyber Command (USCYBERCOM)"), - ("US Special Operations Command (USSOCOM)", "US Special Operations Command (USSOCOM)"), - ("US Strategic Command (USSTRATCOM)", "US Strategic Command (USSTRATCOM)"), - ("US Transportation Command (USTRANSCOM)", "US Transportation Command (USTRANSCOM)"), - ("Washington Headquarters Services", "Washington Headquarters Services"), - ], + choices=SERVICE_BRANCHES, validators=[Required()] ) From 66eb44afdd8da40f41fdeb9e6c1774cbfa2f046d Mon Sep 17 00:00:00 2001 From: richard-dds Date: Thu, 16 Aug 2018 16:45:22 -0400 Subject: [PATCH 19/36] Add forms.data --- atst/forms/data.py | 48 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 48 insertions(+) create mode 100644 atst/forms/data.py diff --git a/atst/forms/data.py b/atst/forms/data.py new file mode 100644 index 00000000..8e55ff33 --- /dev/null +++ b/atst/forms/data.py @@ -0,0 +1,48 @@ +SERVICE_BRANCHES = [ + (None, "Select an option"), + ("Air Force, Department of the", "Air Force, Department of the"), + ("Army and Air Force Exchange Service", "Army and Air Force Exchange Service"), + ("Army, Department of the", "Army, Department of the"), + ("Defense Advanced Research Projects Agency", "Defense Advanced Research Projects Agency"), + ("Defense Commissary Agency", "Defense Commissary Agency"), + ("Defense Contract Audit Agency", "Defense Contract Audit Agency"), + ("Defense Contract Management Agency", "Defense Contract Management Agency"), + ("Defense Finance & Accounting Service", "Defense Finance & Accounting Service"), + ("Defense Health Agency", "Defense Health Agency"), + ("Defense Information System Agency", "Defense Information System Agency"), + ("Defense Intelligence Agency", "Defense Intelligence Agency"), + ("Defense Legal Services Agency", "Defense Legal Services Agency"), + ("Defense Logistics Agency", "Defense Logistics Agency"), + ("Defense Media Activity", "Defense Media Activity"), + ("Defense Micro Electronics Activity", "Defense Micro Electronics Activity"), + ("Defense POW-MIA Accounting Agency", "Defense POW-MIA Accounting Agency"), + ("Defense Security Cooperation Agency", "Defense Security Cooperation Agency"), + ("Defense Security Service", "Defense Security Service"), + ("Defense Technical Information Center", "Defense Technical Information Center"), + ("Defense Technology Security Administration", "Defense Technology Security Administration"), + ("Defense Threat Reduction Agency", "Defense Threat Reduction Agency"), + ("DoD Education Activity", "DoD Education Activity"), + ("DoD Human Recourses Activity", "DoD Human Recourses Activity"), + ("DoD Inspector General", "DoD Inspector General"), + ("DoD Test Resource Management Center", "DoD Test Resource Management Center"), + ("Headquarters Defense Human Resource Activity ", "Headquarters Defense Human Resource Activity "), + ("Joint Staff", "Joint Staff"), + ("Missile Defense Agency", "Missile Defense Agency"), + ("National Defense University", "National Defense University"), + ("National Geospatial Intelligence Agency (NGA)", "National Geospatial Intelligence Agency (NGA)"), + ("National Oceanic and Atmospheric Administration (NOAA)", "National Oceanic and Atmospheric Administration (NOAA)"), + ("National Reconnaissance Office", "National Reconnaissance Office"), + ("National Reconnaissance Office (NRO)", "National Reconnaissance Office (NRO)"), + ("National Security Agency (NSA)", "National Security Agency (NSA)"), + ("National Security Agency-Central Security Service", "National Security Agency-Central Security Service"), + ("Navy, Department of the", "Navy, Department of the"), + ("Office of Economic Adjustment", "Office of Economic Adjustment"), + ("Office of the Secretary of Defense", "Office of the Secretary of Defense"), + ("Pentagon Force Protection Agency", "Pentagon Force Protection Agency"), + ("Uniform Services University of the Health Sciences", "Uniform Services University of the Health Sciences"), + ("US Cyber Command (USCYBERCOM)", "US Cyber Command (USCYBERCOM)"), + ("US Special Operations Command (USSOCOM)", "US Special Operations Command (USSOCOM)"), + ("US Strategic Command (USSTRATCOM)", "US Strategic Command (USSTRATCOM)"), + ("US Transportation Command (USTRANSCOM)", "US Transportation Command (USTRANSCOM)"), + ("Washington Headquarters Services", "Washington Headquarters Services"), +] From b41cfa095545c7e5f4694c1a302b46e6e73dfcc9 Mon Sep 17 00:00:00 2001 From: richard-dds Date: Thu, 16 Aug 2018 16:55:58 -0400 Subject: [PATCH 20/36] Move UII form workaround into a form method --- atst/forms/financial.py | 6 ++++++ atst/routes/requests/financial_verification.py | 4 ++-- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/atst/forms/financial.py b/atst/forms/financial.py index f7aa76a6..2db90e5f 100644 --- a/atst/forms/financial.py +++ b/atst/forms/financial.py @@ -63,6 +63,12 @@ class FinancialForm(ValidatedForm): self.funding_type_other.validators.append(Required()) return super().validate(*args, **kwargs) + def reset(self): + """ + Reset UII info so that it can be de-parsed rendered properly. + This is a stupid workaround, and there's probably a better way. + """ + self.uii_ids.process_data(self.uii_ids.data) def perform_extra_validation(self, existing_request): valid = True diff --git a/atst/routes/requests/financial_verification.py b/atst/routes/requests/financial_verification.py index 2a92b938..966a9680 100644 --- a/atst/routes/requests/financial_verification.py +++ b/atst/routes/requests/financial_verification.py @@ -32,12 +32,12 @@ def update_financial_verification(request_id): if valid: return redirect(url_for("requests.financial_verification_submitted")) else: - # TODO: This is a horrible workaround. - form.uii_ids.process_data(form.uii_ids.data) + form.reset() return render_template( "requests/financial_verification.html", **rerender_args ) else: + form.reset() return render_template("requests/financial_verification.html", **rerender_args) From c5a52bf46ffb398877c289e0e52fb2deff56880e Mon Sep 17 00:00:00 2001 From: luis cielak Date: Fri, 17 Aug 2018 13:39:03 -0400 Subject: [PATCH 21/36] Update tooltip and validation requirements --- atst/forms/financial.py | 2 +- atst/forms/request.py | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/atst/forms/financial.py b/atst/forms/financial.py index 2db90e5f..bf5c0675 100644 --- a/atst/forms/financial.py +++ b/atst/forms/financial.py @@ -89,7 +89,7 @@ class FinancialForm(ValidatedForm): treasury_code = StringField("Program Treasury Code", validators=[Required(), Regexp(TREASURY_CODE_REGEX)]) - ba_code = StringField("Program BA Code", validators=[Required(), Regexp(BA_CODE_REGEX)]) + ba_code = StringField("Program Budget Activity (BA) Code", validators=[Required(), Regexp(BA_CODE_REGEX)]) fname_co = StringField("Contracting Officer First Name", validators=[Required()]) lname_co = StringField("Contracting Officer Last Name", validators=[Required()]) diff --git a/atst/forms/request.py b/atst/forms/request.py index 576d9879..4ee83cae 100644 --- a/atst/forms/request.py +++ b/atst/forms/request.py @@ -64,7 +64,7 @@ class RequestForm(ValidatedForm): rationalization_software_systems = RadioField( description="Have you completed a “rationalization” of your software systems to move to the cloud?", - choices=[("yes", "Yes"), ("no", "No"), ("in_progress", "In Progress")], + choices=[("yes", "Yes"), ("no", "No"), ("In Progress", "In Progress")], default="", ) @@ -87,14 +87,14 @@ class RequestForm(ValidatedForm): engineering_assessment = RadioField( description="Have you completed an engineering assessment of your systems for cloud readiness?", - choices=[("yes", "Yes"), ("no", "No"), ("in_progress", "In Progress")], + choices=[("yes", "Yes"), ("no", "No"), ("In Progress", "In Progress")], default="", ) data_transfers = SelectField( description="How much data is being transferred to the cloud?", choices=[ - ("null", "Select an option"), + ("", "Select an option"), ("Less than 100GB", "Less than 100GB"), ("100GB-500GB", "100GB-500GB"), ("500GB-1TB", "500GB-1TB"), @@ -111,7 +111,7 @@ class RequestForm(ValidatedForm): expected_completion_date = SelectField( description="When do you expect to complete your migration to the JEDI Cloud?", choices=[ - ("null", "Select an option"), + ("", "Select an option"), ("Less than 1 month", "Less than 1 month"), ("1-3 months", "1-3 months"), ("3-6 months", "3-6 months"), From 9c6eee5c855d49e8a6fe8d74d5a9b24d03aeccd8 Mon Sep 17 00:00:00 2001 From: luis cielak Date: Fri, 17 Aug 2018 13:39:11 -0400 Subject: [PATCH 22/36] Update tooltip and validation requirements --- templates/requests/financial_verification.html | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/templates/requests/financial_verification.html b/templates/requests/financial_verification.html index 5f4c14e7..c45a0889 100644 --- a/templates/requests/financial_verification.html +++ b/templates/requests/financial_verification.html @@ -38,7 +38,7 @@ {{ TextInput( f.task_order_id, placeholder="e.g.: 1234567899C0001", - tooltip="Note that there may be a lag between the time you have created and approved the task order to the time it is searchable within the electronic.
A Contracting Officer will likely be the best source for this number.", + tooltip="A Contracting Officer will likely be the best source for this number.", validation="anything" ) }} From bd65341b886756e05ce652c23fe749f813f77c80 Mon Sep 17 00:00:00 2001 From: luis cielak Date: Fri, 17 Aug 2018 13:39:24 -0400 Subject: [PATCH 23/36] Update tooltip and validation requirements --- atst/forms/financial.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/atst/forms/financial.py b/atst/forms/financial.py index bf5c0675..5e39eae0 100644 --- a/atst/forms/financial.py +++ b/atst/forms/financial.py @@ -77,7 +77,9 @@ class FinancialForm(ValidatedForm): return valid task_order_id = StringField( - "Task Order Number associated with this request", validators=[Required()] + "Task Order Number associated with this request", + description="Include the original Task Order number (including the 000X at the end). Do not include any modification numbers. Note that there may be a lag between approving a task order and when it becomes available in our system.", + validators=[Required()] ) uii_ids = NewlineListField( From e1486066130edb378e9468f18478a2f8d9626673 Mon Sep 17 00:00:00 2001 From: luis cielak Date: Fri, 17 Aug 2018 13:44:39 -0400 Subject: [PATCH 24/36] Add description --- atst/forms/financial.py | 1 + 1 file changed, 1 insertion(+) diff --git a/atst/forms/financial.py b/atst/forms/financial.py index 5e39eae0..394fab56 100644 --- a/atst/forms/financial.py +++ b/atst/forms/financial.py @@ -118,6 +118,7 @@ class FinancialForm(ValidatedForm): ) funding_type = SelectField( + description="What is the source of funding?", choices=[ ("", "- Select -"), ("RDTE", "Research, Development, Testing & Evaluation (RDT&E)"), From 06c44f86c93ead2a6b6b87eb82009893cee7bf54 Mon Sep 17 00:00:00 2001 From: richard-dds Date: Fri, 17 Aug 2018 14:24:15 -0400 Subject: [PATCH 25/36] Fix tests, linting errors --- atst/forms/financial.py | 2 +- atst/forms/request.py | 8 ++-- tests/factories.py | 14 +++---- tests/forms/test_request.py | 84 ++++++++++++++++++------------------- 4 files changed, 53 insertions(+), 55 deletions(-) diff --git a/atst/forms/financial.py b/atst/forms/financial.py index 394fab56..42a29d39 100644 --- a/atst/forms/financial.py +++ b/atst/forms/financial.py @@ -1,7 +1,7 @@ import re from wtforms.fields.html5 import EmailField from wtforms.fields import StringField -from wtforms.validators import Required, Email, InputRequired, Regexp, DataRequired +from wtforms.validators import Required, Email, Regexp from atst.domain.exceptions import NotFoundError from atst.domain.pe_numbers import PENumbers diff --git a/atst/forms/request.py b/atst/forms/request.py index 4ee83cae..a938142f 100644 --- a/atst/forms/request.py +++ b/atst/forms/request.py @@ -77,10 +77,10 @@ class RequestForm(ValidatedForm): organization_providing_assistance = RadioField( # this needs to be updated to use checkboxes instead of radio description="If you are receiving migration assistance, what is the type of organization providing assistance?", choices=[ - ("in_house_staff", "In-house staff"), - ("contractor", "Contractor"), - ("other_dod_organization", "Other DoD organization"), - ("none", "None"), + ("In-house staff", "In-house staff"), + ("Contractor", "Contractor"), + ("Other DoD Organization", "Other DoD Organization"), + ("None", "None"), ], default="", ) diff --git a/tests/factories.py b/tests/factories.py index 07073424..a47b38ab 100644 --- a/tests/factories.py +++ b/tests/factories.py @@ -13,7 +13,6 @@ from atst.models.request_status_event import RequestStatusEvent from atst.domain.roles import Roles - class RoleFactory(factory.alchemy.SQLAlchemyModelFactory): class Meta: model = Role @@ -34,7 +33,6 @@ class UserFactory(factory.alchemy.SQLAlchemyModelFactory): class RequestStatusEventFactory(factory.alchemy.SQLAlchemyModelFactory): - class Meta: model = RequestStatusEvent @@ -61,7 +59,7 @@ class RequestFactory(factory.alchemy.SQLAlchemyModelFactory): "dodid_poc": user.dod_id, "email_poc": user.email, "fname_poc": user.first_name, - "lname_poc": user.last_name + "lname_poc": user.last_name, }, "details_of_use": { "jedi_usage": "adf", @@ -69,7 +67,8 @@ class RequestFactory(factory.alchemy.SQLAlchemyModelFactory): "cloud_native": "yes", "dollar_value": dollar_value, "dod_component": "Army and Air Force Exchange Service", - "data_transfers": "less_than_100gb", + "data_transfers": "Less than 100GB", + "expected_completion_date": "Less than 1 month", "jedi_migration": "yes", "num_software_systems": 1, "number_user_sessions": 2, @@ -78,9 +77,8 @@ class RequestFactory(factory.alchemy.SQLAlchemyModelFactory): "technical_support_team": "yes", "estimated_monthly_spend": 100, "average_daily_traffic_gb": 4, - "expected_completion_date": "less_than_1_month", "rationalization_software_systems": "yes", - "organization_providing_assistance": "in_house_staff" + "organization_providing_assistance": "In-house staff", }, "information_about_you": { "citizenship": "United States", @@ -90,8 +88,8 @@ class RequestFactory(factory.alchemy.SQLAlchemyModelFactory): "fname_request": user.first_name, "lname_request": user.last_name, "service_branch": "Air Force, Department of the", - "date_latest_training": "2018-08-06" - } + "date_latest_training": "2018-08-06", + }, } diff --git a/tests/forms/test_request.py b/tests/forms/test_request.py index 9d5e4201..5f55a561 100644 --- a/tests/forms/test_request.py +++ b/tests/forms/test_request.py @@ -6,80 +6,80 @@ from atst.forms.request import RequestForm class TestRequestForm: form_data = { - 'dod_component': 'Army and Air Force Exchange Service', - 'jedi_usage': 'cloud-ify all the things', - 'num_software_systems': '12', - 'estimated_monthly_spend': '1000000', - 'dollar_value': '42', - 'number_user_sessions': '6', - 'average_daily_traffic': '0', - 'start_date': '12/12/2012', + "dod_component": "Army and Air Force Exchange Service", + "jedi_usage": "cloud-ify all the things", + "num_software_systems": "12", + "estimated_monthly_spend": "1000000", + "dollar_value": "42", + "number_user_sessions": "6", + "average_daily_traffic": "0", + "start_date": "12/12/2012", } migration_data = { - 'jedi_migration': 'yes', - 'rationalization_software_systems': 'yes', - 'technical_support_team': 'yes', - 'organization_providing_assistance': 'in_house_staff', - 'engineering_assessment': 'yes', - 'data_transfers': 'less_than_100gb', - 'expected_completion_date': 'less_than_1_month' + "jedi_migration": "yes", + "rationalization_software_systems": "yes", + "technical_support_team": "yes", + "organization_providing_assistance": "In-house staff", + "engineering_assessment": "yes", + "data_transfers": "Less than 100GB", + "expected_completion_date": "Less than 1 month", } def test_require_cloud_native_when_not_migrating(self): - extra_data = { 'jedi_migration': 'no' } - request_form = RequestForm(data={ **self.form_data, **extra_data }) + extra_data = {"jedi_migration": "no"} + request_form = RequestForm(data={**self.form_data, **extra_data}) assert not request_form.validate() - assert request_form.errors == { 'cloud_native': ['Not a valid choice'] } + assert request_form.errors == {"cloud_native": ["Not a valid choice"]} def test_require_migration_questions_when_migrating(self): - extra_data = { 'jedi_migration': 'yes' } - request_form = RequestForm(data={ **self.form_data, **extra_data }) + extra_data = {"jedi_migration": "yes"} + request_form = RequestForm(data={**self.form_data, **extra_data}) assert not request_form.validate() assert request_form.errors == { - 'rationalization_software_systems': ['Not a valid choice'], - 'technical_support_team': ['Not a valid choice'], - 'organization_providing_assistance': ['Not a valid choice'], - 'engineering_assessment': ['Not a valid choice'], - 'data_transfers': ['Not a valid choice'], - 'expected_completion_date': ['Not a valid choice'] + "rationalization_software_systems": ["Not a valid choice"], + "technical_support_team": ["Not a valid choice"], + "organization_providing_assistance": ["Not a valid choice"], + "engineering_assessment": ["Not a valid choice"], + "data_transfers": ["Not a valid choice"], + "expected_completion_date": ["Not a valid choice"], } def test_require_organization_when_technical_support_team(self): - data = { **self.form_data, **self.migration_data } - del data['organization_providing_assistance'] + data = {**self.form_data, **self.migration_data} + del data["organization_providing_assistance"] request_form = RequestForm(data=data) assert not request_form.validate() assert request_form.errors == { - 'organization_providing_assistance': ['Not a valid choice'], + "organization_providing_assistance": ["Not a valid choice"] } def test_valid_form_data(self): - data = { **self.form_data, **self.migration_data } - data['technical_support_team'] = 'no' - del data['organization_providing_assistance'] + data = {**self.form_data, **self.migration_data} + data["technical_support_team"] = "no" + del data["organization_providing_assistance"] request_form = RequestForm(data=data) assert request_form.validate() def test_sessions_required_for_large_projects(self): - data = { **self.form_data, **self.migration_data } - data['estimated_monthly_spend'] = '9999999' - del data['number_user_sessions'] - del data['average_daily_traffic'] + data = {**self.form_data, **self.migration_data} + data["estimated_monthly_spend"] = "9999999" + del data["number_user_sessions"] + del data["average_daily_traffic"] request_form = RequestForm(data=data) assert not request_form.validate() assert request_form.errors == { - 'number_user_sessions': ['This field is required.'], - 'average_daily_traffic': ['This field is required.'], + "number_user_sessions": ["This field is required."], + "average_daily_traffic": ["This field is required."], } def test_sessions_not_required_invalid_monthly_spend(self): - data = { **self.form_data, **self.migration_data } - data['estimated_monthly_spend'] = 'foo' - del data['number_user_sessions'] - del data['average_daily_traffic'] + data = {**self.form_data, **self.migration_data} + data["estimated_monthly_spend"] = "foo" + del data["number_user_sessions"] + del data["average_daily_traffic"] request_form = RequestForm(data=data) assert request_form.validate() From a72c8498a26e83e327f56c3d3782d58407cb69df Mon Sep 17 00:00:00 2001 From: dandds Date: Thu, 16 Aug 2018 13:02:49 -0400 Subject: [PATCH 26/36] beginning of a CRL loader rewrite --- atst/app.py | 6 +-- atst/domain/authnid/crl/__init__.py | 62 ++++++++++++++++++++++++++++- 2 files changed, 62 insertions(+), 6 deletions(-) diff --git a/atst/app.py b/atst/app.py index ae207b01..cbca4c22 100644 --- a/atst/app.py +++ b/atst/app.py @@ -16,7 +16,7 @@ from atst.routes.workspaces import bp as workspace_routes from atst.routes.requests import requests_bp from atst.routes.dev import bp as dev_routes from atst.routes.errors import make_error_pages -from atst.domain.authnid.crl import Validator +from atst.domain.authnid.crl import Validator, CRLCache from atst.domain.auth import apply_authentication @@ -141,7 +141,5 @@ def make_crl_validator(app): crl_locations = [] for filename in pathlib.Path(app.config["CRL_DIRECTORY"]).glob("*"): crl_locations.append(filename.absolute()) - app.crl_validator = Validator( - roots=[app.config["CA_CHAIN"]], crl_locations=crl_locations, logger=app.logger - ) + app.crl_cache = CRLCache(app.config["CA_CHAIN"], crl_locations) diff --git a/atst/domain/authnid/crl/__init__.py b/atst/domain/authnid/crl/__init__.py index 68358e37..dc988546 100644 --- a/atst/domain/authnid/crl/__init__.py +++ b/atst/domain/authnid/crl/__init__.py @@ -13,6 +13,58 @@ def sha256_checksum(filename, block_size=65536): return sha256.hexdigest() +class CRLCache(): + + _PEM_RE = re.compile( + b"-----BEGIN CERTIFICATE-----\r?.+?\r?-----END CERTIFICATE-----\r?\n?", + re.DOTALL, + ) + + def __init__(self, root_location, crl_locations=[], store_class=crypto.X509Store): + self.crl_cache = {} + self.store_class = store_class + self._load_roots(root_location) + self._build_x509_stores(crl_locations) + + def _load_roots(self, root_location): + self.certificate_authorities = {} + with open(root_location, "rb") as f: + for raw_ca in self._parse_roots(f.read()): + ca = crypto.load_certificate(crypto.FILETYPE_PEM, raw_ca) + self.certificate_authorities[ca.get_subject().der()] = ca + + def _parse_roots(self, root_str): + return [match.group(0) for match in self._PEM_RE.finditer(root_str)] + + def _build_x509_stores(self, crl_locations): + self.x509_stores = {} + for crl_path in crl_locations: + issuer, store = self._build_store(crl_path) + self.x509_stores[issuer] = store + + def _build_store(self, crl_location): + store = self.store_class() + store.set_flags(crypto.X509StoreFlags.CRL_CHECK) + with open(crl_location, "rb") as crl_file: + crl = crypto.load_crl(crypto.FILETYPE_ASN1, crl_file.read()) + self.crl_cache[crl.get_issuer().der()] = (crl_location, sha256_checksum(crl_location)) + store.add_crl(crl) + store = self._add_certificate_chain_to_store(store, crl.get_issuer()) + return (crl.get_issuer().der(), store) + + # this _should_ happen just twice for the DoD PKI (intermediary, root) but + # theoretically it can build a longer certificate chain + def _add_certificate_chain_to_store(self, store, issuer): + ca = self.certificate_authorities.get(issuer.der()) + # i.e., it is the root CA + if issuer == ca.get_subject(): + return store + + store.add_cert(ca) + return self._add_certificate_chain_to_store(store, ca.get_issuer()) + + + class Validator: _PEM_RE = re.compile( @@ -20,13 +72,19 @@ class Validator: re.DOTALL, ) - def __init__(self, crl_locations=[], roots=[], base_store=crypto.X509Store, logger=None): + def __init__(self, root, crl_locations=[], base_store=crypto.X509Store, logger=None): self.crl_locations = crl_locations - self.roots = roots + self.root = root self.base_store = base_store self.logger = logger self._reset() + def _add_roots(self, roots): + with open(filename, "rb") as f: + for raw_ca in self._parse_roots(f.read()): + ca = crypto.load_certificate(crypto.FILETYPE_PEM, raw_ca) + self._add_carefully("add_cert", ca) + def _reset(self): self.cache = {} self.store = self.base_store() From e9d6ee8102d44e15e15c6aa408a8b0c20bc3696e Mon Sep 17 00:00:00 2001 From: dandds Date: Thu, 16 Aug 2018 14:09:18 -0400 Subject: [PATCH 27/36] build individual x509 stores for each CRL --- atst/app.py | 2 +- atst/domain/authnid/__init__.py | 8 +- atst/domain/authnid/crl/__init__.py | 75 +++++++++--------- atst/routes/__init__.py | 2 +- .../authnid/test_authentication_context.py | 45 ++++++----- tests/domain/authnid/test_crl.py | 29 +++---- tests/fixtures/test.der.crl | Bin 768 -> 530 bytes 7 files changed, 80 insertions(+), 81 deletions(-) diff --git a/atst/app.py b/atst/app.py index cbca4c22..ddf66a40 100644 --- a/atst/app.py +++ b/atst/app.py @@ -16,7 +16,7 @@ from atst.routes.workspaces import bp as workspace_routes from atst.routes.requests import requests_bp from atst.routes.dev import bp as dev_routes from atst.routes.errors import make_error_pages -from atst.domain.authnid.crl import Validator, CRLCache +from atst.domain.authnid.crl import CRLCache from atst.domain.auth import apply_authentication diff --git a/atst/domain/authnid/__init__.py b/atst/domain/authnid/__init__.py index 80d645b8..35a4477c 100644 --- a/atst/domain/authnid/__init__.py +++ b/atst/domain/authnid/__init__.py @@ -1,17 +1,18 @@ from atst.domain.exceptions import UnauthenticatedError, NotFoundError from atst.domain.users import Users from .utils import parse_sdn, email_from_certificate +from .crl import Validator class AuthenticationContext(): - def __init__(self, crl_validator, auth_status, sdn, cert): + def __init__(self, crl_cache, auth_status, sdn, cert): if None in locals().values(): raise UnauthenticatedError( "Missing required authentication context components" ) - self.crl_validator = crl_validator + self.crl_cache = crl_cache self.auth_status = auth_status self.sdn = sdn self.cert = cert.encode() @@ -44,8 +45,9 @@ class AuthenticationContext(): return None def _crl_check(self): + validator = Validator(self.crl_cache, self.cert) if self.cert: - result = self.crl_validator.validate(self.cert) + result = validator.validate() return result else: diff --git a/atst/domain/authnid/crl/__init__.py b/atst/domain/authnid/crl/__init__.py index dc988546..5bbc7c72 100644 --- a/atst/domain/authnid/crl/__init__.py +++ b/atst/domain/authnid/crl/__init__.py @@ -56,28 +56,50 @@ class CRLCache(): # theoretically it can build a longer certificate chain def _add_certificate_chain_to_store(self, store, issuer): ca = self.certificate_authorities.get(issuer.der()) - # i.e., it is the root CA - if issuer == ca.get_subject(): - return store - store.add_cert(ca) - return self._add_certificate_chain_to_store(store, ca.get_issuer()) + if issuer == ca.get_subject(): + # i.e., it is the root CA and we are at the end of the chain + return store + else: + return self._add_certificate_chain_to_store(store, ca.get_issuer()) + + def get_store(self, cert): + return self._check_cache(cert.get_issuer().der()) + + def _check_cache(self, issuer): + if issuer in self.crl_cache: + filename, checksum = self.crl_cache[issuer] + if sha256_checksum(filename) != checksum: + issuer, store = self._build_store(filename) + self.x509_stores[issuer] = store + return store + else: + return self.x509_stores[issuer] class Validator: - _PEM_RE = re.compile( - b"-----BEGIN CERTIFICATE-----\r?.+?\r?-----END CERTIFICATE-----\r?\n?", - re.DOTALL, - ) - - def __init__(self, root, crl_locations=[], base_store=crypto.X509Store, logger=None): - self.crl_locations = crl_locations - self.root = root - self.base_store = base_store + def __init__(self, cache, cert, logger=None): + self.cache = cache + self.cert = cert self.logger = logger - self._reset() + + def validate(self): + parsed = crypto.load_certificate(crypto.FILETYPE_PEM, self.cert) + store = self.cache.get_store(parsed) + context = crypto.X509StoreContext(store, parsed) + try: + context.verify_certificate() + return True + + except crypto.X509StoreContextError as err: + self.log_error( + "Certificate revoked or errored. Error: {}. Args: {}".format( + type(err), err.args + ) + ) + return False def _add_roots(self, roots): with open(filename, "rb") as f: @@ -161,26 +183,3 @@ class Validator: return error.args == self.PRELOADED_CRL or error.args == self.PRELOADED_CERT # Checks that the CRL currently in-memory is up-to-date via the checksum. - - def refresh_cache(self, cert): - der = cert.get_issuer().der() - if der in self.cache: - filename, checksum = self.cache[der] - if sha256_checksum(filename) != checksum: - self._reset() - - def validate(self, cert): - parsed = crypto.load_certificate(crypto.FILETYPE_PEM, cert) - self.refresh_cache(parsed) - context = crypto.X509StoreContext(self.store, parsed) - try: - context.verify_certificate() - return True - - except crypto.X509StoreContextError as err: - self.log_error( - "Certificate revoked or errored. Error: {}. Args: {}".format( - type(err), err.args - ) - ) - return False diff --git a/atst/routes/__init__.py b/atst/routes/__init__.py index f8c4199c..68c83437 100644 --- a/atst/routes/__init__.py +++ b/atst/routes/__init__.py @@ -32,7 +32,7 @@ def catch_all(path): def _make_authentication_context(): return AuthenticationContext( - crl_validator=app.crl_validator, + crl_cache=app.crl_cache, auth_status=request.environ.get("HTTP_X_SSL_CLIENT_VERIFY"), sdn=request.environ.get("HTTP_X_SSL_CLIENT_S_DN"), cert=request.environ.get("HTTP_X_SSL_CLIENT_CERT") diff --git a/tests/domain/authnid/test_authentication_context.py b/tests/domain/authnid/test_authentication_context.py index f2a359af..f07d6e94 100644 --- a/tests/domain/authnid/test_authentication_context.py +++ b/tests/domain/authnid/test_authentication_context.py @@ -10,25 +10,23 @@ from tests.factories import UserFactory CERT = open("tests/fixtures/{}.crt".format(FIXTURE_EMAIL_ADDRESS)).read() -class MockCRLValidator(): - - def __init__(self, value): - self.value = value - - def validate(self, cert): - return self.value +class MockCRLCache(): + def get_store(self, cert): + pass -def test_can_authenticate(): +def test_can_authenticate(monkeypatch): + monkeypatch.setattr("atst.domain.authnid.Validator.validate", lambda s: True) auth_context = AuthenticationContext( - MockCRLValidator(True), "SUCCESS", DOD_SDN, CERT + MockCRLCache(), "SUCCESS", DOD_SDN, CERT ) assert auth_context.authenticate() -def test_unsuccessful_status(): +def test_unsuccessful_status(monkeypatch): + monkeypatch.setattr("atst.domain.authnid.Validator.validate", lambda s: True) auth_context = AuthenticationContext( - MockCRLValidator(True), "FAILURE", DOD_SDN, CERT + MockCRLCache(), "FAILURE", DOD_SDN, CERT ) with pytest.raises(UnauthenticatedError) as excinfo: assert auth_context.authenticate() @@ -37,9 +35,10 @@ def test_unsuccessful_status(): assert "client authentication" in message -def test_crl_check_fails(): +def test_crl_check_fails(monkeypatch): + monkeypatch.setattr("atst.domain.authnid.Validator.validate", lambda s: False) auth_context = AuthenticationContext( - MockCRLValidator(False), "SUCCESS", DOD_SDN, CERT + MockCRLCache(), "SUCCESS", DOD_SDN, CERT ) with pytest.raises(UnauthenticatedError) as excinfo: assert auth_context.authenticate() @@ -48,9 +47,10 @@ def test_crl_check_fails(): assert "CRL check" in message -def test_bad_sdn(): +def test_bad_sdn(monkeypatch): + monkeypatch.setattr("atst.domain.authnid.Validator.validate", lambda s: True) auth_context = AuthenticationContext( - MockCRLValidator(True), "SUCCESS", "abc123", CERT + MockCRLCache(), "SUCCESS", "abc123", CERT ) with pytest.raises(UnauthenticatedError) as excinfo: auth_context.get_user() @@ -59,33 +59,36 @@ def test_bad_sdn(): assert "SDN" in message -def test_user_exists(): +def test_user_exists(monkeypatch): + monkeypatch.setattr("atst.domain.authnid.Validator.validate", lambda s: True) user = UserFactory.create(**DOD_SDN_INFO) auth_context = AuthenticationContext( - MockCRLValidator(True), "SUCCESS", DOD_SDN, CERT + MockCRLCache(), "SUCCESS", DOD_SDN, CERT ) auth_user = auth_context.get_user() assert auth_user == user -def test_creates_user(): +def test_creates_user(monkeypatch): + monkeypatch.setattr("atst.domain.authnid.Validator.validate", lambda s: True) # check user does not exist with pytest.raises(NotFoundError): Users.get_by_dod_id(DOD_SDN_INFO["dod_id"]) auth_context = AuthenticationContext( - MockCRLValidator(True), "SUCCESS", DOD_SDN, CERT + MockCRLCache(), "SUCCESS", DOD_SDN, CERT ) user = auth_context.get_user() assert user.dod_id == DOD_SDN_INFO["dod_id"] assert user.email == FIXTURE_EMAIL_ADDRESS -def test_user_cert_has_no_email(): +def test_user_cert_has_no_email(monkeypatch): + monkeypatch.setattr("atst.domain.authnid.Validator.validate", lambda s: True) cert = open("ssl/client-certs/atat.mil.crt").read() auth_context = AuthenticationContext( - MockCRLValidator(True), "SUCCESS", DOD_SDN, cert + MockCRLCache(), "SUCCESS", DOD_SDN, cert ) user = auth_context.get_user() diff --git a/tests/domain/authnid/test_crl.py b/tests/domain/authnid/test_crl.py index 1b9fa2ec..fcd371b3 100644 --- a/tests/domain/authnid/test_crl.py +++ b/tests/domain/authnid/test_crl.py @@ -4,7 +4,7 @@ import re import os import shutil from OpenSSL import crypto, SSL -from atst.domain.authnid.crl import Validator +from atst.domain.authnid.crl import Validator, CRLCache import atst.domain.authnid.crl.util as util @@ -24,38 +24,33 @@ class MockX509Store(): def test_can_build_crl_list(monkeypatch): location = 'ssl/client-certs/client-ca.der.crl' - validator = Validator(crl_locations=[location], base_store=MockX509Store) - assert len(validator.store.crls) == 1 + cache = CRLCache('ssl/client-certs/client-ca.crt', crl_locations=[location], store_class=MockX509Store) + for store in cache.x509_stores.values(): + assert len(store.crls) == 1 def test_can_build_trusted_root_list(): location = 'ssl/server-certs/ca-chain.pem' - validator = Validator(roots=[location], base_store=MockX509Store) + cache = CRLCache(root_location=location, crl_locations=[], store_class=MockX509Store) with open(location) as f: content = f.read() - assert len(validator.store.certs) == content.count('BEGIN CERT') + assert len(cache.certificate_authorities.keys()) == content.count('BEGIN CERT') def test_can_validate_certificate(): - validator = Validator( - roots=['ssl/server-certs/ca-chain.pem'], - crl_locations=['ssl/client-certs/client-ca.der.crl'] - ) + cache = CRLCache('ssl/server-certs/ca-chain.pem', crl_locations=['ssl/client-certs/client-ca.der.crl']) good_cert = open('ssl/client-certs/atat.mil.crt', 'rb').read() bad_cert = open('ssl/client-certs/bad-atat.mil.crt', 'rb').read() - assert validator.validate(good_cert) - assert validator.validate(bad_cert) == False + assert Validator(cache, good_cert).validate() + assert Validator(cache, bad_cert).validate() == False def test_can_dynamically_update_crls(tmpdir): crl_file = tmpdir.join('test.crl') shutil.copyfile('ssl/client-certs/client-ca.der.crl', crl_file) - validator = Validator( - roots=['ssl/server-certs/ca-chain.pem'], - crl_locations=[crl_file] - ) + cache = CRLCache('ssl/server-certs/ca-chain.pem', crl_locations=[crl_file]) cert = open('ssl/client-certs/atat.mil.crt', 'rb').read() - assert validator.validate(cert) + assert Validator(cache, cert).validate() # override the original CRL with one that revokes atat.mil.crt shutil.copyfile('tests/fixtures/test.der.crl', crl_file) - assert validator.validate(cert) == False + assert Validator(cache, cert).validate() == False def test_parse_disa_pki_list(): with open('tests/fixtures/disa-pki.html') as disa: diff --git a/tests/fixtures/test.der.crl b/tests/fixtures/test.der.crl index dc8310f225966e2eb591a9e60ec4eba01991b48a..24708b0bcd0e9d01ed2ef247ea5d4f308848d03f 100644 GIT binary patch literal 530 zcmXqLV&XGs{BFR@#;Mij(e|B}k&%U!!Jx6%klTQhjX9KsO_(V(*ih6!7{uWa<_Sp6 z%PX$TDND@DOoR!u1BEj(a}rZha|$wm!U6_-Ak|#L>~4vr6)yQMFvGZonF7KM1r7K? z(#*mfAZZ1Vyqtjy+yR`7V*F`9iIn^l2O!o<&d(L+HMB6WFf=m+VUs8@*V51w$~CYs zkYeIwSmAAOC+wAYGLm9b1LG*L+`YH4T!qioZ-V8_j0}tnOw0@oqtLv~+{DPppcwUX z?^D^Ejt`G4l``(g|CKdi4F zgqHCh+`7jh_dU{Mqy|^Y+z^SFFWnr3G}#C`L3z#jSdm)HBJuEpW74PTNRID*XZ!XL4vRe#-A`4h LU%7wlq+C1z7&E~0 literal 768 zcmZ9KNpqtx6ol{mirif(W3eP0<{*K0`_b@qZYK ztcP3JGN?Qjjd@z0JcH$um0*=v#TT($9$JMU>Bucwc>Y+r-LnoM#TZd&wJk=_EtbHk z%FX^#UEntKGLT7qjA6ArF(9;Py}2IdMPQtkA0>PqN*Le!$CsA*!$a*lSkGg=y>d56 zl<*!YsA+=4mSVG6Je!-Jb?WVdUT{g*`8KcA`2$DbdJEE_%EnT-XVB(Xny;b8m@ni| zITs4UNh>`cWGg-~aGKEzLT@LFL7sI?HH~Gx+Rw5m0QzY$^VIK;{Js8x_T}*)@9gri zTSU7QN-GX>bn@N#7grr<%vN??s@~+CV$z8D%BcEk>XnuW;|{=vO{1=(lpW8tHCeVp4+j_lo?R#4&?ReEZWz2@$DJD$}YLo@Nwm(Zx P!2d|ERpZe5Z{~jicbxO@ From e64c12da46b536ef12ca75b9d27bb3ab95ee05f3 Mon Sep 17 00:00:00 2001 From: dandds Date: Thu, 16 Aug 2018 14:45:46 -0400 Subject: [PATCH 28/36] more straightforward crl check function --- atst/domain/authnid/__init__.py | 16 +-- atst/domain/authnid/crl/__init__.py | 124 +++--------------- .../authnid/test_authentication_context.py | 18 +-- tests/domain/authnid/test_crl.py | 12 +- 4 files changed, 41 insertions(+), 129 deletions(-) diff --git a/atst/domain/authnid/__init__.py b/atst/domain/authnid/__init__.py index 35a4477c..b31e00fb 100644 --- a/atst/domain/authnid/__init__.py +++ b/atst/domain/authnid/__init__.py @@ -1,7 +1,7 @@ from atst.domain.exceptions import UnauthenticatedError, NotFoundError from atst.domain.users import Users from .utils import parse_sdn, email_from_certificate -from .crl import Validator +from .crl import crl_check, CRLException class AuthenticationContext(): @@ -22,8 +22,7 @@ class AuthenticationContext(): if not self.auth_status == "SUCCESS": raise UnauthenticatedError("SSL/TLS client authentication failed") - elif not self._crl_check(): - raise UnauthenticatedError("Client certificate failed CRL check") + self._crl_check() return True @@ -45,13 +44,10 @@ class AuthenticationContext(): return None def _crl_check(self): - validator = Validator(self.crl_cache, self.cert) - if self.cert: - result = validator.validate() - return result - - else: - return False + try: + crl_check(self.crl_cache, self.cert) + except CRLException as exc: + raise UnauthenticatedError("CRL check failed. " + str(exc)) @property def parsed_sdn(self): diff --git a/atst/domain/authnid/crl/__init__.py b/atst/domain/authnid/crl/__init__.py index 5bbc7c72..5c53ffe9 100644 --- a/atst/domain/authnid/crl/__init__.py +++ b/atst/domain/authnid/crl/__init__.py @@ -4,6 +4,8 @@ import re import hashlib from OpenSSL import crypto, SSL +class CRLException(Exception): + pass def sha256_checksum(filename, block_size=65536): sha256 = hashlib.sha256() @@ -13,6 +15,22 @@ def sha256_checksum(filename, block_size=65536): return sha256.hexdigest() +def crl_check(cache, cert): + parsed = crypto.load_certificate(crypto.FILETYPE_PEM, cert) + store = cache.get_store(parsed) + context = crypto.X509StoreContext(store, parsed) + try: + context.verify_certificate() + return True + + except crypto.X509StoreContextError as err: + raise CRLException( + "Certificate revoked or errored. Error: {}. Args: {}".format( + type(err), err.args + ) + ) + + class CRLCache(): _PEM_RE = re.compile( @@ -77,109 +95,3 @@ class CRLCache(): else: return self.x509_stores[issuer] - -class Validator: - - def __init__(self, cache, cert, logger=None): - self.cache = cache - self.cert = cert - self.logger = logger - - def validate(self): - parsed = crypto.load_certificate(crypto.FILETYPE_PEM, self.cert) - store = self.cache.get_store(parsed) - context = crypto.X509StoreContext(store, parsed) - try: - context.verify_certificate() - return True - - except crypto.X509StoreContextError as err: - self.log_error( - "Certificate revoked or errored. Error: {}. Args: {}".format( - type(err), err.args - ) - ) - return False - - def _add_roots(self, roots): - with open(filename, "rb") as f: - for raw_ca in self._parse_roots(f.read()): - ca = crypto.load_certificate(crypto.FILETYPE_PEM, raw_ca) - self._add_carefully("add_cert", ca) - - def _reset(self): - self.cache = {} - self.store = self.base_store() - self._add_crls(self.crl_locations) - self._add_roots(self.roots) - self.store.set_flags(crypto.X509StoreFlags.CRL_CHECK) - - def log_error(self, message): - if self.logger: - self.logger.error(message) - - def _add_crls(self, locations): - for filename in locations: - try: - self._add_crl(filename) - except crypto.Error as err: - self.log_error( - "CRL could not be parsed. Filename: {}, Error: {}, args: {}".format( - filename, type(err), err.args - ) - ) - - # This caches the CRL issuer with the CRL filepath and a checksum, in addition to adding the CRL to the store. - - def _add_crl(self, filename): - with open(filename, "rb") as crl_file: - crl = crypto.load_crl(crypto.FILETYPE_ASN1, crl_file.read()) - self.cache[crl.get_issuer().der()] = (filename, sha256_checksum(filename)) - self._add_carefully("add_crl", crl) - - def _parse_roots(self, root_str): - return [match.group(0) for match in self._PEM_RE.finditer(root_str)] - - def _add_roots(self, roots): - for filename in roots: - with open(filename, "rb") as f: - for raw_ca in self._parse_roots(f.read()): - ca = crypto.load_certificate(crypto.FILETYPE_PEM, raw_ca) - self._add_carefully("add_cert", ca) - - # in testing, it seems that openssl is maintaining a local cache of certs - # in a hash table and throws errors if you try to add redundant certs or - # CRLs. For now, we catch and ignore that error with great specificity. - - def _add_carefully(self, method_name, obj): - try: - getattr(self.store, method_name)(obj) - except crypto.Error as error: - if self._is_preloaded_error(error): - pass - else: - raise error - - PRELOADED_CRL = ( - [ - ( - "x509 certificate routines", - "X509_STORE_add_crl", - "cert already in hash table", - ) - ], - ) - PRELOADED_CERT = ( - [ - ( - "x509 certificate routines", - "X509_STORE_add_cert", - "cert already in hash table", - ) - ], - ) - - def _is_preloaded_error(self, error): - return error.args == self.PRELOADED_CRL or error.args == self.PRELOADED_CERT - - # Checks that the CRL currently in-memory is up-to-date via the checksum. diff --git a/tests/domain/authnid/test_authentication_context.py b/tests/domain/authnid/test_authentication_context.py index f07d6e94..d7c8e157 100644 --- a/tests/domain/authnid/test_authentication_context.py +++ b/tests/domain/authnid/test_authentication_context.py @@ -1,6 +1,7 @@ import pytest from atst.domain.authnid import AuthenticationContext +from atst.domain.authnid.crl import CRLCache from atst.domain.exceptions import UnauthenticatedError, NotFoundError from atst.domain.users import Users @@ -16,7 +17,7 @@ class MockCRLCache(): def test_can_authenticate(monkeypatch): - monkeypatch.setattr("atst.domain.authnid.Validator.validate", lambda s: True) + monkeypatch.setattr("atst.domain.authnid.crl_check", lambda *args: True) auth_context = AuthenticationContext( MockCRLCache(), "SUCCESS", DOD_SDN, CERT ) @@ -24,7 +25,7 @@ def test_can_authenticate(monkeypatch): def test_unsuccessful_status(monkeypatch): - monkeypatch.setattr("atst.domain.authnid.Validator.validate", lambda s: True) + monkeypatch.setattr("atst.domain.authnid.crl_check", lambda *args: True) auth_context = AuthenticationContext( MockCRLCache(), "FAILURE", DOD_SDN, CERT ) @@ -36,9 +37,10 @@ def test_unsuccessful_status(monkeypatch): def test_crl_check_fails(monkeypatch): - monkeypatch.setattr("atst.domain.authnid.Validator.validate", lambda s: False) + cache = CRLCache('ssl/client-certs/client-ca.crt', crl_locations=['ssl/client-certs/client-ca.der.crl']) + cert = open("ssl/client-certs/bad-atat.mil.crt", "r").read() auth_context = AuthenticationContext( - MockCRLCache(), "SUCCESS", DOD_SDN, CERT + cache, "SUCCESS", DOD_SDN, cert ) with pytest.raises(UnauthenticatedError) as excinfo: assert auth_context.authenticate() @@ -48,7 +50,7 @@ def test_crl_check_fails(monkeypatch): def test_bad_sdn(monkeypatch): - monkeypatch.setattr("atst.domain.authnid.Validator.validate", lambda s: True) + monkeypatch.setattr("atst.domain.authnid.crl_check", lambda *args: True) auth_context = AuthenticationContext( MockCRLCache(), "SUCCESS", "abc123", CERT ) @@ -60,7 +62,7 @@ def test_bad_sdn(monkeypatch): def test_user_exists(monkeypatch): - monkeypatch.setattr("atst.domain.authnid.Validator.validate", lambda s: True) + monkeypatch.setattr("atst.domain.authnid.crl_check", lambda *args: True) user = UserFactory.create(**DOD_SDN_INFO) auth_context = AuthenticationContext( MockCRLCache(), "SUCCESS", DOD_SDN, CERT @@ -71,7 +73,7 @@ def test_user_exists(monkeypatch): def test_creates_user(monkeypatch): - monkeypatch.setattr("atst.domain.authnid.Validator.validate", lambda s: True) + monkeypatch.setattr("atst.domain.authnid.crl_check", lambda *args: True) # check user does not exist with pytest.raises(NotFoundError): Users.get_by_dod_id(DOD_SDN_INFO["dod_id"]) @@ -85,7 +87,7 @@ def test_creates_user(monkeypatch): def test_user_cert_has_no_email(monkeypatch): - monkeypatch.setattr("atst.domain.authnid.Validator.validate", lambda s: True) + monkeypatch.setattr("atst.domain.authnid.crl_check", lambda *args: True) cert = open("ssl/client-certs/atat.mil.crt").read() auth_context = AuthenticationContext( MockCRLCache(), "SUCCESS", DOD_SDN, cert diff --git a/tests/domain/authnid/test_crl.py b/tests/domain/authnid/test_crl.py index fcd371b3..34828567 100644 --- a/tests/domain/authnid/test_crl.py +++ b/tests/domain/authnid/test_crl.py @@ -4,7 +4,7 @@ import re import os import shutil from OpenSSL import crypto, SSL -from atst.domain.authnid.crl import Validator, CRLCache +from atst.domain.authnid.crl import crl_check, CRLCache, CRLException import atst.domain.authnid.crl.util as util @@ -39,18 +39,20 @@ def test_can_validate_certificate(): cache = CRLCache('ssl/server-certs/ca-chain.pem', crl_locations=['ssl/client-certs/client-ca.der.crl']) good_cert = open('ssl/client-certs/atat.mil.crt', 'rb').read() bad_cert = open('ssl/client-certs/bad-atat.mil.crt', 'rb').read() - assert Validator(cache, good_cert).validate() - assert Validator(cache, bad_cert).validate() == False + assert crl_check(cache, good_cert) + with pytest.raises(CRLException): + crl_check(cache, bad_cert) def test_can_dynamically_update_crls(tmpdir): crl_file = tmpdir.join('test.crl') shutil.copyfile('ssl/client-certs/client-ca.der.crl', crl_file) cache = CRLCache('ssl/server-certs/ca-chain.pem', crl_locations=[crl_file]) cert = open('ssl/client-certs/atat.mil.crt', 'rb').read() - assert Validator(cache, cert).validate() + assert crl_check(cache, cert) # override the original CRL with one that revokes atat.mil.crt shutil.copyfile('tests/fixtures/test.der.crl', crl_file) - assert Validator(cache, cert).validate() == False + with pytest.raises(CRLException): + assert crl_check(cache, cert) def test_parse_disa_pki_list(): with open('tests/fixtures/disa-pki.html') as disa: From c59f207227a5de9450ec14269b3243b77d805818 Mon Sep 17 00:00:00 2001 From: dandds Date: Thu, 16 Aug 2018 15:31:36 -0400 Subject: [PATCH 29/36] simpler CRL implementation; load as-need because we cannot marshal openssl objects in python --- atst/domain/authnid/crl/__init__.py | 43 +++++++++++++---------------- tests/domain/authnid/test_crl.py | 5 ++-- 2 files changed, 22 insertions(+), 26 deletions(-) diff --git a/atst/domain/authnid/crl/__init__.py b/atst/domain/authnid/crl/__init__.py index 5c53ffe9..da1d0b4d 100644 --- a/atst/domain/authnid/crl/__init__.py +++ b/atst/domain/authnid/crl/__init__.py @@ -4,9 +4,11 @@ import re import hashlib from OpenSSL import crypto, SSL + class CRLException(Exception): pass + def sha256_checksum(filename, block_size=65536): sha256 = hashlib.sha256() with open(filename, "rb") as f: @@ -39,13 +41,15 @@ class CRLCache(): ) def __init__(self, root_location, crl_locations=[], store_class=crypto.X509Store): - self.crl_cache = {} self.store_class = store_class + self.certificate_authorities = {} self._load_roots(root_location) - self._build_x509_stores(crl_locations) + self._build_crl_cache(crl_locations) + + def get_store(self, cert): + return self._build_store(cert.get_issuer().der()) def _load_roots(self, root_location): - self.certificate_authorities = {} with open(root_location, "rb") as f: for raw_ca in self._parse_roots(f.read()): ca = crypto.load_certificate(crypto.FILETYPE_PEM, raw_ca) @@ -54,21 +58,25 @@ class CRLCache(): def _parse_roots(self, root_str): return [match.group(0) for match in self._PEM_RE.finditer(root_str)] - def _build_x509_stores(self, crl_locations): - self.x509_stores = {} - for crl_path in crl_locations: - issuer, store = self._build_store(crl_path) - self.x509_stores[issuer] = store + def _build_crl_cache(self, crl_locations): + self.crl_cache = {} + for crl_location in crl_locations: + crl = self._load_crl(crl_location) + self.crl_cache[crl.get_issuer().der()] = crl_location - def _build_store(self, crl_location): + def _load_crl(self, crl_location): + with open(crl_location, "rb") as crl_file: + return crypto.load_crl(crypto.FILETYPE_ASN1, crl_file.read()) + + def _build_store(self, issuer): store = self.store_class() store.set_flags(crypto.X509StoreFlags.CRL_CHECK) + crl_location = self.crl_cache[issuer] with open(crl_location, "rb") as crl_file: crl = crypto.load_crl(crypto.FILETYPE_ASN1, crl_file.read()) - self.crl_cache[crl.get_issuer().der()] = (crl_location, sha256_checksum(crl_location)) store.add_crl(crl) store = self._add_certificate_chain_to_store(store, crl.get_issuer()) - return (crl.get_issuer().der(), store) + return store # this _should_ happen just twice for the DoD PKI (intermediary, root) but # theoretically it can build a longer certificate chain @@ -82,16 +90,3 @@ class CRLCache(): else: return self._add_certificate_chain_to_store(store, ca.get_issuer()) - def get_store(self, cert): - return self._check_cache(cert.get_issuer().der()) - - def _check_cache(self, issuer): - if issuer in self.crl_cache: - filename, checksum = self.crl_cache[issuer] - if sha256_checksum(filename) != checksum: - issuer, store = self._build_store(filename) - self.x509_stores[issuer] = store - return store - else: - return self.x509_stores[issuer] - diff --git a/tests/domain/authnid/test_crl.py b/tests/domain/authnid/test_crl.py index 34828567..8fdc74a4 100644 --- a/tests/domain/authnid/test_crl.py +++ b/tests/domain/authnid/test_crl.py @@ -22,11 +22,12 @@ class MockX509Store(): def set_flags(self, flag): pass + def test_can_build_crl_list(monkeypatch): location = 'ssl/client-certs/client-ca.der.crl' cache = CRLCache('ssl/client-certs/client-ca.crt', crl_locations=[location], store_class=MockX509Store) - for store in cache.x509_stores.values(): - assert len(store.crls) == 1 + assert len(cache.crl_cache.keys()) == 1 + def test_can_build_trusted_root_list(): location = 'ssl/server-certs/ca-chain.pem' From b80701e200bd3298d9b86980160b5ff14d3d7cbc Mon Sep 17 00:00:00 2001 From: dandds Date: Fri, 17 Aug 2018 10:48:49 -0400 Subject: [PATCH 30/36] more specific name for CRL revocation exception --- atst/domain/authnid/__init__.py | 4 ++-- atst/domain/authnid/crl/__init__.py | 4 ++-- tests/domain/authnid/test_crl.py | 6 +++--- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/atst/domain/authnid/__init__.py b/atst/domain/authnid/__init__.py index b31e00fb..46dc9445 100644 --- a/atst/domain/authnid/__init__.py +++ b/atst/domain/authnid/__init__.py @@ -1,7 +1,7 @@ from atst.domain.exceptions import UnauthenticatedError, NotFoundError from atst.domain.users import Users from .utils import parse_sdn, email_from_certificate -from .crl import crl_check, CRLException +from .crl import crl_check, CRLRevocationException class AuthenticationContext(): @@ -46,7 +46,7 @@ class AuthenticationContext(): def _crl_check(self): try: crl_check(self.crl_cache, self.cert) - except CRLException as exc: + except CRLRevocationException as exc: raise UnauthenticatedError("CRL check failed. " + str(exc)) @property diff --git a/atst/domain/authnid/crl/__init__.py b/atst/domain/authnid/crl/__init__.py index da1d0b4d..0dcf1fc0 100644 --- a/atst/domain/authnid/crl/__init__.py +++ b/atst/domain/authnid/crl/__init__.py @@ -5,7 +5,7 @@ import hashlib from OpenSSL import crypto, SSL -class CRLException(Exception): +class CRLRevocationException(Exception): pass @@ -26,7 +26,7 @@ def crl_check(cache, cert): return True except crypto.X509StoreContextError as err: - raise CRLException( + raise CRLRevocationException( "Certificate revoked or errored. Error: {}. Args: {}".format( type(err), err.args ) diff --git a/tests/domain/authnid/test_crl.py b/tests/domain/authnid/test_crl.py index 8fdc74a4..5bd009be 100644 --- a/tests/domain/authnid/test_crl.py +++ b/tests/domain/authnid/test_crl.py @@ -4,7 +4,7 @@ import re import os import shutil from OpenSSL import crypto, SSL -from atst.domain.authnid.crl import crl_check, CRLCache, CRLException +from atst.domain.authnid.crl import crl_check, CRLCache, CRLRevocationException import atst.domain.authnid.crl.util as util @@ -41,7 +41,7 @@ def test_can_validate_certificate(): good_cert = open('ssl/client-certs/atat.mil.crt', 'rb').read() bad_cert = open('ssl/client-certs/bad-atat.mil.crt', 'rb').read() assert crl_check(cache, good_cert) - with pytest.raises(CRLException): + with pytest.raises(CRLRevocationException): crl_check(cache, bad_cert) def test_can_dynamically_update_crls(tmpdir): @@ -52,7 +52,7 @@ def test_can_dynamically_update_crls(tmpdir): assert crl_check(cache, cert) # override the original CRL with one that revokes atat.mil.crt shutil.copyfile('tests/fixtures/test.der.crl', crl_file) - with pytest.raises(CRLException): + with pytest.raises(CRLRevocationException): assert crl_check(cache, cert) def test_parse_disa_pki_list(): From 504b138765b7580c999fabc27dd1507d9006877d Mon Sep 17 00:00:00 2001 From: dandds Date: Fri, 17 Aug 2018 11:02:20 -0400 Subject: [PATCH 31/36] handle case where certificate issuer is not in existing cache --- atst/domain/authnid/crl/__init__.py | 13 ++++- tests/domain/authnid/test_crl.py | 73 +++++++++++++++++++++-------- 2 files changed, 64 insertions(+), 22 deletions(-) diff --git a/atst/domain/authnid/crl/__init__.py b/atst/domain/authnid/crl/__init__.py index 0dcf1fc0..f1c93fb7 100644 --- a/atst/domain/authnid/crl/__init__.py +++ b/atst/domain/authnid/crl/__init__.py @@ -71,15 +71,24 @@ class CRLCache(): def _build_store(self, issuer): store = self.store_class() store.set_flags(crypto.X509StoreFlags.CRL_CHECK) - crl_location = self.crl_cache[issuer] + crl_location = self._get_crl_location(issuer) with open(crl_location, "rb") as crl_file: crl = crypto.load_crl(crypto.FILETYPE_ASN1, crl_file.read()) store.add_crl(crl) store = self._add_certificate_chain_to_store(store, crl.get_issuer()) return store + def _get_crl_location(self, issuer): + crl_location = self.crl_cache.get(issuer) + + if not crl_location: + raise CRLRevocationException("Could not find matching CRL for issuer") + + return crl_location + # this _should_ happen just twice for the DoD PKI (intermediary, root) but # theoretically it can build a longer certificate chain + def _add_certificate_chain_to_store(self, store, issuer): ca = self.certificate_authorities.get(issuer.der()) store.add_cert(ca) @@ -87,6 +96,6 @@ class CRLCache(): if issuer == ca.get_subject(): # i.e., it is the root CA and we are at the end of the chain return store + else: return self._add_certificate_chain_to_store(store, ca.get_issuer()) - diff --git a/tests/domain/authnid/test_crl.py b/tests/domain/authnid/test_crl.py index 5bd009be..19757353 100644 --- a/tests/domain/authnid/test_crl.py +++ b/tests/domain/authnid/test_crl.py @@ -4,11 +4,15 @@ import re import os import shutil from OpenSSL import crypto, SSL + from atst.domain.authnid.crl import crl_check, CRLCache, CRLRevocationException import atst.domain.authnid.crl.util as util +from tests.mocks import FIXTURE_EMAIL_ADDRESS + class MockX509Store(): + def __init__(self): self.crls = [] self.certs = [] @@ -24,46 +28,69 @@ class MockX509Store(): def test_can_build_crl_list(monkeypatch): - location = 'ssl/client-certs/client-ca.der.crl' - cache = CRLCache('ssl/client-certs/client-ca.crt', crl_locations=[location], store_class=MockX509Store) + location = "ssl/client-certs/client-ca.der.crl" + cache = CRLCache( + "ssl/client-certs/client-ca.crt", + crl_locations=[location], + store_class=MockX509Store, + ) assert len(cache.crl_cache.keys()) == 1 def test_can_build_trusted_root_list(): - location = 'ssl/server-certs/ca-chain.pem' - cache = CRLCache(root_location=location, crl_locations=[], store_class=MockX509Store) + location = "ssl/server-certs/ca-chain.pem" + cache = CRLCache( + root_location=location, crl_locations=[], store_class=MockX509Store + ) with open(location) as f: content = f.read() - assert len(cache.certificate_authorities.keys()) == content.count('BEGIN CERT') + assert len(cache.certificate_authorities.keys()) == content.count("BEGIN CERT") + def test_can_validate_certificate(): - cache = CRLCache('ssl/server-certs/ca-chain.pem', crl_locations=['ssl/client-certs/client-ca.der.crl']) - good_cert = open('ssl/client-certs/atat.mil.crt', 'rb').read() - bad_cert = open('ssl/client-certs/bad-atat.mil.crt', 'rb').read() + cache = CRLCache( + "ssl/server-certs/ca-chain.pem", + crl_locations=["ssl/client-certs/client-ca.der.crl"], + ) + good_cert = open("ssl/client-certs/atat.mil.crt", "rb").read() + bad_cert = open("ssl/client-certs/bad-atat.mil.crt", "rb").read() assert crl_check(cache, good_cert) with pytest.raises(CRLRevocationException): crl_check(cache, bad_cert) + def test_can_dynamically_update_crls(tmpdir): - crl_file = tmpdir.join('test.crl') - shutil.copyfile('ssl/client-certs/client-ca.der.crl', crl_file) - cache = CRLCache('ssl/server-certs/ca-chain.pem', crl_locations=[crl_file]) - cert = open('ssl/client-certs/atat.mil.crt', 'rb').read() + crl_file = tmpdir.join("test.crl") + shutil.copyfile("ssl/client-certs/client-ca.der.crl", crl_file) + cache = CRLCache("ssl/server-certs/ca-chain.pem", crl_locations=[crl_file]) + cert = open("ssl/client-certs/atat.mil.crt", "rb").read() assert crl_check(cache, cert) # override the original CRL with one that revokes atat.mil.crt - shutil.copyfile('tests/fixtures/test.der.crl', crl_file) + shutil.copyfile("tests/fixtures/test.der.crl", crl_file) with pytest.raises(CRLRevocationException): assert crl_check(cache, cert) + +def test_throws_error_for_missing_issuer(): + cache = CRLCache("ssl/server-certs/ca-chain.pem", crl_locations=[]) + cert = open("tests/fixtures/{}.crt".format(FIXTURE_EMAIL_ADDRESS), "rb").read() + with pytest.raises(CRLRevocationException) as exc: + assert crl_check(cache, cert) + (message,) = exc.value.args + assert "issuer" in message + + def test_parse_disa_pki_list(): - with open('tests/fixtures/disa-pki.html') as disa: + with open("tests/fixtures/disa-pki.html") as disa: disa_html = disa.read() crl_list = util.crl_list_from_disa_html(disa_html) - href_matches = re.findall('DOD(ROOT|EMAIL|ID)?CA', disa_html) + href_matches = re.findall("DOD(ROOT|EMAIL|ID)?CA", disa_html) assert len(crl_list) > 0 assert len(crl_list) == len(href_matches) + class MockStreamingResponse(): + def __init__(self, content_chunks, code=200): self.content_chunks = content_chunks self.status_code = code @@ -77,13 +104,19 @@ class MockStreamingResponse(): def __exit__(self, *args): pass + def test_write_crl(tmpdir, monkeypatch): - monkeypatch.setattr('requests.get', lambda u, **kwargs: MockStreamingResponse([b'it worked'])) - crl = 'crl_1' + monkeypatch.setattr( + "requests.get", lambda u, **kwargs: MockStreamingResponse([b"it worked"]) + ) + crl = "crl_1" assert util.write_crl(tmpdir, "random_target_dir", crl) assert [p.basename for p in tmpdir.listdir()] == [crl] - assert [p.read() for p in tmpdir.listdir()] == ['it worked'] + assert [p.read() for p in tmpdir.listdir()] == ["it worked"] + def test_skips_crl_if_it_has_not_been_modified(tmpdir, monkeypatch): - monkeypatch.setattr('requests.get', lambda u, **kwargs: MockStreamingResponse([b'it worked'], 304)) - assert not util.write_crl(tmpdir, "random_target_dir", 'crl_file_name') + monkeypatch.setattr( + "requests.get", lambda u, **kwargs: MockStreamingResponse([b"it worked"], 304) + ) + assert not util.write_crl(tmpdir, "random_target_dir", "crl_file_name") From f5cc9daee9609d4f694a63df51ebcbb6c96631be Mon Sep 17 00:00:00 2001 From: dandds Date: Fri, 17 Aug 2018 11:03:27 -0400 Subject: [PATCH 32/36] remove unused checksum function --- atst/domain/authnid/crl/__init__.py | 8 -------- 1 file changed, 8 deletions(-) diff --git a/atst/domain/authnid/crl/__init__.py b/atst/domain/authnid/crl/__init__.py index f1c93fb7..b43b5a8e 100644 --- a/atst/domain/authnid/crl/__init__.py +++ b/atst/domain/authnid/crl/__init__.py @@ -9,14 +9,6 @@ class CRLRevocationException(Exception): pass -def sha256_checksum(filename, block_size=65536): - sha256 = hashlib.sha256() - with open(filename, "rb") as f: - for block in iter(lambda: f.read(block_size), b""): - sha256.update(block) - return sha256.hexdigest() - - def crl_check(cache, cert): parsed = crypto.load_certificate(crypto.FILETYPE_PEM, cert) store = cache.get_store(parsed) From 7fb407a3b1ab8bd690bd7d26e285199a40c35ed6 Mon Sep 17 00:00:00 2001 From: dandds Date: Fri, 17 Aug 2018 11:13:01 -0400 Subject: [PATCH 33/36] make crl_check a CRLCache method --- atst/domain/authnid/__init__.py | 4 +-- atst/domain/authnid/crl/__init__.py | 33 +++++++++-------- .../authnid/test_authentication_context.py | 36 +++++++++---------- tests/domain/authnid/test_crl.py | 12 +++---- 4 files changed, 41 insertions(+), 44 deletions(-) diff --git a/atst/domain/authnid/__init__.py b/atst/domain/authnid/__init__.py index 46dc9445..b7f31717 100644 --- a/atst/domain/authnid/__init__.py +++ b/atst/domain/authnid/__init__.py @@ -1,7 +1,7 @@ from atst.domain.exceptions import UnauthenticatedError, NotFoundError from atst.domain.users import Users from .utils import parse_sdn, email_from_certificate -from .crl import crl_check, CRLRevocationException +from .crl import CRLRevocationException class AuthenticationContext(): @@ -45,7 +45,7 @@ class AuthenticationContext(): def _crl_check(self): try: - crl_check(self.crl_cache, self.cert) + self.crl_cache.crl_check(self.cert) except CRLRevocationException as exc: raise UnauthenticatedError("CRL check failed. " + str(exc)) diff --git a/atst/domain/authnid/crl/__init__.py b/atst/domain/authnid/crl/__init__.py index b43b5a8e..cad27b79 100644 --- a/atst/domain/authnid/crl/__init__.py +++ b/atst/domain/authnid/crl/__init__.py @@ -9,22 +9,6 @@ class CRLRevocationException(Exception): pass -def crl_check(cache, cert): - parsed = crypto.load_certificate(crypto.FILETYPE_PEM, cert) - store = cache.get_store(parsed) - context = crypto.X509StoreContext(store, parsed) - try: - context.verify_certificate() - return True - - except crypto.X509StoreContextError as err: - raise CRLRevocationException( - "Certificate revoked or errored. Error: {}. Args: {}".format( - type(err), err.args - ) - ) - - class CRLCache(): _PEM_RE = re.compile( @@ -38,7 +22,7 @@ class CRLCache(): self._load_roots(root_location) self._build_crl_cache(crl_locations) - def get_store(self, cert): + def _get_store(self, cert): return self._build_store(cert.get_issuer().der()) def _load_roots(self, root_location): @@ -91,3 +75,18 @@ class CRLCache(): else: return self._add_certificate_chain_to_store(store, ca.get_issuer()) + + def crl_check(self, cert): + parsed = crypto.load_certificate(crypto.FILETYPE_PEM, cert) + store = self._get_store(parsed) + context = crypto.X509StoreContext(store, parsed) + try: + context.verify_certificate() + return True + + except crypto.X509StoreContextError as err: + raise CRLRevocationException( + "Certificate revoked or errored. Error: {}. Args: {}".format( + type(err), err.args + ) + ) diff --git a/tests/domain/authnid/test_authentication_context.py b/tests/domain/authnid/test_authentication_context.py index d7c8e157..81a9b5d3 100644 --- a/tests/domain/authnid/test_authentication_context.py +++ b/tests/domain/authnid/test_authentication_context.py @@ -1,7 +1,7 @@ import pytest from atst.domain.authnid import AuthenticationContext -from atst.domain.authnid.crl import CRLCache +from atst.domain.authnid.crl import CRLCache, CRLRevocationException from atst.domain.exceptions import UnauthenticatedError, NotFoundError from atst.domain.users import Users @@ -12,20 +12,24 @@ CERT = open("tests/fixtures/{}.crt".format(FIXTURE_EMAIL_ADDRESS)).read() class MockCRLCache(): - def get_store(self, cert): - pass + def __init__(self, valid=True): + self.valid = valid + + def crl_check(self, cert): + if self.valid: + return True + + raise CRLRevocationException() -def test_can_authenticate(monkeypatch): - monkeypatch.setattr("atst.domain.authnid.crl_check", lambda *args: True) +def test_can_authenticate(): auth_context = AuthenticationContext( MockCRLCache(), "SUCCESS", DOD_SDN, CERT ) assert auth_context.authenticate() -def test_unsuccessful_status(monkeypatch): - monkeypatch.setattr("atst.domain.authnid.crl_check", lambda *args: True) +def test_unsuccessful_status(): auth_context = AuthenticationContext( MockCRLCache(), "FAILURE", DOD_SDN, CERT ) @@ -36,11 +40,9 @@ def test_unsuccessful_status(monkeypatch): assert "client authentication" in message -def test_crl_check_fails(monkeypatch): - cache = CRLCache('ssl/client-certs/client-ca.crt', crl_locations=['ssl/client-certs/client-ca.der.crl']) - cert = open("ssl/client-certs/bad-atat.mil.crt", "r").read() +def test_crl_check_fails(): auth_context = AuthenticationContext( - cache, "SUCCESS", DOD_SDN, cert + MockCRLCache(False), "SUCCESS", DOD_SDN, CERT ) with pytest.raises(UnauthenticatedError) as excinfo: assert auth_context.authenticate() @@ -49,8 +51,7 @@ def test_crl_check_fails(monkeypatch): assert "CRL check" in message -def test_bad_sdn(monkeypatch): - monkeypatch.setattr("atst.domain.authnid.crl_check", lambda *args: True) +def test_bad_sdn(): auth_context = AuthenticationContext( MockCRLCache(), "SUCCESS", "abc123", CERT ) @@ -61,8 +62,7 @@ def test_bad_sdn(monkeypatch): assert "SDN" in message -def test_user_exists(monkeypatch): - monkeypatch.setattr("atst.domain.authnid.crl_check", lambda *args: True) +def test_user_exists(): user = UserFactory.create(**DOD_SDN_INFO) auth_context = AuthenticationContext( MockCRLCache(), "SUCCESS", DOD_SDN, CERT @@ -72,8 +72,7 @@ def test_user_exists(monkeypatch): assert auth_user == user -def test_creates_user(monkeypatch): - monkeypatch.setattr("atst.domain.authnid.crl_check", lambda *args: True) +def test_creates_user(): # check user does not exist with pytest.raises(NotFoundError): Users.get_by_dod_id(DOD_SDN_INFO["dod_id"]) @@ -86,8 +85,7 @@ def test_creates_user(monkeypatch): assert user.email == FIXTURE_EMAIL_ADDRESS -def test_user_cert_has_no_email(monkeypatch): - monkeypatch.setattr("atst.domain.authnid.crl_check", lambda *args: True) +def test_user_cert_has_no_email(): cert = open("ssl/client-certs/atat.mil.crt").read() auth_context = AuthenticationContext( MockCRLCache(), "SUCCESS", DOD_SDN, cert diff --git a/tests/domain/authnid/test_crl.py b/tests/domain/authnid/test_crl.py index 19757353..7ba3a213 100644 --- a/tests/domain/authnid/test_crl.py +++ b/tests/domain/authnid/test_crl.py @@ -5,7 +5,7 @@ import os import shutil from OpenSSL import crypto, SSL -from atst.domain.authnid.crl import crl_check, CRLCache, CRLRevocationException +from atst.domain.authnid.crl import CRLCache, CRLRevocationException import atst.domain.authnid.crl.util as util from tests.mocks import FIXTURE_EMAIL_ADDRESS @@ -54,9 +54,9 @@ def test_can_validate_certificate(): ) good_cert = open("ssl/client-certs/atat.mil.crt", "rb").read() bad_cert = open("ssl/client-certs/bad-atat.mil.crt", "rb").read() - assert crl_check(cache, good_cert) + assert cache.crl_check(good_cert) with pytest.raises(CRLRevocationException): - crl_check(cache, bad_cert) + cache.crl_check(bad_cert) def test_can_dynamically_update_crls(tmpdir): @@ -64,18 +64,18 @@ def test_can_dynamically_update_crls(tmpdir): shutil.copyfile("ssl/client-certs/client-ca.der.crl", crl_file) cache = CRLCache("ssl/server-certs/ca-chain.pem", crl_locations=[crl_file]) cert = open("ssl/client-certs/atat.mil.crt", "rb").read() - assert crl_check(cache, cert) + assert cache.crl_check(cert) # override the original CRL with one that revokes atat.mil.crt shutil.copyfile("tests/fixtures/test.der.crl", crl_file) with pytest.raises(CRLRevocationException): - assert crl_check(cache, cert) + assert cache.crl_check(cert) def test_throws_error_for_missing_issuer(): cache = CRLCache("ssl/server-certs/ca-chain.pem", crl_locations=[]) cert = open("tests/fixtures/{}.crt".format(FIXTURE_EMAIL_ADDRESS), "rb").read() with pytest.raises(CRLRevocationException) as exc: - assert crl_check(cache, cert) + assert cache.crl_check(cert) (message,) = exc.value.args assert "issuer" in message From deaf4eb5b6367a3686881d6eeff6e565a8505696 Mon Sep 17 00:00:00 2001 From: dandds Date: Fri, 17 Aug 2018 16:05:49 -0400 Subject: [PATCH 34/36] additional logging to CRL checks --- atst/app.py | 2 +- atst/domain/authnid/crl/__init__.py | 10 +++++++++- 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/atst/app.py b/atst/app.py index ddf66a40..d8a781ac 100644 --- a/atst/app.py +++ b/atst/app.py @@ -141,5 +141,5 @@ def make_crl_validator(app): crl_locations = [] for filename in pathlib.Path(app.config["CRL_DIRECTORY"]).glob("*"): crl_locations.append(filename.absolute()) - app.crl_cache = CRLCache(app.config["CA_CHAIN"], crl_locations) + app.crl_cache = CRLCache(app.config["CA_CHAIN"], crl_locations, logger=app.logger) diff --git a/atst/domain/authnid/crl/__init__.py b/atst/domain/authnid/crl/__init__.py index cad27b79..64594a93 100644 --- a/atst/domain/authnid/crl/__init__.py +++ b/atst/domain/authnid/crl/__init__.py @@ -16,11 +16,16 @@ class CRLCache(): re.DOTALL, ) - def __init__(self, root_location, crl_locations=[], store_class=crypto.X509Store): + def __init__(self, root_location, crl_locations=[], store_class=crypto.X509Store, logger=None): self.store_class = store_class self.certificate_authorities = {} self._load_roots(root_location) self._build_crl_cache(crl_locations) + self.logger = logger + + def log_info(self, message): + if self.logger: + self.logger.info(message) def _get_store(self, cert): return self._build_store(cert.get_issuer().der()) @@ -46,11 +51,13 @@ class CRLCache(): def _build_store(self, issuer): store = self.store_class() + self.log_info("STORE ID: {}. Building store.".format(id(store))) store.set_flags(crypto.X509StoreFlags.CRL_CHECK) crl_location = self._get_crl_location(issuer) with open(crl_location, "rb") as crl_file: crl = crypto.load_crl(crypto.FILETYPE_ASN1, crl_file.read()) store.add_crl(crl) + self.log_info("STORE ID: {}. Adding CRL with issuer {}".format(id(store), crl.get_issuer())) store = self._add_certificate_chain_to_store(store, crl.get_issuer()) return store @@ -68,6 +75,7 @@ class CRLCache(): def _add_certificate_chain_to_store(self, store, issuer): ca = self.certificate_authorities.get(issuer.der()) store.add_cert(ca) + self.log_info("STORE ID: {}. Adding CA with subject {}".format(id(store), ca.get_subject())) if issuer == ca.get_subject(): # i.e., it is the root CA and we are at the end of the chain From 81268736a148964c48e05354bad1dc3f5c6cdf2d Mon Sep 17 00:00:00 2001 From: luis cielak Date: Mon, 20 Aug 2018 10:24:07 -0400 Subject: [PATCH 35/36] Update placeholder example to match valid regex --- templates/requests/financial_verification.html | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/templates/requests/financial_verification.html b/templates/requests/financial_verification.html index c45a0889..71057795 100644 --- a/templates/requests/financial_verification.html +++ b/templates/requests/financial_verification.html @@ -49,13 +49,13 @@ ) }} {{ TextInput(f.pe_id, - placeholder="e.g.: 0203752A", + placeholder="e.g.: 0302400A", tooltip="Program Element numbers helps the Department of Defense identify which offices\\' budgets are contributing towards this resource use." ) }} - {{ TextInput(f.treasury_code,placeholder="e.g.: 1200") }} + {{ TextInput(f.treasury_code,placeholder="e.g.: 00123456") }} - {{ TextInput(f.ba_code,placeholder="e.g.: 02") }} + {{ TextInput(f.ba_code,placeholder="e.g.: 02A") }}

Contracting Officer (KO) Information

{{ TextInput(f.fname_co,placeholder="Contracting Officer First Name") }} From 78e9cdd82c5617390a91c5b7dcda6512fbe17df8 Mon Sep 17 00:00:00 2001 From: dandds Date: Mon, 20 Aug 2018 10:56:29 -0400 Subject: [PATCH 36/36] use SERVICE_BRANCHES constant for requests factory --- tests/factories.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/tests/factories.py b/tests/factories.py index a47b38ab..a6370060 100644 --- a/tests/factories.py +++ b/tests/factories.py @@ -3,6 +3,7 @@ import string import factory from uuid import uuid4 +from atst.forms.data import SERVICE_BRANCHES from atst.models.request import Request from atst.models.request_status_event import RequestStatusEvent, RequestStatus from atst.models.pe_number import PENumber @@ -66,7 +67,7 @@ class RequestFactory(factory.alchemy.SQLAlchemyModelFactory): "start_date": "2018-08-08", "cloud_native": "yes", "dollar_value": dollar_value, - "dod_component": "Army and Air Force Exchange Service", + "dod_component": SERVICE_BRANCHES[2][1], "data_transfers": "Less than 100GB", "expected_completion_date": "Less than 1 month", "jedi_migration": "yes", @@ -87,7 +88,7 @@ class RequestFactory(factory.alchemy.SQLAlchemyModelFactory): "email_request": user.email, "fname_request": user.first_name, "lname_request": user.last_name, - "service_branch": "Air Force, Department of the", + "service_branch": SERVICE_BRANCHES[1][1], "date_latest_training": "2018-08-06", }, }