-
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
Filling some gaps in the DTS manifest #78
Changes from 7 commits
f1ad61e
a4b0a57
2093c2a
ef2158c
0bc9141
8412c85
e1388d7
80e602d
d528a9c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -44,6 +44,22 @@ type KBaseAuthServer struct { | |
AccessToken string | ||
} | ||
|
||
// a record containing information about a user logged into the KBase Auth2 | ||
// server | ||
type kbaseUserInfo struct { | ||
// KBase username | ||
Username string `json:"user"` | ||
// KBase user display name | ||
Display string `json:"display"` | ||
// User email address | ||
Email string `json:"email"` | ||
// Identities with associated providers | ||
Idents []struct { | ||
Provider string `json:"provider"` | ||
UserName string `json:"provusername"` | ||
} `json:"idents"` | ||
} | ||
|
||
// here's how KBase represents errors in responses to API calls | ||
type kbaseAuthErrorResponse struct { | ||
HttpCode int `json:"httpcode"` | ||
|
@@ -55,15 +71,6 @@ type kbaseAuthErrorResponse struct { | |
Time time.Duration `json:"time"` | ||
} | ||
|
||
// here's what we use to fetch user information | ||
type kbaseAuthUserInfo struct { | ||
Username string `json:"user"` | ||
Idents []struct { | ||
Provider string `json:"provider"` | ||
UserName string `json:"provusername"` | ||
} `json:"idents"` | ||
} | ||
|
||
// here's a set of instances to the KBase auth server, mapped by OAuth2 | ||
// access token | ||
var instances map[string]*KBaseAuthServer | ||
|
@@ -87,7 +94,7 @@ func NewKBaseAuthServer(accessToken string) (*KBaseAuthServer, error) { | |
} | ||
|
||
// verify that the access token works (i.e. that the user is logged in) | ||
userInfo, err := server.getUserInfo() | ||
userInfo, err := server.kbaseUserInfo() | ||
if err != nil { | ||
return nil, err | ||
} | ||
|
@@ -162,8 +169,9 @@ func (server *KBaseAuthServer) get(resource string) (*http.Response, error) { | |
return client.Do(req) | ||
} | ||
|
||
func (server *KBaseAuthServer) getUserInfo() (kbaseAuthUserInfo, error) { | ||
var userInfo kbaseAuthUserInfo | ||
// returns information for the current KBase user accessing the auth server | ||
func (server *KBaseAuthServer) kbaseUserInfo() (kbaseUserInfo, error) { | ||
var userInfo kbaseUserInfo | ||
resp, err := server.get("me") | ||
if err != nil { | ||
return userInfo, err | ||
|
@@ -180,31 +188,39 @@ func (server *KBaseAuthServer) getUserInfo() (kbaseAuthUserInfo, error) { | |
return userInfo, err | ||
} | ||
err = json.Unmarshal(body, &userInfo) | ||
return userInfo, err | ||
} | ||
|
||
// returns the username for the current KBase user accessing the | ||
// KBase auth server | ||
func (server *KBaseAuthServer) Username() (string, error) { | ||
userInfo, err := server.getUserInfo() | ||
return userInfo.Username, err | ||
// make sure we have at least one ORCID for this user | ||
if len(userInfo.Idents) < 1 { | ||
return userInfo, fmt.Errorf("KBase Auth2: No providers associated with this user!") | ||
} | ||
foundOrcid := false | ||
for _, pid := range userInfo.Idents { | ||
if pid.Provider == "OrcID" { | ||
foundOrcid = true | ||
break | ||
} | ||
} | ||
if !foundOrcid { | ||
return userInfo, fmt.Errorf("KBase Auth2: No ORCID IDs associated with this user!") | ||
} | ||
return userInfo, err | ||
} | ||
|
||
// returns the current KBase user's registered ORCID identifiers (and/or an error) | ||
// a user can have 0, 1, or many associated ORCID identifiers | ||
func (server *KBaseAuthServer) Orcids() ([]string, error) { | ||
userInfo, err := server.getUserInfo() | ||
// returns a normalized user info record for the current KBase user | ||
func (server *KBaseAuthServer) UserInfo() (UserInfo, error) { | ||
kbUserInfo, err := server.kbaseUserInfo() | ||
if err != nil { | ||
return nil, err | ||
return UserInfo{}, err | ||
} | ||
if len(userInfo.Idents) < 1 { | ||
return nil, fmt.Errorf("No ORCID IDs associated with this user!") | ||
userInfo := UserInfo{ | ||
Name: kbUserInfo.Display, | ||
Username: kbUserInfo.Username, | ||
Email: kbUserInfo.Email, | ||
} | ||
orcidIds := make([]string, 0) | ||
for _, pid := range userInfo.Idents { | ||
for _, pid := range kbUserInfo.Idents { | ||
if pid.Provider == "OrcID" { | ||
orcidIds = append(orcidIds, pid.UserName) | ||
userInfo.Orcid = pid.UserName | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is it guaranteed that there is only ever one Orcid associated with the user? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, but I don't know what we would do with more than one ORCID, so I'm assuming for now that we need to select one, and that the one we select is the first one. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What happens if the user has different OrcIDs associated with their accounts on different platforms? (well, I know what would happen -- nothing. The more useful case to think about is where they have a bunch of OrcIDs assoc'd with their KBase account, one of which is associated with their account on another platform). Not deliberately trying to be annoying here -- just thinking of the kinds of annoying situations that may pop up in the wild. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Makes sense! I guess my logic is that I'd like to start simple, encounter real problems, and address accordingly. At least we have separate KBase and DTS user information so we can articulate our assumptions. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have been burned too many times by trying to keep it simple and assuming that KBase is doing sensible things... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe it! Here's an issue: #81 |
||
} | ||
} | ||
return orcidIds, nil | ||
return userInfo, nil | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,36 @@ | ||
// Copyright (c) 2023 The KBase Project and its Contributors | ||
// Copyright (c) 2023 Cohere Consulting, LLC | ||
// | ||
// Permission is hereby granted, free of charge, to any person obtaining a copy of | ||
// this software and associated documentation files (the "Software"), to deal in | ||
// the Software without restriction, including without limitation the rights to | ||
// use, copy, modify, merge, publish, distribute, sublicense, and/or sell copies | ||
// of the Software, and to permit persons to whom the Software is furnished to do | ||
// so, subject to the following conditions: | ||
// | ||
// The above copyright notice and this permission notice shall be included in all | ||
// copies or substantial portions of the Software. | ||
// | ||
// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR | ||
// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, | ||
// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE | ||
// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER | ||
// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, | ||
// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE | ||
// SOFTWARE. | ||
|
||
package auth | ||
|
||
// a record containing information about a DTS user | ||
type UserInfo struct { | ||
// user's name (human-readable and display-friendly) | ||
Name string | ||
// username used to access DTS | ||
Username string | ||
// user's email address | ||
Email string | ||
// ORCID identifier associated with this user | ||
Orcid string | ||
// organization with which this user is affiliated | ||
Organization string | ||
} |
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.
is it worth setting the OrcID here, so that in the
UserInfo
function you don't do the same iteration through the list ofIdents
?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 think it matters, unless a user has tens of thousands of ORCIDs associated(!). As it is now,
kbaseUserInfo
mirrors the information maintained by the KBase auth server, which doesn't prioritize the first ORCID. This mirrored structure is separate from our interpretation, which provides some clarity as to what we assume, which to me is more valuable than avoiding an extra loop.