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

Use UidNumber and GidNumber fields in User objects #1573

Merged
merged 5 commits into from
Jun 14, 2021

Conversation

sudo-sturbia
Copy link
Contributor

No description provided.

pkg/auth/manager/ldap/ldap.go Outdated Show resolved Hide resolved
pkg/auth/manager/ldap/ldap.go Outdated Show resolved Hide resolved
pkg/storage/utils/eosfs/eosfs.go Show resolved Hide resolved
@sudo-sturbia sudo-sturbia force-pushed the user-fields branch 2 times, most recently from fabd746 to 0cb5c88 Compare March 19, 2021 20:04
@ishank011
Copy link
Contributor

@sudo-sturbia majorly looks good, nice work! I'll test it since you don't have access to an EOS instance so there might be a delay in merging. We're working on getting EOS tests integrated in the CI so we should have that soon

Meanwhile, please rebase and fix the failing integration tests.

@sudo-sturbia sudo-sturbia force-pushed the user-fields branch 4 times, most recently from cb79532 to 999730a Compare March 23, 2021 12:51
@sudo-sturbia
Copy link
Contributor Author

@ishank011 Hi, I thought I'd let you know the latest updates: I looked at the integration tests and was able to find the errors, the changes are mainly in 999730a and e685753. As for the old commits, struct tags for UIDNumber and GIDNumber in pkg/auth/manager/json were updated to match User struct. Currently all tests pass, except for LGTM: JavaScript, which is odd as i haven't updated any JavaScript, so I'd appreciate it if you let me know how to recreate it locally so i can maybe find the problem.

ishank011
ishank011 previously approved these changes Mar 23, 2021
Copy link
Contributor

@ishank011 ishank011 left a comment

Choose a reason for hiding this comment

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

@sudo-sturbia LGTM! Good work on finding the issues with the integration tests. The LGTM tool is not that mature yet, so we do see intermittent build failures sometimes; we can ignore that. I'll test the changes tomorrow and merge.

@sudo-sturbia
Copy link
Contributor Author

@sudo-sturbia LGTM! Good work on finding the issues with the integration tests. The LGTM tool is not that mature yet, so we do see intermittent build failures sometimes; we can ignore that. I'll test the changes tomorrow and merge.

That sounds great. Thank you!

sudo-sturbia and others added 5 commits June 11, 2021 23:24
Update getUser to verify that uid and gid are not zero to avoid
granting access to users by mistake.
Remove "opaque" map and use the correct value for "uid_number".
Update assertGetUserByClaimResponses integration test to verify that
user is not nil.
@ishank011 ishank011 merged commit 9a68ad1 into cs3org:master Jun 14, 2021
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.

2 participants