Skip to content
This repository has been archived by the owner. It is now read-only.

v2.2 #248

Merged
merged 16 commits into from Mar 16, 2020
Merged

v2.2 #248

merged 16 commits into from Mar 16, 2020

Conversation

nsarno
Copy link
Owner

@nsarno nsarno commented Dec 9, 2019

Draft Pull Request for v2.2

Feel free to comment with links to issues or changes you believe should be added to this version.

Changes

  • Update the dependencies
  • Remove the generators
  • Fix deprecation warnings and autoloading

@nsarno nsarno added the WIP label Dec 9, 2019
nsarno added 5 commits Dec 9, 2019
The generator is used to created very simple files. They're adding
complexity to the gem without providing much value.

More details should be added to the README instead.
Give more details about initializer in the Readme to compensate for the
removal of generator.
@FunkyloverOne
Copy link

@FunkyloverOne FunkyloverOne commented Dec 9, 2019

hey @nsarno, please take a look at this one: #240

And here's how people suggest to fix it to work nicely with Zeitwerk: rails/rails#36381 (comment)

@nsarno
Copy link
Owner Author

@nsarno nsarno commented Dec 9, 2019

@FunkyloverOne I've made some changes to address the issue.

Here is what I get when running the zeitwerk check in a Rails 6.0.1 app with Knock install from this branch.

$ bin/rails zeitwerk:check
Hold on, I am eager loading the application.
All is good!

nsarno added 6 commits Dec 9, 2019
- Implement `respond_to_missing?`
- Fix some minor styling and convention issues
`Knock::Tokenizable` can be included in the entity model (e.g. User) to
provide token serialization and deserialization.

Works out of the box with the same sensible default payload format and
fetching method using `#find`.

Or the methods can be overriden as usual.

This brings some convenience to implement a custom token creation
endpoint without relying on Knock's AuthTokenController which I'm trying
to move away from.

This allows for the possibility to make Knock even simpler by packaging
it as a simple gem instead of Rails engine. Less magic, more simplicity
and ease of customization. Just as easy to get started with clear
documentation.
@andrerpbts
Copy link
Collaborator

@andrerpbts andrerpbts commented Dec 13, 2019

Hello @nsarno

Here are some issues I think would be welcome on 2.2:

  • #228 -> It seems to be a security flaw, and it isn't the first time I saw an issue involving a problem with this method (#220). There's a PR related with this that would solve the problem, but I think we probably can change it from using method missing strategy and turn it more configurable...

  • #226 -> Probably it's already in the map, but I think we could use API controller in the place of the base controller...

  • #183 -> This is a thing I had some problems when I used Knock recently. I wanted to authenticate with document and password, and I turned it back to email instead patching the method like docs suggested. Like in devise, where you can set what fields the user is going to be identified, I think that would be a good move for us as well. We can simply add an entry into knock configuration file, let it default to email, and use that fields to enforce params and seek the user.

I think that's it... What do you think?

@Lax
Copy link

@Lax Lax commented Dec 20, 2019

Hi, I'm here to asking when will this update version been published? Now my rails 6.0 project is having some mentioned issues that would be fixed by this PR.

@nsarno
Copy link
Owner Author

@nsarno nsarno commented Jan 4, 2020

@andrerpbts

#228 -> It seems to be a security flaw, and it isn't the first time I saw an issue involving a problem with this method (#220). There's a PR related with this that would solve the problem, but I think we probably can change it from using method missing strategy and turn it more configurable...

For now, we can simply update the doc. Ultimately, I would like to move away from relying on method_missing but this would probably be part of a major version release.

I'll try to address the other issues in this version.

@Lax I'm aiming at releasing it this month.

* fixing default code style

* allowing gem's user to change token lifetime

* changing the default to user

* wip applying changes

* wip apply suggestions from code review

* wip apply suggestions from code review (README.md)

* apply suggestions from code review (README.md)

* test added and entity class access changed

* pry added to the project

* tests for default user and superuser

* removing pry and fixing documentation

* rewriting test message
@maxrosecollins
Copy link

@maxrosecollins maxrosecollins commented Mar 16, 2020

@nsarno When will this be released? Thanks!

@nsarno
Copy link
Owner Author

@nsarno nsarno commented Mar 16, 2020

@maxrosecollins This looks ready actually. I was hoping to fix some other issues as part of this version but I didn't get to it so I'll just merge it as is for now.

@nsarno nsarno merged commit 9214cd0 into master Mar 16, 2020
1 of 2 checks passed
@nsarno nsarno deleted the v2.2 branch Mar 16, 2020
@nsarno
Copy link
Owner Author

@nsarno nsarno commented Mar 16, 2020

@maxrosecollins master is green again, good times. Thanks for the gentle ping.

If you (or anyone else here) test their project with the new master and can confirm everything is working well, that would be greatly appreciated and would give me some extra confidence to release the new version to RubyGems. Cheers!

@nsarno nsarno removed the WIP label Mar 17, 2020
@swrobel
Copy link

@swrobel swrobel commented Apr 8, 2020

@nsarno still don't see this on rubygems...

@nsarno
Copy link
Owner Author

@nsarno nsarno commented Apr 8, 2020

@swrobel are you confirming that you've tested the new master, think it's working great and validating I can ship this to RubyGems? Or is this just your polite way to point out I'm not doing this unpaid job fast enough for you.

@swrobel
Copy link

@swrobel swrobel commented Apr 8, 2020

Would you mind tagging v2.2 at least? I'd be happy to test in that case, but I'm not going to just add master to my Gemfile willy-nilly.

@maxrosecollins
Copy link

@maxrosecollins maxrosecollins commented Apr 30, 2020

@nsarno It seems to run smoothly locally but when I put it into my production environment, I'm getting the following error. I'm not sure if that just something I've done wrong...

TypeError (no implicit conversion of nil into String):
app/controllers/api/v1/user_token_controller.rb:8:in `create'

This line is
user = User.find(auth_token.payload[:sub])

And I get the same error if I call

Knock::AuthToken.new payload: { sub: user.id }

@maxrosecollins
Copy link

@maxrosecollins maxrosecollins commented Apr 30, 2020

@nsarno Okay, my mistake, I had Rails.application.secrets.secret_key_base insead of Rails.application.credentials.secret_key_base

boolean added a commit to boolean/knock that referenced this issue May 29, 2020
@dtgay
Copy link

@dtgay dtgay commented Jun 17, 2020

@swrobel I don't think you have to add master willy-nilly, you can specify a commit like so:

gem "knock", github: "nsarno/knock", branch: "master",
    ref: "9214cd027422df8dc31eb67c60032fbbf8fc100b"

@dtgay
Copy link

@dtgay dtgay commented Jun 17, 2020

@nsarno Great work, I'm successfully using knock at commit 9214cd027422df8dc31eb67c60032fbbf8fc100b in my Rails 6 API. Consider it a 👍 from me. And thanks!

For anyone trying to get things working in their own app, I wrote a blog post explaining how I did things: https://davidgay.org/programming/jwt-auth-rails-6-knock/

@maxrosecollins
Copy link

@maxrosecollins maxrosecollins commented Jun 18, 2020

@nsarno It's working smoothly for me in prod too 👍

@jusantana
Copy link

@jusantana jusantana commented Aug 10, 2020

Working nice for me as well. Thank you @dtgay for the guide!

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

Successfully merging this pull request may close these issues.

None yet

9 participants