@tancnle good find! I just added a severity3 label. Would you feel comfortable estimating a Weight already, or would it take a bit more time to evaluate?
I would say we drop the [1] call in PersonalAccessTokens::CreateService but then realised we need that for when the user creates personal access tokens. I think the interaction between ResourceAccessTokens <-> PersonalAccessTokens needs to be broken up to fix this issue 🤔.
@tancnle that makes perfect sense. And hopefully there's no hidden complexity in doing that. Thanks for adding the weight, I just bumped to workflowscheduling.
Just noting here that I'm still working on the issue. Weighing the duplication of PAT creation code (which is fairly small) against the broader issue with services calling other services and possibly creating duplicate audit events. Leaning towards the former.
Got maintainer feedback on !109032 (closed) that removing the call to PersonalAccessTokens::CreateService from ResourceAccessTokens::CreateService is less than ideal, since the PAT service does additional logging and permission checking. I was suggested that the audit event created by ResourceAccessTokens::CreateService may actually be the redundant one.
Right now the RAT service creates four separate audit events:
the RAT service audit
PAT creation
creation of the "bot" user to associate with the generated PAT
creation of associated email
So there are already a number of audit events being created from this one operation. I am curious if this could also be addressed by rewording the audit event message for project token creation - perhaps adding the token scope details to PersonalAccessToken and/or making it clearer that the extra events are initiated from the Project level.
As it stands, I'm not completely confident that the additional audit event is an actual bug. I will solicit more comments on this.
As it stands, I'm not completely confident that the additional audit event is an actual bug. I will solicit more comments on this.
@ahuntsman I am inclined to agree with your assessment that this isn't a bug. In the screenshot on the issue description, the 2 line items answer separate questions...so I would expect separate events.
In an audit, you may want to answer the question "who has created project access tokens, and for whom?"
In an audit, you may want to answer the question "who currently has personal access tokens?"
for 1 you could filter by the first line item, for 2 you could filter by the second. It seems like creating project access tokens at an admin level is an action with multiple effects, and It seems like it makes sense for each of those effects to have an audit event
If we think of only the auditing model being created here then we should audit PAT creation using PAT service only as it's the one responsible for creating tokens. for this, we might need to pass down resource_type so it can differ between personal access tokens and resource access tokens.
RAT service does create other audit events also which we can leave as it is as they are auditing models being created/updated.
Based on discussion above, I propose closing this issue, as the code, to the best of our understanding, is working as intended. The original issue also omits some context that shows Resource Access Token generation is comprised of a few other auditable events.