jaceklaskowski / librarian-clojure

Book manager in Clojure
http://librarian-clojure.herokuapp.com/
22 stars 4 forks source link

Integrate cemerick/friend #30

Closed zoldar closed 11 years ago

zoldar commented 11 years ago

How about that instead of self-baked solution? It brings pretty wide variety of authentication mechanisms and AFAIK is slowly becoming a goto solution for authorization/authentication in clojure apps.

https://github.com/cemerick/friend

jaceklaskowski commented 11 years ago

That was the idea quite ago and if there's anybody who thinks it's worth the efforts I'm for it!

zoldar commented 11 years ago

I think that I'll give it a shot in a free moment or two.

zoldar commented 11 years ago

I've started the swap operation. Unfortunately I'm forced to leave the code (in my branch of course) in a non-operational state for today :( I couldn't even make authentication work. Furthermore, for some reason it insists on using redirects where it shouldn't. I'll investigate the case further down this week as time permits. I'm almost sure it's due to me misusing the tool (sleep deprivation may play some role here too).

Just to clarify why I did bulk login -> username replace: I didn't see an easy way (that doesn't imply there isn't one) to use custom field names. I've seen that there's some flexibility with regards to the password but afaik only when retrieving it from datasource. I've chosen the easier way due to small project size - otherwise I would have to parametrize it in some way, but the code clarity would probably suffer. If you'll see anything terribly wrong with what I'm doing, please let me know. I wouldn't be surprised if current changes needed scrapping and starting over.

https://github.com/zoldar/librarian-clojure/commit/95d2c0fefefb64bb2e6b5f72e9460a9fb81d0f01

zoldar commented 11 years ago

Just to keep you informed, I've made a bit of exploratory fiddling with compojure and friend in order to understand them both better. I've made it in a form of an (somewhat) interactive session with a bit of narration. Because I don't want to come out with an update empty handed, I've put that session into a gist. I didn't do (almost) any editing, even some slip ups were kept along with their resolution (however not all of them, because then that gist would be waaay longer). So, at the risk of embarassment and making an ass out of myself, I'm putting it here:

https://gist.github.com/4302375

Note: In case of facts the results of running them are missing, so there's a bit of a context missing. Simple evaluation gave just either true or false. I've had full midje test ouput at some of them, but because I've been using emacs midje-mode the test results where removed from buffer every time I ran a clean command. I've noticed it only after some time and by then I gave up with rerunning all the tests and reformatting the results. Sorry.

jaceklaskowski commented 11 years ago

I reviewed your first set of changes in your repo and was about to have committed to mine when you added the other comment. Do you think I should hold committing anything or would you accept the first from https://github.com/zoldar/librarian-clojure/commit/95d2c0fefefb64bb2e6b5f72e9460a9fb81d0f01? Even though it doesn't work completely fine, it'd be a good starting point, wouldn't it?

zoldar commented 11 years ago

I've started a rebooted effort at adding friend integration which is less invasive.

https://github.com/zoldar/librarian-clojure/tree/issue-30-befriend

However, I would rather not push it into upstream master. Master branch should contain code at least in a fully working condition. It would be better if you've created a clear branch with a name like "friend" and then I would do the pull request to that branch. Then, if somebody had time and will to play around with that feature it in the meantime, he would just checkout that topic branch and submit the pull requests there as well. Eventually, when everything will be done and all the auth features will be in place (and tested) it could be pushed to the master. That's how I see it. What do you think?

BTW It seems that I have broken librarian-clojure.repl with my last reflection warnings cleanup, I'll submit a patch sometime this week to fix that regression. Sorry.

jaceklaskowski commented 11 years ago

Here's the deal - do the integration (as much as you are able) in your repo and I'll have a look. Once it settles, we merge it with mine. Truly distributed team with distributed source code repo :-)

zoldar commented 11 years ago

As you wish. But this may have to wait until holidays.

jaceklaskowski commented 11 years ago

Make it as painless as possible so it won't interfere with your daily assignments. If there's a commit/push I should have a look and integrate, let me know. I'm up for some self-developments.

zoldar commented 11 years ago

The https://github.com/zoldar/librarian-clojure/tree/issue-30-befriend finally contains a fully working integration of cemerick/friend - in other words, librarian-clojure got befriended!

However, there are currently two problems:

  1. I have no tests to prove it. I know what I was promising (to myself) about proper approach to testing but having limited resources (time) and jumping through some hoops (either due to my idiotic mistakes/misconceptions/lack of observance or just having to bend internals of friend to obey the librarian's way) I didn't keep that promise. Having said that, I will add proper tests post-factum (I promise!).
  2. Unfortunately, I was a couple of hours late with my submission and a couple of conflicts surfaced. I did a merge which (hopefully) resolved those conflicts. Still, the new version of signup form does not pop up so I couldn't confirm that path of execution (I've checked it with the current master branch and that particular problem doesn't seem to be related to my changes). Either way, I can assure that the sign up process along with the log-in-on-sign-up workflow does (did) actually work with the code before today's changes.

I don't want to fall too far behind with the changes in the codebase so I've decided to jump the gun a bit. I'm leaving it to your judgement if my branch is worth merging as it stands now.

https://github.com/zoldar/librarian-clojure/tree/issue-30-befriend

P.S. Happy Holidays!

jaceklaskowski commented 11 years ago

Committed and pushed to origin/master. Should be available in github already. Since I made the changes on the command line, troubles may lurk here and there. Be warned yet fear not!