Minor refactor on role/group code. #54
Conversation
| @@ -588,8 +591,6 @@ def _get_roles_from_saml_etree(self, signed_xml): | |||
| return xpath_result | |||
|
|
|||
| self.log.warning('Could not find role from role XPath') | |||
| else: | |||
| self.log.warning('Role XPath not set') | |||
There was a problem hiding this comment.
This was a dead branch because this issue was taken care of on (new) line 666.
|
|
||
| def _check_role(self, user_roles): | ||
| if self.allowed_roles: |
There was a problem hiding this comment.
This check was removed because it is now taken care of by (new) line 662.
| user_roles_result = self._check_role(user_roles) | ||
| if not user_roles_result: | ||
| self.log.error('User role not authorized') | ||
| return None |
There was a problem hiding this comment.
This logic was moved into _valid_config_and_roles.
| return False | ||
| # Failed to validate username or failed list check | ||
| self.log.error('Failed to validate username or failed list check') | ||
| return None |
There was a problem hiding this comment.
Instead of having this function return a boolean, I think it's more literate to have the function return str or None, and return this function (new line 696).
Codecov Report
@@ Coverage Diff @@
## master #54 +/- ##
==========================================
+ Coverage 91.75% 92.05% +0.29%
==========================================
Files 2 2
Lines 376 390 +14
==========================================
+ Hits 345 359 +14
Misses 31 31
Continue to review full report at Codecov.
|
|
Hey @0nebody I'd appreciate if you could take a look at this and give feedback. I'll merge this in a week if I don't hear back from you. |
| return username | ||
| if self._valid_config_and_roles(signed_xml, saml_doc_etree): | ||
| self.log.debug('Optionally create and return user: ' + username) | ||
| return self._check_username_and_add_user(username) |
There was a problem hiding this comment.
I like this changes to this function. Its easier to see the checks performed to authenticate a user.
This is a minor refactor on the role/group code, and a little on the code that returns the username as well.
I was thinking about this code over the weekend, and I realized that, even if there was no way for the user to log in via groups, we would still run the XPath to get their groups from the SAML Response. I would prefer that we only run the XPath if we have to.
There was some collateral damage to the final code that checks if the user needs to be created in the system and returns the username.
Signed-off-by: Thomas Kelley distortedsignal@gmail.com