Skip to content
This repository has been archived by the owner on Jul 24, 2020. It is now read-only.

#2 Authentication #1032

Merged
merged 157 commits into from
Nov 25, 2014
Merged

#2 Authentication #1032

merged 157 commits into from
Nov 25, 2014

Conversation

orenyk
Copy link
Contributor

@orenyk orenyk commented Oct 27, 2014

Implements #2, allowing for both CAS and email/password authentication. Refer to the issue for commentary; this PR includes significant changes including guest user functionality, integration tests, and more!

Issues Resolved (please check all during review):

orenyk added 30 commits October 12, 2014 13:46
@orenyk
Copy link
Contributor Author

orenyk commented Nov 24, 2014

I don't understand these freaking build failures, they just show up intermittently :-(

@orenyk
Copy link
Contributor Author

orenyk commented Nov 24, 2014

Ok, updated app setup script to account for passwords and also cleaned up seed script. Review when ready :-)

@orenyk
Copy link
Contributor Author

orenyk commented Nov 24, 2014

So frustrating... you can get the tests to apss by just restarting the builds (sometimes). I wish I knew why those two tests were failing.

@squidgetx
Copy link
Contributor

Everything looks good to me that I haven't already commented on I've also managed to cleanly spoof the guest user so we don't need to actually keep the record in the database.

@orenyk
Copy link
Contributor Author

orenyk commented Nov 24, 2014

Awesome! Did you get a chance to look at the app setup script update? I'll look over your changes and we can hopefully get this merged in tonight :-)

@squidgetx
Copy link
Contributor

Yea the app setup script changes are perfect

@orenyk
Copy link
Contributor Author

orenyk commented Nov 24, 2014

Also, did you get a chance to make sure all of the user actions (creation, editing, etc) work as expected under both authentication models, particularly creating a new user from a valid CAS login? That was one of the trickier things I ran into and I want to make sure that nothing's broken (although I tried to write integration tests for them). Thanks!

@squidgetx
Copy link
Contributor

  • editing someone's email (nonCAS) will successfully change the email itself but not the username
  • password field doesn't exist in CAS mode but the setup script tries to do it anyway (Looks like I didn't go over the setup script as thoroughly as I thought; I fixed this in the latest commit)

@orenyk
Copy link
Contributor Author

orenyk commented Nov 24, 2014

Good catches. I believe that in non-CAS mode Devise is using email as the login parameter, not username, but we should keep them up to date regardless. The password parameter does exist in CAS mode (since our database is the same); that said, I'm not sure if there's any harm in defaulting to nil. Frankly, the app isn't designed to switch between the two authentication systems so I guess it doesn't matter. I'm fixing the first issue you mentioned, thanks!

@orenyk
Copy link
Contributor Author

orenyk commented Nov 24, 2014

Ok, fix pushed (and hiding username on user#show when not using CAS). I think that's it :-)

@squidgetx
Copy link
Contributor

👍

@orenyk
Copy link
Contributor Author

orenyk commented Nov 24, 2014

There are those freaking build failures again... any ideas?

EDIT I don't understand why re-running the build fixes it. So infuriating.

@orenyk
Copy link
Contributor Author

orenyk commented Nov 25, 2014

ok, merging!

orenyk added a commit that referenced this pull request Nov 25, 2014
resolves #2, resolves #694, resolves #777, resolves #695, resolves #973, and resolves #1018
@orenyk orenyk merged commit 7374131 into master Nov 25, 2014
@squidgetx
Copy link
Contributor

:D

I have no idea why they were failing, I tried messing with the tests a little bit to see and they accidentally got included in git. Didn't really learn anything valuable though

@mnquintana mnquintana deleted the 2_authentication_II branch February 20, 2015 13:40
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.

4 participants