nautilus: mgr/dashboard: fix error when enabling SSO with cert. file #34129
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
epuertat
approved these changes
Mar 23, 2020
| @@ -186,13 +187,13 @@ def handle_sso_command(cmd): | |||
| # pylint: disable=redefined-builtin | |||
| FileNotFoundError = IOError | |||
| try: | |||
| f = open(sp_x_509_cert, 'r') | |||
| f = open(sp_x_509_cert, 'r', encoding='utf-8') if six.PY3 else open(sp_x_509_cert, 'rb') | |||
There was a problem hiding this comment.
2 alternatives:
- From this:
from io import open # py27 - py3 compatible
open(sp_x_509_cert, 'r') # io.open 'encoding' defaults to 'UTF-8'
>>> <_io.TextIOWrapper name='/tmp/ycm_0lpood4k.log' encoding='utf-8'>- Alternatively, what about a more compact form?
Suggested change
| f = open(sp_x_509_cert, 'r', encoding='utf-8') if six.PY3 else open(sp_x_509_cert, 'rb') | |
| f = open(sp_x_509_cert, 'r', **{'encoding':'utf-8'} if six.PY3 else {}) |
There was a problem hiding this comment.
I tried this solution (from io) but in this case it does not work for py2. i.e. later idp_metadata parsing crashes.
Comment on lines
190
to
196
| f = open(sp_x_509_cert, 'r', encoding='utf-8') if six.PY3 else open(sp_x_509_cert, 'rb') | ||
| sp_x_509_cert = f.read() | ||
| f.close() | ||
| except FileNotFoundError: | ||
| pass | ||
| try: | ||
| f = open(sp_private_key, 'r') | ||
| f = open(sp_private_key, 'r', encoding='utf-8') if six.PY3 else open(sp_private_key, 'rb') |
There was a problem hiding this comment.
I understand you don't want to change this too much compared to master/octopus, but I wanted to bring in the context manager form:
with open() as f:
sp_private_key = f.read()
# no need to explicitly closeThere was a problem hiding this comment.
Provided it's a bugfix, I prefer to introduce the minimal changes in code structure.
Nautilus dedicated fix: added py2 compatibility code. Also: * Disabled security setting 'wantNameIdEncrypted': not all Identity Providers support this and we are already requiring encrypted assertions (which is the default). Fixes: https://tracker.ceph.com/issues/44666 Signed-off-by: Alfonso Martínez <almartin@redhat.com>
alfonsomthd
force-pushed
the
nautilus-44666-fix-sso-certificate-file
branch
from
d6aac21
to
3dbadfd
Compare
smithfarm
changed the title
|
jenkins test make check |
1 similar comment
|
jenkins test make check |
i was testing the python3 changes in qa/ directory along with this change, so there were lots of noises. but none of the failures were related to this change. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Since in master the solution is pure py3 code,
instead of backporting I've created this commit that provides py2 compatibility code.
Fixes: https://tracker.ceph.com/issues/44666
Checklist
Show available Jenkins commands
jenkins retest this pleasejenkins test crimson perfjenkins test signedjenkins test make checkjenkins test make check arm64jenkins test submodulesjenkins test dashboardjenkins test dashboard backendjenkins test docsjenkins render docsjenkins test ceph-volume alljenkins test ceph-volume tox