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

Administrators can disable a client #747

Merged
merged 13 commits into from
May 24, 2024
Merged

Administrators can disable a client #747

merged 13 commits into from
May 24, 2024

Conversation

enricovianello
Copy link
Member

Replaces #715

@garaimanoj @enricovianello
Add ability to disable client
9451f09 
* Add API and service methods to change client status
* Add new columns for client status
* Set status as active for new client
* Add suspended label next to client name
* Add column status_changed_by to client_details
* Make client suspension details available to client owner
@enricovianello enricovianello mentioned this pull request Apr 15, 2024
Closed
@garaimanoj garaimanoj self-assigned this Apr 15, 2024
enricovianello
enricovianello commented May 2, 2024
View reviewed changes
Copy link
Member Author

@enricovianello enricovianello left a comment

Choose a reason for hiding this comment

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

I would suggest a refactoring that aligns the new functionality with the others in "FindAccount" API. Mainly by returning ScimUser and splitting the status endpoint in two enable|disable endpoints so the status is not necessary in body, just the clientId.

iam-login-service/src/main/java/it/infn/mw/iam/api/account/find/FindAccountController.java Show resolved Hide resolved
iam-login-service/src/main/java/it/infn/mw/iam/api/account/find/DefaultFindAccountService.java Show resolved Hide resolved
iam-login-service/src/main/java/it/infn/mw/iam/api/account/find/FindAccountService.java Outdated Show resolved Hide resolved
iam-login-service/src/main/java/it/infn/mw/iam/api/account/find/FindAccountController.java Outdated Show resolved Hide resolved
iam-login-service/src/main/java/it/infn/mw/iam/api/account/find/FindAccountController.java Outdated Show resolved Hide resolved
...in/java/it/infn/mw/iam/api/client/registration/service/DefaultClientRegistrationService.java Show resolved Hide resolved
...ervice/src/main/java/it/infn/mw/iam/api/client/management/ClientManagementAPIController.java Outdated Show resolved Hide resolved
enricovianello
enricovianello commented May 15, 2024
View reviewed changes
Copy link
Member Author

@enricovianello enricovianello left a comment

Choose a reason for hiding this comment

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

I'd move the authz checks at controller level. In the cases users is not allowed to restore a disabled client the exception and a FORBIDDEN response could be returned.

...in/java/it/infn/mw/iam/api/client/registration/service/DefaultClientRegistrationService.java Outdated Show resolved Hide resolved
...ervice/src/main/java/it/infn/mw/iam/api/client/management/ClientManagementAPIController.java Outdated Show resolved Hide resolved
...ervice/src/main/java/it/infn/mw/iam/api/client/management/ClientManagementAPIController.java Outdated Show resolved Hide resolved
...ce/src/main/java/it/infn/mw/iam/api/client/registration/ClientRegistrationApiController.java Outdated Show resolved Hide resolved
@federicaagostini federicaagostini mentioned this pull request May 16, 2024
Closed
@rmiccoli rmiccoli linked an issue May 16, 2024 that may be closed by this pull request
IAM admins should be able to suspend clients #705
Closed
enricovianello
enricovianello commented May 22, 2024
View reviewed changes
Copy link
Member Author

@enricovianello enricovianello left a comment

Choose a reason for hiding this comment

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

Just a question about status_changed_on default value.
I think after this it's all ok!

...in/resources/db/migration/mysql/V103__add_active_and_status_changed_on_to_client_details.sql Outdated Show resolved Hide resolved
@sonarcloud SonarCloud
Copy link

sonarcloud bot commented May 24, 2024

Quality Gate Passed Quality Gate passed

Issues
12 New issues
0 Accepted issues

Measures
0 Security Hotspots
90.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

@enricovianello enricovianello merged commit b0fe8a1 into develop May 24, 2024
4 checks passed
@enricovianello enricovianello deleted the issue-705 branch May 24, 2024 14:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@garaimanoj garaimanoj garaimanoj left review comments

@federicaagostini federicaagostini Awaiting requested review from federicaagostini

@rmiccoli rmiccoli Awaiting requested review from rmiccoli

Assignees

@garaimanoj garaimanoj

Labels
None yet
Projects
v1.9.0
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

IAM admins should be able to suspend clients
2 participants
@enricovianello @garaimanoj