Skip to content

Commit a8f8945

Browse files
jaxoncreedjaxoncreed
authored
Merge pull request #41 from solid/consent-screen
Consent screen update
2 parents ab01849 + 273c693 commit a8f8945

File tree

3 files changed

+119
-95
lines changed

3 files changed

+119
-95
lines changed

‎src/handlers/login-consent-request.js‎

Lines changed: 29 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,8 @@
11
'use strict'
22

3+
const AuthResponseSent = require('../errors/auth-response-sent')
4+
const url = require('url')
5+
36
class LoginConsentRequest {
47
constructor (options) {
58
this.opAuthRequest = options.opAuthRequest
@@ -62,15 +65,18 @@ class LoginConsentRequest {
6265
static obtainConsent (consentRequest) {
6366
let { opAuthRequest, clientId } = consentRequest
6467

68+
const parsedAppOrigin = url.parse(consentRequest.opAuthRequest.params.redirect_uri)
69+
const appOrigin = `${parsedAppOrigin.protocol}//${parsedAppOrigin.host}`
70+
6571
// Consent for the local RP client (the home pod) is implied
66-
if (consentRequest.isLocalRpClient(clientId)) {
72+
if (consentRequest.isLocalRpClient(appOrigin)) {
6773
return Promise.resolve()
6874
.then(() => { consentRequest.markConsentSuccess(opAuthRequest) })
6975
.then(() => opAuthRequest)
7076
}
7177

7278
// Check if user has submitted this from a Consent page
73-
if (consentRequest.params.consent) {
79+
if (consentRequest.hasAlreadyConsented(appOrigin)) {
7480
return consentRequest.saveConsentForClient(clientId)
7581
.then(() => { consentRequest.markConsentSuccess(opAuthRequest) })
7682
.then(() => opAuthRequest)
@@ -82,7 +88,7 @@ class LoginConsentRequest {
8288
if (priorConsent) {
8389
consentRequest.markConsentSuccess(opAuthRequest)
8490
} else {
85-
consentRequest.renderConsentPage()
91+
consentRequest.redirectToConsent()
8692
}
8793
})
8894
.then(() => opAuthRequest)
@@ -95,10 +101,13 @@ class LoginConsentRequest {
95101
return this.params['client_id']
96102
}
97103

98-
isLocalRpClient (clientId) {
99-
let host = this.opAuthRequest.host || {}
104+
isLocalRpClient (appOrigin) {
105+
return this.opAuthRequest.req.app.locals.ldp.serverUri === appOrigin
106+
}
100107

101-
return !!clientId && clientId === host.localClientId
108+
hasAlreadyConsented (appOrigin) {
109+
return this.opAuthRequest.req.session.consentedOrigins &&
110+
this.opAuthRequest.req.session.consentedOrigins.includes(appOrigin)
102111
}
103112

104113
checkSavedConsentFor (opAuthRequest) {
@@ -114,11 +123,21 @@ class LoginConsentRequest {
114123
return Promise.resolve(clientId)
115124
}
116125

117-
renderConsentPage () {
118-
let { response, params, opAuthRequest } = this
126+
redirectToConsent (authRequest) {
127+
let { opAuthRequest } = this
128+
let consentUrl = url.parse('/consent')
129+
consentUrl.query = opAuthRequest.req.query
130+
131+
consentUrl = url.format(consentUrl)
132+
opAuthRequest.subject = null
133+
134+
opAuthRequest.res.redirect(consentUrl)
135+
136+
this.signalResponseSent()
137+
}
119138

120-
response.render('auth/consent', params)
121-
opAuthRequest.headersSent = true
139+
signalResponseSent () {
140+
throw new AuthResponseSent('User redirected')
122141
}
123142
}
124143

‎src/host-api.js‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -83,10 +83,13 @@ function initSubjectClaim (authRequest, webId) {
8383

8484
function obtainConsent (authRequest) {
8585
let debug = authRequest.host.debug || console.error.bind(console)
86-
let skipConsent = true
86+
let skipConsent = false
8787

8888
return LoginConsentRequest.handle(authRequest, skipConsent)
8989
.catch(error => {
90+
if (error instanceof AuthResponseSent) {
91+
throw error
92+
}
9093
debug('Error in auth Consent step: ', error)
9194
})
9295
}

‎test/unit/login-consent-request.js‎

Lines changed: 86 additions & 84 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,30 @@ const HttpMocks = require('node-mocks-http')
1313

1414
const LoginConsentRequest = require('../../src/handlers/login-consent-request')
1515

16+
function createOpAuthRequest (overwrite) {
17+
return Object.assign({
18+
req: {
19+
body: {},
20+
app: {
21+
locals: {
22+
ldp: {
23+
serverUri: 'https://pod.example'
24+
}
25+
}
26+
},
27+
session: {
28+
consentedOrigins: ['https://example.com']
29+
}
30+
},
31+
res: HttpMocks.createResponse(),
32+
subject: {},
33+
params: {
34+
redirect_uri: 'https://example.com'
35+
},
36+
host: {}
37+
}, overwrite)
38+
}
39+
1640
describe('LoginConsentRequest', () => {
1741
describe('constructor()', () => {
1842
it('should initialize a new instance', () => {
@@ -70,8 +94,7 @@ describe('LoginConsentRequest', () => {
7094

7195
describe('handle()', () => {
7296
it('should return the opAuthRequest object', () => {
73-
let res = HttpMocks.createResponse()
74-
let opAuthRequest = { req: { body: {} }, res, subject: {} }
97+
let opAuthRequest = createOpAuthRequest()
7598

7699
return LoginConsentRequest.handle(opAuthRequest)
77100
.then(returnedRequest => {
@@ -80,8 +103,7 @@ describe('LoginConsentRequest', () => {
80103
})
81104

82105
it('should invoke obtainConsent()', () => {
83-
let res = HttpMocks.createResponse()
84-
let opAuthRequest = { req: { body: {} }, res, subject: {} }
106+
let opAuthRequest = createOpAuthRequest()
85107

86108
let obtainConsent = sinon.spy(LoginConsentRequest, 'obtainConsent')
87109

@@ -93,8 +115,7 @@ describe('LoginConsentRequest', () => {
93115
})
94116

95117
it('should pass through opAuthRequest if skipConsent is set', () => {
96-
let res = HttpMocks.createResponse()
97-
let opAuthRequest = { req: { body: {} }, res, subject: {} }
118+
let opAuthRequest = createOpAuthRequest()
98119
let skipConsent = true
99120

100121
return LoginConsentRequest.handle(opAuthRequest, skipConsent)
@@ -103,16 +124,6 @@ describe('LoginConsentRequest', () => {
103124
LoginConsentRequest.obtainConsent.resetHistory()
104125
})
105126
})
106-
107-
it('should not invoke obtainConsent() if subject is missing', () => {
108-
let res = HttpMocks.createResponse()
109-
let opAuthRequest = { req: { body: {} }, res }
110-
111-
return LoginConsentRequest.handle(opAuthRequest)
112-
.then(() => {
113-
expect(LoginConsentRequest.obtainConsent).to.not.have.been.called()
114-
})
115-
})
116127
})
117128

118129
describe('clientId getter', () => {
@@ -130,52 +141,46 @@ describe('LoginConsentRequest', () => {
130141
describe('isLocalRpClient()', () => {
131142
it('should be false if host has no local client initialized', () => {
132143
let params = { 'client_id': '1234' }
133-
let response = HttpMocks.createResponse()
134-
let opAuthRequest = { host: {} }
144+
let res = HttpMocks.createResponse()
145+
let opAuthRequest = createOpAuthRequest({ res })
135146

136-
let request = new LoginConsentRequest({ params, response, opAuthRequest })
147+
let request = new LoginConsentRequest({ params, res, opAuthRequest })
137148

138149
expect(request.isLocalRpClient('1234')).to.be.false()
139150
})
140151

141152
it('should be false if params has no client id', () => {
142153
let params = {}
143-
let response = HttpMocks.createResponse()
144-
let opAuthRequest = {
145-
host: {}
146-
}
154+
let res = HttpMocks.createResponse()
155+
let opAuthRequest = createOpAuthRequest({ res })
147156

148-
let request = new LoginConsentRequest({ params, response, opAuthRequest })
157+
let request = new LoginConsentRequest({ params, res, opAuthRequest })
149158

150159
expect(request.isLocalRpClient(undefined)).to.be.false()
151160
})
152161

153-
it('should be false if host local client id does not match params', () => {
154-
let params = { 'client_id': '1234' }
155-
let response = HttpMocks.createResponse()
156-
let opAuthRequest = {
157-
host: {
158-
localClientId: '5678'
159-
}
160-
}
162+
it('should be false if host local app origin does not equal param server uri', () => {
163+
let params = {}
164+
let res = HttpMocks.createResponse()
165+
let opAuthRequest = createOpAuthRequest({
166+
res
167+
})
161168

162-
let request = new LoginConsentRequest({ params, response, opAuthRequest })
169+
let request = new LoginConsentRequest({ params, res, opAuthRequest })
163170

164-
expect(request.isLocalRpClient('1234')).to.be.false()
171+
expect(request.isLocalRpClient('https://example.com')).to.be.false()
165172
})
166173

167-
it('should be true if host local client id equals param client_id', () => {
168-
let params = { 'client_id': '1234' }
169-
let response = HttpMocks.createResponse()
170-
let opAuthRequest = {
171-
host: {
172-
localClientId: '1234'
173-
}
174-
}
174+
it('should be true if host local app origin equals param server uri', () => {
175+
let params = {}
176+
let res = HttpMocks.createResponse()
177+
let opAuthRequest = createOpAuthRequest({
178+
res
179+
})
175180

176-
let request = new LoginConsentRequest({ params, response, opAuthRequest })
181+
let request = new LoginConsentRequest({ params, res, opAuthRequest })
177182

178-
expect(request.isLocalRpClient('1234')).to.be.true()
183+
expect(request.isLocalRpClient('https://pod.example')).to.be.true()
179184
})
180185
})
181186

@@ -188,7 +193,12 @@ describe('LoginConsentRequest', () => {
188193
beforeEach(() => {
189194
req = { body: { scope: 'openid', client_id: clientId } }
190195
res = HttpMocks.createResponse()
191-
opAuthRequest = { req, res, host }
196+
opAuthRequest = createOpAuthRequest({ res, host })
197+
opAuthRequest = Object.assign(opAuthRequest, {
198+
req: Object.assign(opAuthRequest.req, {
199+
body: req.body
200+
})
201+
})
192202
})
193203

194204
it('should mark successful consent automatically', () => {
@@ -221,7 +231,12 @@ describe('LoginConsentRequest', () => {
221231
beforeEach(() => {
222232
req = { body: { consent: true, scope: 'openid', client_id: clientId } }
223233
res = HttpMocks.createResponse()
224-
opAuthRequest = { req, res, host }
234+
opAuthRequest = createOpAuthRequest({ res, host })
235+
opAuthRequest = Object.assign(opAuthRequest, {
236+
req: Object.assign(opAuthRequest.req, {
237+
body: req.body
238+
})
239+
})
225240
})
226241

227242
it('should call saveConsentForClient()', () => {
@@ -270,19 +285,15 @@ describe('LoginConsentRequest', () => {
270285
beforeEach(() => {
271286
req = { body: { scope: 'openid' } }
272287
res = HttpMocks.createResponse()
273-
opAuthRequest = { req, res }
274-
})
275-
276-
it('should check for previously saved consent', () => {
277-
let request = LoginConsentRequest.from(opAuthRequest)
278-
279-
request.checkSavedConsentFor = sinon.mock()
280-
.returns(Promise.resolve(false))
281-
282-
return LoginConsentRequest.obtainConsent(request)
283-
.then(() => {
284-
expect(request.checkSavedConsentFor).to.have.been.called()
288+
opAuthRequest = createOpAuthRequest({ res })
289+
opAuthRequest = Object.assign(opAuthRequest, {
290+
req: Object.assign(opAuthRequest.req, {
291+
session: {
292+
consentedOrigins: []
293+
},
294+
body: req.body
285295
})
296+
})
286297
})
287298

288299
describe('if user consent has been previously saved', () => {
@@ -305,16 +316,17 @@ describe('LoginConsentRequest', () => {
305316
})
306317

307318
describe('if user consent has NOT been previously saved', () => {
308-
it('should call renderConsentPage()', () => {
319+
it('should call redirectToConsent()', () => {
309320
let request = LoginConsentRequest.from(opAuthRequest)
310321

311322
request.checkSavedConsentFor = sinon.mock()
312323
.returns(Promise.resolve(false))
313324
request.response.render = sinon.mock()
314325

315-
let renderConsentPage = sinon.spy(request, 'renderConsentPage')
326+
let renderConsentPage = sinon.spy(request, 'redirectToConsent')
316327

317328
return LoginConsentRequest.obtainConsent(request)
329+
.catch(() => {})
318330
.then(() => {
319331
expect(renderConsentPage).to.have.been.called()
320332
})
@@ -328,6 +340,7 @@ describe('LoginConsentRequest', () => {
328340
request.response.render = sinon.mock()
329341

330342
return LoginConsentRequest.obtainConsent(request)
343+
.catch((opAuthRequest) => opAuthRequest)
331344
.then(opAuthRequest => {
332345
expect(opAuthRequest.consent).to.not.exist()
333346
expect(opAuthRequest.scope).to.not.exist()
@@ -337,37 +350,26 @@ describe('LoginConsentRequest', () => {
337350
})
338351
})
339352

340-
describe('renderConsentPage()', () => {
341-
it('should call res.render', () => {
342-
let req = { body: {} }
353+
describe('redirectToConsent()', () => {
354+
it('should call res.redirect', () => {
343355
let res = HttpMocks.createResponse()
344356

345-
let render = sinon.stub(res, 'render')
357+
let redirect = sinon.stub(res, 'redirect')
346358

347-
let opAuthRequest = { req, res }
348-
let request = LoginConsentRequest.from(opAuthRequest)
349-
350-
return LoginConsentRequest.obtainConsent(request)
351-
.then(() => {
352-
expect(render).to.have.been.calledWith('auth/consent')
359+
let opAuthRequest = createOpAuthRequest({ res })
360+
opAuthRequest = Object.assign(opAuthRequest, {
361+
req: Object.assign(opAuthRequest.req, {
362+
session: {
363+
consentedOrigins: []
364+
}
353365
})
354-
})
355-
356-
it('should set the headerSent property on opAuthRequest', () => {
357-
let req = { body: {} }
358-
let res = HttpMocks.createResponse()
359-
360-
sinon.stub(res, 'render')
361-
362-
let opAuthRequest = { req, res }
366+
})
363367
let request = LoginConsentRequest.from(opAuthRequest)
364368

365-
request.checkSavedConsentFor = sinon.mock()
366-
.returns(Promise.resolve(false))
367-
368369
return LoginConsentRequest.obtainConsent(request)
369-
.then(opAuthRequest => {
370-
expect(opAuthRequest.headersSent).to.be.true()
370+
.catch(() => {})
371+
.then(() => {
372+
expect(redirect).to.have.been.called()
371373
})
372374
})
373375
})

0 commit comments

Comments
 (0)