Name the usage a 51Did was created for, and take the raw byte surface off FodId - #127
Open
jwrosewell wants to merge 2 commits into
Open
Name the usage a 51Did was created for, and take the raw byte surface off FodId#127jwrosewell wants to merge 2 commits into
jwrosewell wants to merge 2 commits into
Conversation
FodId exposed the flags byte and a type read from bits 6-7, and nothing for the usage in bits 0-2, so a caller deciding whether an identifier may be passed to a demand source had to mask the byte by hand. The three usages are cumulative in the byte, non-marketing 0b001, standard 0b011 and personalized 0b111, so a caller masking for the non-marketing bit alone would read every marketing identifier as non-marketing, the wrong way round for a data protection decision. Usage names the four states, NONE where no bit is set, and getUsage() answers with the highest granted. isUsageFromConsent() reads bit 3, which records that the usage came from a consent string rather than being stated. The names match the cloud's id.usage values, which getIdUsage() gives, and are the same in every 51Did package, which all gain the accessor together. Found by the Trusted Server work, which must work only through this package. pipeline.did tests: 129 run, 0 failures, two new.
A 51Did gave a caller two ways to read the same thing, being a typed accessor for each field and the raw bytes with the offsets to walk them by hand. The second way is the one that produces the mistake the usage accessor was added to stop, because the usage bits are cumulative, so non-marketing sets one bit, standard sets two and personalized sets three, and code that masks the byte for the non-marketing bit alone reads every marketing identifier as non-marketing, which is backwards for a rule that says a non-marketing identifier must never reach a demand source. These packages are not widely deployed, so the raw surface is removed now rather than deprecated, and the same removal is being made in every language so that the six packages keep one surface. Gone from the public surface. 1. getFlags() is now package-private. The byte itself stays because getType(), getUsage() and isUsageFromConsent() are built on it and this package's own tests assert the exact byte, and those tests sit in the same package so they keep working unchanged. 2. getHash(), HASH_OFFSET and HASH_LENGTH are deleted. They were aliases left over from the match key rename and nothing called them. The two tests that only asserted the aliases matched their new names go with them. 3. FLAGS_OFFSET, LICENSE_ID_OFFSET, LICENSE_ID_LENGTH, MATCH_KEY_OFFSET, MATCH_KEY_LENGTH, HEADER_LENGTH, GUID_LENGTH, RANDOM_PAYLOAD_LENGTH and PAYLOAD_LENGTH keep their names and become package-private. The only reason to want an offset is to read a bit that now has a name. 4. getDateMinutes() is deleted. Nothing in any of the seven 51Degrees repositories called it, the Rust package never had it, and its one documented purpose, being the value the OWID public-key date parameter takes, is built by DidClient itself. Nothing inside the package needed the minutes, so the constant that converted them went too and no helper replaced it. The two tests that asserted on the minutes now assert the same round trip through the typed getDate(), including that the high bit of the envelope's unsigned 32-bit minutes field does not read back as a date before 2020. Every typed accessor is unchanged, being getUsage(), isUsageFromConsent(), getType(), getLicenseId(), getMatchKey() and the OWID level fields. The 51Did example builds a payload byte by byte because it stands in for the cloud, which is a writer's job rather than a reader's, so it now spells out the three offsets it needs as its own constants and says why. It prints the usage and whether the usage came from a consent string in place of the raw flags byte, and its sample payload now carries standard marketing usage so the accessor shows something. Documentation. The class javadoc and the README no longer repeat the byte layout table. Both name the 51Did specification as the authority for it, at https://github.com/51Degrees/specifications/blob/main/did-specification/identifier-layout.md and https://github.com/51Degrees/specifications/blob/main/did-specification/package-surface.md keeping only the short summary of the identifier type that changes how the package behaves. The README gains a section on what a 51Did may be used for, covering getUsage() and isUsageFromConsent(), the four usage values and the cumulative bits, and it loses the lines that used the deleted accessors. Verified with mvn -pl pipeline.did,pipeline.developer-examples/pipeline.developer-examples.fodid -am test on JDK 21, which builds with -Xlint:all -Werror. 127 tests pass in pipeline.did with 2 live cloud tests skipped, and 9 pass in the 51Did example. Written with AI assistance and needs human review. Closes #128
jwrosewell
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two changes to the same surface, in the order they were made.
1. Name the usage a 51Did was created for
FodIdgainsgetUsage()(None, NonMarketing, Standard, Personalized) andisUsageFromConsent(), reading bits 0 to 2 and bit 3 of the flags byte. The usage bits are cumulative, so the accessor answers the highest usage granted rather than leaving callers to mask bits and misread every marketing identifier as non-marketing. Same accessor, same names, in all six 51Did packages. Found by the Trusted Server work, which must work only through this package.2. Take the raw byte surface off FodId
This is issue 128, raised on all six packages so the surface stays the same in every language. Once every bit has a name, the raw byte and the offsets are the remaining way to get it wrong, so they go.
getFlags()is package-private. The byte stays, because the typed accessors are built on it and the package's own tests assert it. All fifteen assertions on the exact byte are in the same package and compile unchanged.getHash(),HASH_OFFSETandHASH_LENGTHare deleted, along with the two tests that existed only to prove the aliases matched their new names.public.DidClientand the tests are in the same package and reach them unchanged.getDateMinutes()is deleted, with the private date origin constant it used. Nothing inside the package needed the minutes, so no helper replaces it. Its two tests now assert the same facts throughgetDate().The open question, decided
The issue left the raw date minutes as a candidate rather than a definite. It goes, in all five packages that have it. No caller in any of the seven repositories used it, Rust never had one and nobody missed it, and the single purpose the documentation gave it, the OWID public key request, is built by the client itself.
One thing the issue missed
The issue lists the package's own tests and the cloud service as the code that must keep working. In Java the
pipeline.developer-examples.fodidmodule is in a different package and usedPAYLOAD_LENGTH,FLAGS_OFFSET,LICENSE_ID_OFFSET,MATCH_KEY_OFFSET,MATCH_KEY_LENGTHandgetFlags(), so making the constants package-private breaks it. It is fixed in the same commit: the example states the offsets it needs as its own local constants, with a comment saying that building a payload is the creator's job rather than a reader's, and it prints the usage and the consent bit in place of the raw byte.The specification
The byte structure is now specified once for all six languages, on specifications 25, at Identifier layout with the surface each package exposes at Package surface. The class javadoc and the README link there instead of repeating the tables. Those links resolve once that pull request merges.
Verification
mvn -pl pipeline.did,pipeline.developer-examples/pipeline.developer-examples.fodid -am teston JDK 21 with-Werror:The two skips are the live tests, which need cloud credentials and were skipped before this change as well.
Closes #128
Produced with AI assistance and needs human review.