Skip to content
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

Fix 1285 #1297

Merged
merged 26 commits into from
Mar 13, 2019
Merged

Fix 1285 #1297

merged 26 commits into from
Mar 13, 2019

Conversation

kminehart
Copy link
Contributor

@kminehart kminehart commented Mar 5, 2019

Related issue

#1285

Proposed changes

Summary: Changed the defaults for Name, Description, and Hint from consent/types.go.

Changes:

  1. Changed the ToRFCError() function to only provide defaults if all 3 (Name, Description, and Hint) are left blank.
    • I think it's safe to assume that if a developer provides any combination of "Name", "Description", or "Hint", injecting any defaults will cause confusion.
  2. Changed the default values of Name, Description, and Hint to be more generic, and clear that they were left blank.
    • Name: "Consent request denied"
    • Description: "The request was denied and no further description was provided"
    • Given no context at the time of generation, no valuable hint could be provided, so it will remain empty.

Checklist

  • I have read the contributing guidelines
  • I confirm that this pull request does not address a security vulnerability. If this pull request addresses a security
    vulnerability, I confirm that I got green light (please contact hi@ory.sh) from the maintainers to push the changes.
  • I signed the Developer's Certificate of Origin
    by signing my commit(s). You can amend your signature to the most recent commit by using git commit --amend -s. If you
    amend the commit, you might need to force push using git push --force HEAD:<branch>. Please be very careful when using
    force push.
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation within the code base (if appropriate)
  • I have documented my changes in the developer guide (if appropriate)

@aeneasr
Copy link
Member

aeneasr commented Mar 5, 2019

I don't get why crypto is failing all the time. You can fix this with go mod tidy though!

consent/types.go Outdated Show resolved Hide resolved
consent/types.go Outdated Show resolved Hide resolved
@aeneasr
Copy link
Member

aeneasr commented Mar 5, 2019

Alternatively go get -u golang.org/x/crypto with go modules enabled.

@kminehart
Copy link
Contributor Author

My bad, go modules are a new thing for me. I ran go mod tidy and committed the go.mod and go.sum files. That should suffice, right?

@aeneasr
Copy link
Member

aeneasr commented Mar 5, 2019

Perfect, last thing would be to add a test so this doesn't break in a future patch by accident :)

@kminehart
Copy link
Contributor Author

Cool. I wasn't sure what the right place was to put the unit tests for this change, or if they were necessary given the nature of the tests in hydra/consent.

I also am not sure if there needs to be any documentation changes. I couldn't find anything relevant but I could have missed something.

consent/types.go Outdated Show resolved Hide resolved
consent/types_test.go Outdated Show resolved Hide resolved
Sawada Shota and others added 17 commits March 12, 2019 16:52
…#1283)

Closes #1282

Signed-off-by: Shota SAWADA <xiootas@gmail.com>
Signed-off-by: Kevin Minehart <kmineh0151@gmail.com>
Closes #1274

Signed-off-by: aeneasr <aeneas@ory.sh>
Signed-off-by: Kevin Minehart <kmineh0151@gmail.com>
Signed-off-by: Kevin Minehart <kmineh0151@gmail.com>
https://golang.org/doc/go1.12

Signed-off-by: Shota SAWADA <xiootas@gmail.com>
Signed-off-by: Kevin Minehart <kmineh0151@gmail.com>
This allows the creation of clients permitted to make CORS requests from
specific domains.

Signed-off-by: Josh Giles <jgiles@paxos.com>
Signed-off-by: Kevin Minehart <kmineh0151@gmail.com>
Signed-off-by: Kevin Minehart <kmineh0151@gmail.com>
Signed-off-by: Kevin Minehart <kmineh0151@gmail.com>
Signed-off-by: Kevin Minehart <kmineh0151@gmail.com>
Signed-off-by: Kevin Minehart <kmineh0151@gmail.com>
Signed-off-by: Kevin Minehart <kmineh0151@gmail.com>
Signed-off-by: Kevin Minehart <kmineh0151@gmail.com>
Signed-off-by: Amir Aslaminejad <aslaminejad@gmail.com>
Signed-off-by: Kevin Minehart <kmineh0151@gmail.com>
Signed-off-by: Shota SAWADA <xiootas@gmail.com>
Signed-off-by: Kevin Minehart <kmineh0151@gmail.com>
Signed-off-by: Roman Minkin <roman.minkin@keysight.com>
Signed-off-by: Kevin Minehart <kmineh0151@gmail.com>
Signed-off-by: Kevin Minehart <kmineh0151@gmail.com>
Signed-off-by: Kevin Minehart <kmineh0151@gmail.com>
@kminehart
Copy link
Contributor Author

kminehart commented Mar 12, 2019

@aeneasr I think this one is ready.

Pretty basic, if Name is not provided, it'll use the default. Description and Hint no longer have defaults. All of the commits were me just getting used to the way everything is structured. 😅

Looks like there's go mod conflicts so I'll run a rebase again, I guess.

Signed-off-by: Kevin Minehart <kmineh0151@gmail.com>
Signed-off-by: Kevin Minehart <kmineh0151@gmail.com>
Signed-off-by: Kevin Minehart <kmineh0151@gmail.com>
Signed-off-by: Kevin Minehart <kmineh0151@gmail.com>
Signed-off-by: Kevin Minehart <kmineh0151@gmail.com>
Signed-off-by: Kevin Minehart <kmineh0151@gmail.com>
Signed-off-by: Kevin Minehart <kmineh0151@gmail.com>
Signed-off-by: Kevin Minehart <kmineh0151@gmail.com>
@aeneasr aeneasr merged commit 0fc875a into ory:master Mar 13, 2019
@aeneasr
Copy link
Member

aeneasr commented Mar 13, 2019

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants