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

Support developer authentication flow in bananas-cli #20

Closed
wants to merge 1 commit into from

Conversation

@erenes
Copy link

@erenes erenes commented Apr 5, 2021

While setting up my dev environment for bananas, I had trouble authenticating to the API.

  • I've allowed the audience to be configurable (kept the default to be github).
  • The postback to get the token was always requested with the hardcoded client_id ape, so I made that configurable as well.
  • If we get a success code, but the mime type is text/html, I assume it should have been a 302 with the request URL instead. Parsing the response as json will definitely crash, so it will not deteriorate the experience very much. Ideally the bananas-api would actually return the 302, but on that side I did not see a straightforward way to make a difference between an automated request from the cli and a browser request (perhaps that will come as my python skills increase).
  • Finally, added .env to .gitignore for obvious reasons ;)
@erenes erenes force-pushed the oauth-developer branch from 64f4214 to 8141f50 Apr 6, 2021
Copy link
Member

@TrueBrain TrueBrain left a comment

This really should be 4 commits. I don't really mind it is in a single PR, but I do mind it is in a single commit. That makes it really difficult for future-us to understand why something was done, basically :)

For example, the .env change has nothing to do with developer flow. So that is confusing in a git blame scenario.

So if you don't mind splitting this up in a few commits, that would be really appreciated :)

@@ -1,2 +1,3 @@
__pycache__/
*.pyc
.env/
Copy link
Member

@TrueBrain TrueBrain Apr 12, 2021

Choose a reason for hiding this comment

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

Please make this /.env/, as not to ignore any .env in any folder :)

@@ -27,6 +27,8 @@ async def stop(self):

async def _read_response(self, response):
if response.status in (200, 201, 400, 404):
if response.content_type == "text/html":
return 302, response.url
Copy link
Member

@TrueBrain TrueBrain Apr 12, 2021

Choose a reason for hiding this comment

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

I need a bit more detail what this does and why it is needed. For sure, if it is correct, it requires a comment in code explaining why this is there. But it feels off .. like something else is wrong :)

Copy link
Author

@erenes erenes Apr 12, 2021

Choose a reason for hiding this comment

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

With github mode, bananas-api will redirect the user to the login page:
https://github.com/OpenTTD/bananas-api/blob/7e9f4cf8c79cd6954607c8e43595d3c4d27e516e/bananas_api/user/github.py#L48

However, in developer mode, bananas-api will just return a login form:
https://github.com/OpenTTD/bananas-api/blob/7e9f4cf8c79cd6954607c8e43595d3c4d27e516e/bananas_api/user/developer.py#L16

The CLI tool expects to be redirected in case the request was succesful, so this is what is being simulated here.

The proper solution would be to also redirect in developer mode. Once I get more comfortable writing Python and with the bananas-api project I may be able to fix that, but not right now. I could create an issue in bananas-api and refer to that in a comment here perhaps?

@erenes
Copy link
Author

@erenes erenes commented Apr 18, 2021

Because of OpenTTD/bananas-api#88 this PR is no longer up to date, I'll send a new one somewhere next week when I have time to re-do this one taking the review comments into account.

@erenes erenes closed this Apr 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants