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

Implement UserMap and GroupMap output. #159

Merged
merged 7 commits into from
Oct 23, 2023
Merged

Conversation

at88mph
Copy link
Member

@at88mph at88mph commented Oct 18, 2023

Fixes #158

Copy link
Member

@pdowler pdowler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for PosixMapperClient, you can already get the whole list by querying with an empty arg list so I thought we considered those iterator methods to be convenience that we didn't need (because the init container would just hit the URL itself and not involve java code). What's the use cases for adding this other than iterator is more scalable?

aside: the CADC/CANFAR uidmap is 250KiB and the gidmap is 375KiB

as for the other change, I don't know that preferred_username is really spec compliant. That is one of the discrepancies between IAM and Rubin/LSST, which also claim OIDC complaince. So I'm not sure what that is fixing exactly... is it supposed to make ac more OIDC compliant than it is?

These things sometimes need to be discussed before working on them

@at88mph
Copy link
Member Author

at88mph commented Oct 19, 2023

The comments of the getUserMap and getGroupsMap stated the exact use case that Skaha has, so it appeared as though it just wasn't complete, especially since Skaha used to just call the endpoint directly, but no longer does. It now relies on the API of the PosixMapperClient. Also, there was no previous way to get all of the PosixPrincipal instances that I could see.

As for adding the preferred_username, that's stated in the StandardIdentityManager:
https://github.com/opencadc/ac/blob/master/cadc-gms/src/main/java/org/opencadc/auth/StandardIdentityManager.java#L329

Plus it lines up with the SKAO IAM service, and what openid.net suggests:
https://openid.net/specs/openid-connect-basic-1_0.html#Scopes

Copy link
Member

@pdowler pdowler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PosixMapperClient work looks fine, but I should have defined the API using ResourceIterator instead of Iterator (so the underlying input stream close() gets called).

Can you make that improvement and remove the changes from cadc-access-control-server?

@at88mph
Copy link
Member Author

at88mph commented Oct 23, 2023

Alright, that's complete now.

@pdowler pdowler merged commit 5b59ec5 into opencadc:master Oct 23, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement GroupMap and UserMap in PosixMapperClient
2 participants