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

Feature/cas #349

Closed
wants to merge 17 commits into from
Closed

Feature/cas #349

wants to merge 17 commits into from

Conversation

loganfreeman
Copy link
Contributor

add support for omniauth_cas

need to add the following setting in application.yml to enable it:

use_cas: true
use_cas_only: false 

use_cas: flag to turn on and off omniauth_cas
use_cas_only: if false will use both database and omniauth_cas

@identity = Identity.find_for_cas_oauth(request.env["omniauth.auth"], current_identity)

if @identity.persisted?
sign_in_and_redirect @identity, :event => :authentication
Copy link
Contributor

Choose a reason for hiding this comment

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

We're working on better styling our Ruby code. Reference - https://github.com/bbatsov/ruby-style-guide

Here, use new Ruby syntax instead of hash rockets, e.g.

 event: :authentication

Parentheses for method arguments

 sign_in_and_redirect(@identity ...)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks

@loganfreeman
Copy link
Contributor Author

Sorry, my fault. I will reopen it.

@loganfreeman
Copy link
Contributor Author

I will correct the styling and yml file and resend the request

@loganfreeman loganfreeman reopened this Apr 19, 2016
@loganfreeman
Copy link
Contributor Author

I corrected the styling issue, removed the change to en.yml.
I added a helper to deal with the issue of sign in button text using different institution name.

Please review

@loganfreeman
Copy link
Contributor Author

loganfreeman commented Apr 19, 2016

just found there is a local_override. removed the helper, and just use key from locale file.

has_many :received_toast_messages, :class_name => 'ToastMessage', :foreign_key => 'to', :dependent => :destroy
has_many :sent_toast_messages, :class_name => 'ToastMessage', :foreign_key => 'from', :dependent => :destroy
has_many :notes, :dependent => :destroy
has_many :approvals, dependent: :destroy
Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for updating this file to follow our style guide specifications!

@amcates
Copy link
Collaborator

amcates commented Apr 27, 2016

@loganfreeman I have a couple issues with this pull request. All of which I think can be easily resolved. I feel like the refactoring should be done separately from the actual logic added. It makes it much harder to determine exactly what has changed. Second, why did you extract from obis_setup? This isn't something that was required for your pull request. I would prefer that pull requests be concise and easily explainable.

With my comment about 'persisted?', the system creates new local identities once authenticated. This is both to make the system faster (returning identities verses querying external systems) and it gives us a hard record of what the identity looked like.

I think prior to future pull requests we should probably have a 10-15 conversation.

Thanks,

Andrew

@loganfreeman
Copy link
Contributor Author

@amcates for the obis_setup, its purpose is to load some constants. But the calling of the initializers has some order, such that the obis_setup initializer is not the first one to be called and the constants are not available for those called before obis_setup.
So I move the constants loading stuff to environement.rb.

@loganfreeman
Copy link
Contributor Author

@amcates I can make a new pull request without the obis_setup change. the devise initializer needs to know from application.yml what autheticattion provider to use. but it is called after obis_setup. So it needs read the application.yml again.

@loganfreeman loganfreeman deleted the feature/cas branch June 22, 2016 18:58
Stuart-Johnson added a commit that referenced this pull request Jul 17, 2017
…nd_specs

Kg ml sc config model and specs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants