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

Refactor Bearer Auth #112

Merged
merged 26 commits into from
Apr 17, 2023
Merged

Refactor Bearer Auth #112

merged 26 commits into from
Apr 17, 2023

Conversation

gevorgmansuryan
Copy link
Contributor

No description provided.

@gevorgmansuryan gevorgmansuryan requested a review from luke- March 30, 2023 18:50
Copy link
Contributor

@luke- luke- left a comment

Choose a reason for hiding this comment

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

@gevorgmansuryan Thanks, looks very good!

The current admin config is already very overloaded as we have a many options here. What do you think about splitting/rebuilding it a bit

E.g.

  • Tabs for the different features

    • General (Enabled Users)
    • Authentication
    • Modules
  • Maybe we can leave out the checkbox for "Bearer Auth". If no tokens have been created, it is not active.

  • Maybe don't auto generate JWT Key. And leave empty for disable JWT Auth.


Other points:

  • Adjust documentation for new auths
  • Tests for different auths

@gevorgmansuryan
Copy link
Contributor Author

gevorgmansuryan commented Mar 31, 2023

@luke- Okay Will be done. What if we make jwt auth configurable and add enableJwtAuth checkbox like others?

@luke-
Copy link
Contributor

luke- commented Apr 3, 2023

@gevorgmansuryan Thanks, looks really nice.

Here some points:
image
image
image

Tests & Swagger Docs would also be good, especially for the tokens.

@gevorgmansuryan
Copy link
Contributor Author

@luke- Enabled users used for JWT auth only.

@luke-
Copy link
Contributor

luke- commented Apr 5, 2023

@gevorgmansuryan

@luke- Enabled users used for JWT auth only.

This means that with Basic Auth all users are currently always enabled? I think it would be good to change this, and with authentication by Username & Password (Basic Auth & JWT), always allow the activated users or if checked all users.

  • Maybe we should also add a hint that, the users are used for these auth modes.

image

@luke- luke- linked an issue Apr 5, 2023 that may be closed by this pull request
@gevorgmansuryan
Copy link
Contributor Author

gevorgmansuryan commented Apr 5, 2023

@luke-

This means that with Basic Auth all users are currently always enabled?

Yes, it's always worked in this way

Okay Will add user check for Basic Auth too, + hint

@gevorgmansuryan
Copy link
Contributor Author

@luke-

query param auth

Yes. It will work when bearer auth is enabled.
When bearer auth checkbox is unchecked it will uncheck too. It is not possible to check query param auth checkbox without checking bearer auth checkbox

@gevorgmansuryan
Copy link
Contributor Author

@luke-
8F0FACAB-43C5-48FC-9107-3ADDC44EC5AB

@gevorgmansuryan gevorgmansuryan requested a review from luke- April 6, 2023 17:47
@luke-
Copy link
Contributor

luke- commented Apr 11, 2023

@gevorgmansuryan Can you please check the tests?

Currently only this test should fail: #114

@luke- 8F0FACAB-43C5-48FC-9107-3ADDC44EC5AB 8F0FACAB-43C5-48FC-9107-3ADDC44EC5AB

@gevorgmansuryan Looks very good. Thank you!

@gevorgmansuryan gevorgmansuryan requested a review from luke- April 14, 2023 18:01
@luke- luke- merged commit 556812d into master Apr 17, 2023
@luke- luke- deleted the refactor-bearer-auth branch April 17, 2023 09:55
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.

Refactor Bearer Auth
2 participants