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

feat: JWT Authenticator #195

Merged
merged 101 commits into from
Apr 21, 2023
Merged

feat: JWT Authenticator #195

merged 101 commits into from
Apr 21, 2023

Conversation

kenjis
Copy link
Member

@kenjis kenjis commented Jun 1, 2022

Needs #703
Needs to rebase after merging #194, #199

Add:

  • Config\AuthJWT
  • Authentication\Authenticators\JWT
  • Filters\JWTAuth
  • Authentication\JWTManager = service((jwtmanager)

How to Test/Use:

Sample Test App:

TODO:

  • login recording specification
  • update docs

@kenjis kenjis marked this pull request as draft June 1, 2022 07:52
Copy link
Member

@MGatner MGatner left a comment

Choose a reason for hiding this comment

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

This is looking awesome! I had no idea you were working on this, I would have guessed version 1.1 or 1.2.

src/Authentication/Authenticators/JWT/Firebase.php Outdated Show resolved Hide resolved
src/Config/Auth.php Outdated Show resolved Hide resolved
Copy link
Member

@lonnieezell lonnieezell left a comment

Choose a reason for hiding this comment

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

It's great to see a JWT implementation coming! Thanks for this.

However, I do have a couple of concerns with the current implementation.

  1. Choice of implementation. Why choose Firebase's implementation? I think the best reason is probably that it's backed by Google, but according to the the JWT site, it doesn't provide the most complete implementation. That might not be all bad, though. Honestly, I kind of had it in mind that if we ever included JWT we'd roll our own since the implementation of a JWT is pretty straight-forward. Not sure if that's worth considering to remove the reliance on a third-party libs changes, etc.

  2. I think you may have over-architected it. I've commented on a few classes along the way, but in general I think we should strive for as simple of an architecture as the library allows.

This would also require updating all docs and adding new docs.

I don't think we'll be able to get this one in before the initial release.

src/Authentication/Authenticators/JWT.php Show resolved Hide resolved
src/Authentication/Authenticators/JWT.php Show resolved Hide resolved
src/Authentication/Authenticators/JWT.php Outdated Show resolved Hide resolved
src/Authentication/Authenticators/JWT/Firebase.php Outdated Show resolved Hide resolved
src/Authentication/TokenGenerator/JWTGenerator.php Outdated Show resolved Hide resolved
src/Config/Auth.php Outdated Show resolved Hide resolved
@kenjis
Copy link
Member Author

kenjis commented Jun 1, 2022

I don't think we'll be able to get this one in before the initial release.

There are some parts of this feature that require consideration of specifications.
I think it will take some time.

@kenjis
Copy link
Member Author

kenjis commented Jun 2, 2022

Why choose Firebase's implementation?

I googled CodeIgniter4 jwt, and found most tutorials use Firebase implementation.
And it is commonly used in my country. So first of all, I chose it.

But I know it doesn't provide the most complete implementation, so I made it replaceable.

@datamweb
Copy link
Collaborator

datamweb commented Jun 2, 2022

It is also commonly used in my country.

@kenjis kenjis force-pushed the feat-jwt branch 4 times, most recently from 443000b to 0a0920e Compare June 3, 2022 05:22
@kenjis kenjis added the new feature PRs for new features label Aug 8, 2022
src/Filters/JWTAuth.php Outdated Show resolved Hide resolved
@kenjis kenjis force-pushed the feat-jwt branch 2 times, most recently from 7c2720f to a85c422 Compare October 21, 2022 08:31
@kenjis
Copy link
Member Author

kenjis commented Oct 21, 2022

I don't remember much of the implementation as a lot of time has passed, but I think the implementation itself was done in one way or another.

If there is someone who wants to try JWT, please test. Of course code reviews are also welcome.
I am going to run the code and see if this really works.

@MGatner
Copy link
Member

MGatner commented Oct 21, 2022

My only JWT CI4 project currently uses Myth and I've had issues installing Shield alongside because they have some conflicting services and factories. I know some community members have been keen on this - maybe check the forums for volunteers?

@kenjis
Copy link
Member Author

kenjis commented Oct 21, 2022

Good idea! I've posted the forum.

@hainm0912
Copy link

it finished? how to use this branch?

@kenjis
Copy link
Member Author

kenjis commented Jan 7, 2023

This should work. You can get the code from my repository:
https://github.com/kenjis/codeigniter-shield/tree/feat-jwt

@kenjis
Copy link
Member Author

kenjis commented Jan 7, 2023

how to use this branch?

Update your composer.json:

--- a/composer.json
+++ b/composer.json
@@ -7,7 +7,8 @@
     "require": {
         "php": "^7.4 || ^8.0",
         "codeigniter4/framework": "^4.0",
-        "codeigniter4/shield": "^1.0@beta"
+        "codeigniter4/shield": "dev-feat-jwt",
+        "firebase/php-jwt": "^6.2"
     },
     "require-dev": {
         "fakerphp/faker": "^1.9",
@@ -36,5 +37,11 @@
         "slack": "https://codeigniterchat.slack.com"
     },
     "minimum-stability": "dev",
-    "prefer-stable": true
+    "prefer-stable": true,
+    "repositories": [
+        {
+            "type": "vcs",
+            "url": "https://github.com/kenjis/codeigniter-shield.git"
+        }
+    ]
 }

