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

Allow sign in using personal accounts #3892

Merged
merged 78 commits into from
Aug 18, 2022
Merged
Show file tree
Hide file tree
Changes from 73 commits
Commits
Show all changes
78 commits
Select commit Hold shift + click to select a range
f3103ea
Add registration only for institutional users setting
jorg-vr Jun 29, 2022
6510b25
Add institutional users visibility setting
jorg-vr Jun 29, 2022
22e13d4
Add vissibility and registration info translations
jorg-vr Jun 29, 2022
d2d4abf
Limit visibillity of courses
jorg-vr Jun 29, 2022
f10dfe8
Limit registration for institutional users
jorg-vr Jun 30, 2022
e55d787
Fix tests
jorg-vr Jun 30, 2022
080cfc2
Add tests
jorg-vr Jun 30, 2022
8f2dfa0
Merge branch 'develop' into feature/institutional-users-course-privacy
jorg-vr Jun 30, 2022
b550457
Allow personal users
jorg-vr Jul 13, 2022
50dfa17
Allow personal accounts for office 365
jorg-vr Jul 13, 2022
a7916e9
Don't ask for contact details
jorg-vr Jul 13, 2022
f9920a1
Put raw info in log
jorg-vr Jul 14, 2022
82d01a1
Readd contacts to scope to see difference
jorg-vr Jul 14, 2022
6d27c92
Fix office365 provider to use new api
jorg-vr Jul 15, 2022
562dfbb
Fix bug
jorg-vr Jul 15, 2022
4a7f0d3
Lift restrictions that blocked personal providers
jorg-vr Jul 15, 2022
148ad02
Add and fix tests
jorg-vr Jul 15, 2022
f4925f1
Add test for office 365 migration
jorg-vr Jul 15, 2022
224a4a4
Only allow identfier based on email log in to work for old identifiers
jorg-vr Jul 15, 2022
8d447af
Fix linting
jorg-vr Jul 15, 2022
5177737
Update sign in page
jorg-vr Jul 15, 2022
1ede9bf
Move comment to the correct line
jorg-vr Jul 15, 2022
8008231
Update config/locales/views/auth/nl.yml
jorg-vr Jul 15, 2022
0d1e1d4
Update privacy announcement to also cover personal accounts
jorg-vr Aug 2, 2022
daf0620
Update english version of the privacy statement
jorg-vr Aug 2, 2022
146fa13
Update dutch privacy statement
jorg-vr Aug 3, 2022
f3c5ee0
Change the name of google workspace to google
jorg-vr Aug 3, 2022
abf7b4f
Merge branch 'develop' into feature/institutional-users-course-privacy
jorg-vr Aug 3, 2022
3ec2615
Remove visible for institutional users setting
jorg-vr Aug 3, 2022
9b0ff8d
Make texts clearer
jorg-vr Aug 3, 2022
276e39a
Fix bugs
jorg-vr Aug 3, 2022
fefa1cf
Merge branch 'develop' into feature/institutional-users-course-privacy
jorg-vr Aug 3, 2022
990cec4
Update english privacy stetement
jorg-vr Aug 3, 2022
3142b23
Update data pages
jorg-vr Aug 3, 2022
3160868
Fix unwanted change
jorg-vr Aug 3, 2022
85edbde
Fix unwanted change
jorg-vr Aug 3, 2022
d9d635e
Improve sentence
jorg-vr Aug 3, 2022
291d6fa
Merge branch 'feature/personal-accounts' into feature/update-privacy-…
jorg-vr Aug 3, 2022
bd288b1
Force new users to accept the privacy policy before creating an account
jorg-vr Aug 4, 2022
a368b37
Fix misnamed method
jorg-vr Aug 4, 2022
15a91fd
Fix flanders oidc test
jorg-vr Aug 4, 2022
d92504e
Fix show privacy prompt
jorg-vr Aug 4, 2022
eee496a
Update text and provide a decline option
jorg-vr Aug 4, 2022
7665d43
Fix lti auth tests
jorg-vr Aug 4, 2022
f3016a1
Apply suggestions from code review
jorg-vr Aug 5, 2022
ed4297b
Apply suggestions from code review
jorg-vr Aug 5, 2022
1a124a6
Apply suggestions from code review
jorg-vr Aug 8, 2022
429980d
Update config/locales/views/courses/en.yml
jorg-vr Aug 12, 2022
e611f71
Update config/locales/views/courses/nl.yml
jorg-vr Aug 12, 2022
0482cbc
Apply suggestions from code review
jorg-vr Aug 12, 2022
3bfa9fc
Merge pull request #3752 from dodona-edu/feature/institutional-users-…
jorg-vr Aug 16, 2022
d6ff809
Rework login sceen
jorg-vr Aug 16, 2022
b26b8b7
Don't generate a token for private users
jorg-vr Aug 16, 2022
6f3bb18
Revert to old behaviour for institutional accounts
jorg-vr Aug 16, 2022
76f6ac6
Merge branch 'feature/personal-accounts' into feature/force-privacy-p…
jorg-vr Aug 16, 2022
9b3de25
Only show privacy disclaimer again to new accounts
jorg-vr Aug 16, 2022
67f8588
Merge branch 'feature/allow-personal-users' into feature/force-privac…
jorg-vr Aug 16, 2022
7f1c725
Merge branch 'feature/personal-accounts' into feature/allow-personal-…
jorg-vr Aug 16, 2022
86e98cf
Merge branch 'feature/allow-personal-users' into feature/force-privac…
jorg-vr Aug 16, 2022
8aa9076
Add privacy prompt check before private account creation in test
jorg-vr Aug 16, 2022
30782ab
Add failure state test
jorg-vr Aug 16, 2022
4255ecc
Merge branch 'develop' into feature/personal-accounts
jorg-vr Aug 16, 2022
50f8409
Merge branch 'feature/personal-accounts' into feature/allow-personal-…
jorg-vr Aug 16, 2022
673a6f3
Merge branch 'feature/allow-personal-users' into feature/force-privac…
jorg-vr Aug 16, 2022
3811e9b
Apply suggestions from code review
jorg-vr Aug 16, 2022
6dd4a51
Update dutch privacy stateement to better differentiate between priva…
jorg-vr Aug 16, 2022
7792121
Update english privacy statement
jorg-vr Aug 16, 2022
e32c603
Update app/views/pages/privacy.en.html.erb
jorg-vr Aug 17, 2022
514424c
Update app/views/pages/privacy.nl.html.erb
jorg-vr Aug 17, 2022
3f122d0
Merge pull request #3855 from dodona-edu/feature/update-privacy-policy
jorg-vr Aug 17, 2022
7d708a8
Make changes a migration
jorg-vr Aug 18, 2022
8f8cdd0
Merge pull request #3806 from dodona-edu/feature/allow-personal-users
jorg-vr Aug 18, 2022
b2faeb0
Merge pull request #3859 from dodona-edu/feature/force-privacy-policy
jorg-vr Aug 18, 2022
f87348a
Apply suggestions from code review
jorg-vr Aug 18, 2022
e4b0516
Allow finding a matching user with institution id nil
jorg-vr Aug 18, 2022
e62458a
Fix redirect message error for personal users
jorg-vr Aug 18, 2022
8418fd9
Don't break on institution nil
jorg-vr Aug 18, 2022
3c709de
Don't break on institution nil
jorg-vr Aug 18, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion app/controllers/application_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ class ApplicationController < ActionController::Base
protect_from_forgery with: :null_session

