-
Notifications
You must be signed in to change notification settings - Fork 2
Fix duplicate service ID #306
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
Merged
Merged
Changes from all commits
Commits
Show all changes
12 commits
Select commit
Hold shift + click to select a range
716f46c
Merge pull request #298 from RADAR-base/release-4.4.9
mpgxvii 116ac4e
Merge remote-tracking branch 'origin/master' into dev
yatharthranjan 0dc5c2a
add check for duplicate external user ids (service ids)
yatharthranjan 7d48d61
add unique constraint at JPA layer
yatharthranjan 4d40ddd
Merge branch 'dev' into fix_duplicate_service_id
yatharthranjan a9e0ad1
Update db changelog
mpgxvii f8fa7bd
Add separate existing external user id check when updating the user t…
mpgxvii 4a3d761
Remove unnecessary spaces
mpgxvii 49109ac
Show subjectId in error message for duplicate service ids
mpgxvii fc94f82
Remove unused userRepository in RegistrationResource
mpgxvii 071de94
Throw error when external user id is null when validating this
mpgxvii 9287d15
Merge branch 'dev' into fix_duplicate_service_id
this-Aditya File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,6 +5,7 @@ import jakarta.ws.rs.core.Context | |
| import jakarta.ws.rs.core.Response | ||
| import org.radarbase.auth.authorization.EntityDetails | ||
| import org.radarbase.auth.authorization.Permission | ||
| import org.radarbase.authorizer.api.RestOauth2AccessToken | ||
| import org.radarbase.authorizer.api.RestSourceUserDTO | ||
| import org.radarbase.authorizer.api.RestSourceUserMapper | ||
| import org.radarbase.authorizer.api.TokenDTO | ||
|
|
@@ -13,6 +14,7 @@ import org.radarbase.authorizer.doa.entity.RestSourceUser | |
| import org.radarbase.jersey.auth.AuthService | ||
| import org.radarbase.jersey.exception.HttpApplicationException | ||
| import org.radarbase.jersey.exception.HttpBadRequestException | ||
| import org.radarbase.jersey.exception.HttpConflictException | ||
| import org.radarbase.jersey.exception.HttpNotFoundException | ||
| import kotlin.time.Duration.Companion.seconds | ||
|
|
||
|
|
@@ -106,6 +108,36 @@ class RestSourceUserService( | |
| ) | ||
| } | ||
|
|
||
| /** | ||
| * Validates that an external user ID is not already in use by another user. | ||
| * Should be called before updating a user's token with an external user ID. | ||
| */ | ||
| private suspend fun validateExternalUserId(token: RestOauth2AccessToken?, user: RestSourceUser) { | ||
| if (token?.externalUserId != null) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we add an else block to handle when
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added now 👍 |
||
| val existingUser = userRepository.findByExternalId(token.externalUserId, user.sourceType) | ||
| if (existingUser != null && existingUser.id != user.id) { | ||
| throw HttpConflictException( | ||
| "external_user_id_already_exists", | ||
| "External user ID ${token.externalUserId} is already registered for another user of source type ${user.sourceType} and user id ${existingUser.userId}", | ||
| ) | ||
| } | ||
| } else { | ||
| throw HttpBadRequestException( | ||
| "missing_external_user_id", | ||
| "External user ID cannot be empty", | ||
| ) | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Updates a user's token after validating the external user ID. | ||
| * This method ensures no duplicate external user IDs exist in the system. | ||
| */ | ||
| suspend fun updateUserToken(token: RestOauth2AccessToken?, user: RestSourceUser): RestSourceUser { | ||
| validateExternalUserId(token, user) | ||
| return userRepository.updateToken(token, user) | ||
| } | ||
|
|
||
| suspend fun ensureToken(userId: Long): TokenDTO { | ||
| ensureUser(userId, Permission.MEASUREMENT_CREATE) | ||
| return runLocked(userId) { user -> | ||
|
|
@@ -151,7 +183,7 @@ class RestSourceUserService( | |
| } | ||
|
|
||
| val token = authorizationService.refreshToken(user) | ||
| val updatedUser = userRepository.updateToken(token, user) | ||
| val updatedUser = updateUserToken(token, user) | ||
|
|
||
| if (!updatedUser.authorized) { | ||
| throw HttpApplicationException( | ||
|
|
||
41 changes: 41 additions & 0 deletions
41
.../resources/db/changelog/changes/20240607120000_add_unique_constraint_external_user_id.xml
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,41 @@ | ||
| <?xml version="1.0" encoding="utf-8"?> | ||
| <databaseChangeLog | ||
| xmlns="http://www.liquibase.org/xml/ns/dbchangelog" | ||
| xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" | ||
| xsi:schemaLocation="http://www.liquibase.org/xml/ns/dbchangelog | ||
| https://www.liquibase.org/xml/ns/dbchangelog/dbchangelog-3.10.xsd"> | ||
|
|
||
| <changeSet author="yatharth" id="20240607120000-add-unique-constraint-external-user-id"> | ||
| <!-- Update duplicates to a fixed value for investigation, appending the external_user_id, except the row with the lowest id --> | ||
| <preConditions onFail="CONTINUE"> | ||
| <not> | ||
| <sqlCheck expectedResult="0"> | ||
| SELECT COUNT(*) FROM ( | ||
| SELECT external_user_id, source_type FROM rest_source_user | ||
| WHERE external_user_id IS NOT NULL | ||
| GROUP BY external_user_id, source_type | ||
| HAVING COUNT(*) > 1 | ||
| ) t; | ||
| </sqlCheck> | ||
| </not> | ||
| </preConditions> | ||
| <sql dbms="postgresql"> | ||
| UPDATE rest_source_user a | ||
| SET external_user_id = 'DUPLICATE_INVESTIGATE_' || a.id || '_' || a.external_user_id | ||
| FROM rest_source_user b | ||
| WHERE a.external_user_id = b.external_user_id | ||
| AND a.source_type = b.source_type | ||
| AND a.id > b.id | ||
| AND a.external_user_id IS NOT NULL | ||
| AND b.id = ( | ||
| SELECT MIN(id) FROM rest_source_user c | ||
| WHERE c.external_user_id = a.external_user_id | ||
| AND c.source_type = a.source_type | ||
| ); | ||
| </sql> | ||
| <addUniqueConstraint | ||
| tableName="rest_source_user" | ||
| columnNames="external_user_id,source_type" | ||
| constraintName="uk_rest_source_user_external_user_id_source_type"/> | ||
| </changeSet> | ||
| </databaseChangeLog> |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Since we are replacing
userRepository::updateToken, the variableuserRepositoryis now unused and can be removedThere 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.
Thanks for reviewing this. Removed now.