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

Issue 705 iam admins should be able to suspend clients #715

Conversation

garaimanoj
Copy link
Contributor

@garaimanoj garaimanoj commented Feb 26, 2024

  • Button added to enable/disable client
  • New columns added to CLIENT_DETAILS table
  • Setting status as active (True) during new client and client copy operation
  • Test cases to check new database fields are updated after client disable operation

Based on the active status of client, disable and restore button will toggle the status
@garaimanoj garaimanoj linked an issue Feb 26, 2024 that may be closed by this pull request
@garaimanoj garaimanoj changed the base branch from master to develop February 26, 2024 16:26
<h1>
<i class="fa fa-rocket"></i>&nbsp;&nbsp; <span
ng-if="!$ctrl.newClient">{{$ctrl.clientVal.client_name}}</span>
<span ng-if="!$ctrl.newClient && !$ctrl.clientVal.active"> (Suspended)</span>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Change 1

Copy link
Member

Choose a reason for hiding this comment

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

You can format this suspended text into a colored label

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image

  • I have changed the style of Suspended label.

  • On hover you can have the tooltip with more details of the suspension.

<button class="btn btn-success" ng-click="$ctrl.saveClient()">Save client</button>
<button class="btn btn-danger" ng-click="$ctrl.deleteClient()" ng-if="!$ctrl.newClient">Delete
client</button>
<client-status client="$ctrl.clientVal" ng-if="!$ctrl.newClient"></client-status>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Change 2

@@ -107,6 +107,13 @@ public ClientDetailsEntity updateClient(ClientDetailsEntity client) {
return clientRepo.save(client);
}

@Override
@CacheEvict(cacheNames = DefaultScopeMatcherRegistry.SCOPE_CACHE_KEY, key = "{#clientId}")
Copy link
Contributor

Choose a reason for hiding this comment

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

I would use the same key as updateClient()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Using same key as updateClient() now. Thanks.

Copy link
Member

Choose a reason for hiding this comment

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

We can avoid the cache eviction here because client caching is currently only related to scope matchers.

@federicaagostini
Copy link
Contributor

federicaagostini commented Mar 19, 2024

Looks good to me, let's wait for the others :)

In the meantime, I don't know if it would make sense to add the possibility to disable client also to users who own the client (even if not admins). Right now, I don't see a side effect for that.

@enricovianello enricovianello marked this pull request as ready for review March 19, 2024 19:18
@enricovianello enricovianello self-requested a review March 19, 2024 19:18
@garaimanoj
Copy link
Contributor Author

Thanks for your review. I have noticed some JavaScript error. Currently trying to fix those. I will let you know once those are fixed.
Shall I add the information of by whom and when the client was suspended in the dashboard in this pull request?

@garaimanoj garaimanoj self-assigned this Mar 21, 2024
(18, 'admin-client-rw', 'secret', 'Admin client (read-write)', false, null, 3600, 600, true, 'SECRET_POST',false, null, CURRENT_TIMESTAMP()),
(19, 'public-client', null, 'Public client', false, 3600, 3600, 600, true, 'NONE', false, null, CURRENT_TIMESTAMP());
token_endpoint_auth_method, require_auth_time, device_code_validity_seconds, created_at, active) VALUES
(1, 'client', 'secret', 'Test Client', false, null, 3600, 600, true, 'SECRET_BASIC',false, null, CURRENT_TIMESTAMP(), true),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is it ok to populate the test data here in this file or shall I do it in V102__add_active_and_status_changed_on_to_client_details.sql

Copy link
Contributor

Choose a reason for hiding this comment

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

Here is fine! Just keep in mind that if you use a local mysql it might give you an error when you run the tests and so you should delete it and repopulate it

@@ -88,6 +88,7 @@
<div>
<a ui-sref="client({id: c.client_id})">
{{c.client_name}}
<span ng-if="!c.active"> (Suspended)</span>
</a>
Copy link
Member

Choose a reason for hiding this comment

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

We can separate (Suspended) from the client link and add it as a colored label under the client UUID

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image

Copy link
Contributor

Choose a reason for hiding this comment

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

image

I personally would change the style/colour of the label to differentiate it from scopes, for example:

Screenshot from 2024-03-25 12-20-15

What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not bad. This one looks prominent and distinct.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image

I have used label-danger from Bootstrap for the label.

Copy link

sonarcloud bot commented Apr 5, 2024

darcato and others added 4 commits April 10, 2024 14:06
* Add db migration to add LAST_USED column on IAM_ACCOUNT_CLIENT

* Revert "Add db migration to add LAST_USED column on IAM_ACCOUNT_CLIENT"

This reverts commit 56aef73.

* Adding table CLIENT_LAST_USED

* Adding last used column on clients table on webapp

* Adding Client Last Used entity and repository

* Simple GET api

* Fix repo type

* Reading last_used as ClientDetails column

* Update frontend

* Updating client last_used when generating access token

* Last used on refreshAccessToken

* Update mitreid.version to 1.3.6.iam-issue-605

* Add TOKEN event to IamAuditApplicationEvent and publish AccessTokenIssuedEvent and AccessTokenRefreshedEvent in IamTokenService

* Header

* Add updateLastUsed methods to ClientService

* Remove update of last_used on client update

* Adding mysql migration

* Save client on update of last_used

* Do not save on clientRepo when updating last_used

* Do not write to DB when same date

* Adding IAM_CLIENT_TRACK_LAST_USED env variable

* Remove unused clientRepo

* Suppress deprecation warnings

* Move LAST_USED to its own table

* Add missing mysql migration

* Rename client_last_used and last_used_id

* Rename ClientLastUsedEntity and use LocalDate

* Serialize LocalDate as String

* Use a shared primary key

* Bump migration from V97 to V100 after rebase

* Remove unused import

* Add first test

* Update MitreID version

* Add more tests

* Refactor tests with TestTokenUtils

* Use final version of MitreID

* Change assertThat  with assertTrue

* Hide column on dashboard when tracking disabled

* Enable the tracking by default

* Update migrations to latest available number

---------

Co-authored-by: Enrico Vianello <enrico.vianello@cnaf.infn.it>
@enricovianello
Copy link
Member

Replaced by #747

@enricovianello enricovianello deleted the issue-705-iam-admins-should-be-able-to-suspend-clients branch April 15, 2024 15:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants