Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

IFP: Resolve group names from GIDs if required #267

Closed
wants to merge 3 commits into from

Conversation

Projects
None yet
3 participants
@jhrozek
Copy link
Contributor

commented May 10, 2017

Resolves:
https://pagure.io/SSSD/sssd/issue/3392

The AD provider only converts SIDs to GIDs during initgroups to improve
performance. But this is not sufficient for the
org.freedesktop.sssd.infopipe.GetUserGroups method, which needs to return
names.

We need to resolve the GIDs to names ourselves in that method. In the ticket,
@fidencio suggested that solving this might be easier in the cache_req
module. We should discuss if this alternative approach makes sense or not,
but since I already had these patches, we might as well discuss that in
this PR.

@jhrozek

This comment has been minimized.

Copy link
Contributor Author

commented May 22, 2017

retest this please

@pbrezina

This comment has been minimized.

Copy link
Member

commented May 23, 2017

It can be done in cache_req by creating a new module, say CACHE_REQ_USERGROUPS_BY_NAME/UPN and by implementing the resolution logic inside dp_send_fn. But I think this would be misusing cache_req since plugins should have no logic. And implementing it correctly in cache_req doesn't really fit in. But it may be separated from the InfoPipe and put into a reusable module.

@jhrozek

This comment has been minimized.

Copy link
Contributor Author

commented May 24, 2017

@pbrezina

This comment has been minimized.

Copy link
Member

commented May 24, 2017

jhrozek added some commits May 24, 2017

RESP: Provide a reusable request to fully resolve incomplete groups
After initgroups, the group objects might not be complete, but just
stubs that contain the SID and the GID. If the caller needs to know the
group names as well, this request allows them to iterate over the list
of the groups and resolve them one-by-one.
IFP: Only format the output name to the short version before output
The ifp_user_get_attr_done() request handler was reused for both
GetUserGroups and GetUserAttrs requests. Yet, it performed output
formatting of name and nameAlias.

This is bad, because the output formatting should really be done only
during output. Also, it broke any post-processing of the returned
message which the request might do later.
IFP: Resolve group names from GIDs if required
Resolves:
    https://pagure.io/SSSD/sssd/issue/3392

The AD provider only converts SIDs to GIDs during initgroups
to improve performance. But this is not sufficient for the
org.freedesktop.sssd.infopipe.GetUserGroups method, which needs to return
names.

We need to resolve the GIDs to names ourselves in that method.

@jhrozek jhrozek force-pushed the jhrozek:ifp_groupnames branch from 29da2b2 to 6eab62f May 26, 2017

@jhrozek

This comment has been minimized.

Copy link
Contributor Author

commented May 26, 2017

New patches that use a reusable request were pushed

@jhrozek

This comment has been minimized.

Copy link
Contributor Author

commented May 26, 2017

@jhrozek

This comment has been minimized.

Copy link
Contributor Author

commented May 29, 2017

@pbrezina not trying to be pushy here, but this is something we should get in for the next downstream version, does this patch look better?

@pbrezina

This comment has been minimized.

Copy link
Member

commented May 31, 2017

Ack. Sorry for the delay.

@jhrozek jhrozek added the Accepted label May 31, 2017

@lslebodn

This comment has been minimized.

Copy link
Contributor

commented May 31, 2017

@lslebodn lslebodn closed this May 31, 2017

@lslebodn lslebodn added the Pushed label May 31, 2017

@jhrozek jhrozek deleted the jhrozek:ifp_groupnames branch Jan 20, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.