search

furkan3ayraktar / clojure-polylith-realworld-example-app

Clojure, Polylith and Ring codebase containing real world examples (CRUD, auth, advanced patterns, etc) that adheres to the RealWorld spec and API.
MIT License
448 stars 79 forks source link

Improve authentication logic #17

Closed furkan3ayraktar closed 3 years ago

furkan3ayraktar commented 3 years ago

Instead of querying the database with user's JWT, it could be nice to validate the token with clj-jwt and extract the user id from claims. Then, the database can be queried by the user id instead of the token. This will eliminate the need for saving the JWT in to the database in the first place.

Thanks to @stask for pointing out this issue.

furkan3ayraktar commented 3 years ago

@stask I was checking out again the Realworld api specifications and I noticed that actually it expects that all endpoints to return user with the token. That's why, I'll still need to save the token to the database. However, the validation part can still be done with JWT, instead of querying the database.

stask commented 3 years ago

@furkan3ayraktar what do you think about adding the token to the user map instead of reading it from the database? Sorry for nitpicking, it just feels wrong to store the token for two reasons:

furkan3ayraktar commented 3 years ago

@stask not at all! I really appreciate you taking time to improve this repository!

I think it could be alright to pass the token we receive from the request in the response user map. I can make that change. Also, what do you think about the correct logic when the token is expired? I noticed, at the moment it doesn't matter if the token is expired or not, as long as it is a valid token.

stask commented 3 years ago

@furkan3ayraktar thanks, happy to help, I like the polylith idea.

I think the simplest way would be to verify the token on each request that needs authorization and return 401 if token is expired. The UI would then show the login screen to generate a new one.

Let me know if you prefer a PR instead of suggestions :)

furkan3ayraktar commented 3 years ago

Sounds good to me! I already made the changes we discussed in PR #18. I can just add the check for expiration and reject the requests easily.

If you have time, please have a look at the branch and if all looks good, I can merge.