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

[GT-184] Add support for renewing API credentials #464

Open
wants to merge 2 commits into
base: dev
Choose a base branch
from

Conversation

Sae126V
Copy link
Contributor

@Sae126V Sae126V commented Aug 4, 2023

Resolves

" #438 and #453 "

@Sae126V Sae126V force-pushed the GT-184-Extend-API-Credential-Management-to-handle-periodic-renewal branch 3 times, most recently from 5728cd9 to 6f26459 Compare August 7, 2023 07:52
@Sae126V Sae126V marked this pull request as ready for review August 7, 2023 07:55
@Sae126V Sae126V requested a review from a team as a code owner August 7, 2023 07:55
Comment on lines 335 to 366
// This would probably be the place hook for any future policy acceptance tracking
if (($user->getId() != $authEntity->getUser()) || $isRenewalRequest) {
$authEntity->setLastRenewTime();
}
Copy link
Member

Choose a reason for hiding this comment

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

As future work, we should probably always update the renewal time when an API credential is edited, regardless of whether the editing user is the owning user or not, which I think would simplify the logic here.

@Sae126V, could you open an issue for this?

Comment on lines 257 to 270
//If the entity is of type OIDC subject, do a more thorough check again
if (
$type == 'OIDC Subject' &&
!preg_match("/^([a-f0-9]{8}\-[a-f0-9]{4}\-[a-f0-9]{4}\-[a-f0-9]{4}\-[a-f0-9]{12})$/", $identifier)
!preg_match(
"/^([a-f0-9]{8}\-[a-f0-9]{4}\-[a-f0-9]{4}\-[a-f0-9]{4}\-[a-f0-9]{12})$/",
$identifier
)
) {
throw new \Exception("Invalid OIDC Subject");
}
Copy link
Member

Choose a reason for hiding this comment

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

We have very similar checks on OIDC subject validity here and in /lib/Gocdb_Services/Site.php (and of course the checks differ slightly, though I believe this one is more correct.

Factoring this out, maybe pushing it (and X.509 validation as above) into the existing validation layer is a way forward.

@Sae126V, can you open an issue?

@@ -94,41 +103,61 @@ public static function usage($message = '')
print
(
"Usage: php ManageAPICredentials.php [--help | -h] [--dry-run] \\\ \n" .
" [--renewal | -r] \\\ \n" .
Copy link
Member

Choose a reason for hiding this comment

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

can we pass the property name as an option directly, rather than have a flag we specificy for one use case but not the other.

something like --property [ lastUseTime | lastRenewTime ] ?

Copy link
Member

@tofu-rocketry tofu-rocketry Aug 14, 2023

Choose a reason for hiding this comment

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

"inactive" and "renewals" as the properties.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Specified two flags now one for lastUseTime and the other for lastRenewTime. instead of one.

"the last $elapsedMonths months and will be deleted if this period of inactivity\n" .
"reaches $deletionThreshold months.\n\n";
list($headers, $siteName) = $this->getHeaderContent($api, $fromEmail, $replyToEmail);
list($body, $userEmail) = $this->getInitialBodyContent($api);
Copy link
Member

Choose a reason for hiding this comment

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

With the body being split up like this, I find it hard to get a sense of what is being sent to the user.

Maybe a mapping between properties and body text would be easier alleviate this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wrapped into single function that would provide fix to ease complexity.

@Sae126V Sae126V marked this pull request as draft August 18, 2023 14:37
@Sae126V Sae126V force-pushed the GT-184-Extend-API-Credential-Management-to-handle-periodic-renewal branch from bc37f58 to 6259cc5 Compare August 18, 2023 14:50
@Sae126V Sae126V marked this pull request as ready for review August 18, 2023 14:51
$params = array();

if ($_REQUEST['isRenewalRequest']) {
$newValues['isRenewalRequest'] = $params['isRenewalRequest'] = true;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
$newValues['isRenewalRequest'] = $params['isRenewalRequest'] = true;
$newValues['isRenewalRequest'] = true;
$params['isRenewalRequest'] = true;

if that is indeed the desired behaviour, it's clearer what's going on and doesn't have two assignments on one line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is indented behaviour. Sure, I have removed assigning multiple variable at once.

@Sae126V Sae126V marked this pull request as draft September 11, 2023 08:32
@Sae126V Sae126V force-pushed the GT-184-Extend-API-Credential-Management-to-handle-periodic-renewal branch from 8afb986 to cd82c9e Compare September 13, 2023 07:54
@Sae126V
Copy link
Contributor Author

Sae126V commented Sep 13, 2023

@gregcorbett , This is ready for the functionality review.

@Sae126V Sae126V marked this pull request as ready for review September 13, 2023 07:55
Copy link
Member

@gregcorbett gregcorbett left a comment

Choose a reason for hiding this comment

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

Functionality looks good. I noticed that when emails were generated for both unrenewed and inactive API credentitals, I got:

> php resources/ManageAPICredentials/ManageUnusedAPICredentials.php --warning_threshold 9 --deletion_threshold 15 --renewals
...
The API credential associated with the following identifier
registered at site X has not been renewed for
the last 0 months and will be deleted if it reaches 15 months.

Where 0 should have been closer to 12, which is why the warning email was generated. Could you look into that?

@gregcorbett
Copy link
Member

Could you also add printing out the last renewed time to reportDryRun?

@Sae126V
Copy link
Contributor Author

Sae126V commented Sep 14, 2023

Could you also add printing out the last renewed time to reportDryRun?

Sure, Greg. Added the support for last renewed time in 0d1ec9a

@Sae126V Sae126V marked this pull request as draft September 14, 2023 09:35
@Sae126V
Copy link
Contributor Author

Sae126V commented Sep 14, 2023

Functionality looks good. I noticed that when emails were generated for both unrenewed and inactive API credentitals, I got:

> php resources/ManageAPICredentials/ManageUnusedAPICredentials.php --warning_threshold 9 --deletion_threshold 15 --renewals
...
The API credential associated with the following identifier
registered at site X has not been renewed for
the last 0 months and will be deleted if it reaches 15 months.

Where 0 should have been closer to 12, which is why the warning email was generated. Could you look into that?

Right. I see the flow of code was designed such that it calculates the month difference i.e. say, Sept to sept the month difference is zero and it ignored the year part. I have provided a fix in 0d1ec9a.

@Sae126V
Copy link
Contributor Author

Sae126V commented Sep 14, 2023

Ready for the functionality review with the fix and suggestions @gregcorbett.

@Sae126V Sae126V marked this pull request as ready for review September 14, 2023 14:07
Sae126V added a commit to Sae126V/gocdb that referenced this pull request Sep 19, 2023
Sae126V added a commit to Sae126V/gocdb that referenced this pull request Sep 19, 2023
Sae126V added a commit to Sae126V/gocdb that referenced this pull request Sep 20, 2023
Sae126V added a commit to Sae126V/gocdb that referenced this pull request Sep 20, 2023
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.

3 participants