Run composer update.

@miguel-rn
Copy link
Contributor

What is the status on JWT authentication?

Any TODOs I could help with?

@kenjis kenjis force-pushed the feat-jwt branch 2 times, most recently from 58f6d62 to 324bcb7 Compare April 13, 2023 23:43
@kenjis
Copy link
Member Author

kenjis commented Apr 13, 2023

Rebased to resolve conflicts.

@kenjis
Copy link
Member Author

kenjis commented Apr 13, 2023

What is the status on JWT authentication?

The implementation was finished. I need to write docs.

Any TODOs I could help with?

Testing and review. As you see, no one has approved this PR yet.

@kenjis
Copy link
Member Author

kenjis commented Apr 20, 2023

@MGatner @datamweb Can you review?

@datamweb
Copy link
Collaborator

@kenjis will try to do today.

docs/addons/jwt.md Show resolved Hide resolved
docs/addons/jwt.md Show resolved Hide resolved
docs/addons/jwt.md Outdated Show resolved Hide resolved
docs/addons/jwt.md Outdated Show resolved Hide resolved
docs/addons/jwt.md Outdated Show resolved Hide resolved
src/Authentication/JWTManager.php Show resolved Hide resolved
src/Config/Auth.php Outdated Show resolved Hide resolved
src/Config/AuthJWT.php Show resolved Hide resolved
src/Language/fa/Auth.php Outdated Show resolved Hide resolved
@datamweb
Copy link
Collaborator

Question, how can the site administrator make the tokens expire in general? (I think he should change the secret code. If so, I'd prefer you explain it in the documentation.)

And the next question is there a way to expire the token for a specific user?

Please update the README file, the reference to support JWT is good.

@kenjis
Copy link
Member Author

kenjis commented Apr 20, 2023

Question, how can the site administrator make the tokens expire in general? (I think he should change the secret code. If so, I'd prefer you explain it in the documentation.)
And the next question is there a way to expire the token for a specific user?

Tokens are to be validated by defining the conditions that make it invalid.

If you want to invalidate tokens to a specific user, you can do it by specifying the user ID and issued at.

Also, as you say, If you change the key, all tokens signed with that key will be invalidated.

Copy link
Collaborator

@datamweb datamweb left a comment

Choose a reason for hiding this comment

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

@kenjis, thanks, everything seems to be working fine now.

@kenjis
Copy link
Member Author

kenjis commented Apr 21, 2023

@datamweb Thank you for the detailed review!

@kenjis
Copy link
Member Author

kenjis commented Apr 21, 2023

@MGatner Can you approve? Without your approve, I cannot merge this.

kenjis added a commit to kenjis/codeigniter-shield that referenced this pull request Apr 21, 2023
There is no need to change the status code since a validation error is still an authentication failure.
See codeigniter4#195 (comment)
@kenjis kenjis merged commit 392bd48 into codeigniter4:develop Apr 21, 2023
@kenjis kenjis deleted the feat-jwt branch April 21, 2023 12:26
@kenjis
Copy link
Member Author

kenjis commented Apr 21, 2023

Thank you all!

@kenjis kenjis mentioned this pull request Apr 22, 2023
kenjis added a commit to kenjis/CodeIgniter4 that referenced this pull request Apr 25, 2023
kenjis added a commit to kenjis/CodeIgniter4 that referenced this pull request Apr 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new feature PRs for new features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants