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.

Sign up for GitHub

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

Jump to bottom

Spring Security 5 OAuth2 #1339

Merged
merged 41 commits into from
Aug 18, 2022
Merged

Spring Security 5 OAuth2 #1339

merged 41 commits into from
Aug 18, 2022

Conversation

ericenns
Copy link
Member

@ericenns ericenns commented Jul 12, 2022 •
edited
Loading

Description of changes

Replaced deprecated OAuth implementation using spring-security-oauth2 with spring security 5 OAuth2 resource and authorization server.

Related issue

Link to the GitHub issue this pull request addresses using the #issuenum format. If it completes an issue, use Fixes #issuenum to automatically close the issue.

Checklist

Things for the developer to confirm they've done before the PR should be accepted:

  • CHANGELOG.md (and UPGRADING.md if necessary) updated with information for new change.
  • Tests added (or description of how to test) for any new features.
  • User documentation updated for UI or technical changes.

ericenns added 21 commits July 6, 2022 17:29
@ericenns
Initial working version of OAuth using in memory stores.
f72c4bc
@ericenns
Update how current user is accessed in services and repositories.
2d8d1fb
@ericenns
Add back CURRENT_USER_DETAILS to session.
2ebef56
@ericenns
Working OAuth authentication using db backend.
3bd8ffe
@ericenns
OAuth2 working again with IridaClientDetails
18e5cbf
@ericenns
Added in liquibase migration for adding in OAuth2 authorization tables.
fa964e5
@ericenns
Remvoved old OAuth code and fixed some tests.
b0d1052
@ericenns
Fix OAuth2 password grant scope parameter validation.
aa508e9
@ericenns
Update OAuth2 flow for remote IRIDA connections to use state variable…
3263cf0 
… instead of adding in URL params.
@ericenns
Change remote_api_token token storage to use TEXT instead of BLOB.
f54a286
@ericenns
Fix javadoc issues.
03a6764
@ericenns
Fix javadoc
4c1da4e
@ericenns
Added in token revocation
3b4b6cc
@ericenns
Update JWT to use provided RSA public and private keys specified in c…
5e9f927 
…onfig file.
@ericenns
Attempt fix galaxy integration tests.
04a87ed
@ericenns
Merge branch 'development' into spring-security-5-oauth2
3faba73
@ericenns
Attempt fix galaxy integration tests take two.
2c476bc
@ericenns
Add back in missing auto config for galaxy integration tests.
6676ee0
@ericenns
Add Transactional annotations to all methods in IridaOAuth2Authorizat…
5f6552f 
…ion that use db operations.
@ericenns
Moved stateMap for remote api OAuth flow into session object, instead…
579846f 
… of just encoding it directly in the variable.
@ericenns
Updated SecurityIT to check for expected authentication scheme in res…
1fbc42d 
…ponse header.
@JeffreyThiessen
Copy link
Member

I manually tested happy paths and error paths with the uploader, and then ran the integration suite on them and everything is working as intended.

JeffreyThiessen
JeffreyThiessen approved these changes Jul 20, 2022
View reviewed changes
Copy link
Member

@JeffreyThiessen JeffreyThiessen left a comment

Choose a reason for hiding this comment

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

Looks good

@ericenns ericenns requested a review from apetkau July 22, 2022 14:10
@ericenns ericenns marked this pull request as ready for review July 22, 2022 19:13
@ericenns
Copy link
Member Author

Just want to comment here that @deepsidhu85 confirmed that this update works with irida-galaxy-importer

deepsidhu85
deepsidhu85 requested changes Jul 26, 2022
View reviewed changes
Copy link
Contributor

@deepsidhu85 deepsidhu85 left a comment

Choose a reason for hiding this comment

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

This is looking good. Just a few things below. Just have a few things left to manually test as well

src/main/java/ca/corefacility/bioinformatics/irida/jackson2/mixin/RoleMixin.java Show resolved Hide resolved
src/main/java/ca/corefacility/bioinformatics/irida/oauth2/IridaOAuth2AuthorizationService.java Show resolved Hide resolved
src/main/java/ca/corefacility/bioinformatics/irida/oauth2/IridaRegisteredClientsRepository.java Show resolved Hide resolved
...facility/bioinformatics/irida/oauth2/OAuth2ResourceOwnerPasswordAuthenticationConverter.java Show resolved Hide resolved
...efacility/bioinformatics/irida/oauth2/OAuth2ResourceOwnerPasswordAuthenticationProvider.java Show resolved Hide resolved
...a/ca/corefacility/bioinformatics/irida/repositories/relational/auditing/UserRevListener.java Outdated Show resolved Hide resolved
...ain/java/ca/corefacility/bioinformatics/irida/ria/web/oauth/OltuAuthorizationController.java Show resolved Hide resolved
...acility/bioinformatics/irida/web/controller/api/RESTOAuthAuthorizationConsentController.java Show resolved Hide resolved
src/main/webapp/pages/oauth/authorization_consent.html Outdated Show resolved Hide resolved
@deepsidhu85
Copy link
Contributor

Update the documentation for the Remote IRIDA Connection OAuth approval modal

apetkau
apetkau requested changes Aug 5, 2022
View reviewed changes
Copy link
Member

@apetkau apetkau left a comment

Choose a reason for hiding this comment

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

Thanks so much for this Eric. It looks great. I did have a few comments/questions for you.

I also have yet to run this and test it out that way, so I may have additional feedback later.

build.gradle.kts Outdated Show resolved Hide resolved
doc/administrator/web/index.md Outdated Show resolved Hide resolved
doc/administrator/web/index.md Outdated Show resolved Hide resolved
doc/administrator/web/index.md Outdated Show resolved Hide resolved
doc/administrator/web/index.md Outdated Show resolved Hide resolved
...facility/bioinformatics/irida/database/changesets/unreleased/oauth2-authorization-tables.xml Show resolved Hide resolved
src/main/resources/ca/corefacility/bioinformatics/irida/sql/oauth-token.sql Show resolved Hide resolved
...corefacility/bioinformatics/irida/database/changesets/unreleased/refactor-client-details.xml Show resolved Hide resolved
src/main/resources/scripts/oauthclients/1-beforeUpdate.pl Show resolved Hide resolved
src/main/resources/application.properties Outdated Show resolved Hide resolved
apetkau
apetkau reviewed Aug 10, 2022
View reviewed changes
build.gradle.kts Outdated Show resolved Hide resolved
doc/administrator/web/index.md Outdated Show resolved Hide resolved
apetkau
apetkau approved these changes Aug 16, 2022
View reviewed changes
Copy link
Member

@apetkau apetkau left a comment

Choose a reason for hiding this comment

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

This all looks great to me. I tested out the uploader and syncing between IRIDA instances and it all works.

Thanks so much for addressing my previous comments.

deepsidhu85
deepsidhu85 approved these changes Aug 18, 2022
View reviewed changes
Copy link
Contributor

@deepsidhu85 deepsidhu85 left a comment

Choose a reason for hiding this comment

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

Tested it out and working as expected. Thanks!

@deepsidhu85 deepsidhu85 merged commit 04df790 into development Aug 18, 2022
@ericenns ericenns deleted the spring-security-5-oauth2 branch April 14, 2023 16:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@deepsidhu85 deepsidhu85 deepsidhu85 approved these changes

@JeffreyThiessen JeffreyThiessen JeffreyThiessen approved these changes

@apetkau apetkau apetkau approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ericenns @JeffreyThiessen @deepsidhu85 @apetkau