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

Added OAuth for Web (when you can keep the secret safe) #124

Open
wants to merge 20 commits into
base: master
Choose a base branch
from

Conversation

GoldenGnu
Copy link
Collaborator

No description provided.

Copy link
Owner

@burberius burberius left a comment

Choose a reason for hiding this comment

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

After a lot of thinking back and forth, in my opinion your solution is the best way to go!
Make the old methods deprecated and throw an exception, to make the users change to the new ones. In a later version we can finally remove them.
Please add comments to the authWeb/authDesktop in the Builder and perhaps also add a class comment that points to the documentation for the "new way".
The builder is the entrance point for the user of the library.

We should create two test classes that show the new implementation ways for web and desktop or document that in a code example in the Readme.md or perhaps create two wiki pages or Readmes that document it. I would try to go the test way, but I'm not into the flows currently and don't know if it is actually possible to create tests for it! Then just link to that tests in the readme.

@GoldenGnu GoldenGnu mentioned this pull request Oct 26, 2021
5 tasks
- Suggested changes
- Added test_config.properties to store the test config, as refresh tokens will be rotating soon. Environment variables will no longer work, as they can not be updated
- Fixed a bug in the Web Flow
@GoldenGnu
Copy link
Collaborator Author

Okay, this is ready for another review.
I also started on the readme, but, I'm really bad at writing stuff like that, so feedback so most welcome!

@GoldenGnu
Copy link
Collaborator Author

Quick'ish recap:

  • How do we safely store the test refresh tokens
  • Should we validate the JWT?
    https://jwt.io have a ton of libraries for Java.
    However, I don't see any use cases for eve-esi, where verifying the access token would be required.
    • Use a library to handle the verifying automatically in eve-esi
    • Refer people to do it themselves.
  • Last and probably least important is the new SSO rate limit for failed requests.
    The Retry-After header specify when It's allowed to retry a failed request.

Comment on lines 2 to 6
SSO_REFRESH_TOKEN_PUBLIC_DATA=lHpAdmZPNU6JdmvLx/QJlA\=\=
SSO_CLIENT_SECRET=fwUmlKCkuyxfTRAtxe0MMCs6U9tz1NSBQCMiJ1Zq
SSO_REFRESH_TOKEN_WEB=spPlRlScqEKoJmSuLhrLSA\=\=
SSO_REFRESH_TOKEN_NATIVE=fJvG7VxoNEmAAcajcIcNcA\=\=
SSO_CLIENT_ID=578b125ec8194eec8ede735514acf2fd
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, that happened!
I have deleted the application now, so, should be okay

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants