Add validation for logout redirect for the console CRD #20
Conversation
openshift-ci-robot
added
the
size/S
|
@spadgett fyi related to
|
|
/assign deads2k enj |
| authentication: | ||
| properties: | ||
| logoutRedirect: | ||
| pattern: ^$|^((https):\/\/?)[^\s()<>]+(?:\([\w\d]+\)|([^[:punct:]\s]|\/?))$ |
There was a problem hiding this comment.
Testing this with a regex tester:
https://foobar // valid, probably should not be?
https://redhat.com/#foo // valid, i dont think we care about fragments
https://redhat.com?red=hat // valid, i dont think we care about query strings
already stated in prev comment:
https://www.redhat.com:8080/logout.html // valid
http://www.redhat.com // invalid
www.redhat.com is invalid // invalid
There was a problem hiding this comment.
https://foobar // valid, probably should not be?
Is there a reason we would require a path? I'd think this is OK
There was a problem hiding this comment.
I wanted https://localhost to work
There was a problem hiding this comment.
@spadgett I was more commenting on the .<something>.
localhost does seem a valid point.
openshift-ci-robot
added
the
needs-rebase
zherman0
force-pushed
the
validation-logout
branch
from
05e7b97 to
cf6a1eb
Compare
openshift-ci-robot
added
size/XS
|
--- manifests/0000_05_config-operator_01_console.crd.yaml 2019-03-08 16:36:01.000000000 +0000
+++ /go/src/github.com/openshift/cluster-config-operator/101316955/0000_05_config-operator_01_console.crd.yaml 2019-03-08 16:37:50.753525862 +0000
@@ -38,7 +38,6 @@
authentication:
properties:
logoutRedirect:
- pattern: ^$|^((https):\/\/?)[^\s()<>]+(?:\([\w\d]+\)|([^[:punct:]\s]|\/?))$
description: 'An optional, absolute URL to redirect web browsers
to after logging out of the console. If not specified, it will
redirect to the default login page. This is required when using |
|
I need openshift/api#249 to merge first before the verify test will pass. |
zherman0
force-pushed
the
validation-logout
branch
from
cf6a1eb to
5278d9c
Compare
|
this doesn't look right. You should need to re-vendor openshift/api and regenerate, rigth? |
|
We will need to bump the deps now that api/pr249 has merged for this to pass |
# usually works
make update-deps && make generate && make verify
# but I think the deps update is manual in this repo, probably something like
glide update --strip-vendor && make update-codegen && make verify |
|
/test verify |
zherman0
force-pushed
the
validation-logout
branch
from
5278d9c to
00af35f
Compare
zherman0
force-pushed
the
validation-logout
branch
from
00af35f to
8efa339
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: zherman0 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci-robot
added
the
approved
|
This update to the manifest was automatically done when the vendor apis was bumped and the pattern auto created. Therefore, closing this ticket. |
|
@zherman0: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
Add validation for logout redirect for the console CRD
1. Validation in the CRD to test that well formed URLs are supplied
a) https://www.redhat.com:8080/logout.html is valid
b) http://www.redhat.com is invalid because it is insecure
c) www.redhat.com is invalid