[GT-184] Add support for renewing API credentials - #464
Conversation
5728cd9 to
6f26459
Compare
| // This would probably be the place hook for any future policy acceptance tracking | ||
| if (($user->getId() != $authEntity->getUser()) || $isRenewalRequest) { | ||
| $authEntity->setLastRenewTime(); | ||
| } |
There was a problem hiding this comment.
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?
| //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"); | ||
| } |
There was a problem hiding this comment.
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?
| ( | ||
| "Usage: php ManageAPICredentials.php [--help | -h] [--dry-run] \\\ \n" . | ||
| " [--renewal | -r] \\\ \n" . |
There was a problem hiding this comment.
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 ] ?
There was a problem hiding this comment.
"inactive" and "renewals" as the properties.
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Wrapped into single function that would provide fix to ease complexity.
bc37f58 to
6259cc5
Compare
| $params = array(); | ||
|
|
||
| if ($_REQUEST['isRenewalRequest']) { | ||
| $newValues['isRenewalRequest'] = $params['isRenewalRequest'] = true; |
There was a problem hiding this comment.
| $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.
There was a problem hiding this comment.
It is indented behaviour. Sure, I have removed assigning multiple variable at once.
8afb986 to
cd82c9e
Compare
|
@gregcorbett , This is ready for the functionality review. |
gregcorbett
left a comment
gregcorbett
left a comment
There was a problem hiding this comment.
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?
|
Could you also add printing out the last renewed time to |
Sure, Greg. Added the support for last renewed time in 0d1ec9a |
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. |
|
Ready for the functionality review with the fix and suggestions @gregcorbett. |
61d1865 to
0577fbc
Compare
| $this->em->getConnection()->beginTransaction(); | ||
| try { | ||
| // This would probably be the place hook for any future policy acceptance tracking | ||
| if ($user->getId() != $authEntity->getUser()) { |
There was a problem hiding this comment.
Need to understand this logic and around .
There was a problem hiding this comment.
This would probably be the place hook for any future policy acceptance tracking
I imagine this comment was around the fact that, if the owner of the API credential was changing - its probably a good thing ™️ to make them (re)agree to the end user notices. But as we don't track acceptance at all at the moment it's future work.
|
@Sae126V , can you remember why Was it to support being able to renew (and hence take ownership of, if necessary) a credential without otherwise editing it (i.e. a renew button in the portal)? If so, I wonder if there is a version of this change that doesn't touch |
Yep, so, it was changed to support the renew button. The problem is that a renewal doesn't send any form fields (the point of "renew without otherwise editing it"), but the existing I'll investigate whether there's a version that doesn't touch this file and get back to you by tomorrow |
Presumably we could just send the current values into |
a7d8e3d to
e123f64
Compare
|
I have changed the logic of renewal flow. This would be now separate from the editing user flow. Ready for the review now |
| * @param \User $user user performing the renewal | ||
| * @return \APIAuthentication the renewed credential | ||
| */ | ||
| public function renewAPIAuthEntity(\APIAuthentication $authEntity, \User $user) { |
There was a problem hiding this comment.
Why are this and renewAPIAuthentication both needed? they look like the take the same arguments?
There was a problem hiding this comment.
TBH, I have followed the pattern of as the other three
- addAPIAuthEntity/addAPIAuthentication
- editAPIAuthEntity/editAPIAuthentication
- deleteAPIAuthEntity/deleteAPIAuthentication.
The Site.php one does the checkUserAuthz permission check, the service one does the DB write.
There was a problem hiding this comment.
okay, please update the comments so the distinction between the two methods is clearer.
| // there should be 2 left after this operation (as 2 of | ||
| // the 6 fetched above have been renewed with 9 months). | ||
| $creds = $actions->deleteCreds($creds, 9); | ||
| $creds = $actions->deleteCreds($creds, 9, true); |
There was a problem hiding this comment.
can you add a comment about what true does here?
| // remove credentials last used more than 13 months ago | ||
| // there should be one left after this operation | ||
| $creds = $actions->deleteCreds($creds, 13); | ||
| $creds = $actions->deleteCreds($creds, 13, false); |
There was a problem hiding this comment.
can you add a comment about what false does here?
e058dda to
00858bd
Compare
00858bd to
491abeb
Compare
3 participants
Resolves
" #438 and #453 "