Avoid creation of new object.

Similarly for the following methods.

I wonder whether MultivaluedHashMap actually makes any sense anymore, and whether we can do just with Map<String, List<AuthenticationExecutionModel>>? That would simplify the code even more

Done

Thank you. Yet I still wonder about the following:

I wonder whether MultivaluedHashMap actually makes any sense anymore, and whether we can do just with Map<String, List>? That would simplify the code even more

WDYT?

Also the other methods below this one should be changed similarly

There are other places we can improve in caches. We should probably have a separate task for it. I would leave it as is and handle this one and others later.

Ack, could you please introduce a JIRA for that?

This call produces an array of hashes like [{id: ..., realm: ...}, ...{id: ..., realm: ...}]. This contains lot of garbage and can be optimized to only return array of pairs like [["id1", "realm1"], ["id2", "realm2], ...]
Could there be an endpoint "/list" that the following implementation:

    @GET
    public Stream<String[]> listRealms() {
        return session.realms().getRealmsStream()
            map(r -> return new String[] {r.getId(), r.getName});
    }

This would remove all the logic linked to serialization of RealmRepresentation which would in turn lead to a more performant behaviour.

Wouldn't that mean changing the console too? If so, I'm not sure if the effort justifies the performance gain.

I would leave it as is, please. Later we can see how to improve it even more.

You're right, that could mean changing the console as well. Could the parameter have a more descriptive name, like idNameOnly?

Basic can mean more than id and name. Ideally, we should be following the rest guidelines so that we can have better control over the response. But that is another work. I would just leave as is.

For something else than full object representation, there is already used briefRepresentation. Please keep parameter names consistent across code base

I agree with renaming the parameter to briefRepresentation we are already using it in many places. One more question, should we default to true? I am not sure about all places where this endpoint is used, but I can't imagine any usecase where it is necessary to obtain all realms in one place, so true as the default value makes sense to me. WDYT?

I'll change the name. A big one though :) But if we are using at other places, so let's keep consistency. Hope we can sort this out better when we review our APIs based on the guidelines.

In regards to default to true, that could break backward compatibility. I would leave it as is.

@mhajas, I believe this is correct but would like you to check

This looks good to me, I like it. However, we should probably add it also to AbstractStorageManager, it should be used everywhere in future, so I think this is the correct place to add it.

@mhajas I've added the check at that method. Is that what you are looking for?

@pedroigor yes, exactly. Thanks!

@pedroigor Here we are skipping the authorization. It seems it is causing that when you open admin console as an administrator of some realm (other than master) you are able to see that there are other realms.

You can reproduce as follows:

1. Create realm1
2. Create realm2
3. Create admin user in realm2 and assign him with all realm-management roles
4. Log in to admin console of realm2 (http://localhost:8081/auth/admin/realm2/console) as user you created
5. Previously you could see only realm2, now you can see all realms and you can choose which one you want to manage (all except realm2 will show forbidden)
6. When I click realm2 I am seeing only this (I am not sure this is caused by this method though):
   

@mhajas Thanks for all the details about the issue. Looks like I missed the checks for filtering realms. It should be fixed now.

I'm not sure why you did not get the menu though. You were supposed to get that only after creating a new realm. Which now is fixed.

I also tried to make some improvements when accessing a particular realm when using admins other than master admin.

I agree we should run UI tests. Please let me know if you spot anything else because I know our UI tests do not provide full coverage.

@mhajas Btw, if you are testing performance as well, you should also consider increasing the number of entries in both realms and users cache.

My latest changes should reduce the time taken to get a token to ms, instead of seconds.

I can share with you the database dump I'm using for tests.

Thank you @pedroigor. It seems all issues I mentioned are fixed now. I played with it a little more and there is one more change in behaviour. When you start fresh keycloak with an admin user, after you navigate to admin console and log in, admin console is always asking what realm you want to manage (at #/realms), even when there is only one, while previously it opened #/realms/master. Is this change intentional?

@mhajas It should be fixed now.

Don't we potentially risk an NPE here:
https://github.com/keycloak/keycloak/blob/5a02edd02663905056d6744f299f3060afcd2232/model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/DefaultLazyLoader.java#L48

Good catch. I'll fix it.

Just out of curiosity, why aren't the scopes lazily loaded (and lazily cached)? Is it better for performance (scopes are always needed)?

Due to some issues discussed in this PR. Mainly, the possibility to have inconsistencies due to how we use it in predicates.

Shouldn't we rather throw 404?

We fallback to the realm inferred from the path if none/wrong was provided.

I still feel that from the REST API perspective it would make more sense to explicitly tell that the realm doesn't exist rather than return something the user didn't ask for.

But I don't want to block it, so let's leave it like it is. :)

Shouldn't we rather throw 404?

In case the realm was inferred from the path.

Just a bit nit picking here. I wonder, could we use a different way to supply the cache objects with models – something else than a method attribute? IMHO this unnecessarily makes the code less clean and harder to read. Maybe somehow use DI instead? I don't know... maybe it's a stupid idea.

Not sure if makes sense. At least not now considering the scope.

What is the reason to remove this?

With changes to admin console code, there is no 403 like that. I'm not sure why it was there. Look at the final assertion, that is what makes sense to assert.