-
Notifications
You must be signed in to change notification settings - Fork 0
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
Separated "user" and "client" concepts for DTS. #95
Conversation
This PR breaks our `auth.UserInfo` into `auth.User` and `auth.Client` types in order to distinguish between the rights/privileges of a DTS client used by one or more users, and each user. This allows us to easily associate a specific user (via ORCID) with each transfer request, and allows us to retain client-specific data (like active database proxies) that aren't tied to individual users. For now, if no user ORCID is specified by a transfer request, the DTS falls back to the client's ORCID. We'll remove this fallback when existing services have user-specific transfer ORCIDs in place within transfer requests. Closes #81
auth/kbase_user_federation.go
Outdated
@@ -34,6 +34,36 @@ import ( | |||
// users who have made requests to the DTS. This prevents us from having to | |||
// rely on a secondary data source for this information. | |||
|
|||
// associates the given KBase username with the given ORCID ID |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Today's instance of super pedantry is noting that sometimes you write "ORCID ID" and sometimes just "ORCID". Given that these are comments, it does not matter a whit, but if you feel motivated to make a change before merging, this is one possibility.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @ialarmedalien . I have taken recently to just using "ORCID", cuz, you know, "ID" is already in there. I'll scan for this and clean it up--easy enough. Thanks also for reviewing all these PRs!
Client: auth.Client{ | ||
Name: "Joe-bob", | ||
Orcid: "1234-5678-9012-3456", | ||
}, | ||
User: auth.User{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know if at some future point, you want to write tests that ensure that the client orcid can auth transfers if the user orcid can't, etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We haven't worked out all the ORCID machinery yet, because the effort on all the other moving parts is so diffuse at the moment. When I have a better idea of what we want to do, I'll make sure all the desired code paths are tested.
|
This PR breaks our
auth.UserInfo
intoauth.User
andauth.Client
types in order to distinguish between the rights/privileges of a DTS client used by one or more users, and each user. This allows us to easily associate a specific user (via ORCID) with each transfer request, and allows us to retain client-specific data (like active database proxies) that aren't tied to individual users.For now, if no user ORCID is specified by a transfer request, the DTS falls back to the client's ORCID. We'll remove this fallback when existing services have user-specific transfer ORCIDs in place within transfer requests.
Closes #81