-
Notifications
You must be signed in to change notification settings - Fork 44
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
API Contracts for /qr-code-auth Endpoint #134
Conversation
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.
will you please change the origin to qr-code-api-contract this branch ?
Done @shreya-mishra , your branch is not up to date with the latest changes in the main branch. There are four file modifications that can be seen. To ensure that only the correct file changes are included in the pull request, please rebase your branch from the most recent main. Thank you. |
Why are events, monitor, and progress changes here? |
Since the target branch is not updated with the latest main we see those file changes. I have asked @shreya-mishra to take a latest pull from main to resolve this. |
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 🚀
9736674
qr-code-auth/README.md
Outdated
```json | ||
{ | ||
"message": "String", | ||
"data": { | ||
"user_id": "String", | ||
"device_info": "String", | ||
"authorization_status": "String", | ||
"access_token": "String" | ||
} | ||
} | ||
``` | ||
|
||
- **Error Response:** |
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.
GET api returns data which has access token. And we can pass any users Id and get their access tokens right?
Then they will be able to login on behalf of the actual user?
Will it not be concerning?
Correct me if I am missing something please
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.
@shreya-mishra @sakshambhatt shall we remove access token from GET Response ?
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.
Agreed! sure we can do that
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.
since we are not going to store the accesstoken
, i don't think we will be able to return it, on a get call.
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.
will work on route names later, in the second iteration.
qr-code-auth/README.md
Outdated
None | ||
- **Query** | ||
|
||
- user_id : Specifies the ID of the User whose authentication document will be retrieved |
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.
added user_id as well to filter the document on the basis of user_id since we don't have device_id stored in our database.
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
The PR adds API contracts for the QrCodeAuth collection, which includes three endpoints: