-
Notifications
You must be signed in to change notification settings - Fork 360
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
Integration: update experimental external principals API with auth service #7566
Conversation
api/swagger.yml
Outdated
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.
assets/something../swagger
which is a copy of this one, should be updated too (happens automatically when you run build locally)
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.
it was just WIP, its added now
♻️ PR Preview 05e4840 has been successfully destroyed since this PR has been closed. 🤖 By surge-preview |
@@ -1133,14 +1133,14 @@ paths: | |||
default: | |||
$ref: "#/components/responses/ServerError" | |||
|
|||
/auth/users/{userId}/external/principals/{principalId}: | |||
/auth/users/{userId}/external/principals: |
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 think userID should also be in the query
It will be less painful in case we decide we don't really want to attach principal to users
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.
Why? it operates on a user resource, a different functionality would require a new endpoint.
The only reason removing the principalId is due to limitation of the swagger.
This pattern is similar to our GetObject / UploadObject / ListObjects API's.
api/authorization.yml
Outdated
default: | ||
$ref: "#/components/responses/ServerError" | ||
|
||
|
||
/auth/external/principals: |
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.
why twice?
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.
redundancy 😉 (removed now)
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.
LGTM
Closes #7564