User Profile - Update APIs to work with domain #83570
Conversation
This PR updates the three existing profile APIs to work with the new security domain concept: * Domain info gets recoreded when activating profile * Domain info is returned appropriately based on the context * "Same" profile is returned based on checking realm name and domain definition
ywangd
added
>enhancement
:Security/Security
|
Pinging @elastic/es-security (Team:Security) |
|
Hi @ywangd, I've created a changelog YAML for you. |
|
|
||
| public Profile.ProfileUser toProfileUser(@Nullable String realmDomain) { | ||
| return new Profile.ProfileUser(username, roles, realm.getName(), realmDomain, email, fullName, displayName, active); | ||
| public Profile.ProfileUser toProfileUser(@Nullable String domainName) { |
There was a problem hiding this comment.
domainName can come from realm.getDomain().name(), no? Is there a reason we're not using that?
There was a problem hiding this comment.
Short answer: We should just use the domain information from the ProfileDocument itself and I updated accordingly.
Long answer: Yes there was a reason. The domain recorded in the document is a snapshot of the configuration at the moment of creation. We agreed that the actual domain information will still be derived from the incoming authentication (if there is one). That is, the final domain info may not be the same as the one recorded in the doc.
That said, the use cases so far in this PR do not see any difference in the two because the document is either newly created or just updated with the incomig authentication. So the two should always have the identical domain info. For simplicity, I agree that this method should not take parameter and just uses the doc's domain info.
| .getDomain() | ||
| .realms() | ||
| .stream() | ||
| .map(RealmConfig.RealmIdentifier::getName) |
There was a problem hiding this comment.
I thought we were going to rely on the realm name-type tuple for these checks, no?
The fact that realm name is unique in a given node yml config file is not good enough to generally qualify a unique user, as I recall.
There was a problem hiding this comment.
Yes you are right. I had a different thought (along the direction that we may want ultimately rely only on name). But even that is a thing in the future and for now we should still use both name and type. I updated accordingly.
...k/plugin/security/src/main/java/org/elasticsearch/xpack/security/profile/ProfileService.java
Outdated
Show resolved
Hide resolved
| import static org.hamcrest.Matchers.notNullValue; | ||
| import static org.hamcrest.Matchers.nullValue; | ||
|
|
||
| public class ProfileDomainSingleNodeTests extends AbstractProfileSingleNodeTestCase { |
There was a problem hiding this comment.
I appreciate the conscientiousness in these tests.
albertzaharovits
left a comment
albertzaharovits
left a comment
There was a problem hiding this comment.
I had a couple of comments, and I'll take another look afterwards to make sure I comprehend it all.
|
Not part of this PR but do you think we should do another profile search after a new one has been created, in order to fail if there is a race? |
We definitely need to handle potential racing condition. It should be of its own PR given the likely complexity. I have some idea and we can discuss about it offline. |
albertzaharovits
left a comment
albertzaharovits
left a comment
There was a problem hiding this comment.
LGTM
GH is misbehaving for me, so I'll comment in here:
- in tests (IT) it would be nice to cover the activate using the token grant
- the query to search for profiles given a domain, can always use
filterinstead ofshould(later, in a follow-up, maybe we can cache it by domain, to avoid the query building on every activate call)
I mean instead of |
Good point. I updated the test helper method to always randomly activate profile using each the user password or a token created from it.
Yes filter is better since we don't care about score. I have replaced all |
When locating existing profile document for the given authentication, use only realm type to search for file and native realms. This is because there can only ever be a single file or native realm and it does not matter what name it takes. Relates: elastic#83570
When locating existing profile document for the given authentication, use only realm type to search for file and native realms. This is because there can only ever be a single file or native realm and it does not matter what name it takes. Relates: #83570
4 participants
This PR updates the three existing profile APIs to work with the new
security domain concept:
definition