refactor: optimize keycloak interactions

[shreddedbacon]

Checklist

• Affected Issues have been mentioned in the Closing issues section
• Documentation has been written/updated
• PR title is ready for inclusion in changelog

To improve API performance, a number of changes have been made to keycloak queries to get even more information in the first request to reduce the number of loops that were done previously.

This also removes Redis, as the request times for even large users and groups are considerably faster now than before, but there is still the possibility of refactoring to re-introduce Redis later on, if required.

For example, previously a user in 130 groups could see the allProjects query that the UI uses taking over 60 seconds to load on an initial load against the API directly, when no redis cache exists, and still a few seconds to load even when redis cache was used. With these improvements, it is now reduced down to a few milliseconds, without redis.

Another example is querying the groups resolver directly, for example, if there were 2000 groups, before this would eventually time out (90seconds +) in the API because it took too long. This now takes a few seconds to return, longer if the members of groups are requested too.

The bulk of the changes are to interactions with groups, by utilising the briefRepresentation: false flag when retrieving groups from keycloak, this flag means that attributes of groups are returned in the queries, so this eliminated the requirement to loop over the groups to get the additional information like before.

With these improvements, it also means that hasPermissions is considerably faster than it was before (even with redis), and previously we had to hardcode a permission override which has now been removed so all queries perform permissions as required, and still see a performance increase.

lagoon/services/api/src/resources/project/resolvers.ts

Lines 91 to 99 in f80eb45

	// This resolver is used for the main UI page and is quite slow. Since we've
	// already authorized the user has access to all the projects we are
	// returning, AND all user roles are allowed to view all environments, we can
	// short-circuit the slow keycloak check in the getEnvironmentsByProjectId
	// resolver.
	//
	// @TODO: When this performance issue is fixed for real, remove this hack as
	// it hardcodes a "everyone can view environments" authz rule.
	return withK8s.map(row => ({ ...row, environmentAuthz: true }));

[shreddedbacon]

I've marked this as ready for review, with this note though:

This contains a bunch of commented out redis code that would need to be refactored considerably if redis was to be turned back on. I've left it in place where it was used, but I'd like to see this work without redis before we throw it back in, as I think it won't really help much even if it is turned on with the type of data being requested (you'd basically be storing the request from keycloak directly rather than ID references like before) but all the iteration to gather information was still there. Where we now collect more information up front and filter it way faster, the resolving of further information is faster already.

EDIT: I've updated this PR so this comment is out of date

[smlx]

This makes some fairly invasive changes to the authentication logic. Would it be possible to add some unit tests around at least auth.ts to validate the behaviour?

[shreddedbacon]

This makes some fairly invasive changes to the authentication logic. Would it be possible to add some unit tests around at least auth.ts to validate the behaviour?

I don't fully understand testing JS frameworks, but I can look into this. But I'm also curious which parts concern you here, so that I can focus efforts.

[smlx]

Well I mean all the new code really, but particularly the new JS mapper, and the new functions in auth.ts. Here's one example that might break: in the new Keycloak mapper you have code that splits the lagoon-projects attribute and parses the Integers. However as seen in #3358 this field can have buggy data including empty array entries. When you call parseInt() on an empty string it returns NaN.

Does the mapper put NaN in the token under some circumstances? What happens when this comes back in the token and is handled by getUserProjectIdsFromToken()? It would be great to test this to ensure it doesn't result in unexpected behaviour.

[shreddedbacon]

Does the mapper put NaN in the token under some circumstances? What happens when this comes back in the token and is handled by getUserProjectIdsFromToken()? It would be great to test this to ensure it doesn't result in unexpected behaviour.

It doesdid 🙈

[rocketeerbkw]

Functionally LGTM and tested locally. Any nitpicks can be addressed in a follow-up.