Refactor validate token create

[majst01]

Description

Use tokenpermissions to validate the token create/update/refresh call.

[codecov]

Codecov Report

❌ Patch coverage is 86.66667% with 14 lines in your changes missing coverage. Please review.
✅ Project coverage is 68.86%. Comparing base (a9bb726) to head (0df3e61).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
pkg/service/token/token-service.go	84.61%	7 Missing and 5 partials ⚠️
pkg/request/tokenpermissions.go	90.47%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #97      +/-   ##
==========================================
- Coverage   69.20%   68.86%   -0.35%     
==========================================
  Files         113      113              
  Lines        9629     9626       -3     
==========================================
- Hits         6664     6629      -35     
- Misses       2259     2297      +38     
+ Partials      706      700       -6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[iljarotar]

Error messages look good. One thing I'm still missing is the information which tenant or project some requested role is not allowed for. E.g. if I request editor role for two projects and it's denied for one of them, I'd like to know which one.

[majst01]

Error messages look good. One thing I'm still missing is the information which tenant or project some requested role is not allowed for. E.g. if I request editor role for two projects and it's denied for one of them, I'd like to know which one.

This would be possible if we also add the deniedSubjects to the list of errors instead of returning early after the deniedRoles are found.

[iljarotar]

Error messages look good. One thing I'm still missing is the information which tenant or project some requested role is not allowed for. E.g. if I request editor role for two projects and it's denied for one of them, I'd like to know which one.

This would be possible if we also add the deniedSubjects to the list of errors instead of returning early after the deniedRoles are found.

Yes, maybe as a map role -> project/tenant.

[majst01]

Error messages look good. One thing I'm still missing is the information which tenant or project some requested role is not allowed for. E.g. if I request editor role for two projects and it's denied for one of them, I'd like to know which one.

This would be possible if we also add the deniedSubjects to the list of errors instead of returning early after the deniedRoles are found.

Yes, maybe as a map role -> project/tenant.

Will try

[Gerrit91]

Maybe to make it clearer that the permissions are actually compared with the permissions from the database and not the contents of the requesting token?

Suggested change

	wantErrMessage: `permission_denied: requested roles: [PROJECT_ROLE_EDITOR] are not allowed with your current token`,
	wantErrMessage: `permission_denied: requested roles: [PROJECT_ROLE_EDITOR] are not allowed with your current user permissions`,

[Gerrit91]

I am not sure if we should attempt to map permissions back to roles. 😅

1. This function would look really simple without the "reverse lookup". For this package I think it's beneficial that the code strives to be as simple as it gets.
2. An early return with the first found problem is resource-efficient and it should be enough for a user to figure out the problem with his request quite quickly. I assume 90% of the use-cases are not as complex and people want to create an API token for a specific project for example. When you have a UI, this will not even show invalid input parameters for the requests. For CLI or CI scenarios, when it says "MachineCreate is not allowed on project-a with your current user permissions" it should be pretty clear what needs to be done by the user. Having the correct role probably resolves all the other problems that this function would return. Of course, there can be more complex scenarios but I don't think it will be incredibly time-consuming for a user to identify and resolve the problems even if you do not present him with the complete information.
3. I hope that at some point we have dynamic roles (e.g. a role that's called "metal-core" and has a set of methods that comes with it). The static roles that come from the API definitions are not enough in this case and this introduces another place that needs to be adapted when implementing this feature.

[iljarotar]

Agreed.

[majst01]

Simplified, shift responsibility back to the user which wants to create/update a token for the sake of much simpler validation logic.