-
Notifications
You must be signed in to change notification settings - Fork 0
feat: enhance OAuth scopes and expand user data fields #1
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
base: master
Are you sure you want to change the base?
Conversation
- Update OAuth scopes to include full, oneroster, profile, and openid - Add comprehensive user data mapping with 20+ new fields: - User identity (login_id, user_id) - Personal information (display_name) - Role and access (role_level, authentication_type, last_access_time) - Organization (tenant, building, org_id) - Geographic (state_id, state_name) - Profile preferences (language, time_format, profile_id) - Maintain backward compatibility with existing field aliases - Add comprehensive RSpec test suite with Faker integration - Fix OAuth2 authorization parameter handling - Bump version to 0.4.0 following semver conventions BREAKING CHANGE: None - all existing field names preserved for compatibility
- Add Ruby gem-specific test workflow with matrix testing (Ruby 3.0-3.3) - Add comprehensive pull request template for gem development - Configure Dependabot for Ruby gem dependency management - Include Asana integration workflows for project management
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR enhances the OmniAuth ClassLink strategy by expanding OAuth scopes, adding comprehensive user data mapping with 20+ new fields, and implementing a complete test suite. The changes maintain backward compatibility while providing more detailed user information and proper test coverage.
- Updated OAuth scopes to include full, oneroster, profile, and openid for expanded data access
- Added comprehensive user data mapping across identity, personal, organizational, geographic, and preference categories
- Implemented comprehensive RSpec test suite with Faker integration for realistic test data generation
Reviewed Changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| lib/omniauth/strategies/class_link.rb | Enhanced OAuth scopes and expanded user info mapping with 20+ new fields |
| lib/omniauth/class_link/version.rb | Bumped version to 0.4.0 following semver conventions |
| omniauth-classlink.gemspec | Updated author/email info and added development dependencies |
| spec/spec_helper.rb | Added RSpec configuration with testing framework setup |
| spec/support/test_helper.rb | Created Faker-based test data generation helpers |
| spec/omniauth/strategies/class_link_spec.rb | Added comprehensive unit tests for strategy implementation |
| spec/integration/omniauth_classlink_spec.rb | Added integration tests for complete authentication flow |
| spec/README.md | Added comprehensive documentation for test structure and usage |
| Rakefile | Added RSpec task configuration |
| .github/workflows/tests.yml | Added GitHub Actions CI pipeline for automated testing |
| .github/workflows/asana-create-attachment.yml | Added Asana integration for PR attachments |
| .github/workflows/asana-comment-on-task.yml | Added Asana integration for PR comments |
| .github/dependabot.yml | Added Dependabot configuration for dependency updates |
| .github/PULL_REQUEST_TEMPLATE.md | Added structured PR template for consistent submissions |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
- Revert authorize_params to use safer super.tap pattern instead of super || {}
- Extract hardcoded language array to SUPPORTED_LANGUAGES constant
- Extract hardcoded roles array to CLASSLINK_ROLES constant
- Improve maintainability by making supported languages and roles configurable
- Add documentation comments for the new constants
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd recommend installing rubocop and the rubocop-rspec plugin to help keep the tests nice and clean, and conformant with the RSpec Style Guide.
| require 'spec_helper' | ||
|
|
||
| describe OmniAuth::Strategies::ClassLink do | ||
| subject do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The RSpec style guide recommends named subject objects, rather than the anonymous usage as used here. Instead, we should treat it as a specific instance of let, e.g.:
subject(:strategy) { OmniAuth::Strategies::ClassLink.new({}) }
# then later
expect(strategy.options.client_options.site).to eq('https://launchpad.classlink.com')
# etc
Anonymous subjects are useful for shared examples where we don't precisely know the context.
| it 'should have correct site' do | ||
| expect(subject.options.client_options.site).to eq('https://launchpad.classlink.com') | ||
| end | ||
|
|
||
| it 'should have correct authorize url' do | ||
| expect(subject.options.client_options.authorize_url).to eq('/oauth2/v2/auth') | ||
| end | ||
|
|
||
| it 'should have correct token url' do | ||
| expect(subject.options.client_options.token_url).to eq('/oauth2/v2/token') | ||
| end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could specify a new subject for this group. Also descriptions should be emphatic – "it is" or "it does" rather than "it should"
| it 'should have correct site' do | |
| expect(subject.options.client_options.site).to eq('https://launchpad.classlink.com') | |
| end | |
| it 'should have correct authorize url' do | |
| expect(subject.options.client_options.authorize_url).to eq('/oauth2/v2/auth') | |
| end | |
| it 'should have correct token url' do | |
| expect(subject.options.client_options.token_url).to eq('/oauth2/v2/token') | |
| end | |
| subject(:client_options) { strategy.options.client_options } | |
| it 'has the correct site' do | |
| expect(client_options.site).to eq('https://launchpad.classlink.com') | |
| end | |
| it 'has the correct authorize url' do | |
| expect(client_options.authorize_url).to eq('/oauth2/v2/auth') | |
| end | |
| it 'has the correct token url' do | |
| expect(client_options.token_url).to eq('/oauth2/v2/token') | |
| end |
To reduce the common boilerplate we could either use inline expectations without the need for separate methods (and we'd not need the describe block either):
it 'includes all the correct client options`, :aggregate_failures' do
client_options = strategy.options.client_options
expect(client_options.site).to eq('https://launchpad.classlink.com')
expect(client_options.authorize_url).to eq('/oauth2/v2/auth')
expect(client_options.token_url).to eq('/oauth2/v2/token')
end
Another option would be to keep the group, but use a one-line syntax dropping the text description:
| it 'should have correct site' do | |
| expect(subject.options.client_options.site).to eq('https://launchpad.classlink.com') | |
| end | |
| it 'should have correct authorize url' do | |
| expect(subject.options.client_options.authorize_url).to eq('/oauth2/v2/auth') | |
| end | |
| it 'should have correct token url' do | |
| expect(subject.options.client_options.token_url).to eq('/oauth2/v2/token') | |
| end | |
| subject(:client_options) { strategy.options.client_options } | |
| specify { expect(client_options.site).to eq('https://launchpad.classlink.com') } | |
| specify { expect(client_options.authorize_url).to eq('/oauth2/v2/auth') } | |
| specify { expect(client_options.token_url).to eq('/oauth2/v2/token') } |
| allow(subject).to receive(:session).and_return({}) | ||
| end | ||
|
|
||
| it 'should have correct default scopes' do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd question the need for context blocks and before blocks for a single spec, when we could put everything into a single expectation.
Co-authored-by: Scott Matthewman <[email protected]>
Co-authored-by: Scott Matthewman <[email protected]>
Co-authored-by: Scott Matthewman <[email protected]>
BREAKING CHANGE: None - all existing field names preserved for compatibility