Moved sector_identifier checks outside #321
Conversation
src/oic/oic/provider.py
Outdated
| _cinfo["sector_id"] = si_url | ||
| _cinfo["si_redirects"], _cinfo["sector_id"] = self._verify_sector_identifier(request) | ||
| except InvalidSectorIdentifier as err: | ||
| return error_response("invalid_configuration_parameter", |
There was a problem hiding this comment.
Suppose we can make this all on one line? Could be nitpicking ;)
src/oic/oic/provider.py
Outdated
| pass | ||
| n_keys = len(self.keyjar.get(client_id, [])) | ||
| if n_keys: | ||
| logger.debug("Found %s keys for client_id=%s", n_keys, client_id) | ||
| except Exception as err: |
There was a problem hiding this comment.
Can we be more specific than Exception for the self.keyjar.load_keys(request, client_id) failure when we catch?
There was a problem hiding this comment.
We should, I will look into this.
There was a problem hiding this comment.
Mental note to myself: KeyIOError and UnknownKeyType are clearly visible as raised exceptions from called methods. Other could be possibly hidden.
src/oic/oic/provider.py
Outdated
| try: | ||
| res = self.server.http_request(si_url) | ||
| except ConnectionError as err: | ||
| logger.error("{}", err) |
There was a problem hiding this comment.
Can just be logger.error(err)?
src/oic/oic/provider.py
Outdated
| logger.error("{}", err) | ||
| res = None | ||
|
|
||
| if not res: |
There was a problem hiding this comment.
This raise could be moved into the except block above?
There was a problem hiding this comment.
Probably not, if you look at the previous code, it was doing the same thing. This catches the case when the response doesn't raise an exception but returns None.
src/oic/oic/provider.py
Outdated
| raise InvalidSectorIdentifier("Error deserializing sector_identifier_uri content") | ||
|
|
||
| if "redirect_uris" in request: | ||
| logger.debug("redirect_uris: %s". request["redirect_uris"]) |
There was a problem hiding this comment.
What's that full stop doing!? Is this some new cool syntax that I didn't know about? 😄
There was a problem hiding this comment.
Typos... typos everywhere...
src/oic/oic/provider.py
Outdated
|
|
||
| if "redirect_uris" in request: | ||
| logger.debug("redirect_uris: %s". request["redirect_uris"]) | ||
| for uri in request["redirect_uris"]: |
There was a problem hiding this comment.
Might be just my preference but I like to skip nested fors (feel free to ignore):
if not all([uri in request["redirect_uris"] for uri in si_redirects]):
raise InvalidSectorIdentifier("redirect_uri missing from sector_identifiers")There was a problem hiding this comment.
I always find dict comprehension unreadable (personal thing) so I will gladly go with the ignore on this one :)
|
Thanks for the review, but I will probably work on this one a little bit anyways. Since the code I am refactoring is completely untested, I would like to add some test to be sure it is working properly. I will address some of your comments in further commits :) |
🍻 🌈 🍷 🏳️🌈 🍻 |
tpazderka
force-pushed
the
refactor-do-client-registration
branch
from
ecd0808
to
b251b08
Compare
tpazderka
changed the title
|
Tests for the method added. It is marked as internal since this is something that shouldn't be changed. The score went down by |
|
@lwm, could you please have a look so we can merge this? |
Refactoring of
oic.oic.provider.Provider.do_clinet_registrationin order to decrease McCabe score.CHANGELOG.md.