-
Notifications
You must be signed in to change notification settings - Fork 146
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
feat: exposing sessions to the api #4352
base: main
Are you sure you want to change the base?
Changes from all commits
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 |
---|---|---|
|
@@ -32,22 +32,6 @@ describe("Consent integration Test", () => { | |
cy.get("[data-testid=verification_code_input]").should("not.be.disabled") | ||
cy.get("[data-testid=verification_code_input]").type(code) | ||
|
||
cy.contains("label", "read").should("exist") | ||
cy.contains("label", "read").should("be.visible") | ||
cy.contains("label", "read").should("not.be.disabled") | ||
cy.contains("label", "read").should("not.be.disabled") | ||
cy.contains("label", "read").click() | ||
|
||
cy.contains("label", "write").should("exist") | ||
cy.contains("label", "write").should("be.visible") | ||
cy.contains("label", "write").should("not.be.disabled") | ||
cy.contains("label", "write").click() | ||
|
||
cy.get("[data-testid=submit_consent_btn]").should("exist") | ||
cy.get("[data-testid=submit_consent_btn]").should("be.visible") | ||
cy.get("[data-testid=submit_consent_btn]").should("not.be.disabled") | ||
cy.get("[data-testid=submit_consent_btn]").click() | ||
|
||
Comment on lines
-35
to
-50
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. same here 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. It is, we're skipping consent in the tests |
||
cy.url().should("eq", Cypress.config().baseUrl + "/") | ||
cy.getCookie("next-auth.session-token").then((cookie) => { | ||
if (cookie && cookie.value) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,17 @@ | ||
query userDetails { | ||
me { | ||
id | ||
phone | ||
mobileSessions { | ||
id | ||
expiresAt | ||
issuedAt | ||
} | ||
delegations { | ||
app | ||
handledAt | ||
remember | ||
scope | ||
} | ||
Comment on lines
+5
to
+15
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. probably something to discuss at ACE or another meeting... why not just have a sessions query? 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. you mean, as opposed to segmenting between delegation and sessions? we also have API keys btw. I think the reason to not have them combined is because they have different properties. for instance, delegated token can currently have smaller scope than a mobile token, which currently is always full access. delegated token are also attached to a particular application. |
||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -182,6 +182,7 @@ | |
"pino-pretty": "^10.3.1", | ||
"prettier": "^3.2.5", | ||
"protoc-gen-js": "^3.21.2", | ||
"puppeteer": "^22.7.1", | ||
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. not sure about this.. integrations tests are already flacky and now introducing a package like this that runs a headless chromium can be problematic.. is not another way to test it? 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 understand your point. I tried with axios and basically spent a day on this, and I could not make it work, so I gave up. the issue with a OAuth2 flow is that there is a lot of context carried around different screen, whether it's via parameters over the URL or via Cookie. it's quite hard to replicate this, specially for someone not having the full context of OAuth2 in its head. using puppeteer make it a lot more straightforward. I suggest we could try it and revert/change if this introduce flackiness to the test suite? |
||
"react": "^18.2.0", | ||
"spectaql": "^2.3.1", | ||
"tiny-secp256k1": "^2.2.3", | ||
|
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 this was removed? not related to the PR
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 is, we're skipping consent in the tests
https://github.com/GaloyMoney/blink/pull/4352/files#diff-837b4f391cfd25241cfd9947e05f126a3078d0b3109844600ec949dcc3dd1ebdR25