-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
Access: authorization flow #1271
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 |
---|---|---|
@@ -0,0 +1,33 @@ | ||
/** | ||
* Copyright 2015 The AMP HTML Authors. All Rights Reserved. | ||
* | ||
* Licensed under the Apache License, Version 2.0 (the "License"); | ||
* you may not use this file except in compliance with the License. | ||
* You may obtain a copy of the License at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, software | ||
* distributed under the License is distributed on an "AS-IS" BASIS, | ||
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
* See the License for the specific language governing permissions and | ||
* limitations under the License. | ||
*/ | ||
|
||
|
||
/** | ||
* Evaluates access expression. | ||
* @param {string} expr | ||
* @param {!JSONObjectDef} data | ||
* @return {boolean} | ||
*/ | ||
export function evaluateAccessExpr(expr, data) { | ||
// TODO(dvoytenko): the complete expression semantics | ||
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. we have a doc or issue we can link to? 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 yet. A separate work in progress thing. |
||
if (expr == 'access') { | ||
return !!data.access; | ||
} | ||
if (expr == 'NOT access') { | ||
return !data.access; | ||
} | ||
return false; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,10 +16,15 @@ | |
|
||
import {actionServiceFor} from '../../../src/action'; | ||
import {assertHttpsUrl} from '../../../src/url'; | ||
import {evaluateAccessExpr} from './access-expr'; | ||
import {getService} from '../../../src/service'; | ||
import {installStyles} from '../../../src/styles'; | ||
import {isExperimentOn} from '../../../src/experiments'; | ||
import {log} from '../../../src/log'; | ||
import {onDocumentReady} from '../../../src/document-state'; | ||
import {urlReplacementsFor} from '../../../src/url-replacements'; | ||
import {vsyncFor} from '../../../src/vsync'; | ||
import {xhrFor} from '../../../src/xhr'; | ||
|
||
|
||
/** | ||
|
@@ -72,8 +77,17 @@ export class AccessService { | |
/** @const @private {!Element} */ | ||
this.accessElement_ = accessElement; | ||
|
||
/** @const {!AccessConfigDef} */ | ||
/** @const @private {!AccessConfigDef} */ | ||
this.config_ = this.buildConfig_(); | ||
|
||
/** @const @private {!Vsync} */ | ||
this.vsync_ = vsyncFor(this.win); | ||
|
||
/** @const @private {!Xhr} */ | ||
this.xhr_ = xhrFor(this.win); | ||
|
||
/** @const @private {!UrlReplacements} */ | ||
this.urlReplacements_ = urlReplacementsFor(this.win); | ||
} | ||
|
||
/** | ||
|
@@ -125,6 +139,78 @@ export class AccessService { | |
startInternal_() { | ||
actionServiceFor(this.win).installActionHandler( | ||
this.accessElement_, this.handleAction_.bind(this)); | ||
|
||
// Start authorization XHR immediately. | ||
this.runAuthorization_(); | ||
} | ||
|
||
/** | ||
* @return {!Promise} | ||
* @private | ||
*/ | ||
runAuthorization_() { | ||
log.fine(TAG, 'Start authorization via ', this.config_.authorization); | ||
this.toggleTopClass_('amp-access-loading', true); | ||
|
||
// TODO(dvoytenko): produce READER_ID and create the URL substition for 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. can we create an issue for this? (guessing its just another |
||
return this.urlReplacements_.expand(this.config_.authorization) | ||
.then(url => { | ||
log.fine(TAG, 'Authorization URL: ', url); | ||
return this.xhr_.fetchJson(url, {credentials: 'include'}); | ||
}) | ||
.then(response => { | ||
log.fine(TAG, 'Authorization response: ', response); | ||
this.toggleTopClass_('amp-access-loading', false); | ||
onDocumentReady(this.win.document, () => { | ||
this.applyAuthorization_(response); | ||
}); | ||
}) | ||
.catch(error => { | ||
log.error(TAG, 'Authorization failed: ', error); | ||
this.toggleTopClass_('amp-access-loading', false); | ||
}); | ||
} | ||
|
||
/** | ||
* @param {!JSONObjectDef} response | ||
* @private | ||
*/ | ||
applyAuthorization_(response) { | ||
const elements = this.win.document.querySelectorAll('[amp-access]'); | ||
for (let i = 0; i < elements.length; i++) { | ||
this.applyAuthorizationToElement_(elements[i], response); | ||
} | ||
} | ||
|
||
/** | ||
* @param {!Element} element | ||
* @param {!JSONObjectDef} response | ||
* @private | ||
*/ | ||
applyAuthorizationToElement_(element, response) { | ||
const expr = element.getAttribute('amp-access'); | ||
const on = evaluateAccessExpr(expr, response); | ||
|
||
// TODO(dvoytenko): support templates | ||
|
||
this.vsync_.mutate(() => { | ||
if (on) { | ||
element.removeAttribute('amp-access-off'); | ||
} else { | ||
element.setAttribute('amp-access-off', ''); | ||
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. shouldn't it already be there? 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. The current state is that it's not on by default. However, another important aspect: authorization can run multiple times, so the code is repeatable. |
||
} | ||
}); | ||
} | ||
|
||
/** | ||
* @param {string} className | ||
* @param {boolean} on | ||
* @private | ||
*/ | ||
toggleTopClass_(className, on) { | ||
this.vsync_.mutate(() => { | ||
this.win.document.documentElement.classList.toggle(className, on); | ||
}); | ||
} | ||
|
||
/** | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,47 @@ | ||
/** | ||
* Copyright 2015 The AMP HTML Authors. All Rights Reserved. | ||
* | ||
* Licensed under the Apache License, Version 2.0 (the "License"); | ||
* you may not use this file except in compliance with the License. | ||
* You may obtain a copy of the License at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, software | ||
* distributed under the License is distributed on an "AS-IS" BASIS, | ||
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
* See the License for the specific language governing permissions and | ||
* limitations under the License. | ||
*/ | ||
|
||
import {evaluateAccessExpr} from '../access-expr'; | ||
|
||
|
||
describe('evaluateAccessExpr', () => { | ||
|
||
it('should evaluate simple boolean expressions', () => { | ||
expect(evaluateAccessExpr('access', {})).to.be.false; | ||
|
||
expect(evaluateAccessExpr('access', {access: true})).to.be.true; | ||
expect(evaluateAccessExpr('access', {access: false})).to.be.false; | ||
|
||
expect(evaluateAccessExpr('access', {access: 1})).to.be.true; | ||
expect(evaluateAccessExpr('access', {access: 0})).to.be.false; | ||
|
||
expect(evaluateAccessExpr('access', {access: '1'})).to.be.true; | ||
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. qq, why do we allow all the truthy/falsy expressions? 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. That's not fully decided yet. However, on the surface, I don't see a bit reason not to do so. E.g. an expression could be built on top of "viewsLeft" number that is either |
||
expect(evaluateAccessExpr('access', {access: ''})).to.be.false; | ||
}); | ||
|
||
it('should evaluate simple boolean NOT expressions', () => { | ||
expect(evaluateAccessExpr('NOT access', {})).to.be.true; | ||
|
||
expect(evaluateAccessExpr('NOT access', {access: true})).to.be.false; | ||
expect(evaluateAccessExpr('NOT access', {access: false})).to.be.true; | ||
|
||
expect(evaluateAccessExpr('NOT access', {access: 1})).to.be.false; | ||
expect(evaluateAccessExpr('NOT access', {access: 0})).to.be.true; | ||
|
||
expect(evaluateAccessExpr('NOT access', {access: '1'})).to.be.false; | ||
expect(evaluateAccessExpr('NOT access', {access: ''})).to.be.true; | ||
}); | ||
}); |
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.
Can we change this to something like
allow
/deny
? It's not clear to me whetheraccess
blocks should allow access for unregistered users orNOT access
blocks do it.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 like
allow
/deny
, however I need to think about it. Expressions are fairly open and do not necessarily have semantics of "allow/deny". E.g. "access" comes from the user defined response and could mean anything - the section is visible of hidden depending on whether this expression evaluates totrue
.