before_action :store_current_location,
except: %i[media sign_in institution_not_supported],
except: %i[media sign_in institution_not_supported privacy_prompt accept_privacy_policy],
unless: -> { devise_controller? || remote_request? }

before_action :skip_session,
Expand Down
2 changes: 2 additions & 0 deletions app/controllers/auth/authentication_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,8 @@ def sign_in
Provider::Surf
]

@private_providers = Provider.where(institution: nil)

@providers = Provider.all
@title = I18n.t('auth.sign_in.sign_in')
@oauth_providers = apply_scopes(@providers
Expand Down
81 changes: 69 additions & 12 deletions app/controllers/auth/omniauth_callbacks_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -47,13 +47,21 @@ def smartschool
generic_oauth
end

# ==> Privacy agreement acceptance before new account creation

def privacy_prompt
render 'auth/privacy_prompt'
end

def accept_privacy_policy
sign_in_new_user_from_session!
end

private

# ==> Authentication logic.

def generic_oauth
return provider_missing! if oauth_provider_id.blank?

# Find the provider for the current institution. If no provider exists yet,
# a new one will be created.
return redirect_with_flash!(I18n.t('auth.sign_in.errors.institution-creation')) if provider.blank?
Expand Down Expand Up @@ -96,7 +104,11 @@ def try_login!
return redirect_to_known_provider!(user) if user.present?

# No existing user was found
# Create a new user
# Redirect to privacy prompt before we create a new private user
return redirect_to_privacy_prompt if provider&.institution.nil?

# Institutional users don't need to accept the privacy policy
# Thus we can immediately create a new user
user = User.new institution: provider&.institution
# Create a new identity for the newly created user
identity = user.identities.build identifier: auth_uid, provider: provider
Expand All @@ -111,6 +123,12 @@ def try_login!
user.update_from_provider(auth_hash, provider)
return redirect_with_errors!(user) if user.errors.any?

sign_in!(user)
end

# ==> Utilities.

def sign_in!(user)
# If the session contains credentials for another identity, add this identity to the signed in user
create_identity_from_session!(user)

Expand All @@ -122,7 +140,38 @@ def try_login!
redirect_to_target!(user)
end

# ==> Utilities.
def redirect_to_privacy_prompt
session[:new_user_identifier] = auth_hash.uid
session[:new_user_email] = auth_hash.info.email
session[:new_user_first_name] = auth_hash.info.first_name
session[:new_user_last_name] = auth_hash.info.last_name
session[:new_user_provider_id] = provider&.id
session[:new_user_auth_target] = auth_target

redirect_to privacy_prompt_path
end

def sign_in_new_user_from_session!
identifier = session.delete(:new_user_identifier)
email = session.delete(:new_user_email)
first_name = session.delete(:new_user_first_name)
last_name = session.delete(:new_user_last_name)
provider_id = session.delete(:new_user_provider_id)

provider = Provider.find(provider_id)

# Create a new user
user = User.new institution: provider&.institution, email: email, first_name: first_name, last_name: last_name,
username: identifier

# Create a new identity for the newly created user
user.identities.build identifier: identifier, provider: provider
user.save

return redirect_with_errors!(user) if user.errors.any?

sign_in!(user)
end

def create_identity_from_session!(user)
# Find the original provider and uid in the session.
Expand Down Expand Up @@ -155,7 +204,20 @@ def create_identity_from_session!(user)

def find_identity_by_uid
# In case of provider without uids, don't return any identity (As it won't be matching a unique user)
Identity.find_by(identifier: auth_uid, provider: provider) if auth_uid.present?
return nil if auth_uid.nil?

identity = Identity.find_by(identifier: auth_uid, provider: provider)

return identity unless identity.nil? && provider.class.sym == :office365 && auth_email.present?

# This code supports a migration of the office365 oauth api from v1 to v2
# Try to find the user by the legacy identifier
identity = Identity.find_by(identifier: auth_email.split('@').first, provider: provider, identifier_based_on_email: true)
return nil if identity.nil?

# Update the identifier to the new uid
identity.update(identifier: auth_uid, identifier_based_on_email: false)
identity
end

def find_identity_by_user(user)
Expand Down Expand Up @@ -297,7 +359,7 @@ def redirect_to_target!(user)
# ==> Shorthands.

def target_path(user)
auth_target || after_sign_in_path_for(user)
auth_target || session.delete(:new_user_auth_target) || after_sign_in_path_for(user)
end

def auth_hash
Expand All @@ -319,7 +381,7 @@ def auth_redirect_params
end

def auth_target
return nil if auth_hash.extra[:target].blank?
return nil if auth_hash.blank? || auth_hash.extra[:target].blank?

"#{auth_hash.extra[:target]}?#{auth_redirect_params.to_param}"
end
Expand Down Expand Up @@ -366,9 +428,4 @@ def institution_create_failed(errors)
.institution_creation_failed
.deliver_later
end

def provider_missing!
flash_failure I18n.t('auth.sign_in.errors.missing-provider')
redirect_to sign_in_path
end
end
18 changes: 3 additions & 15 deletions app/controllers/courses_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -342,26 +342,14 @@ def subscribe
success_method = method(:signup_succeeded_response)
end

case @course.registration
when 'open_for_all'
if @course.open_for_user?(current_user)
if try_to_subscribe_current_user status: status
success_method.call(format)
else
subscription_failed_response format
end
when 'open_for_institution'
if @course.institution == current_user.institution
if try_to_subscribe_current_user status: status
success_method.call(format)
else
subscription_failed_response format
end
else
format.html { redirect_to(course_url(@course, secret: params[:secret]), alert: I18n.t('courses.registration.closed')) }
format.json { render json: { errors: ['course closed'] }, status: :unprocessable_entity }
end
when 'closed'
format.html { redirect_to(@course, alert: I18n.t('courses.registration.closed')) }
else
format.html { redirect_to(course_url(@course, secret: params[:secret]), alert: I18n.t('courses.registration.closed')) }
format.json { render json: { errors: ['course closed'] }, status: :unprocessable_entity }
end
end
Expand Down
2 changes: 1 addition & 1 deletion app/helpers/courses_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ def registration_action_for(**args)
membership = args[:membership]

if membership.nil? || membership.unsubscribed?
if course.open_for_all? || (course.open_for_institution? && (course.institution == current_user&.institution || current_user.nil?))
if course.open_for_user?(current_user) || current_user.nil?
if course.moderated
link_to t('courses.show.request_registration'),
subscribe_course_path(@course, secret: secret),
Expand Down
2 changes: 1 addition & 1 deletion app/models/activity.rb
Original file line number Diff line number Diff line change
Expand Up @@ -280,7 +280,7 @@ def accessible?(user, course)
return true if user&.member_of? course
return false if course.moderated && access_private?

course.open_for_all? || (course.open_for_institution? && course.institution == user&.institution)
course.open_for_user?(user)
else
return true if user&.repository_admin? repository

Expand Down
8 changes: 6 additions & 2 deletions app/models/course.rb
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ class Course < ApplicationRecord
has_many :course_labels, dependent: :destroy

enum visibility: { visible_for_all: 0, visible_for_institution: 1, hidden: 2 }
enum registration: { open_for_all: 0, open_for_institution: 1, closed: 2 }
enum registration: { open_for_all: 3, open_for_institutional_users: 0, open_for_institution: 1, closed: 2 }

# TODO: Remove and use activities?
has_many :content_pages,
Expand Down Expand Up @@ -168,7 +168,7 @@ class Course < ApplicationRecord
# Default year & enum values
after_initialize do |course|
self.visibility ||= 'visible_for_all'
self.registration ||= 'open_for_all'
self.registration ||= 'open_for_institutional_users'
unless year
now = Time.zone.now
y = now.year
Expand Down Expand Up @@ -214,6 +214,10 @@ def all_activities_accessible?
activities.where(access: :private).where.not(repository_id: usable_repositories).count.zero?
end

def open_for_user?(user)
open_for_all? || (open_for_institution? && institution == user&.institution) || (open_for_institutional_users? && user&.institutional?)
end

def invalidate_subscribed_members_count_cache
Rails.cache.delete(format(SUBSCRIBED_MEMBERS_COUNT_CACHE_STRING, id: id))
end
Expand Down
13 changes: 7 additions & 6 deletions app/models/identity.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,13 @@
#
# Table name: identities
#
# id :bigint not null, primary key
# identifier :string(255) not null
# provider_id :bigint not null
# user_id :integer not null
# created_at :datetime not null
# updated_at :datetime not null
# id :bigint not null, primary key
# identifier :string(255) not null
# provider_id :bigint not null
# user_id :integer not null
# created_at :datetime not null
# updated_at :datetime not null
# identifier_based_on_email :boolean default(FALSE), not null
#
class Identity < ApplicationRecord
belongs_to :provider, inverse_of: :identities
Expand Down
4 changes: 2 additions & 2 deletions app/models/provider.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
#
# id :bigint not null, primary key
# type :string(255) default("Provider::Saml"), not null
# institution_id :bigint not null
# institution_id :bigint
# identifier :string(255)
# certificate :text(16777215)
# entity_id :string(255)
Expand All @@ -24,7 +24,7 @@ class Provider < ApplicationRecord

PROVIDERS = [Provider::GSuite, Provider::Lti, Provider::Office365, Provider::Oidc, Provider::Saml, Provider::Smartschool, Provider::Surf].freeze

belongs_to :institution, inverse_of: :providers
belongs_to :institution, inverse_of: :providers, optional: true

has_many :identities, inverse_of: :provider, dependent: :destroy

Expand Down
11 changes: 9 additions & 2 deletions app/models/provider/g_suite.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
#
# id :bigint not null, primary key
# type :string(255) default("Provider::Saml"), not null
# institution_id :bigint not null
# institution_id :bigint
# identifier :string(255)
# certificate :text(16777215)
# entity_id :string(255)
Expand All @@ -22,7 +22,7 @@
class Provider::GSuite < Provider
validates :certificate, :entity_id, :sso_url, :slo_url, absence: true
validates :authorization_uri, :client_id, :issuer, :jwks_uri, absence: true
validates :identifier, uniqueness: { case_sensitive: false }, presence: true
validates :identifier, uniqueness: { case_sensitive: false }

def self.sym
:google_oauth2
Expand All @@ -46,4 +46,11 @@ def self.extract_institution_name(auth_hash)
Provider.extract_institution_name(auth_hash)
end
end

def readable_name
# We want to display gmail for private accounts
return 'Gmail' if institution.nil?

super.readable_name
end
end
2 changes: 1 addition & 1 deletion app/models/provider/lti.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
#
# id :bigint not null, primary key
# type :string(255) default("Provider::Saml"), not null
# institution_id :bigint not null
# institution_id :bigint
# identifier :string(255)
# certificate :text(16777215)
# entity_id :string(255)
Expand Down
2 changes: 1 addition & 1 deletion app/models/provider/office365.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
#
# id :bigint not null, primary key
# type :string(255) default("Provider::Saml"), not null
# institution_id :bigint not null
# institution_id :bigint
# identifier :string(255)
# certificate :text(16777215)
# entity_id :string(255)
Expand Down
2 changes: 1 addition & 1 deletion app/models/provider/oidc.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
#
# id :bigint not null, primary key
# type :string(255) default("Provider::Saml"), not null
# institution_id :bigint not null
# institution_id :bigint
# identifier :string(255)
# certificate :text(16777215)
# entity_id :string(255)
Expand Down
2 changes: 1 addition & 1 deletion app/models/provider/saml.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
#
# id :bigint not null, primary key
# type :string(255) default("Provider::Saml"), not null
# institution_id :bigint not null
# institution_id :bigint
# identifier :string(255)
# certificate :text(16777215)
# entity_id :string(255)
Expand Down
2 changes: 1 addition & 1 deletion app/models/provider/smartschool.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
#
# id :bigint not null, primary key
# type :string(255) default("Provider::Saml"), not null
# institution_id :bigint not null
# institution_id :bigint
# identifier :string(255)
# certificate :text(16777215)
# entity_id :string(255)
Expand Down
2 changes: 1 addition & 1 deletion app/models/provider/surf.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
#
# id :bigint not null, primary key
# type :string(255) default("Provider::Saml"), not null
# institution_id :bigint not null
# institution_id :bigint
# identifier :string(255)
# certificate :text(16777215)
# entity_id :string(255)
Expand Down
10 changes: 9 additions & 1 deletion app/models/user.rb
Original file line number Diff line number Diff line change
Expand Up @@ -273,6 +273,14 @@ def repository_admin?(repository)
@repository_admin.include?(repository.id)
end

def personal?
institution.nil?
end

def institutional?
institution.present?
end

def attempted_exercises(options)
s = submissions.judged
s = s.in_course(options[:course]) if options[:course].present?
Expand Down Expand Up @@ -453,7 +461,7 @@ def merge_into(other, force: false, force_institution: false)
private

def set_token
if institution.present?
if identities.present?
self.token = nil
elsif token.blank?
generate_token
Expand Down
Loading