-
Notifications
You must be signed in to change notification settings - Fork 28
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
Refer to X.509, not (x|X)509 #324
Conversation
I've rebased this on top of the latest dev, but it still needs a second pass. |
a1d1cf0
to
f9799fe
Compare
I have now done a second, wider, pass. |
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.
We should probably be correct (if I'm right) for the regex.
I'm resisting tackling the codeclimate issues as part of this PR because it's like pulling a thread from a tapestry. |
The commit message on 3e6f7be doesn't at all match or explain what the commit is actually doing. |
- it's more technically correct, the best kind of correct - brings API credentials in line with other (GOCDB#294) around user credentials and account linking - this does "break" API access for existing credentials (until they next get edited), so we should release it: - after GOCDB#260 - maybe after GOCDB#294 - at the same as GOCDB#284, as users will have to edit API credentials anyway
- the code and ancient todo list reference X.509 so came up in my grep-ing.
- the slash is needed to prevent matching things like X+509, though the portal currently prevents such strings getting this far. Co-authored-by: ineilson <[email protected]>
Rebased to resolve conflicts |
It's more technically correct, (the best kind of correct), and brings the naming of the X.509 API credential in line with the X.509 user credential (#294).
so we should merge/release it:
After Refactor API Auth to support non X.509 identifiers #368or not, as this is ready for review.