Skip to content

improve grant page appearance, allow partial scope approval#10321

Merged
openshift-bot merged 1 commit intoopenshift:masterfrom
liggitt:grant-page
Aug 15, 2016
Merged

improve grant page appearance, allow partial scope approval#10321
openshift-bot merged 1 commit intoopenshift:masterfrom
liggitt:grant-page

Conversation

@liggitt
Copy link
Copy Markdown
Contributor

@liggitt liggitt commented Aug 10, 2016 •
edited
Loading

  • displays descriptions of the requested scopes
  • displays warnings about escalating scopes
  • displays service account oauth clients nicely (Service account <foo> in project <bar>...)
  • allows withholding some of the requested scopes
  • ensures the username present when the grant page was shown matches the authenticated user when submitting the form

To exercise various permutations of the page:

  1. Create a prompting OAuth client and an OAuth service account:

    oc create -f - <<< '{"kind":"OAuthClient","apiVersion":"v1","metadata":{"name":"myclient"},"grantMethod":"prompt","redirectURIs":["http://localhost"]}'
oc annotate sa/builder -n default 'serviceaccounts.openshift.io/oauth-redirecturi.one=http://localhost'

  2. Start the OAuth flow requesting a bunch of scopes ([example oauth client link](https://localhost:8443/oauth/authorize?client_id=myclient&response_type=token&scope=user:info user:check-access user:full role:admin:* role:admin:*:! role:admin:my-namespace role:admin:my-namespace:!), [example service account link](https://localhost:8443/oauth/authorize?client_id=system:serviceaccount:default:builder&response_type=token&scope=user:info user:check-access role:admin:default role:admin:default:!))

  3. Go through the flow several times, exercising various permutations:

    • Click "Deny", you are redirected to localhost with an access_denied error
    • Uncheck all permissions and "approve selected", you are redirected to localhost with an access_denied error
    • Check only one or two permissions and "approve selected", you are redirected to localhost with an access token and the approved scopes in a scope parameter
    • After approving a couple permissions, the page shows existing access and allows selecting/unselecting remaining requested permissions
    • Once all permissions have been approved, token request flows no longer show the prompting page

At each step, the OAuthClientAuthorization object for the user should include all the scopes approved thus far, and the created OAuthAccessToken objects should include the scopes that were both requested AND approved.

Before:
screen shot 2016-08-10 at 1 39 02 am

After
No existing permissions:
screen shot 2016-08-12 at 11 49 46 am

With existing permissions:
screen shot 2016-08-12 at 11 50 40 am

When the client is a service account:
screen shot 2016-08-12 at 2 09 39 pm

Mobile view:
screen shot 2016-08-15 at 10 28 49
screen shot 2016-08-15 at 10 29 03

screen shot 2016-08-15 at 10 29 43

@liggitt
Copy link
Copy Markdown
Contributor Author

liggitt commented Aug 10, 2016

@jwforres @spadgett for UI feedback
@deads2k for partial scope approval

@liggitt
Copy link
Copy Markdown
Contributor Author

liggitt commented Aug 10, 2016

[test]

@liggitt liggitt changed the title WIP - improve grant page appearance improve grant page appearance, allow partial scope approval Aug 11, 2016
@smarterclayton
Copy link
Copy Markdown
Contributor

smarterclayton commented Aug 12, 2016

LOVE. IT.

@deads2k
Copy link
Copy Markdown
Contributor

deads2k commented Aug 12, 2016

How do service accounts render?

@smarterclayton
Copy link
Copy Markdown
Contributor

smarterclayton commented Aug 12, 2016

Should we only check by default the risky things, and then put a warning if any of them have to be opted in? I.e. force users to opt into the scarier ones?

@liggitt
Copy link
Copy Markdown
Contributor Author

liggitt commented Aug 12, 2016 •
edited
Loading

How do service accounts render?

Working on that now, currently something like this:
screen shot 2016-08-12 at 2 09 39 pm

Should we only check by default the risky things, and then put a warning if any of them have to be opted in? I.e. force users to opt into the scarier ones?

Not sure, I have mixed feelings about that. I've never seen a prompting page that defaulted requested permissions off...

@liggitt
Copy link
Copy Markdown
Contributor Author

liggitt commented Aug 12, 2016

updated with service account rendering, ready for review

deads2k
deads2k reviewed Aug 15, 2016
View reviewed changes
pkg/auth/server/grant/templates.go

var defaultGrantTemplate = template.Must(template.New("defaultGrantForm").Parse(defaultGrantTemplateString))

const defaultGrantTemplateString = `<!DOCTYPE html>
Copy link
Copy Markdown
Contributor

@deads2k deads2k Aug 15, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@spadgett @jwforres web-ui stuff here, ptal

@deads2k
Copy link
Copy Markdown
Contributor

deads2k commented Aug 15, 2016

nit. Go code lgtm otherwise, but @spadgett or @jwforres from UI should review the web bits.

@deads2k deads2k assigned deads2k and jwforres and unassigned deads2k Aug 15, 2016
jwforres
jwforres reviewed Aug 15, 2016
View reviewed changes
pkg/auth/server/grant/templates.go Outdated
<head>
<title>Authorize {{ .Values.ClientID }}</title>
<style>
body { font-family: sans-serif; line-height: 1.2em; margin: 2em 5%; color: #222; }
Copy link
Copy Markdown
Member

@jwforres jwforres Aug 15, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#363636 is our base font color in openshift

Copy link
Copy Markdown
Contributor Author

@liggitt liggitt Aug 15, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

@deads2k
Copy link
Copy Markdown
Contributor

deads2k commented Aug 15, 2016

go code lgtm.

@jwforres merge is yours.

@jwforres
Copy link
Copy Markdown
Member

jwforres commented Aug 15, 2016

template LGTM

@liggitt
Copy link
Copy Markdown
Contributor Author

liggitt commented Aug 15, 2016

[merge]

@openshift-bot
Copy link
Copy Markdown
Contributor

openshift-bot commented Aug 15, 2016 •
edited
Loading

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/7918/) (Image: devenv-rhel7_4831)

@openshift-bot
Copy link
Copy Markdown
Contributor

openshift-bot commented Aug 15, 2016

Evaluated for origin merge up to 3905a09

@openshift-bot
Copy link
Copy Markdown
Contributor

openshift-bot commented Aug 15, 2016

Evaluated for origin test up to 3905a09

@openshift-bot
Copy link
Copy Markdown
Contributor

openshift-bot commented Aug 15, 2016

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/7918/)

@openshift-bot openshift-bot merged commit bf5062d into openshift:master Aug 15, 2016
@liggitt liggitt deleted the grant-page branch August 15, 2016 22:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@jwforres jwforres

@deads2k deads2k

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@liggitt @smarterclayton @deads2k @jwforres @openshift-bot @spadgett