Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Customizable forms field names #31

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

mateuszboryn
Copy link

Customizable forms field names: user can specify non-default names of fields that are in ADFS HTML form.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@codecov-io
Copy link

codecov-io commented Jul 2, 2019

Codecov Report

Merging #31 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #31      +/-   ##
==========================================
+ Coverage    98.1%   98.11%   +<.01%     
==========================================
  Files           5        5              
  Lines         317      318       +1     
  Branches       42       42              
==========================================
+ Hits          311      312       +1     
  Misses          3        3              
  Partials        3        3
Impacted Files Coverage Δ
awsprocesscreds/saml.py 100% <100%> (ø) ⬆️
awsprocesscreds/cli.py 96.29% <100%> (+0.14%) ⬆️
awsprocesscreds/__init__.py 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a216e70...810c9d5. Read the comment docs.

@@ -177,7 +178,7 @@ def _fill_in_form_values(self, config, form_data):
username = config['saml_username']
if self.USERNAME_FIELD not in form_data:
raise SAMLError(
self._ERROR_MISSING_FORM_FIELD % self.USERNAME_FIELD)
self._ERROR_MISSING_FORM_FIELD % (self.USERNAME_FIELD, ", ".join(form_data.keys())))
else:
form_data[self.USERNAME_FIELD] = username
if self.PASSWORD_FIELD in form_data:
Copy link

Choose a reason for hiding this comment

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

The password field needs a field validation check too.

Copy link
Author

Choose a reason for hiding this comment

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

Password field may not always be present in the form, for example, when user is remembered.
This is also taken into account in already-existing unit tests which explicitly check if missing password field will not break the program.

Copy link

Choose a reason for hiding this comment

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

Ah, fair enough - I didn't see that test. I was merely relaying my experience of mistyping the password field and not getting an error that it was missing on the form.

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

Successfully merging this pull request may close these issues.

None yet

3 participants