-
Notifications
You must be signed in to change notification settings - Fork 43
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
Openidc #188
Openidc #188
Conversation
fedora/client/openidcclient.py
Outdated
data={'client_id': self.client_id, | ||
'client_secret': self.client_secret, | ||
'token': token['access_token']}) | ||
# TODO: Be a bit more stable in handling if we get a non-JSON object |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know this is a work-in-progress that you haven't tested and kindly opened for me, but here's some drive-by feedback
I'd recommend doing something like
# Throw an HTTPError-based exception for a HTTP 400+ code
resp.raise_for_status()
try:
return resp.json()
except JSONDecodeError:
raise SomeExceptionThatTellsUsersTheServerShouldHaveGivenUsJSON
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like @jeremycline's suggestion 👍
Just FYI, module-build-service is blocking on this PR over here: https://pagure.io/fm-orchestrator/issue/268 |
fedora/client/openidcclient.py
Outdated
@@ -0,0 +1,385 @@ | |||
#!/usr/bin/env python2 -tt |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bad idea to have shebangs (especially python2's ;))
testopenidc.py
Outdated
|
||
inst = OpenIDCBaseClient('http://myapp.com', 'myapp', id_provider='http://172.17.0.2:8080/openidc/') | ||
|
||
print inst.get_token(['ipsilon'], False) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't it be print functions?
fedora/client/openidcclient.py
Outdated
|
||
# Just return a message | ||
out = StringIO() | ||
print >>out, "You can close this window and return to the CLI" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
py2.. meh
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
too much py2-only stuff =(
fedora/client/openidcclient.py
Outdated
query['redirect_uri'] = return_uri | ||
query['state'] = 'ignored' | ||
query['response_mode'] = 'query' | ||
query = urllib.urlencode(query) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not gonna work on py3.. it's in urllib.parse there. you can use six.moves to get urllib_parse
f8e5b57
to
fbdb27b
Compare
LGTM 👍 |
Signed-off-by: Patrick Uiterwijk <puiterwijk@redhat.com>
No description provided.